🐛 Include machines in deleting status when calculate machineset replicas #3434

jzhoucliqr · 2020-07-30T20:17:38Z

What this PR does / why we need it:
This PR fix the issue that MachineDeployment RollingUpdate may launch machines more than (MD.Spec.Replicas + maxSurge). It include machines in deleting status when calculate MachineSet status replicas.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3417

k8s-ci-robot · 2020-07-30T20:17:46Z

Hi @jzhoucliqr. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

controllers/machineset_controller.go

detiber · 2020-07-30T20:45:16Z

controllers/mdutil/util.go

@@ -521,7 +536,7 @@ func NewMSNewReplicas(deployment *clusterv1.MachineDeployment, allMSs []*cluster
 			return 0, err
 		}
 		// Find the total number of machines
-		currentMachineCount := GetReplicaCountForMachineSets(allMSs)
+		currentMachineCount := GetTotalReplicaCountForMachineSets(allMSs)


If we can assume that a MachineSet's status.replicas includes deleting or not yet ready Machines, then it seems like it should be okay to use GetActualReplicaCountForMachineSets rather than needing to take the max here.

The max is for the case that, for two machinesets, one spec=0,status=2, the other spec=1,status=0. Then at this moment total count should be 3 instead of 2.

Gotcha, I'm wondering if we need to avoid increasing the spec for the new machineSet until things have finished ensuring that the old machineSet has finished stabilizing if increasing would exceed the max surge... I guess I'm wondering if this is the correct place to try to do that enforcement, especially since using the max of spec.replicas and status.replicas seems to be more of an approximation of trying to do the right thing vs giving assurance that we are doing the right thing.

I'm trying to understand this. Do you mean we could allow exceed max surge here in MS.spec, and do enforce within MS controller that won't actually create the Machine object if it already exceed surge? If this understanding is correct, I'm not sure how MS controller will be able to do that, because maxSurge and total number of replicas only available in MD.

I think Jason was suggesting if we need a check in the webhook validator to ensure that the status is up to date before allowing any type of resize. Probably worth a different issue though

controllers/mdutil/util.go

controllers/machineset_controller.go

controllers/mdutil/util.go

ncdc · 2020-07-30T20:49:34Z

/ok-to-test

jzhoucliqr · 2020-07-30T21:57:52Z

/test pull-cluster-api-verify

jzhoucliqr · 2020-08-08T04:15:10Z

/retest

controllers/mdutil/util.go

controllers/machineset_controller.go

controllers/mdutil/util.go

jzhoucliqr · 2020-08-20T18:48:13Z

/retest

jzhoucliqr · 2020-08-20T18:49:23Z

controllers/mdutil/util.go

+// Use max(spec.Replicas,status.Replicas) to cover the cases that:
+// 1. Scale up, where spec.Replicas increased but no machine created yet, so spec.Replicas > status.Replicas
+// 2. Scale down, where spec.Replicas decreased but machine not deleted yet, so spec.Replicas < status.Replicas
+func GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) int32 {


does this make it better or worse ..

controllers/mdutil/util.go

vincepri · 2020-08-21T17:50:14Z

LGTM pending squash

jzhoucliqr · 2020-08-21T17:54:23Z

Thanks a lot @vincepri

vincepri

/approve
/assign @ncdc @fabriziopandini

k8s-ci-robot · 2020-08-21T18:35:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~OWNERS~~ [vincepri]

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

vincepri · 2020-09-28T17:26:48Z

/hold cancel

vincepri · 2020-09-28T17:26:52Z

/retest

vincepri · 2020-09-28T17:27:11Z

/assign @ncdc @detiber

vincepri · 2020-09-29T14:27:39Z

/milestone v0.3.10

vincepri · 2020-09-30T13:58:30Z

Moving this to v0.4

/milestone v0.4.0

jzhoucliqr · 2020-10-29T16:34:47Z

@vincepri do you think we if we can merge this in to v0.3.11 ? There is no API change, only minor behavior change.
I'll resolve the conflicts.

vincepri · 2020-11-09T14:53:55Z

@jzhoucliqr Apologies missed your message, let's rebase and merge for v0.4.0 first and get another round of review. If the fix is appropriate, we should backport it to release-0.3 this week.

vincepri · 2020-11-09T14:54:13Z

/assign

jzhoucliqr · 2020-11-10T00:28:46Z

Thanks @vincepri . Rebase done.

ncdc

These comments are super minor nits that are non-blocking if we'd like to get this in without additional revisions, given the PR has been sitting for almost a month (so sorry about this!!!). @detiber did you get all your questions/comments answered & are you ok to proceed with this PR?

controllers/mdutil/util.go

controllers/machineset_controller.go

jzhoucliqr · 2020-12-10T19:08:27Z

/retest

vincepri · 2020-12-15T20:29:36Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2020

k8s-ci-robot requested review from JoelSpeed and vincepri July 30, 2020 20:17

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 30, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2020

jzhoucliqr changed the title ~~🐛 include machines in deleting status when calculate machineset replicas~~ 🐛 Include machines in deleting status when calculate machineset replicas Jul 30, 2020

detiber reviewed Jul 30, 2020

View reviewed changes

ncdc reviewed Jul 30, 2020

View reviewed changes

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/mdutil/util.go Outdated Show resolved Hide resolved

controllers/mdutil/util.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2020

jzhoucliqr force-pushed the ms-rep-count branch from be3034d to adcf5fa Compare August 3, 2020 21:24

vincepri reviewed Aug 18, 2020

View reviewed changes

controllers/mdutil/util.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/mdutil/util.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020

jzhoucliqr force-pushed the ms-rep-count branch from adcf5fa to 276cd4c Compare August 20, 2020 17:43

jzhoucliqr commented Aug 20, 2020

View reviewed changes

jzhoucliqr force-pushed the ms-rep-count branch 2 times, most recently from c899fc4 to babff3f Compare August 20, 2020 23:24

vincepri reviewed Aug 21, 2020

View reviewed changes

controllers/mdutil/util.go Outdated Show resolved Hide resolved

jzhoucliqr force-pushed the ms-rep-count branch from 2fcdc20 to 01a3e3c Compare August 21, 2020 17:52

vincepri approved these changes Aug 21, 2020

View reviewed changes

k8s-ci-robot assigned fabriziopandini and ncdc Aug 21, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2020

k8s-ci-robot assigned detiber Sep 28, 2020

k8s-ci-robot added this to the v0.3.10 milestone Sep 29, 2020

k8s-ci-robot modified the milestones: v0.3.10, v0.4.0 Sep 30, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2020

k8s-ci-robot assigned vincepri Nov 9, 2020

jzhoucliqr force-pushed the ms-rep-count branch from 01a3e3c to 58fb498 Compare November 10, 2020 00:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020

jzhoucliqr force-pushed the ms-rep-count branch from 58fb498 to 5da4031 Compare November 10, 2020 00:43

ncdc reviewed Dec 8, 2020

View reviewed changes

controllers/mdutil/util.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

jzhoucliqr force-pushed the ms-rep-count branch from 5da4031 to 214a31a Compare December 10, 2020 17:24

include machines in deleting status when calculate machineset replicas

bec5e12

jzhoucliqr force-pushed the ms-rep-count branch from 214a31a to bec5e12 Compare December 10, 2020 17:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2020

k8s-ci-robot merged commit 9b261b2 into kubernetes-sigs:master Dec 15, 2020

Levi080513 mentioned this pull request Jul 19, 2023

🐛 Skipping cleanup unhealthy replicas when oldMSReplicas is lower than oldMSAvailableReplicas #9004

Closed

Oats87 mentioned this pull request Aug 29, 2023

MachineSet reconcilation too eager during scale down when using Oldest or Newest deletion policies #9334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Include machines in deleting status when calculate machineset replicas #3434

🐛 Include machines in deleting status when calculate machineset replicas #3434

jzhoucliqr commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

detiber Jul 30, 2020

jzhoucliqr Jul 30, 2020

detiber Jul 30, 2020

jzhoucliqr Jul 30, 2020

vincepri Aug 18, 2020

ncdc commented Jul 30, 2020

jzhoucliqr commented Jul 30, 2020

jzhoucliqr commented Aug 8, 2020

jzhoucliqr commented Aug 20, 2020

jzhoucliqr Aug 20, 2020

vincepri commented Aug 21, 2020

jzhoucliqr commented Aug 21, 2020

vincepri left a comment

k8s-ci-robot commented Aug 21, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 29, 2020

vincepri commented Sep 30, 2020

jzhoucliqr commented Oct 29, 2020

vincepri commented Nov 9, 2020

vincepri commented Nov 9, 2020

jzhoucliqr commented Nov 10, 2020

ncdc left a comment

jzhoucliqr commented Dec 10, 2020

vincepri commented Dec 15, 2020

🐛 Include machines in deleting status when calculate machineset replicas #3434

🐛 Include machines in deleting status when calculate machineset replicas #3434

Conversation

jzhoucliqr commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

detiber Jul 30, 2020

Choose a reason for hiding this comment

jzhoucliqr Jul 30, 2020

Choose a reason for hiding this comment

detiber Jul 30, 2020

Choose a reason for hiding this comment

jzhoucliqr Jul 30, 2020

Choose a reason for hiding this comment

vincepri Aug 18, 2020

Choose a reason for hiding this comment

ncdc commented Jul 30, 2020

jzhoucliqr commented Jul 30, 2020

jzhoucliqr commented Aug 8, 2020

jzhoucliqr commented Aug 20, 2020

jzhoucliqr Aug 20, 2020

Choose a reason for hiding this comment

vincepri commented Aug 21, 2020

jzhoucliqr commented Aug 21, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 21, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 28, 2020

vincepri commented Sep 29, 2020

vincepri commented Sep 30, 2020

jzhoucliqr commented Oct 29, 2020

vincepri commented Nov 9, 2020

vincepri commented Nov 9, 2020

jzhoucliqr commented Nov 10, 2020

ncdc left a comment

Choose a reason for hiding this comment

jzhoucliqr commented Dec 10, 2020

vincepri commented Dec 15, 2020