Skip to content

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

Merged
merged 3 commits into from
Jun 29, 2024
Merged

Conversation

rueian
Copy link
Contributor

@rueian rueian commented Jun 21, 2024

Why are these changes needed?

Move the autoscaler e2e tests to the buildkite.

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests

…package

Signed-off-by: Rueian <rueiancsie@gmail.com>
@rueian rueian changed the title Sep autoscaler e2e Buildkite autoscaler e2e Jun 21, 2024
@rueian rueian force-pushed the sep-autoscaler-e2e branch 2 times, most recently from 8a2865d to 23e1e9e Compare June 21, 2024 16:09
Signed-off-by: Rueian <rueiancsie@gmail.com>
@rueian rueian force-pushed the sep-autoscaler-e2e branch 5 times, most recently from f98b88a to 374babf Compare June 22, 2024 00:23
… linter happy

Signed-off-by: Rueian <rueiancsie@gmail.com>
@rueian rueian force-pushed the sep-autoscaler-e2e branch from 374babf to 4879ee1 Compare June 22, 2024 01:34
@@ -1,4 +1,4 @@
package e2e
package e2eautoscaler
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

@MortalHappiness MortalHappiness Jun 27, 2024

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@rueian rueian Jun 27, 2024

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.

#2207

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.

@rueian rueian marked this pull request as ready for review June 22, 2024 02:30
@kevin85421 kevin85421 self-assigned this Jun 26, 2024
- 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
Copy link
Member

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?

Copy link
Contributor Author

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:

- name: Run e2e tests
run: |
export KUBERAY_TEST_TIMEOUT_SHORT=1m
export KUBERAY_TEST_TIMEOUT_MEDIUM=5m
export KUBERAY_TEST_TIMEOUT_LONG=10m

@kevin85421 kevin85421 merged commit 51b64f6 into ray-project:master Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants