Skip to content
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

[Feature]: Add a new event type FailedToDeleteWorkerPodCollection #2680

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CheyuWu
Copy link
Contributor

@CheyuWu CheyuWu commented Dec 22, 2024

Why are these changes needed?

Currently, we are reusing the FailedToDeleteWorkerPod event type for scenarios where both Delete (requiring the delete verb in Role or ClusterRole) and DeleteAllOf (requiring the deletecollection verb) are involved. However, this reuse may lead to confusion because these operations rely on different verbs and permissions.

To improve clarity and maintainability, we should consider introducing a new event type specific to the DeleteAllOf operation.

Related issue number

Closes #2672

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@CheyuWu
Copy link
Contributor Author

CheyuWu commented Dec 22, 2024

@kevin85421 @MortalHappiness PTAL

@CheyuWu CheyuWu changed the title feat: Add a new event type FailedToDeleteWorkerPodCollection [Feature]: Add a new event type FailedToDeleteWorkerPodCollection Dec 22, 2024
Copy link
Member

@MortalHappiness MortalHappiness left a comment

Choose a reason for hiding this comment

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

Check all places that use FailedToDeleteWorkerPod, and change them to FailedToDeleteWorkerPodCollection if DeleteAllOf is used instead of Delete.

Ref: https://github.com/ray-project/kuberay/pull/2663/files#r1893148428

@CheyuWu
Copy link
Contributor Author

CheyuWu commented Dec 22, 2024

@MortalHappiness I searched for functions related to r.deleteAllPods, but I couldn't find anything specifically related to FailedToDeleteWorkerPod. However, I did find something related to FailedToDeletePod in the r.deleteAllPods function. Therefore, I added an additional type, FailedToDeletePodCollection.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

I will review this after #2663 is merged.

@kevin85421
Copy link
Member

@CheyuWu #2663 is merged. You can rebase with the master branch.

@CheyuWu CheyuWu force-pushed the feat/eventType/FailedToDeleteWorkerPodCollection branch from e101a6b to debd1e1 Compare December 28, 2024 05:24
@CheyuWu
Copy link
Contributor Author

CheyuWu commented Dec 28, 2024

@MortalHappiness @kevin85421 I have checked all of the functions related to r.deleteAllPods and changed it to deleteCollection type
PTAL

Copy link
Member

@MortalHappiness MortalHappiness left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add a new event type FailedToDeleteWorkerPodCollection
3 participants