Improve RayJob InTreeAutoscaling e2e test #8174
Conversation
k8s-ci-robot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hiboyang The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @hiboyang. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
size/L
|
@mimowo @yaroslava-serdiuk this is the PR to improve e2e test for RayJob with autoscaling. |
|
Thank you! I will leave the first review pass to Yaroslava |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| g.Expect(k8sClient.List(ctx, podList, client.InNamespace(ns.Name))).To(gomega.Succeed()) | ||
| // Count pods that have "workers" in their name | ||
| workerPodCount := countWorkerPods(podList) | ||
| g.Expect(workerPodCount).To(gomega.Equal(1), "Expected exactly 5 pods with 'workers' in the name") |
There was a problem hiding this comment.
| g.Expect(workerPodCount).To(gomega.Equal(1), "Expected exactly 5 pods with 'workers' in the name") | |
| g.Expect(workerPodCount).To(gomega.Equal(1), "Expected exactly 1 pods with 'workers' in the name") |
| # run tasks in parallel to trigger autoscaling (scaling up) | ||
| print(ray.get([my_task.remote(i, 10) for i in range(10)])) | ||
| # run tasks in sequence to trigger scaling down | ||
| print([ray.get(my_task.remote(i, 1)) for i in range(40)])`, |
There was a problem hiding this comment.
Is there a reason why you put such high numbers? I'd anticipate a maximum of around 5 being adequate here. If we want to run with 20 or 40 iterations, I suggest we reduce the sleep time accordingly to optimize the test's execution speed.
There was a problem hiding this comment.
The high numbers are to make the rayjob have enough time to scale up and down. Let me tune these numbers to set them a bit lower.
There was a problem hiding this comment.
@yaroslava-serdiuk I updated the tests to make waiting time shorter in rayjob, tried different values multiple times, if setting too short, the test is not stable and may fail randomly. Now get the final values. Would you check?
There was a problem hiding this comment.
Thanks @yaroslava-serdiuk for checking! I updated the PR again due to git conflict. Would you help to approve again?
|
/release-note-none |
k8s-ci-robot
added
release-note-none
|
/retest |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: f63009d2190179cca826c4fdb02319e3de6e04a8
|
k8s-ci-robot
added
the
needs-rebase
Co-authored-by: Yaroslava Serdiuk <yaroslava@google.com>
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
needs-rebase
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This is a follow-up for PR #8082, which adds support for RayJob with autoscaling. This PR improves e2e test to wait for scaling-up/down.
Which issue(s) this PR fixes:
Improve e2e test from PR #8082.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
No