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] When CloneSet volumeClaimTemplates changed, Pod must ReCreate update #1561

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

ABNER-1
Copy link
Member

@ABNER-1 ABNER-1 commented Apr 3, 2024

Ⅰ. Describe what this PR does

Implemented "When volumeClaimTemplates are modified, the pod must ReCreate update"

  1. If both the volumeClaimTemplates spec and the image are modified -> recreate.
  2. If only the volumeClaimTemplates spec is modified -> no changes util next template spec changes
  3. next modification of the image or other configurations -> recreate update
  4. To prevent unnecessary pod recreate update, the initial upgrade of CloneSets that previously utilized the volumeClaimTemplates feature will bypass the comparison of historical revisions lacking hash values after the Kruise upgrade.
    • Abandoned the implementation of calculating the hash from pod volumes and existing PVCs: some PVCs have inherent fields that do not exist in volumeClaimTemplates, leading to calculated hash values that are inconsistent with the hash values calculated based on volumeClaimTemplates.

Corner case in 4

  1. Before the Kruise upgrade, CloneSet with image 1 and vcTemplates size 1Mi.
  2. After the Kruise upgrade, an update was applied to use image 2 and vcTemplate size 10Mi. However, due to the absence of hash values for historical revisions, this update will not trigger a recreation, and the actual pod will be like image 2 and vcTemplate size 1Mi. The revision will incorrectly record the hash value for the 10Mi size. Future updates in this situation need to be cautious.
  3. It is recommended not to modify the vcTemplate during the first update after the Kruise upgrade.

detail in #1536

Ⅱ. Does this pull request fix one issue?

detail in #1536
fixes #1536

Ⅲ. Describe how to verify it

detail case in #1536

Main test cases:

  1. image 1, vctemplate size of 1MB.
  2. Image 2, vcTemplate size of 1MB.
  3. Image 3, vcTemplate size of 1MB. (in function testUpdate)
  4. Image 4, vcTemplate size of 10MB => recreate.
  5. Image 2, vcTemplate size of 10MB => (expected in-place upgrade)
    --- Although the history revision hash is consistent, the vct hash is not, update history revision id and hash value
  6. Image 4, vcTemplate size of 10MB => in-place upgrade --- Update history revision.
  7. Image 4, vcTemplate size of 100MB => (temporarily unchanged) --- Do not generate a new revision.
  8. Image 5, vcTemplate size of 100MB => recreate.

Ⅳ. Special notes for reviews

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.28%. Comparing base (0d0031a) to head (d5e6703).
Report is 25 commits behind head on master.

❗ Current head d5e6703 differs from pull request most recent head 15fab30. Consider uploading reports for the commit 15fab30 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1561      +/-   ##
==========================================
+ Coverage   47.91%   49.28%   +1.36%     
==========================================
  Files         162      164       +2     
  Lines       23491    18811    -4680     
==========================================
- Hits        11256     9271    -1985     
+ Misses      11014     8318    -2696     
- Partials     1221     1222       +1     
Flag Coverage Δ
unittests 49.28% <100.00%> (+1.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ABNER-1 ABNER-1 marked this pull request as ready for review April 3, 2024 07:47
@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch 2 times, most recently from 7849a57 to dd43493 Compare April 7, 2024 07:28
@kruise-bot kruise-bot added size/XXL and removed size/XL size/XL: 500-999 labels Apr 7, 2024
@ABNER-1
Copy link
Member Author

ABNER-1 commented Apr 8, 2024

/retest @kruise-bot

@kruise-bot
Copy link

@ABNER-1: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch from 3dc389f to aedf80d Compare April 8, 2024 04:04
@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch 6 times, most recently from f068dbe to 8aaaf72 Compare April 15, 2024 05:08
@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch from 8aaaf72 to af8d778 Compare April 18, 2024 12:38
@ABNER-1
Copy link
Member Author

ABNER-1 commented Apr 26, 2024

I will add a feature gate about this feature, and set default value to false.

@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch from df29123 to d5e6703 Compare April 26, 2024 15:00
…in cloneset e2e case

ignore vct hash changes when inplace-only update strategy type
add feature gate and test both case in ut

Signed-off-by: Abner-1 <Abner199709@gmail.com>
@ABNER-1 ABNER-1 force-pushed the recreate_pod_when_vct_changed branch from d5e6703 to 15fab30 Compare April 29, 2024 11:15
@ABNER-1
Copy link
Member Author

ABNER-1 commented Apr 29, 2024

squash commits and retest

Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@zmberg
Copy link
Member

zmberg commented Apr 30, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 879777b into openkruise:master Apr 30, 2024
32 checks passed
@HURUIZHE
Copy link

HURUIZHE commented Jun 24, 2024

I would like to ask a question, since the VolumeClaimTemplate is not stored in the ControllerRevision of the controller, how can I roll it back when I modified VolumeClaimTemplate in cloneSet? And how do I create the correct old instance when I publish @ABNER-1

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

Successfully merging this pull request may close these issues.

[feature request] When CloneSet volumeClaimTemplates changed, Pod must ReCreate update
5 participants