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

[feature request] Optimize UnitedDeployment replicas settings #1220

Closed
veophi opened this issue Mar 14, 2023 · 9 comments · Fixed by #1247
Closed

[feature request] Optimize UnitedDeployment replicas settings #1220

veophi opened this issue Mar 14, 2023 · 9 comments · Fixed by #1247
Assignees
Labels
Milestone

Comments

@veophi
Copy link
Member

veophi commented Mar 14, 2023

What would you like to be added:
Currently, UnitedDeployment requires the sum of subsets[x].replicas must equal to spec.replicas. Actually, it is very clumsy.

We expect that:

  • If spec.replicas is not set (nil), subsets[i] just follows subsets[i].replicas (should validate all of subsets replicas are not nil);
  • If spec.replicas is set (not nil), we should allow at most ONE subset replicas is nil, and it should equals to spec.replicas - <sum of other subset replicas>;
@veophi
Copy link
Member Author

veophi commented Mar 14, 2023

/label first-good-issue

@kruise-bot
Copy link

@veophi: The label(s) /label first-good-issue cannot be applied. These labels are supported: ``. Is this label configured under labels -> additional_labels or `labels -> restricted_labels` in `plugin.yaml`?

In response to this:

/label first-good-issue

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/test-infra repository.

@veophi veophi added kind/good-first-issue Good for newcomers kind/good-idea Good Idea kind/enhancement New feature or request labels Mar 14, 2023
@y-ykcir
Copy link
Contributor

y-ykcir commented Mar 22, 2023

I want to try this, can you assign this to me?

@y-ykcir
Copy link
Contributor

y-ykcir commented Mar 24, 2023

@veophi Since Subsets[i].replicas can be set to a percentage, if spec.replicas is not set (nil), should we not allow Subsets[i].replicas to be set to a percentage?

@y-ykcir
Copy link
Contributor

y-ykcir commented Mar 28, 2023

@veophi I browsed the code here, and it shows that all subsets which are not set replicas will be averagely allocated by the rest replicas, this means that the current logic allows more than two subset.replicas to be unset. Should we change this logic to only allow at most ONE subset replicas is nil? If so, the code for these average logic may need to be modified. PTAL, thanks.

@veophi
Copy link
Member Author

veophi commented Apr 4, 2023

@veophi Since Subsets[i].replicas can be set to a percentage, if spec.replicas is not set (nil), should we not allow Subsets[i].replicas to be set to a percentage?

No, we should reject this configuration

@veophi
Copy link
Member Author

veophi commented Apr 4, 2023

@veophi I browsed the code here, and it shows that all subsets which are not set replicas will be averagely allocated by the rest replicas, this means that the current logic allows more than two subset.replicas to be unset. Should we change this logic to only allow at most ONE subset replicas is nil? If so, the code for these average logic may need to be modified. PTAL, thanks.

@y-ykcir Sorry, I didn't notice this code before. I think the logic of this code is reasonable and doesn't need any modification. What do you think?

@y-ykcir
Copy link
Contributor

y-ykcir commented Apr 4, 2023

@veophi I browsed the code here, and it shows that all subsets which are not set replicas will be averagely allocated by the rest replicas, this means that the current logic allows more than two subset.replicas to be unset. Should we change this logic to only allow at most ONE subset replicas is nil? If so, the code for these average logic may need to be modified. PTAL, thanks.

@y-ykcir Sorry, I didn't notice this code before. I think the logic of this code is reasonable and doesn't need any modification. What do you think?

@veophi I think if spec.replicas is set, the current logic of averaging may be more reasonable. Additionally, it seems that the original logic did not mention the situation where spec.replicas was not set as mentioned in the issue, but instead defaulted to 1. Maybe we can add optimization for this situation?

@veophi
Copy link
Member Author

veophi commented Apr 7, 2023

I think if spec.replicas is set, the current logic of averaging may be more reasonable. Additionally, it seems that the original logic did not mention the situation where spec.replicas was not set as mentioned in the issue, but instead defaulted to 1. Maybe we can add optimization for this situation?

@y-ykcir I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants