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

[Feat]: Make sure at least one pod is upgraded if CloneSet Partition != 100% #954

Merged
merged 1 commit into from
Apr 24, 2022

Conversation

veophi
Copy link
Member

@veophi veophi commented Apr 18, 2022

Signed-off-by: veophi vec.g.sun@gmail.com

Ⅰ. Describe what this PR does

Make sure at least one pod is upgraded if CloneSet Partition != 100%:

  • If replicas >= 2 && partition != 100%, at least ONE pod will be upgraded;
  • If replicas >= 1 && partition != 0%, at least ONE pod will keep old revision (same behaviors as before);

Ⅱ. Does this pull request fix one issue?

fixes #953
fixes #926

@kruise-bot kruise-bot requested review from FillZpp and hellolijj April 18, 2022 09:22
@kruise-bot kruise-bot added the size/L size/L: 100-499 label Apr 18, 2022
@veophi veophi force-pushed the cloneset-partition branch from a317e38 to 6242d35 Compare April 18, 2022 09:26
@codecov-commenter
Copy link

Codecov Report

Merging #954 (6242d35) into master (92e437b) will increase coverage by 0.01%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
+ Coverage   49.96%   49.97%   +0.01%     
==========================================
  Files         119      119              
  Lines       11535    11551      +16     
==========================================
+ Hits         5763     5773      +10     
- Misses       4898     4902       +4     
- Partials      874      876       +2     
Flag Coverage Δ
unittests 49.97% <78.94%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...kg/controller/cloneset/sync/cloneset_sync_utils.go 87.50% <75.00%> (-2.13%) ⬇️
.../controller/cloneset/cloneset_predownload_image.go 63.63% <100.00%> (ø)
pkg/controller/cloneset/cloneset_status.go 89.74% <100.00%> (+0.55%) ⬆️
pkg/controller/cloneset/cloneset_controller.go 52.17% <0.00%> (-1.98%) ⬇️
...er/uniteddeployment/uniteddeployment_controller.go 71.92% <0.00%> (+1.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92e437b...6242d35. Read the comment docs.

@veophi veophi force-pushed the cloneset-partition branch 3 times, most recently from 40c1f59 to 328cb5c Compare April 19, 2022 07:19
pkg/controller/cloneset/sync/cloneset_sync_utils.go Outdated Show resolved Hide resolved
pkg/controller/cloneset/sync/cloneset_sync_utils.go Outdated Show resolved Hide resolved
pkg/controller/cloneset/sync/cloneset_sync_utils.go Outdated Show resolved Hide resolved
apis/apps/v1alpha1/cloneset_types.go Outdated Show resolved Hide resolved
@furykerry
Copy link
Member

this patch should also fix #926

@veophi veophi force-pushed the cloneset-partition branch 3 times, most recently from c871885 to a953c9f Compare April 22, 2022 02:31
@@ -101,4 +102,12 @@ func (r *realStatusUpdater) calculateStatus(cs *appsv1alpha1.CloneSet, newStatus
if newStatus.UpdatedReplicas == newStatus.Replicas {
newStatus.CurrentRevision = newStatus.UpdateRevision
}

if newStatus.UpdateRevision == newStatus.CurrentRevision {
newStatus.ExpectedUpdatedReplicas = 0
Copy link
Member

Choose a reason for hiding this comment

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

if UpdateRevision= CurrentRevision, ExpectedUpdatedReplicas=replicas sounds more reasonable to me

@@ -466,6 +466,43 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
},
expectResult: expectationDiffs{},
},
{
name: "update recreate partition=99% with maxSurge=2 (step 1/3)",
Copy link
Member

Choose a reason for hiding this comment

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

plz consider add cases for percentage type maxUnavailable?

@veophi veophi force-pushed the cloneset-partition branch from a953c9f to 84497d6 Compare April 22, 2022 06:31
Signed-off-by: veophi <vec.g.sun@gmail.com>
@veophi veophi force-pushed the cloneset-partition branch from 84497d6 to 84e5e9d Compare April 24, 2022 02:56
@furykerry
Copy link
Member

/lgtm

@furykerry
Copy link
Member

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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 8330213 into openkruise:master Apr 24, 2022
diannaowa pushed a commit to diannaowa/kruise that referenced this pull request Sep 14, 2022
…penkruise#954)

Signed-off-by: veophi <vec.g.sun@gmail.com>
Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants