[Feature][kubectl-plugin] Implement kubectl ray session #2298

MortalHappiness · 2024-08-09T20:20:05Z

Why are these changes needed?

This PR implements the Ray session part in this REP.

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 :(

MortalHappiness · 2024-08-09T20:29:08Z

@chiayi @andrewsykim @kevin85421 PTAL

In this draft PR I only implemented the port-forwarding part of the Ray Dashboard. I use the kubectl port-forward command directly. But there are some drawbacks by using kubectl port-forward directly:

We cannot print message after the port-forwarding is created and the kubectl port-forward command is still running.
By using kubectl port-forward <head-pod-svc> 0:8265, a random port on localhost will be port-forwarded, but it's hard to get this port number to print it to stdout.

If we don't use kubectl port-forward, we need to implement the port-forwarding using client-go ourselves, like the following file does.

https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/portforward/portforward.go

What do you think?

andrewsykim · 2024-09-03T19:55:57Z

I think usnig client-go portforward library makes sense. Can you share the output you see when you run kubectl ray cluster session?

andrewsykim · 2024-09-03T19:56:52Z

kubectl-plugin/pkg/cmd/cluster/cluster_session.go

+	rayHeadSrc := rayHeadSvcs.Items[0]
+
+	portForwardCmd := portforward.NewCmdPortForward(factory, *options.ioStreams)
+	portForwardCmd.SetArgs([]string{"service/" + rayHeadSrc.Name, "0:8265"})


Another possibility instead of using 0:8265 is to default the local port to 8265 and make it configurable?

andrewsykim · 2024-09-03T19:57:44Z

kubectl-plugin/pkg/cmd/cluster/cluster_session.go

+	rayHeadSrc := rayHeadSvcs.Items[0]
+
+	portForwardCmd := portforward.NewCmdPortForward(factory, *options.ioStreams)
+	portForwardCmd.SetArgs([]string{"service/" + rayHeadSrc.Name, "0:8265"})


Also consider port-forwarding the client port at 10001?

MortalHappiness · 2024-09-04T16:27:31Z

@andrewsykim The above is the output of the command. Currently I hard-coded the port to 8265 and 10001. We can make these 2 ports configurable if needed in the future.

andrewsykim · 2024-09-04T15:21:33Z

kubectl-plugin/pkg/cmd/cluster/cluster_session.go

+	portForwardCmd := portforward.NewCmdPortForward(factory, *options.ioStreams)
+	portForwardCmd.SetArgs([]string{"service/" + rayHeadSrc.Name, "0:8265"})
+
+	if err := portForwardCmd.ExecuteContext(ctx); err != nil {


Run this as a goroutine with a stop channel?

Are there any benefits to run it as a goroutine?

Feel free to ignore, I thought the stop channel would be necessary to handle stop signal but looks like it's already handled by client-go

andrewsykim · 2024-09-04T16:36:27Z

kubectl-plugin/pkg/cmd/session/session.go

+
+	fmt.Printf("Ray Dashboard: http://localhost:%d\nRay Interactive Client: http://localhost:%d\n\n", DASHBOARD_PORT, CLIENT_PORT)
+
+	if err := portForwardCmd.ExecuteContext(ctx); err != nil {


Will interrupt signal work here? (^C)

Yes. You can see from the screenshot above that the last line is ^C.

thanks, I didnt' see the terminal prompt after so it wasn't clear if it actually stopped the program

Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>

MortalHappiness · 2024-09-05T17:22:42Z

@andrewsykim @kevin85421 I've added tests and marked this PR as ready for review. Thanks.

andrewsykim

LGTM, @chiayi can you review this too please?

chiayi

Just one thing I want to clear up but other than that LGTM!

chiayi · 2024-09-09T21:51:11Z

kubectl-plugin/pkg/cmd/session/session.go

+		return err
+	}
+
+	portForwardCmd := portforward.NewCmdPortForward(factory, *options.ioStreams)


@andrewsykim @MortalHappiness Want to make sure I'm on the same page, since I know there were some discussion on the port forwarding. Is the plan to currently use kubectl port forward since it's more straightfoward and change to client-go in the future for more configurability?

Yes, now in #2298 I use the kubectl port-forward directly by calling ExecuteContext (from cobra package).

But I don't know whether we want to change to client-go in the future.

https://pkg.go.dev/github.com/spf13/cobra#Command.ExecuteContext

kevin85421

Merge this PR because @andrewsykim has already approved the PR.

kevin85421 added the 1.3.0 label Aug 12, 2024

andrewsykim reviewed Sep 3, 2024

View reviewed changes

MortalHappiness force-pushed the feature/ray-kubectl-session branch from 0b23893 to ac6f6fb Compare September 4, 2024 16:23

MortalHappiness changed the title ~~[Feature][kubectl-plugin] Implement kubectl ray cluster session~~ [Feature][kubectl-plugin] Implement kubectl ray session Sep 4, 2024

andrewsykim reviewed Sep 4, 2024

View reviewed changes

[Feature][kubectl-plugin] Implement kubectl ray session

37f133e

Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>

MortalHappiness force-pushed the feature/ray-kubectl-session branch from ac6f6fb to 37f133e Compare September 5, 2024 16:56

MortalHappiness marked this pull request as ready for review September 5, 2024 16:57

andrewsykim reviewed Sep 9, 2024

View reviewed changes

chiayi reviewed Sep 9, 2024

View reviewed changes

kevin85421 approved these changes Sep 10, 2024

View reviewed changes

kevin85421 merged commit 22c2b45 into ray-project:master Sep 10, 2024
27 checks passed

MortalHappiness mentioned this pull request Sep 13, 2024

[Feat][Kubectl-Plugin] Implement kubectl session for RayJob and RayService #2379

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature][kubectl-plugin] Implement kubectl ray session #2298

[Feature][kubectl-plugin] Implement kubectl ray session #2298

MortalHappiness commented Aug 9, 2024 •

edited

Loading

MortalHappiness commented Aug 9, 2024

andrewsykim commented Sep 3, 2024

andrewsykim Sep 3, 2024

andrewsykim Sep 3, 2024

MortalHappiness commented Sep 4, 2024

andrewsykim Sep 4, 2024

MortalHappiness Sep 4, 2024

andrewsykim Sep 4, 2024 •

edited

Loading

andrewsykim Sep 4, 2024

MortalHappiness Sep 4, 2024

andrewsykim Sep 4, 2024

MortalHappiness commented Sep 5, 2024

andrewsykim left a comment

chiayi left a comment

chiayi Sep 9, 2024

MortalHappiness Sep 9, 2024 •

edited

Loading

kevin85421 left a comment


		fmt.Printf("Ray Dashboard: http://localhost:%d\nRay Interactive Client: http://localhost:%d\n\n", DASHBOARD_PORT, CLIENT_PORT)

		if err := portForwardCmd.ExecuteContext(ctx); err != nil {

[Feature][kubectl-plugin] Implement kubectl ray session #2298

[Feature][kubectl-plugin] Implement kubectl ray session #2298

Conversation

MortalHappiness commented Aug 9, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

MortalHappiness commented Aug 9, 2024

andrewsykim commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness commented Sep 5, 2024

andrewsykim left a comment

Choose a reason for hiding this comment

chiayi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

MortalHappiness commented Aug 9, 2024 •

edited

Loading

andrewsykim Sep 4, 2024 •

edited

Loading

MortalHappiness Sep 9, 2024 •

edited

Loading