-
Notifications
You must be signed in to change notification settings - Fork 662
[e2e] RayJob Auth Mode E2E #4229
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
Conversation
Signed-off-by: seanlaii <qazwsx0939059006@gmail.com>
f5c06a1 to
fabaf58
Compare
|
Hi @Future-Outlier @rueian , please help review the PR when you have the chance. Thank you! |
| VerifyContainerAuthTokenEnvVars(test, rayCluster, &workerPod, utils.RayContainerIndex) | ||
| } | ||
|
|
||
| // TODO(andrewsykim): add job submission test with and without token once a Ray version with token support is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we open a issue to track this? I think we can find someone to pick it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can also create a PR to add it. The supported Ray version has been released. I will create another PR to address this. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR is fine. We can also probably just remove this given the submission is also exercised with RayJob
| VerifyContainerAuthTokenEnvVars(test, rayCluster, &workerPod, utils.RayContainerIndex) | ||
| } | ||
|
|
||
| // TODO(andrewsykim): add job submission test with and without token once a Ray version with token support is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
400Ping
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides needing to fix the CI, overall LGTM.
| VerifyContainerAuthTokenEnvVars(test, rayCluster, &workerPod, utils.RayContainerIndex) | ||
| } | ||
|
|
||
| // TODO(andrewsykim): add job submission test with and without token once a Ray version with token support is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR is fine. We can also probably just remove this given the submission is also exercised with RayJob
ray-operator/test/support/ray.go
Outdated
| t.T().Helper() | ||
| g := NewWithT(t.T()) | ||
|
|
||
| g.Expect(len(pod.Spec.Containers)).To(BeNumerically(">", containerIndex), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this assertion and thecontainerIndex parameter, and instead just pass in corev1.Container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks for the review! Updated the tests accordingly.
Thanks, fixed the issue. |
andrewsykim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Why are these changes needed?
This PR adds comprehensive E2E tests for Ray authentication token mode to ensure correct auth token propagation across three RayJob submission modes:
Related issue number
Part of #4203
Checks