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

MortalHappiness · 2024-09-13T15:52:16Z

Why are these changes needed?

#2298 only implemented kubectl ray session for RayCluster. This PR implements kubectl session for RayJob and RayService.

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-09-13T15:52:35Z

@kevin85421 @andrewsykim PTAL

andrewsykim · 2024-09-13T17:37:46Z

cc @chiayi

andrewsykim · 2024-09-13T17:43:06Z

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

+
+	sessionExample = templates.Examples(`
+		# Forward local ports to the RayCluster resource
+		kubectl ray session raycluster/my-raycluster


Will this default to raycluster if the resource type is not specified?

No. Now the resource type is required. Do you think it is better to default to RayCluster if the resource type is not specified?

Do you think it is better to default to RayCluster if the resource type is not specified?

Personally, yes I think so. Similar to how kubectl logs defaults the resource type to Pods, I think we should default the resource type to RayCluster

Make senses to me. Updated in e83ee5b

chiayi

LGTM! Thanks for creating the util package as well!

andrewsykim

Minor comment, otherwise LGTM

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

kevin85421

I haven't reviewed the PR, but based on the screenshot, I think we don't need to forward the Ray client port for RayJob and RayService. In addition, could you print the information to specify which K8s service is being forwarded?

MortalHappiness · 2024-09-17T01:30:59Z

@kevin85421 Port-forwarding for RayJob and RayService removed. Also the screenshot is updated.

kevin85421 · 2024-09-17T06:39:31Z

TODO: Use rayservice-sample-serve-svc and rayservice-sample-head-svc instead.

MortalHappiness · 2024-09-17T09:53:53Z

@kevin85421 I find that they are the same because kubectl source code first find the corresponding pod selected by the service.

https://github.com/kubernetes/kubectl/blob/262825a8a665c7cae467dfaa42b63be5a5b8e5a2/pkg/cmd/portforward/portforward.go#L345

Tested by setting breakpoints in IDE (please see the value of obj.ObjectMeta.Name and forwardablePod.ObjectMeta.Name):

Notice that different obj.ObjectMeta.Name results in the same forwardablePod.ObjectMeta.Name.

Therefore I think we can keep the current implementation because execute multiple kubectl port-forward commands parallelly is more complicated.

kevin85421 · 2024-09-17T12:12:29Z

I find that they are the same because kubectl source code first find the corresponding pod selected by the service.

What is the behavior of forwarding rayservice-sample-head-svc and then performing a zero-downtime upgrade? If the session fails after the old RayCluster is torn down, we can use rayservice-sample-raycluster-xxxxx-head-svc instead.

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

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

…ward for RayJob and RayService Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>

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

MortalHappiness · 2024-09-17T12:37:18Z

@kevin85421 After experiment, the kubectl port-forward service/rayservice-sample-serve-svc 8000:8000 command will fail after a zero-downtime upgrade is performed. So actually there is no differences between which service we choose to do the port-forwarding. I've added a comment for the getRayHeadSvcNameByRayService function.

Explanation of the following screenshot:

Run kubectl port-forward service/rayservice-sample-serve-svc 8000:8000
A zero-downtime upgrade for RayService rayservice-sample is performed.
Connection to the pod is lost when a request is sent to the original pod.
We need to execute kubectl port-forward service/rayservice-sample-serve-svc 8000:8000 and everything is fine because a different pod is connected.

kevin85421 · 2024-09-17T21:37:02Z

Note: If we port-forward a K8s service, it will pick a Pod under the hood. If the Pod is killed, the port-forward command will not pick other K8s service endpoints automatically. Instead, we need to run the port-forward command again.

andrewsykim · 2024-09-17T22:41:08Z

Could we run the port forward in a go routine and retry the connection if it fails?

kevin85421 · 2024-09-17T23:19:41Z

Could we run the port forward in a go routine and retry the connection if it fails?

make sense

kevin85421 · 2024-09-17T23:25:18Z

open an issue to track the progress: #2388

MortalHappiness force-pushed the feature/kubectl-session-rayjob-service branch 2 times, most recently from e069d65 to c4b5964 Compare September 13, 2024 16:01

andrewsykim reviewed Sep 13, 2024

View reviewed changes

chiayi approved these changes Sep 16, 2024

View reviewed changes

andrewsykim reviewed Sep 16, 2024

View reviewed changes

kubectl-plugin/pkg/cmd/session/session.go Outdated Show resolved Hide resolved

kubectl-plugin/pkg/cmd/session/session.go Outdated Show resolved Hide resolved

andrewsykim approved these changes Sep 16, 2024

View reviewed changes

kevin85421 reviewed Sep 16, 2024

View reviewed changes

kevin85421 self-assigned this Sep 16, 2024

MortalHappiness force-pushed the feature/kubectl-session-rayjob-service branch from e83ee5b to 0fa67b9 Compare September 17, 2024 01:28

kevin85421 added the 1.3.0 label Sep 17, 2024

MortalHappiness added 4 commits September 17, 2024 20:34

[Feat][Kubectl-Plugin] Implement kubectl session for RayJob and RaySe…

241b50b

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

[Feat][Kubectl-Session] Default to RayCluster if no type specified

563f875

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

[Feat][kubectl-plugin] Update comments and remove Ray client port-for…

b08397f

…ward for RayJob and RayService Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>

[Chore][kubectl-plugin] Add comment to explain the selection for RayS…

12e7abe

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

MortalHappiness force-pushed the feature/kubectl-session-rayjob-service branch from 0fa67b9 to 12e7abe Compare September 17, 2024 12:34

kevin85421 approved these changes Sep 17, 2024

View reviewed changes

kevin85421 merged commit 5d3bceb into ray-project:master Sep 17, 2024
27 checks passed

kevin85421 mentioned this pull request Sep 17, 2024

[Feature][kubectl-plugin] Run port-forward in a goroutine and retry if the connection fails #2388

Closed

2 tasks

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

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

Uh oh!

Conversation

MortalHappiness commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

MortalHappiness commented Sep 13, 2024

Uh oh!

andrewsykim commented Sep 13, 2024

Uh oh!

andrewsykim Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

MortalHappiness Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewsykim Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

MortalHappiness Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

chiayi left a comment

Choose a reason for hiding this comment

Uh oh!

andrewsykim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

MortalHappiness commented Sep 17, 2024

Uh oh!

kevin85421 commented Sep 17, 2024

Uh oh!

MortalHappiness commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin85421 commented Sep 17, 2024

Uh oh!

MortalHappiness commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin85421 commented Sep 17, 2024

Uh oh!

Uh oh!

andrewsykim commented Sep 17, 2024

Uh oh!

kevin85421 commented Sep 17, 2024

Uh oh!

kevin85421 commented Sep 17, 2024

Uh oh!

Uh oh!

MortalHappiness commented Sep 13, 2024 •

edited

Loading

MortalHappiness Sep 14, 2024 •

edited

Loading

MortalHappiness commented Sep 17, 2024 •

edited

Loading

MortalHappiness commented Sep 17, 2024 •

edited

Loading