Make VM extension reconcile async and move VMSS extension into scaleset service #2177

Jont828 · 2022-03-17T23:37:32Z

What type of PR is this?

/kind feature
What this PR does / why we need it: Implementation of an async service for VM extensions as a follow up for #1610 and #1541.

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 #2141

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:

Make VM extension reconcile async and move VMSS extension into scaleset service

azure/services/scalesets/scalesets.go

Jont828 · 2022-03-21T21:51:59Z

/assign @CecileRobertMichon

CecileRobertMichon · 2022-03-29T00:22:20Z

azure/services/vmssextensions/spec.go

+			Settings:           nil,
+			ProtectedSettings:  s.ProtectedSettings,
+		},
+		// TODO: should we include location since it's used in VMExtensions too?


not sure why one has location and not the other, is location required for VM extensions?

The code in VMSS created the parameters w/o a location like this while the VM extensions included it. I kept the original behavior since it's a refactor but I wasn't sure if it was a bug.

extensions[i] = compute.VirtualMachineScaleSetExtension{ Name: &extensionSpec.Name, VirtualMachineScaleSetExtensionProperties: &compute.VirtualMachineScaleSetExtensionProperties{ Publisher: to.StringPtr(extensionSpec.Publisher), Type: to.StringPtr(extensionSpec.Name), TypeHandlerVersion: to.StringPtr(extensionSpec.Version), Settings: nil, ProtectedSettings: extensionSpec.ProtectedSettings, }, }

I'm not sure either. You could try 1) removing Location from VM extensions 2) adding Location to VMSS extensions, and see if either breaks?

I agree it doesn't seem right that they aren't consistent, but it's possible that this is due to the Azure API between VM and VMSS not being consistent (if it's not, let's make it consistent).

Sure I can give that a try. Are VM/VMSS extensions covered in the e2e tests?

Are VM/VMSS extensions covered in the e2e tests?

yes they are, since we create an extension for every single VM and every single VMSS to check kubeadm completion.

from your other comment sounds like there is a discrepancy in the API and we can just get rid of this comment?

CecileRobertMichon · 2022-03-29T00:26:14Z

azure/services/vmssextensions/spec.go

+
+// VMSSExtensionSpec defines the specification for a VM or VMScaleSet extension.
+type VMSSExtensionSpec struct {
+	Name              string


instead of duplicating fields here and in VMExtensionSpec (since they are the same) consider nesting the Azure extension spec in both

type VMSSExtensionSpec struct { azure.ExtensionSpec }

CecileRobertMichon · 2022-03-29T00:27:44Z

azure/types.go

-// ExtensionSpec defines the specification for a VM or VMScaleSet extension.
-type ExtensionSpec struct {
+// BootstrapingExtensionSpec defines the specification for a CAPZ Bootstrapping VM or VMSS extension.
+type BootstrapingExtensionSpec struct {


Why the rename? the fields here could be reused later on for any other type of extension, nothing in here is bootstrapping specific AFAIK so I think the generic naming is appropriate

I renamed it cause that struct is populated only by the GetBootstrappingVMExtension() function here and I thought it would be more clear where it's coming from. Additionally, I wanted to make it clear that that type isn't coming from the services but from types.go and defaults.go. But if we want to use it more generically then we can change it back.

cluster-api-provider-azure/azure/defaults.go

Line 361 in 71a01a4

func GetBootstrappingVMExtension(osType string, cloud string, vmName string) *ExtensionSpec {

CecileRobertMichon · 2022-03-30T18:49:30Z

azure/types.go

@@ -96,7 +96,7 @@ type PrivateDNSLinkSpec struct {
 	LinkName          string
 }

-// ExtensionSpec defines the specification for a VM or VMScaleSet extension.
+// ExtensionSpec defines the specification for a CAPZ Bootstrapping VM or VMSS extension.


nit: revert the comment about CAPZ Bootstrapping (VMSS change is good to keep)

CecileRobertMichon · 2022-03-30T18:50:55Z

azure/services/vmextensions/spec.go

+
+// VMExtensionSpec defines the specification for a VM or VMScaleSet extension.
+type VMExtensionSpec struct {
+	Spec          azure.ExtensionSpec


This could just be:

Suggested change

Spec azure.ExtensionSpec

azure.ExtensionSpec

so you can refer to fields directly, for example VMExtensionSpec.Publisher instead of VMExtensionSpec.Spec.Publisher

Jont828 · 2022-04-01T20:20:02Z

@CecileRobertMichon It looks like the reason we don't have a location in compute.VirtualMachineScaleSetExtension is because it simply doesn't have a location field while compute.VirtualMachineExtension does. So I guess it's something built in to the Azure Go API.

azure/services/vmextensions/client.go

azure/services/vmextensions/vmextensions.go

azure/services/vmextensions/vmextensions_test.go

CecileRobertMichon · 2022-04-04T22:27:48Z

azure/services/vmextensions/vmextensions.go

 			// check the extension status and set the associated conditions.
-			if retErr := s.Scope.SetBootstrapConditions(ctx, to.String(existing.ProvisioningState), extensionSpec.Name); retErr != nil {
+			if retErr := s.Scope.SetBootstrapConditions(ctx, to.String(vmextension.ProvisioningState), extensionSpec.ResourceName()); retErr != nil {
+				// TODO: what precedence should this error have?


SetBootstrapConditions might need a bit of refactoring. Instead of taking the error and using that to set the condition like we're doing for other resources, it's taking the provisioning state of the extension and using that to set the condition, then returning an error if the extension is done creating. Can we change it to always run (even if resultErr is not nil) and use the error to determine what the condition should be set to instead? Similar to what we're doing in UpdatePutStatus

CecileRobertMichon

Overall looking good, there are a few outstanding TODOs that need to be resolved before this can merge and looks like you need to rebase due to a conflict.

Jont828 · 2022-04-12T21:46:24Z

azure/services/scalesets/scalesets.go

 	s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, serviceName)
+	// This also means that the VMSS extensions were successfully installed
+	s.Scope.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, nil)


@CecileRobertMichon I went ahead and added the UpdatePutStatus() call in the scaleset service. However, I'm not sure how we want to set the error message for VMSSExtensions. Do we want to just set it with whatever error comes from s.createVMSS(ctx), s.patchVMSSIfNeeded(ctx, fetchedVMSS), and s.getVirtualMachineScaleSetIfDone(ctx, future)? Or should we try to set specific errors caused by the VMSSExtension (and not necessarily the scaleset as a whole)?

That's a good question... looks like right now we're not setting conditions at all for the scale set itself because it hasn't yet been refactored (it was implemented as async before the async proposal landed so it's not fully following the same patterns). I would say let's leave this for now and add a note to handle both VMSS and VMSS extension conditions when an error occurs.

CecileRobertMichon · 2022-04-15T16:31:46Z

api/v1beta1/conditions_consts.go

@@ -43,7 +43,7 @@ const (
 	// WaitingForBootstrapDataReason used when machine is waiting for bootstrap data to be ready before proceeding.
 	WaitingForBootstrapDataReason = "WaitingForBootstrapData"
 	// BootstrapSucceededCondition reports the result of the execution of the boostrap data on the machine.
-	BootstrapSucceededCondition = "BoostrapSucceeded"
+	BootstrapSucceededCondition clusterv1.ConditionType = "BootstrapSucceeded"


CecileRobertMichon · 2022-04-15T16:37:36Z

azure/services/scalesets/scalesets.go

 	s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, serviceName)
+	// This also means that the VMSS extensions were successfully installed
+	s.Scope.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, nil)


That's a good question... looks like right now we're not setting conditions at all for the scale set itself because it hasn't yet been refactored (it was implemented as async before the async proposal landed so it's not fully following the same patterns). I would say let's leave this for now and add a note to handle both VMSS and VMSS extension conditions when an error occurs.

CecileRobertMichon · 2022-04-15T16:38:12Z

azure/services/scalesets/vmssextension_spec.go

+			Settings:           nil,
+			ProtectedSettings:  s.ProtectedSettings,
+		},
+		// TODO: should we include location since it's used in VMExtensions too?


remove the TODO since you've determined location is not included in the API?

CecileRobertMichon · 2022-04-15T16:39:31Z

build is failing with

exp/controllers/azuremachinepool_reconciler.go:28:2: no required module provides package sigs.k8s.io/cluster-api-provider-azure/azure/services/vmssextensions; to add it:
	go get sigs.k8s.io/cluster-api-provider-azure/azure/services/vmssextensions

CecileRobertMichon

/lgtm
/assign @mboersma

jackfrancis · 2022-04-19T20:08:29Z

Thanks @Jont828!

…et service

k8s-ci-robot · 2022-04-20T22:15:37Z

@Jont828: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`d7ad4a8`	link	false	`/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.

Jont828 · 2022-04-21T16:47:06Z

@CecileRobertMichon Squashed and pushed, should be good to merge!

CecileRobertMichon · 2022-04-21T17:57:16Z

/lgtm
/approve

k8s-ci-robot · 2022-04-21T17:57:27Z

[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 requested review from CecileRobertMichon and jackfrancis March 17, 2022 23:37

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Mar 17, 2022

Jont828 commented Mar 18, 2022

View reviewed changes

azure/services/scalesets/scalesets.go Show resolved Hide resolved

k8s-ci-robot assigned CecileRobertMichon Mar 21, 2022

Jont828 force-pushed the async-vmextensions branch from d51ffa9 to b489ad5 Compare March 28, 2022 20:00

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

Jont828 force-pushed the async-vmextensions branch from b489ad5 to 179e48f Compare March 28, 2022 20:02

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

Jont828 force-pushed the async-vmextensions branch 3 times, most recently from f1281fe to 9891e80 Compare March 28, 2022 23:54

CecileRobertMichon reviewed Mar 29, 2022

View reviewed changes

CecileRobertMichon reviewed Mar 30, 2022

View reviewed changes

Jont828 changed the title ~~[WIP] Make VM extension reconcile async~~ Make VM extension reconcile async Apr 4, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2022

Jont828 force-pushed the async-vmextensions branch from 6dac4e3 to 05653ab Compare April 4, 2022 21:03

Jont828 changed the title ~~Make VM extension reconcile async~~ Make VM extension reconcile async and refactor VMSS extension spec Apr 4, 2022