Skip to content

[Kubectl-plugin] ray session --kill-all #3422

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

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

Conversation

troychiu
Copy link
Contributor

@troychiu troychiu commented Apr 18, 2025

Why are these changes needed?

Reference #3232 (comment)
This PR adds an option --kill-all to ray session, which kills all existing ray session.

Example:

$ kubectl ray session -n test rayjob-sample-dj7p5
Forwarding ports to service rayjob-sample-dj7p5-head-svc
Ray Dashboard: http://localhost:8265
Ray Interactive Client: http://localhost:10001

Forwarding from 127.0.0.1:8265 -> 8265
Forwarding from [::1]:8265 -> 8265
Forwarding from 127.0.0.1:10001 -> 10001
Forwarding from [::1]:10001 -> 10001
$ kubectl ray session --kill-all -v 
killAll flag is set, killing all existing Ray sessions...
Found ray session: /Users/troy/.krew/bin/kubectl-ray session -n test rayjob-sample-dj7p5
Killing subprocess with PID 39598
Killing process with PID 39597

Related issue number

N/A

Checks

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

Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
@MortalHappiness
Copy link
Member

Better to use --kill-all flag instead of kill-all subcommand because in the corner case user may creates a RayCluster named kill-all.

Signed-off-by: Troy Chiu <[email protected]>
@troychiu troychiu changed the title [Kubectl-plugin] ray session kill-all [Kubectl-plugin] ray session --kill-all Apr 19, 2025
@troychiu
Copy link
Contributor Author

Hi @MortalHappiness can I get a review from you? Thank you.

Comment on lines +120 to +130
if options.killAll {
if len(args) > 0 {
return cmdutil.UsageErrorf(cmd, "accepts no args when killAll flag is set")
}
return nil
}

if len(args) == 0 {
return cmdutil.UsageErrorf(cmd, "killAll flag is not set, but no args provided")
}

Copy link
Member

Choose a reason for hiding this comment

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

Create another function called Validate and put the validation log there. You can use other files for reference. The order is Complete, Validate, Run.

@@ -216,3 +247,45 @@ func (options *SessionOptions) Run(ctx context.Context, factory cmdutil.Factory)
wg.Wait()
return nil
}

func killAllRaySessions(ctx context.Context, verbose bool) error {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test for this function?

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.

2 participants