Skip to content

Backend delete functionality with queues#81

Closed
Shrutim1505 wants to merge 4 commits intovolcano-sh:mainfrom
Shrutim1505:backend-update-functionality
Closed

Backend delete functionality with queues#81
Shrutim1505 wants to merge 4 commits intovolcano-sh:mainfrom
Shrutim1505:backend-update-functionality

Conversation

@Shrutim1505
Copy link
Member

@Shrutim1505 Shrutim1505 commented Apr 11, 2025

Description:
This PR introduces the backend implementation for the delete functionality. It includes the necessary route, controller logic, and service integration to handle delete operations securely and efficiently.

Changes Made:

Added DELETE API endpoint in the backend

Implemented controller and service logic for delete operation

Integrated appropriate validations and error handling

Updated documentation (if applicable)

Testing:

Verified delete functionality with valid and invalid inputs

Ensured proper status codes and messages are returned

Confirmed no unintended side effects on other endpoints

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng
You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch from 184c33b to 2c4ed56 Compare April 13, 2025 21:41
@Shrutim1505
Copy link
Member Author

/assign @kevin-wangzefeng

@Shrutim1505 Shrutim1505 changed the title Backend update functionality Backend delete functionality Apr 13, 2025
@Monokaix
Copy link
Member

Hi, what's the difference between this and #74 ?

@Shrutim1505
Copy link
Member Author

Shrutim1505 commented Apr 21, 2025

closed that one @Monokaix , this is the correct implementation of backend

@Shrutim1505
Copy link
Member Author

Shrutim1505 commented Apr 21, 2025

this could be merged after #80

@JesseStutler
Copy link
Member

Need to solve merge conflicts and add more details in the description, besides, add more screenshot of the effect will be better

@Shrutim1505
Copy link
Member Author

This will be resolved after #80

@JesseStutler
Copy link
Member

Also need to solve this PR's conflict and give more details for the description

@Shrutim1505
Copy link
Member Author

Also need to solve this PR's conflict and give more details for the description

@JesseStutler this PR is behind #80 so it would be resolved after merge of 80

@Monokaix
Copy link
Member

Please solve the code conflicts.

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch 2 times, most recently from f26c978 to 611afc9 Compare May 13, 2025 06:51
@Shrutim1505
Copy link
Member Author

@Monokaix
Copy link
Member

Does this pr include delete all resources or just pod and queue?

@Shrutim1505
Copy link
Member Author

@Monokaix it is just for queues, but same logic will be implemented with pods and jobs too

@Shrutim1505
Copy link
Member Author

Shrutim1505 commented May 17, 2025

while restructuring the code i will implement that in jobs and pods too till then we can merge this one

@Shrutim1505 Shrutim1505 changed the title Backend delete functionality Backend delete functionality with queues May 17, 2025
@JesseStutler
Copy link
Member

I will test it today

@JesseStutler
Copy link
Member

JesseStutler commented May 19, 2025

There is no permission for dashboard to delete the queues, you should also add a delete in dashboard's clusterrole, otherwise api-server will reject dashboard's deleting request:

resources:
- queues
verbs:
- get
- list
- watch

image

@Shrutim1505

@JesseStutler
Copy link
Member

Also the error message in the pic is not correct, should be like: Can not delete queue "queue-a", the error is: xxx. And we should not show the CANCEL/DELETE button when we meet errors @Shrutim1505

@Shrutim1505
Copy link
Member Author

Also the error message in the pic is not correct, should be like: Can not delete queue "queue-a", the error is: xxx. And we should not show the CANCEL/DELETE button when we meet errors @Shrutim1505

i will check

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch from 962a789 to ca1123d Compare May 19, 2025 05:14
@Shrutim1505
Copy link
Member Author

hi @JesseStutler I guess u can check it now

@JesseStutler
Copy link
Member

JesseStutler commented May 19, 2025

There is no permission for dashboard to delete the queues, you should also add a delete in dashboard's clusterrole, otherwise api-server will reject dashboard's deleting request:

resources:
- queues
verbs:
- get
- list
- watch

image

@Shrutim1505

@Shrutim1505 You should also add a delete permission for queue in clusterrole

@de6p
Copy link
Contributor

de6p commented May 19, 2025

@Shrutim1505 Error msg can be improved here though.

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch from abf0056 to c277111 Compare May 19, 2025 06:15
@Shrutim1505
Copy link
Member Author

@JesseStutler added

namespace: volcano-system
---

# volcano dashboard cluster role
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should not added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

- get
- list
- watch
- delete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dashboard should not have permission to delete namespace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed @JesseStutler

- get
- list
- watch
- delete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add delete for jobs/pods later? Because your PR only support to delete queues currently

size="sm"
className="rounded-pill px-4 py-2 d-flex align-items-center justify-content-center shadow-sm fw-medium border-2"
onClick={handleRefresh}
onClick={handleRefresh} // This is correct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this comment used for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, i guess too its not necessary

});
}
});
// Delete a Pod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR also support delete pods? But seems that pod doesn't have much error cases such as 4XX? Do we need to support pod deletion in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POD and job deletion would be completed in next PR @JesseStutler

const queueName = name.toLowerCase();

/* ---------- 1. Protect the default queue ---------- */
if (queueName === "default") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should protect root queue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will add this too

} catch (error) {
console.error("Error deleting queue:", error?.body || error);

if (error.statusCode === 403) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question: Why not directly return the error code and error message to the user? Why do we need to judge based on the different types of codes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just printing type of error in the terminal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can directly return the error code and error message from kube-apiserver to the user, I think user who are using volcano and k8s will understand the error message, there is no need to judge here and wrap the message

});
}

if (error.body?.includes("admission webhook")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very strange to judge based on the return value of the body

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JesseStutler When you try to delete a queue, Kubernetes may have a safety‑check (an admission webhook) that says “No, this queue is protected.”
The API’s error message usually contains the words “admission webhook.”

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch 2 times, most recently from 5f4756f to 5c73330 Compare May 19, 2025 08:41

// Disallow deleting protected system queues
if (["root", "default"].includes(queueName)) {
return res.status(400).json({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the return code should be 403

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch from 39d4729 to 68dd27f Compare May 20, 2025 09:42
@Shrutim1505
Copy link
Member Author

@JesseStutler @Monokaix please review

@JesseStutler
Copy link
Member

Please clean your commit message:
image

@JesseStutler
Copy link
Member

JesseStutler commented May 20, 2025

image
The current display effect is still incorrect. If the deletion fails, a dialog should be displayed, showing the error message returned from the kube-apiserver. This dialog can be closed( A × button can be showed). Currently, the floating message "Queue "queue-a" cannot be deleted." has been at the top and has not disappeared. @Shrutim1505

@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch 5 times, most recently from 7643b48 to 7468516 Compare May 20, 2025 13:05
Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

undo formatting of contributing.md

Signed-off-by: Sayan Banerjee <sayanbanerjee2002@gmail.com>

update sidebar ui/ux

Signed-off-by: Karan Yadav <xpresskaran98@gmail.com>

fix deprecated package volcano-sh#46

Signed-off-by: Karan Yadav <xpresskaran98@gmail.com>

final rebased commited changes to code-modularity

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

format checker

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

backend working

Signed-off-by: Sayan Banerjee <sayanbanerjee2002@gmail.com>

refractored code for better UI design

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

refracted ui queues

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

maintain code reusability and improve UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

removed package-lock.json from individual workspaces

Signed-off-by: Sayan Banerjee <sayanbanerjee2002@gmail.com>

undo formatting of contributing.md

Signed-off-by: Sayan Banerjee <sayanbanerjee2002@gmail.com>

update sidebar ui/ux

Signed-off-by: Karan Yadav <xpresskaran98@gmail.com>

fix deprecated package volcano-sh#46

Signed-off-by: Karan Yadav <xpresskaran98@gmail.com>

format checker

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

configure vitest

Signed-off-by: Karan Yadav <xpresskaran98@gmail.com>

feat/trpc: Add support for trpc and improve the api response time

Signed-off-by: Kuldeep <de6p97@gmail.com>

Revert "feat/trpc: Add support for trpc and improve the api response time 5x-10x faster"

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

docker error solve

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

implemented delete and update functionality with all conflicts resolved

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

Add security context configuration

Signed-off-by: jessestutler <chenzicong4@huawei.com>

Frontend changes of Jobs and Pods UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

Frontend updation of the Queues UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

Frontend updation of the Queues UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

modified jobs-pods for the UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

prettier check added

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

edit dialogue box changed

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

pods handle search improved

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

npm run format

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

backend delete queue UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

Frontend updation of the Queues UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

Frontend updation of the Queues UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

modified jobs-pods for the UI

Signed-off-by: Shruti78 <shrutimurthy2103@gmail.com>

backend on updating queue/job status

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

delete functionality working

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

updated missing functionalities

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

 format checker

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

handling error cases

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

delete permission in cluster role

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

refractoring of backend

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

refractoring of backend

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

backend error functionality handling

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>

Delete dialogue on showing error

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>
@Shrutim1505 Shrutim1505 force-pushed the backend-update-functionality branch from 7468516 to ac03d47 Compare May 20, 2025 13:06
@Shrutim1505
Copy link
Member Author

image

Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>
Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>
Signed-off-by: Shrutim1505 <shrutimurthy2103@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants