Skip to content
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

Add test for autoscaler and its desired state #2601

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 4, 2024

Closes #2575

This PR adds a integration for autoscaling, basically spins up a bunch of tasks and check whether max pod replica count has reached during the process.

@dentiny dentiny force-pushed the hjiang/add-test-for-autoscale-desired-state branch 2 times, most recently from 223031d to fe713c5 Compare December 4, 2024 02:31
@dentiny dentiny force-pushed the hjiang/add-test-for-autoscale-desired-state branch from fe713c5 to 7e59297 Compare December 4, 2024 02:39
@kevin85421
Copy link
Member

would you mind fixing the CI errors?

@kevin85421 kevin85421 self-assigned this Dec 4, 2024
@dentiny
Copy link
Contributor Author

dentiny commented Dec 4, 2024

would you mind fixing the CI errors?

Emmm ok, though it seems unrelated to my change

test/e2eautoscaler/support.go:44:30: `newConfigMap` - `name` always receives `"scripts"` (unparam)
func newConfigMap(namespace, name string, options ...option[corev1ac.ConfigMapApplyConfiguration]) *corev1ac.ConfigMapApplyConfiguration {

@dentiny
Copy link
Contributor Author

dentiny commented Dec 4, 2024

test/e2eautoscaler/support.go

Updated.

@kevin85421
Copy link
Member

@MortalHappiness would you mind reviewing this PR?

@MortalHappiness
Copy link
Member

MortalHappiness commented Dec 5, 2024

By the way, test.T().Run is redundant. However, since it is not related to this PR, you can either remove it or keep it. But I think removing it would be simpler.

Signed-off-by: hjiang <[email protected]>
@dentiny
Copy link
Contributor Author

dentiny commented Dec 5, 2024

By the way, test.T().Run is redundant. However, since it is not related to this PR, you can either remove it or keep it. But I think removing it would be simpler.

I'm following the existing coding style

@MortalHappiness
Copy link
Member

MortalHappiness commented Dec 5, 2024

I'm following the existing coding style

Yes, I know. I just found it redundant, so I mentioned that it is not related to this PR. However, removing it might make it easier for you to implement this test. It's up to you whether to remove it or keep it. I'll create a separate PR to remove them though.

@dentiny
Copy link
Contributor Author

dentiny commented Dec 5, 2024

@MortalHappiness Updated PTAL

@MortalHappiness
Copy link
Member

BTW please fix linter issue

Signed-off-by: hjiang <[email protected]>
Signed-off-by: hjiang <[email protected]>
Copy link
Member

@MortalHappiness MortalHappiness left a comment

Choose a reason for hiding this comment

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

LGTM

@dentiny dentiny merged commit aeba37e into ray-project:master Dec 5, 2024
23 checks passed
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.

[Feature] Add e2e tests for inconsistency between worker group's replicas and the number of Pods
3 participants