-
Notifications
You must be signed in to change notification settings - Fork 752
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
docs: add in-place workload vertical scaling proposal #1336
docs: add in-place workload vertical scaling proposal #1336
Conversation
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @LavenderQAQ! It looks like this is your first PR to openkruise/kruise 🎉 |
For the case of vscale failure, my current idea is to start the rollback mechanism, is it necessary to provide users with some configurable interfaces? Because for vscale, rolling back is more like starting another vscale. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1336 +/- ##
==========================================
+ Coverage 48.55% 48.62% +0.06%
==========================================
Files 157 157
Lines 22480 22480
==========================================
+ Hits 10916 10930 +14
+ Misses 10360 10349 -11
+ Partials 1204 1201 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Do I need to add more details to |
1dd1eaf
to
d556c31
Compare
/lgtm |
/intive |
@cubxxw: changing LGTM is restricted to collaborators In response to this:
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. |
/chat |
|
||
##### Enhance Expectation | ||
|
||
Create a new VScaleExpectations for use by different controllers. |
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 not reuse ResourceVersionExpectations
?
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 think there might be some judgment here about the vscale status, many Pods might be in the Proposed or InProgress state, and there seems to be no way to judge only resource_version.
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 think there might be some judgment here about the vscale status, many Pods might be in the Proposed or InProgress state, and there seems to be no way to judge only resource_version.
ok, I got it :)
type expectationDiffs struct { | ||
... | ||
// vscale identifies whether resources have changed | ||
vscale bool |
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.
numVScale int32
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.
Yes, I should change the quantity here. This design is not perfect for the time being, and we need to consider the vertical scaling of some Pods (not all) in the future. Then I'll update it with a commit.
|
||
#### Design Objectives | ||
|
||
- Use vscale as a standalone operation rather than integrated in update. For example, for cloneset, three operations will be improved to scale, vscale, and update. |
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.
Good idea! But we should consider how to avoid to break maxUnavailable
limitation in some scene, for example, users update both resources and images fields, and settings RestartRequired
of vscale.
|
||
- Users only need to modify template in workload to achieve vertical scaling for all managed Pods | ||
- Simple and clear configuration strategy, support grayscale capabilities | ||
- Provides the rollback function, and attempts to return to the previous state when the capacity expansion and shrinkage fails. If the rollback still fails, no further action is required |
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.
Maybe, we should provide more policies. For example. if a vscale proposal is rejected by kubelet (Infeasible), we should allow it to be demote to rebuild upgrade.
|
||
```go | ||
// CloneSetVScaleStrategy defines strategies for pods vscale. | ||
type CloneSetVScaleStrategy struct { |
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.
Maybe, it's better to reuse UpdateStrategy
filed for the following considers:
- vscale also should consider
maxUnavailable
; - vscale also need grayscale capabilities (
partition
) ;
d556c31
to
888d446
Compare
Sorry, there may be some significant design updates, and after trying to implement a demo, I believe that treating workload vertical scaling as part of the update operation may be a better choice. We can discuss this new design in the community meeting. I have a wonderful demo here. |
/hold |
7ed0cf9
to
7bbca45
Compare
/unhold |
/hold |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
60e4bd7
to
9b46a52
Compare
/unhold |
9b46a52
to
0c6aa76
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Signed-off-by: LavenderQAQ <1254297317@qq.com>
0c6aa76
to
f9116e7
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I think the current version has been upgraded to 1.28, so consider reopening this pr? |
Describe what this PR does
Add a proposal for In-place Workload Vertical Scaling.
Fixes #1212