-
Notifications
You must be signed in to change notification settings - Fork 50
feat: reVamping the volcano, dashboard, backend, trpc, turboRepo #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@de6p i think you should create small small pr , so that it can be better to review. |
+1 |
|
Hello @de6p, |
…ster Signed-off-by: Deep <[email protected]>
…n UI, next.js Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a massive and impressive refactoring of the Volcano dashboard. The migration to a modern stack with Next.js, tRPC, and Turborepo is a huge step forward. The new UI looks clean and the architecture is much more scalable.
I've conducted a thorough review and identified several areas for improvement, from critical correctness issues like manual YAML parsing to maintainability and efficiency enhancements. Addressing these will help solidify the new foundation and ensure the dashboard is robust and easy to maintain.
| const parseYamlToManifest = (yamlString: string) => { | ||
| const lines = yamlString.trim().split('\n') | ||
| const manifest: any = { | ||
| apiVersion: '', | ||
| kind: '', | ||
| metadata: { name: '' }, | ||
| spec: { containers: [] } | ||
| } | ||
|
|
||
| const requiredFields = ['apiVersion', 'kind', 'metadata', 'spec'] | ||
| const foundFields = new Set<string>() | ||
| let currentSection = '' | ||
| let currentSubSection = '' | ||
| let currentContainer: any = null | ||
| let inContainers = false | ||
|
|
||
| for (const line of lines) { | ||
| const trimmed = line.trim() | ||
| if (!trimmed || trimmed.startsWith('#')) continue | ||
|
|
||
| for (const field of requiredFields) { | ||
| if (trimmed.startsWith(`${field}:`)) { | ||
| foundFields.add(field) | ||
| if (field === 'apiVersion' || field === 'kind') { | ||
| manifest[field] = trimmed.split(':')[1].trim() | ||
| } | ||
| currentSection = field | ||
| currentSubSection = '' | ||
| } | ||
| } | ||
|
|
||
| if (trimmed.startsWith('name:') && currentSection === 'metadata') { | ||
| manifest.metadata.name = trimmed.split(':')[1].trim() | ||
| } | ||
|
|
||
| if (currentSection === 'spec') { | ||
| if (trimmed.startsWith('containers:')) { | ||
| inContainers = true | ||
| currentSubSection = 'containers' | ||
| } else if (trimmed.startsWith('- name:') && inContainers) { | ||
| if (currentContainer) { | ||
| manifest.spec.containers.push(currentContainer) | ||
| } | ||
| currentContainer = { name: trimmed.split(':')[1].trim() } | ||
| } else if (trimmed.startsWith('image:') && currentContainer) { | ||
| currentContainer.image = trimmed.split(':')[1].trim() | ||
| } else if (trimmed.startsWith('ports:') && currentContainer) { | ||
| currentContainer.ports = [] | ||
| currentSubSection = 'ports' | ||
| } else if (trimmed.startsWith('- containerPort:') && currentSubSection === 'ports' && currentContainer) { | ||
| const port = parseInt(trimmed.split(':')[1].trim()) | ||
| currentContainer.ports.push({ containerPort: port }) | ||
| } else if (trimmed.startsWith('restartPolicy:') && currentSection === 'spec') { | ||
| manifest.spec.restartPolicy = trimmed.split(':')[1].trim() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (currentContainer) { | ||
| manifest.spec.containers.push(currentContainer) | ||
| } | ||
|
|
||
| const missingFields = requiredFields.filter(field => !foundFields.has(field)) | ||
| if (missingFields.length > 0) { | ||
| throw new Error(`Missing required fields: ${missingFields.join(', ')}`) | ||
| } | ||
|
|
||
| if (manifest.kind !== 'Pod') { | ||
| throw new Error('Kind must be "Pod"') | ||
| } | ||
|
|
||
| if (!manifest.metadata.name) { | ||
| throw new Error('Missing required field: metadata.name') | ||
| } | ||
|
|
||
| if (!manifest.spec.containers || manifest.spec.containers.length === 0) { | ||
| throw new Error('Pod spec must include at least one container') | ||
| } | ||
|
|
||
| return manifest | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually parsing YAML using string splitting and line-by-line processing is extremely brittle and a significant security and correctness risk. It can easily fail with valid but differently formatted YAML (e.g., different indentation, comments, ordering).
Please use a robust, well-tested library like js-yaml to parse the YAML string. You will need to add it as a dependency.
| const parseYamlToManifest = (yamlString: string) => { | ||
| const lines = yamlString.trim().split('\n') | ||
| const manifest: any = { | ||
| apiVersion: '', | ||
| kind: '', | ||
| metadata: { name: '' }, | ||
| spec: {} | ||
| } | ||
|
|
||
| const requiredFields = ['apiVersion', 'kind', 'metadata', 'spec'] | ||
| const foundFields = new Set<string>() | ||
|
|
||
| for (const line of lines) { | ||
| const trimmed = line.trim() | ||
| if (!trimmed || trimmed.startsWith('#')) continue | ||
|
|
||
| // Check for required top-level fields | ||
| for (const field of requiredFields) { | ||
| if (trimmed.startsWith(`${field}:`)) { | ||
| foundFields.add(field) | ||
| if (field === 'apiVersion' || field === 'kind') { | ||
| manifest[field] = trimmed.split(':')[1].trim() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract metadata.name | ||
| if (trimmed.startsWith('name:') && foundFields.has('metadata')) { | ||
| manifest.metadata.name = trimmed.split(':')[1].trim() | ||
| } | ||
| } | ||
|
|
||
| // Validate required fields | ||
| const missingFields = requiredFields.filter(field => !foundFields.has(field)) | ||
| if (missingFields.length > 0) { | ||
| throw new Error(`Missing required fields: ${missingFields.join(', ')}`) | ||
| } | ||
|
|
||
| if (manifest.kind !== 'Queue') { | ||
| throw new Error('Kind must be "Queue"') | ||
| } | ||
|
|
||
| if (!manifest.metadata.name) { | ||
| throw new Error('Missing required field: metadata.name') | ||
| } | ||
|
|
||
| return manifest | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/web/src/app/page.tsx
Outdated
| <div className="flex"> | ||
| <Sidebar /> | ||
| <main className="w-full flex-1 overflow-hidden"> | ||
| <ScrollArea className="h-[calc(100dvh)]"> | ||
| <section className="container grid items-center gap-6 pb-6 pt-12"> | ||
| <div className="flex max-w-[980px] flex-col items-start gap-2 mx-auto"> | ||
| <h1 className="text-xl font-semibold text-purple text-center"> | ||
| Dashboard | ||
| </h1> | ||
| </div> | ||
| </section> | ||
| <hr className="max-w-x" /> | ||
| <DashboardUtils /> | ||
| </ScrollArea> | ||
| </main> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page duplicates the layout structure from apps/web/src/app/(dashboard)/layout.tsx by including its own <Sidebar /> and <main> tag. This will result in a nested layout with two sidebars on the home page.
To fix this, this file should be moved to apps/web/src/app/(dashboard)/page.tsx. This will make it part of the dashboard route group, and the correct layout will be applied automatically. After moving the file, you can remove the redundant layout elements.
<ScrollArea className="h-[calc(100dvh)]">
<section className="container grid items-center gap-6 pb-6 pt-12">
<div className="flex max-w-[980px] flex-col items-start gap-2 mx-auto">
<h1 className="text-xl font-semibold text-purple text-center">
Dashboard
</h1>
</div>
</section>
<hr className="max-w-x" />
<DashboardUtils />
</ScrollArea>| const queuesQuery = trpc.queueRouter.getAllQueues.useQuery( | ||
| undefined, | ||
| { | ||
| onError: (err) => { | ||
| console.error("Error fetching queues:", err); | ||
| setError(`Queues API error: ${err.message}`); | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let response = await k8sApi.listClusterCustomObject({ | ||
| group: "batch.volcano.sh", | ||
| version: "v1alpha1", | ||
| plural: "jobs", | ||
| pretty: "true", | ||
| }); | ||
|
|
||
|
|
||
| let filteredJobs = response.items || []; | ||
|
|
||
| const startIndex = (page - 1) * pageSize; | ||
| const endIndex = startIndex + pageSize; | ||
| const paginatedJobs = filteredJobs.slice(startIndex, endIndex); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of server-side pagination fetches all jobs from the Kubernetes API and then slices the array. This is inefficient and will not scale well with a large number of jobs, potentially causing performance issues and high memory usage on the server.
The Kubernetes API supports pagination via the limit and continue parameters. You should leverage this for true server-side pagination. This comment also applies to fetchQueues and fetchPods in this file.
|
|
||
| useEffect(() => { | ||
| if (podsQuery.data) { | ||
| console.log(podsQuery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/web/package.json
Outdated
| "version": "0.1.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "dev": "next dev ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| useEffect(() => { | ||
| if (queuesQuery.data) { | ||
| console.log(queuesQuery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export const Icons = { | ||
| dashboard: HomeIcon, | ||
| Cloud: Cloud, | ||
| Notepad: NotepadText, | ||
| Waypoint: Waypoints | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys in the Icons object have inconsistent casing (dashboard vs. Cloud, Notepad, Waypoint). For better maintainability and predictability, it's best to use a consistent casing scheme, such as all lowercase.
| export const Icons = { | |
| dashboard: HomeIcon, | |
| Cloud: Cloud, | |
| Notepad: NotepadText, | |
| Waypoint: Waypoints | |
| } | |
| export const Icons = { | |
| dashboard: HomeIcon, | |
| cloud: Cloud, | |
| notepad: NotepadText, | |
| waypoint: Waypoints | |
| } |
| export interface NavItem { | ||
| title: string; | ||
| icon?: keyof typeof Icons; | ||
| href: string; | ||
| disable?: boolean; | ||
| label: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label property in the NavItem interface appears to be redundant, as its value is always the same as the title property in the navItems array. Removing it would simplify the data structure and reduce duplication.
export interface NavItem {
title: string;
icon?: keyof typeof Icons;
href: string;
disable?: boolean;
}|
Also please fix the CI @de6p , and can we build the image to test now? I don't hope I meet some building errors when I want to test it |
|
fixing it |
…ubernetes deployment configuration Signed-off-by: Deep <[email protected]>
…ld tasks, and refactor TRPC context and router logic Signed-off-by: Deep <[email protected]>
… process; remove outdated formatting check workflow Signed-off-by: Deep <[email protected]>
…dency, and refactor navigation icons for consistency Signed-off-by: Deep <[email protected]>
|
I’ve fixed all the requested changes; we can merge it now. |
|
Please check the gemini review comments and resolve the code conflict first. |
|
@Monokaix I fixed all the comments . |
Please also resolve the code conflicts: ) |
Signed-off-by: Deep <[email protected]>
|
fixed the merge conflicts. |
|
Should also update https://github.com/volcano-sh/dashboard/blob/main/CONTRIBUTING.md as the docker build script changed. |
|
Why deleted test.yaml and build.yaml in workflow? |
|
Want to know if this is still a front-end and back-end separation architecture? |
I deleted test.yaml and build.yaml because we now have a single ci.yaml workflow that runs linting, type-checking, and build steps together. This makes our CI setup simpler to maintain and ensures all checks run consistently in one place |
No, this is not a traditional front-end/back-end separation architecture. Instead, we are using Next.js with tRPC, which results in a tightly coupled setup. This means the front-end and back-end are more integrated, allowing us to share types, ensure end-to-end type safety, and speed up development. However, it differs from a separated architecture where front-end and back-end are completely decoupled services. |
|
OK, please solve other issues. |
|
It worked when I changed the dockerfile to |
Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
Signed-off-by: Deep <[email protected]>
|
All the requested changes have been completed. |










I have completely revamped the Volcano dashboard by migrating it to a modern architecture powered by Next.js, tRPC, shadcn/ui, and Turborepo. With this upgrade, the dashboard now delivers 5 to 10 times better performance compared to the previous version. The new user interface is blazing fast, visually stunning, and highly responsive. Additionally, Turborepo significantly improves the development workflow by enabling efficient monorepo management and faster build times.