-
Notifications
You must be signed in to change notification settings - Fork 604
Buildkite autoscaler e2e #2199
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
Buildkite autoscaler e2e #2199
Conversation
…package Signed-off-by: Rueian <rueiancsie@gmail.com>
8a2865d
to
23e1e9e
Compare
Signed-off-by: Rueian <rueiancsie@gmail.com>
f98b88a
to
374babf
Compare
… linter happy Signed-off-by: Rueian <rueiancsie@gmail.com>
374babf
to
4879ee1
Compare
@@ -1,4 +1,4 @@ | |||
package e2e | |||
package e2eautoscaler |
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.
Move the raycluster_autoscaler_test.go
to the new e2eautoscaler
package.
) | ||
|
||
//go:embed *.py | ||
var _files embed.FS |
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.
Instead of refactoring and sharing the old code, I copy the necessary code from the test/e2e/support.go
to the new test/e2eautoscaler/support.go
now for easy review with minimum code changes.
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.
Just for my understanding, could you please explain why we need to refactor the code from test/e2e/support.go
?
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 suggest that we should not refactor the code from test/e2e/support.go
but just copy the necessary code instead to keep the changes of this PR smaller and simpler.
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.
@rueian So will there be another PR to do the refactoring?
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.
My first attempt at making those functions shareable also makes them complex and hard to maintain. I could give it another try in a later PR if necessary.
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.
Perhaps I'm missing something, but I'm curious why we can't simply share them
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.
Hi @Yicheng-Lu-llll, here is a preview of making them shareable. The changes are quite large, touching 11 files.
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.
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.
Oh, I see! Personally, I prefer the #2207 approach since only one copy needs to be maintained, but I'm also okay with this approach. Let's have @kevin85421 decide.
- IMG=kuberay/operator:nightly make deploy | ||
- kubectl wait --timeout=90s --for=condition=Available=true deployment -n ray-system kuberay-operator | ||
# Run e2e tests | ||
- KUBERAY_TEST_TIMEOUT_SHORT=1m KUBERAY_TEST_TIMEOUT_MEDIUM=5m KUBERAY_TEST_TIMEOUT_LONG=10m go test -timeout 30m -v ./test/e2eautoscaler |
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.
Why do we need to set KUBERAY_TEST_TIMEOUT_XXXX
?
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.
To keep the same settings of e2e tests on Github Actions:
kuberay/.github/workflows/e2e-tests.yaml
Lines 68 to 72 in 51b64f6
- name: Run e2e tests | |
run: | | |
export KUBERAY_TEST_TIMEOUT_SHORT=1m | |
export KUBERAY_TEST_TIMEOUT_MEDIUM=5m | |
export KUBERAY_TEST_TIMEOUT_LONG=10m |
Why are these changes needed?
Move the autoscaler e2e tests to the buildkite.
Related issue number
Checks