Skip to content

Add test for autoscaler and its desired state #2601

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 5, 2024
Merged

Add test for autoscaler and its desired state #2601

merged 7 commits into from
Dec 5, 2024

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 4, 2024 •
edited by MortalHappiness
Loading

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
Add test for autoscaler and its desired state
7e59297 
Signed-off-by: hjiang <hjiang@anyscale.com>
@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
resolve linter issues
0c0db65 
Signed-off-by: hjiang <hjiang@anyscale.com>
@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
MortalHappiness requested changes Dec 5, 2024
View reviewed changes
@MortalHappiness
Copy link
Member

MortalHappiness commented Dec 5, 2024 •
edited
Loading

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.

@dentiny
simplify python list
432cec7 
Signed-off-by: hjiang <hjiang@anyscale.com>
@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 •
edited
Loading

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
cleanup one wait group
39c94e2 
Signed-off-by: hjiang <hjiang@anyscale.com>
@dentiny
use scale down window
ad6729f 
Signed-off-by: hjiang <hjiang@anyscale.com>
@dentiny
Copy link
Contributor Author

dentiny commented Dec 5, 2024

@MortalHappiness Updated PTAL

MortalHappiness
MortalHappiness requested changes Dec 5, 2024
View reviewed changes
@MortalHappiness
Copy link
Member

BTW please fix linter issue

@dentiny
prefer HaveLen
53b19c3 
Signed-off-by: hjiang <hjiang@anyscale.com>
MortalHappiness
MortalHappiness requested changes Dec 5, 2024
View reviewed changes
@dentiny
fix replica count
00eae96 
Signed-off-by: hjiang <hjiang@anyscale.com>
MortalHappiness
MortalHappiness approved these changes Dec 5, 2024
View reviewed changes
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
Ygnas pushed a commit to Ygnas/kuberay that referenced this pull request Mar 20, 2025
@dentiny @Ygnas
Add test for autoscaler and its desired state (ray-project#2601)
a7b78d4 
* Add test for autoscaler and its desired state

Signed-off-by: hjiang <hjiang@anyscale.com>

* resolve linter issues

Signed-off-by: hjiang <hjiang@anyscale.com>

* simplify python list

Signed-off-by: hjiang <hjiang@anyscale.com>

* cleanup one wait group

Signed-off-by: hjiang <hjiang@anyscale.com>

* use scale down window

Signed-off-by: hjiang <hjiang@anyscale.com>

* prefer HaveLen

Signed-off-by: hjiang <hjiang@anyscale.com>

* fix replica count

Signed-off-by: hjiang <hjiang@anyscale.com>

---------

Signed-off-by: hjiang <hjiang@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MortalHappiness MortalHappiness MortalHappiness approved these changes

@kevin85421 kevin85421 Awaiting requested review from kevin85421

Assignees

@kevin85421 kevin85421

Labels
None yet
Projects
None yet
Milestone
No milestone
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
@dentiny @kevin85421 @MortalHappiness