add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy #1332

devigned · 2021-04-22T19:56:46Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces AzureMachinePoolMachine resource to replace AzureMachinePool.Status.Instances in accordance to Azure Machine Pool Machines Proposal.

This also introduces the machine pool rolling upgrade strategy which provides, MaxSurge, MaxUnavailable, and DeletePolicy modeled after machine deployments.

Functional changes:

Ability to delete individual AzureMachinePoolMachines
MaxSurge to surge machine pools upon upgrade of the machine pool template
MaxUnavailable to ensure a minimum number of machines are available at any time
DeletePolicy to enable preference on which nodes to delete, Oldest, Newest, Random
Fully async reconciliation of AzureMachinePool resources

Non-goals of the PR (follow up PRs):

implement cordon and drain per Azure machine pool machine
implement instance update rather than delete / re-create

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy.

devigned · 2021-04-23T20:54:29Z

/retest

CecileRobertMichon

I started reviewing but I'm having a hard time following all the different changes... It would greatly help review if you were able to split out any unrelated improvements to other PRs and structure the changes in this PR in multiple commits.

api/v1alpha4/conditions_consts.go

azure/scope/machinepool.go

exp/api/v1alpha4/azuremachinepool_types.go

CecileRobertMichon

still working through reviewing all the files... thanks for splitting up commits

pkg/coalescing/mocks/doc.go

test/e2e/helpers.go

api/v1alpha4/conditions_consts.go

azure/scope/machinepool.go

CecileRobertMichon · 2021-05-04T22:53:00Z

azure/types.go

+}
+
+// ProviderID returns the K8s provider ID for the VMSS instance
+func (vm VMSSVM) ProviderID() string {


does this stuff really belong in here?

I'm happy to put them where ever. They definitely didn't belong in the ./exp/api where they were before.

I do think they belong here as they are basically the spec structs which have yet to be refactored for the exp controllers.

azure/types.go

nader-ziada · 2021-06-09T13:51:21Z

@nader-ziada and @CecileRobertMichon is there anything else you'd like to see changed in this PR?

lgtm

CecileRobertMichon · 2021-06-09T16:24:29Z

I'll take another pass by EOD

api/v1alpha4/types.go

azure/converters/vmss.go

azure/errors.go

exp/controllers/azuremachinepoolmachine_controller.go

CecileRobertMichon · 2021-06-09T18:25:13Z

main.go

-	if err = controllers.NewAzureMachineReconciler(
-		mgr.GetClient(),
-		ctrl.Log.WithName("controllers").WithName("AzureMachine"),
+	if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil {


is it normal that readyz check takes the same args as healthz check?

I agree. I was just keeping what is already in HEAD.

cluster-api-provider-azure/main.go

Lines 431 to 434 in bd90477

if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil {

setupLog.Error(err, "unable to create ready check")

os.Exit(1)

}

Oddly, the same in CAPI:
https://github.com/kubernetes-sigs/cluster-api/blob/45a0abb58cf1bcd65c9b35b0f7e189ac2cae6f0a/main.go#L217-L220

pkg/coalescing/reconciler.go

test/e2e/config/azure-dev.yaml

CecileRobertMichon · 2021-06-09T18:29:18Z

templates/cluster-template-machinepool-system-assigned-identity.yaml

@@ -162,6 +162,12 @@ metadata:
 spec:
  identity: SystemAssigned
  location: ${AZURE_LOCATION}
+  strategy:


are these required changes to the templates or are they illustrative / used as examples? I don't see any docs updates or additions in this PR, what parts of the PR do you think are user-facing changes that need documentation updates?

These are not required. I only added them for illustrative purposes.

The item that needs documentation is the rollout and delete policies. I'll create an issue to address rather than adding to an already enormous PR.

#1427

…nd deletepolicy

CecileRobertMichon · 2021-06-09T21:35:27Z

test failed because of quota issues in centralus: #1428

CecileRobertMichon · 2021-06-09T22:30:35Z

this fixes #819 and #1361, correct?

CecileRobertMichon · 2021-06-09T22:33:02Z

/retest

CecileRobertMichon · 2021-06-09T22:34:08Z

/lgtm

and, given @nader-ziada's comment above

/approve

/pony

k8s-ci-robot · 2021-06-09T22:34:13Z

@CecileRobertMichon:

In response to this:

/lgtm

and, given @nader-ziada's comment above

/approve

/pony

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.

k8s-ci-robot · 2021-06-09T22:34:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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~~ [CecileRobertMichon]

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

k8s-ci-robot · 2021-06-09T22:34:19Z

@devigned: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`f96fba5`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

devigned · 2021-06-09T22:46:07Z

this fixes #819 and #1361, correct?

#1361, yes. But #819, no, not until cordon and drain are added to the AzureMachinePoolMachine delete.

k8s-ci-robot requested review from CecileRobertMichon and juan-lee April 22, 2021 19:57

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Apr 22, 2021

devigned changed the title ~~add AzureMachinePoolMachine resource to migrate AzureMachinePool~~ add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy Apr 22, 2021

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

CecileRobertMichon reviewed Apr 28, 2021

View reviewed changes

devigned force-pushed the surge-take2 branch from a1dc202 to c101888 Compare May 4, 2021 14:43

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

devigned force-pushed the surge-take2 branch 5 times, most recently from 6409b27 to 1c493b9 Compare May 4, 2021 15:20

devigned mentioned this pull request May 4, 2021

Added support for Azure Bastion. #1300

Merged

3 tasks

devigned force-pushed the surge-take2 branch 2 times, most recently from 4cdbbd0 to 81f945b Compare May 4, 2021 19:02

CecileRobertMichon reviewed May 4, 2021

View reviewed changes

devigned mentioned this pull request May 5, 2021

do not conversion-gen v1alpha3.OSDisk due to warning message non-determinism #1368

Merged

3 tasks

devigned force-pushed the surge-take2 branch from 0c3cd76 to eff8a92 Compare May 6, 2021 01:03

devigned mentioned this pull request May 6, 2021

fix: wait for overprovisioned nodes to be removed from MachinePool #1367

Closed

3 tasks

devigned force-pushed the surge-take2 branch 5 times, most recently from 5fb9099 to a9b1a62 Compare May 6, 2021 13:12

CecileRobertMichon reviewed Jun 9, 2021

View reviewed changes

devigned mentioned this pull request Jun 9, 2021

Add documentation for AzureMachinePool.Spec.Strategy #1427

Closed

devigned force-pushed the surge-take2 branch 3 times, most recently from ff4f6ad to f26e63d Compare June 9, 2021 20:02

devigned added 5 commits June 9, 2021 16:23

add time to live lru cache

cb66502

update machine pool templates to include rolling updates

b2efc45

add coalescing reconciler to enable debouncing of reconciles

b6b38b0

make time sync e2e test more reliable for dynamic infra

7369ccc

add az machine pool rolling upgrades with maxsurge, maxunavailable, a…

f96fba5

…nd deletepolicy

devigned force-pushed the surge-take2 branch from f26e63d to f96fba5 Compare June 9, 2021 20:32

k8s-ci-robot assigned CecileRobertMichon Jun 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2021

CecileRobertMichon linked an issue Jun 9, 2021 that may be closed by this pull request

MachinePool has more worker nodes than the desired value #1361

Closed

k8s-ci-robot merged commit 2cbcf3b into kubernetes-sigs:master Jun 9, 2021

k8s-ci-robot added this to the v0.5.0 milestone Jun 9, 2021

This was referenced Jun 11, 2021

Multitenancy for managed clusters #1386

Merged

Tests fail with MSI not available when some env vars are missing #1399

Closed

CecileRobertMichon mentioned this pull request Sep 16, 2021

Enable coalescing reconciler for more controllers #1691

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy #1332

add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy #1332

devigned commented Apr 22, 2021 •

edited

devigned commented Apr 23, 2021

CecileRobertMichon left a comment

CecileRobertMichon left a comment

CecileRobertMichon May 4, 2021

devigned May 4, 2021

nader-ziada commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon Jun 9, 2021

devigned Jun 9, 2021

devigned Jun 9, 2021

CecileRobertMichon Jun 9, 2021

devigned Jun 9, 2021 •

edited

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021 •

edited

devigned commented Jun 9, 2021

	if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil {
	setupLog.Error(err, "unable to create ready check")
	os.Exit(1)
	}

add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy #1332

add Azure machine pool rolling upgrades with MaxSurge, MaxUnavailable and DeletePolicy #1332

Conversation

devigned commented Apr 22, 2021 • edited

devigned commented Apr 23, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon May 4, 2021

Choose a reason for hiding this comment

devigned May 4, 2021

Choose a reason for hiding this comment

nader-ziada commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon Jun 9, 2021

Choose a reason for hiding this comment

devigned Jun 9, 2021

Choose a reason for hiding this comment

devigned Jun 9, 2021

Choose a reason for hiding this comment

CecileRobertMichon Jun 9, 2021

Choose a reason for hiding this comment

devigned Jun 9, 2021 • edited

Choose a reason for hiding this comment

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

CecileRobertMichon commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021

k8s-ci-robot commented Jun 9, 2021 • edited

devigned commented Jun 9, 2021

devigned commented Apr 22, 2021 •

edited

devigned Jun 9, 2021 •

edited

k8s-ci-robot commented Jun 9, 2021 •

edited