Detect Flex from VMSS orchestration mode

mboersma · mboersma · commit ef1df93df44b · 2023-03-08T09:57:47.000-07:00
diff --git a/azure/converters/vmss.go b/azure/converters/vmss.go
@@ -50,6 +50,7 @@ func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.Vi
 		vmss.Instances = make([]azure.VMSSVM, len(sdkinstances))
 		for i, vm := range sdkinstances {
 			vmss.Instances[i] = *SDKToVMSSVM(vm)
+			vmss.Instances[i].OrchestrationMode = infrav1.OrchestrationModeType(sdkvmss.OrchestrationMode)
 		}
 	}
 
@@ -64,7 +65,7 @@ func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.Vi
 }
 
 // SDKVMToVMSSVM converts an Azure SDK VM to a VMSS VM.
-func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine) *azure.VMSSVM {
+func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine, mode infrav1.OrchestrationModeType) *azure.VMSSVM {
 	instance := azure.VMSSVM{
 		ID: to.String(sdkInstance.ID),
 	}
@@ -92,6 +93,8 @@ func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine) *azure.VMSSVM {
 		instance.AvailabilityZone = to.StringSlice(sdkInstance.Zones)[0]
 	}
 
+	instance.OrchestrationMode = mode
+
 	return &instance
 }
 
diff --git a/azure/converters/vmss_test.go b/azure/converters/vmss_test.go
@@ -350,7 +350,7 @@ func Test_SDKVMToVMSSVM(t *testing.T) {
 		t.Run(c.Name, func(t *testing.T) {
 			t.Parallel()
 			g := gomega.NewGomegaWithT(t)
-			subject := converters.SDKVMToVMSSVM(c.Subject)
+			subject := converters.SDKVMToVMSSVM(c.Subject, "")
 			g.Expect(subject).To(gomega.Equal(c.Expected))
 		})
 	}
diff --git a/azure/scope/machinepool.go b/azure/scope/machinepool.go
@@ -282,7 +282,7 @@ func (m *MachinePoolScope) applyAzureMachinePoolMachines(ctx context.Context) er
 	}
 
 	// determine which machines need to be created to reflect the current state in Azure
-	azureMachinesByProviderID := m.vmssState.InstancesByProviderID()
+	azureMachinesByProviderID := m.vmssState.InstancesByProviderID(m.AzureMachinePool.Spec.OrchestrationMode)
 	for key, val := range azureMachinesByProviderID {
 		if _, ok := existingMachinesByProviderID[key]; !ok {
 			log.V(4).Info("creating AzureMachinePoolMachine", "providerID", key)
diff --git a/azure/scope/machinepoolmachine.go b/azure/scope/machinepoolmachine.go
@@ -163,6 +163,11 @@ func (s *MachinePoolMachineScope) ScaleSetName() string {
 	return s.MachinePoolScope.Name()
 }
 
+// OrchestrationMode is the VMSS orchestration mode, either Uniform or Flexible.
+func (s *MachinePoolMachineScope) OrchestrationMode() infrav1.OrchestrationModeType {
+	return s.AzureMachinePool.Spec.OrchestrationMode
+}
+
 // SetLongRunningOperationState will set the future on the AzureMachinePoolMachine status to allow the resource to continue
 // in the next reconciliation.
 func (s *MachinePoolMachineScope) SetLongRunningOperationState(future *infrav1.Future) {
@@ -259,6 +264,7 @@ func (s *MachinePoolMachineScope) PatchObject(ctx context.Context) error {
 		patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
 			clusterv1.ReadyCondition,
 			clusterv1.MachineNodeHealthyCondition,
+			clusterv1.DrainingSucceededCondition,
 		}})
 }
 
diff --git a/azure/scope/strategies/machinepool_deployments/machinepool_deployment_strategy.go b/azure/scope/strategies/machinepool_deployments/machinepool_deployment_strategy.go
@@ -158,13 +158,7 @@ func (rollingUpdateStrategy rollingUpdateStrategy) SelectMachinesToDelete(ctx co
 		return append(failedMachines, deletingMachines...), nil
 	}
 
-	// if we have failed machines, remove them
-	if len(failedMachines) > 0 {
-		log.Info("failed machines", "desiredReplicaCount", desiredReplicaCount, "maxUnavailable", maxUnavailable, "failedMachines", getProviderIDs(failedMachines))
-		return failedMachines, nil
-	}
-
-	// if we have not yet reached our desired count, don't try to delete anything but failed machines
+	// if we have not yet reached our desired count, don't try to delete anything
 	if len(readyMachines) < int(desiredReplicaCount) {
 		log.Info("not enough ready machines", "desiredReplicaCount", desiredReplicaCount, "readyMachinesCount", len(readyMachines), "machinesByProviderID", len(machinesByProviderID))
 		return []infrav1exp.AzureMachinePoolMachine{}, nil
diff --git a/azure/services/scalesets/scalesets.go b/azure/services/scalesets/scalesets.go
@@ -293,13 +293,7 @@ func (s *Service) patchVMSSIfNeeded(ctx context.Context, infraVMSS *azure.VMSS)
 	}
 
 	hasModelChanges := hasModelModifyingDifferences(infraVMSS, vmss)
-	var isFlex bool
-	for _, instance := range infraVMSS.Instances {
-		if instance.IsFlex() {
-			isFlex = true
-			break
-		}
-	}
+	isFlex := s.Scope.ScaleSetSpec().OrchestrationMode == infrav1.FlexibleOrchestrationMode
 	updated := true
 	if !isFlex {
 		updated = infraVMSS.HasEnoughLatestModelOrNotMixedModel()
diff --git a/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go b/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go
diff --git a/azure/services/scalesetvms/scalesetvms.go b/azure/services/scalesetvms/scalesetvms.go
@@ -42,6 +42,7 @@ type (
 		InstanceID() string
 		ProviderID() string
 		ScaleSetName() string
+		OrchestrationMode() infrav1.OrchestrationModeType
 		SetVMSSVM(vmssvm *azure.VMSSVM)
 	}
 
@@ -77,10 +78,11 @@ func (s *Service) Reconcile(ctx context.Context) error {
 		vmssName      = s.Scope.ScaleSetName()
 		instanceID    = s.Scope.InstanceID()
 		providerID    = s.Scope.ProviderID()
+		isFlex        = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode
 	)
 
 	// Fetch the latest instance or VM data. AzureMachinePoolReconciler handles model mutations.
-	if isFlex(instanceID) {
+	if isFlex {
 		resourceID := strings.TrimPrefix(providerID, azure.ProviderIDPrefix)
 		log.V(4).Info("VMSS is flex", "vmssName", vmssName, "providerID", providerID, "resourceID", resourceID)
 		// Using VMSS Flex, so fetch by resource ID.
@@ -91,7 +93,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 			}
 			return errors.Wrap(err, "failed getting vm")
 		}
-		s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm))
+		s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, infrav1.FlexibleOrchestrationMode))
 		return nil
 	}
 
@@ -116,6 +118,7 @@ func (s *Service) Delete(ctx context.Context) error {
 		vmssName      = s.Scope.ScaleSetName()
 		instanceID    = s.Scope.InstanceID()
 		providerID    = s.Scope.ProviderID()
+		isFlex        = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode
 	)
 
 	ctx, log, done := tele.StartSpanWithLogger(
@@ -127,24 +130,20 @@ func (s *Service) Delete(ctx context.Context) error {
 	)
 	defer done()
 
-	if isFlex(instanceID) {
+	if isFlex {
 		return s.deleteVMSSFlexVM(ctx, strings.TrimPrefix(providerID, azure.ProviderIDPrefix))
 	}
 	return s.deleteVMSSUniformInstance(ctx, resourceGroup, vmssName, instanceID, log)
 }
 
-func isFlex(instanceID string) bool {
-	return instanceID == ""
-}
-
 func (s *Service) deleteVMSSFlexVM(ctx context.Context, resourceID string) error {
 	ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.deleteVMSSFlexVM")
 	defer done()
 
 	defer func() {
 		if vm, err := s.VMClient.GetByID(ctx, resourceID); err == nil && vm.VirtualMachineProperties != nil {
 			log.V(4).Info("vmss vm delete in progress", "state", vm.ProvisioningState)
-			s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm))
+			s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, s.Scope.OrchestrationMode()))
 		}
 	}()
 
diff --git a/azure/services/scalesetvms/scalesetvms_test.go b/azure/services/scalesetvms/scalesetvms_test.go
@@ -150,6 +150,7 @@ func TestService_Reconcile(t *testing.T) {
 			scopeMock.EXPECT().SubscriptionID().Return("subID").AnyTimes()
 			scopeMock.EXPECT().BaseURI().Return("https://localhost/").AnyTimes()
 			scopeMock.EXPECT().Authorizer().Return(nil).AnyTimes()
+			scopeMock.EXPECT().OrchestrationMode().Return(infrav1.UniformOrchestrationMode).AnyTimes()
 
 			service := NewService(scopeMock)
 			service.Client = clientMock
@@ -268,6 +269,7 @@ func TestService_Delete(t *testing.T) {
 			scopeMock.EXPECT().SubscriptionID().Return("subID").AnyTimes()
 			scopeMock.EXPECT().BaseURI().Return("https://localhost/").AnyTimes()
 			scopeMock.EXPECT().Authorizer().Return(nil).AnyTimes()
+			scopeMock.EXPECT().OrchestrationMode().Return(infrav1.UniformOrchestrationMode).AnyTimes()
 
 			service := NewService(scopeMock)
 			service.Client = clientMock
diff --git a/azure/types.go b/azure/types.go
@@ -94,13 +94,14 @@ type ExtensionSpec struct {
 type (
 	// VMSSVM defines a VM in a virtual machine scale set.
 	VMSSVM struct {
-		ID                 string                    `json:"id,omitempty"`
-		InstanceID         string                    `json:"instanceID,omitempty"`
-		Image              infrav1.Image             `json:"image,omitempty"`
-		Name               string                    `json:"name,omitempty"`
-		AvailabilityZone   string                    `json:"availabilityZone,omitempty"`
-		State              infrav1.ProvisioningState `json:"vmState,omitempty"`
-		BootstrappingState infrav1.ProvisioningState `json:"bootstrappingState,omitempty"`
+		ID                 string                        `json:"id,omitempty"`
+		InstanceID         string                        `json:"instanceID,omitempty"`
+		Image              infrav1.Image                 `json:"image,omitempty"`
+		Name               string                        `json:"name,omitempty"`
+		AvailabilityZone   string                        `json:"availabilityZone,omitempty"`
+		State              infrav1.ProvisioningState     `json:"vmState,omitempty"`
+		BootstrappingState infrav1.ProvisioningState     `json:"bootstrappingState,omitempty"`
+		OrchestrationMode  infrav1.OrchestrationModeType `json:"orchestrationMode,omitempty"`
 	}
 
 	// VMSS defines a virtual machine scale set.
@@ -129,9 +130,10 @@ func (vmss VMSS) HasModelChanges(other VMSS) bool {
 }
 
 // InstancesByProviderID returns VMSSVMs by ID.
-func (vmss VMSS) InstancesByProviderID() map[string]VMSSVM {
+func (vmss VMSS) InstancesByProviderID(mode infrav1.OrchestrationModeType) map[string]VMSSVM {
 	instancesByProviderID := make(map[string]VMSSVM, len(vmss.Instances))
 	for _, instance := range vmss.Instances {
+		instance.OrchestrationMode = mode
 		instancesByProviderID[instance.ProviderID()] = instance
 	}
 
@@ -140,7 +142,7 @@ func (vmss VMSS) InstancesByProviderID() map[string]VMSSVM {
 
 // ProviderID returns the K8s provider ID for the VMSS instance.
 func (vm VMSSVM) ProviderID() string {
-	if vm.IsFlex() {
+	if vm.OrchestrationMode == infrav1.FlexibleOrchestrationMode {
 		// ProviderID for Flex scaleset VMs looks like this:
 		// azure:///subscriptions/<sub_id>/resourceGroups/my-cluster/providers/Microsoft.Compute/virtualMachines/my-cluster_1234abcd
 		splitOnSlash := strings.Split(vm.ID, "/")
@@ -153,11 +155,6 @@ func (vm VMSSVM) ProviderID() string {
 	return ProviderIDPrefix + vm.ID
 }
 
-// IsFlex returns true if the VMSS instance is a Flex VM.
-func (vm VMSSVM) IsFlex() bool {
-	return vm.InstanceID == ""
-}
-
 // HasLatestModelAppliedToAll returns true if all VMSS instance have the latest model applied.
 func (vmss VMSS) HasLatestModelAppliedToAll() bool {
 	for _, instance := range vmss.Instances {
diff --git a/azure/types_test.go b/azure/types_test.go
@@ -163,35 +163,3 @@ func getDefaultVMSSForModelTesting() VMSS {
 		},
 	}
 }
-
-func TestIsFlex(t *testing.T) {
-	cases := []struct {
-		Name   string
-		VM     VMSSVM
-		IsFlex bool
-	}{
-		{
-			Name:   "default empty VMSSVM",
-			VM:     VMSSVM{},
-			IsFlex: true,
-		},
-		{
-			Name:   "VMSSVM with an instance ID",
-			VM:     VMSSVM{InstanceID: "instance-id"},
-			IsFlex: false,
-		},
-		{
-			Name:   "VMSSVM with empty instance ID",
-			VM:     VMSSVM{InstanceID: ""},
-			IsFlex: true,
-		},
-	}
-
-	for _, c := range cases {
-		c := c
-		t.Run(c.Name, func(t *testing.T) {
-			g := NewWithT(t)
-			g.Expect(c.VM.IsFlex()).To(Equal(c.IsFlex))
-		})
-	}
-}
diff --git a/test/e2e/azure_machinepools.go b/test/e2e/azure_machinepools.go
diff --git a/test/e2e/azure_test.go b/test/e2e/azure_test.go

Original file line number	Diff line number	Diff line change
`@@ -50,6 +50,7 @@ func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.Vi`
`50`	`50`	`vmss.Instances = make([]azure.VMSSVM, len(sdkinstances))`
`51`	`51`	`for i, vm := range sdkinstances {`
`52`	`52`	`vmss.Instances[i] = *SDKToVMSSVM(vm)`
	`53`	`+ vmss.Instances[i].OrchestrationMode = infrav1.OrchestrationModeType(sdkvmss.OrchestrationMode)`
`53`	`54`	`}`
`54`	`55`	`}`
`55`	`56`
`@@ -64,7 +65,7 @@ func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.Vi`
`64`	`65`	`}`
`65`	`66`
`66`	`67`	`// SDKVMToVMSSVM converts an Azure SDK VM to a VMSS VM.`
`67`		`-func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine) *azure.VMSSVM {`
	`68`	`+func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine, mode infrav1.OrchestrationModeType) *azure.VMSSVM {`
`68`	`69`	`instance := azure.VMSSVM{`
`69`	`70`	`ID: to.String(sdkInstance.ID),`
`70`	`71`	`}`
`@@ -92,6 +93,8 @@ func SDKVMToVMSSVM(sdkInstance compute.VirtualMachine) *azure.VMSSVM {`
`92`	`93`	`instance.AvailabilityZone = to.StringSlice(sdkInstance.Zones)[0]`
`93`	`94`	`}`
`94`	`95`
	`96`	`+ instance.OrchestrationMode = mode`
	`97`	`+`
`95`	`98`	`return &instance`
`96`	`99`	`}`
`97`	`100`
Original file line number	Diff line number	Diff line change
`@@ -350,7 +350,7 @@ func Test_SDKVMToVMSSVM(t *testing.T) {`
`350`	`350`	`t.Run(c.Name, func(t *testing.T) {`
`351`	`351`	`t.Parallel()`
`352`	`352`	`g := gomega.NewGomegaWithT(t)`
`353`		`- subject := converters.SDKVMToVMSSVM(c.Subject)`
	`353`	`+ subject := converters.SDKVMToVMSSVM(c.Subject, "")`
`354`	`354`	`g.Expect(subject).To(gomega.Equal(c.Expected))`
`355`	`355`	`})`
`356`	`356`	`}`
Original file line number	Diff line number	Diff line change
`@@ -282,7 +282,7 @@ func (m *MachinePoolScope) applyAzureMachinePoolMachines(ctx context.Context) er`
`282`	`282`	`}`
`283`	`283`
`284`	`284`	`// determine which machines need to be created to reflect the current state in Azure`
`285`		`- azureMachinesByProviderID := m.vmssState.InstancesByProviderID()`
	`285`	`+ azureMachinesByProviderID := m.vmssState.InstancesByProviderID(m.AzureMachinePool.Spec.OrchestrationMode)`
`286`	`286`	`for key, val := range azureMachinesByProviderID {`
`287`	`287`	`if _, ok := existingMachinesByProviderID[key]; !ok {`
`288`	`288`	`log.V(4).Info("creating AzureMachinePoolMachine", "providerID", key)`
Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,7 @@ type (`
`42`	`42`	`InstanceID() string`
`43`	`43`	`ProviderID() string`
`44`	`44`	`ScaleSetName() string`
	`45`	`+ OrchestrationMode() infrav1.OrchestrationModeType`
`45`	`46`	`SetVMSSVM(vmssvm *azure.VMSSVM)`
`46`	`47`	`}`
`47`	`48`
`@@ -77,10 +78,11 @@ func (s *Service) Reconcile(ctx context.Context) error {`
`77`	`78`	`vmssName = s.Scope.ScaleSetName()`
`78`	`79`	`instanceID = s.Scope.InstanceID()`
`79`	`80`	`providerID = s.Scope.ProviderID()`
	`81`	`+ isFlex = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode`
`80`	`82`	`)`
`81`	`83`
`82`	`84`	`// Fetch the latest instance or VM data. AzureMachinePoolReconciler handles model mutations.`
`83`		`- if isFlex(instanceID) {`
	`85`	`+ if isFlex {`
`84`	`86`	`resourceID := strings.TrimPrefix(providerID, azure.ProviderIDPrefix)`
`85`	`87`	`log.V(4).Info("VMSS is flex", "vmssName", vmssName, "providerID", providerID, "resourceID", resourceID)`
`86`	`88`	`// Using VMSS Flex, so fetch by resource ID.`
`@@ -91,7 +93,7 @@ func (s *Service) Reconcile(ctx context.Context) error {`
`91`	`93`	`}`
`92`	`94`	`return errors.Wrap(err, "failed getting vm")`
`93`	`95`	`}`
`94`		`- s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm))`
	`96`	`+ s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, infrav1.FlexibleOrchestrationMode))`
`95`	`97`	`return nil`
`96`	`98`	`}`
`97`	`99`
`@@ -116,6 +118,7 @@ func (s *Service) Delete(ctx context.Context) error {`
`116`	`118`	`vmssName = s.Scope.ScaleSetName()`
`117`	`119`	`instanceID = s.Scope.InstanceID()`
`118`	`120`	`providerID = s.Scope.ProviderID()`
	`121`	`+ isFlex = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode`
`119`	`122`	`)`
`120`	`123`
`121`	`124`	`ctx, log, done := tele.StartSpanWithLogger(`
`@@ -127,24 +130,20 @@ func (s *Service) Delete(ctx context.Context) error {`
`127`	`130`	`)`
`128`	`131`	`defer done()`
`129`	`132`
`130`		`- if isFlex(instanceID) {`
	`133`	`+ if isFlex {`
`131`	`134`	`return s.deleteVMSSFlexVM(ctx, strings.TrimPrefix(providerID, azure.ProviderIDPrefix))`
`132`	`135`	`}`
`133`	`136`	`return s.deleteVMSSUniformInstance(ctx, resourceGroup, vmssName, instanceID, log)`
`134`	`137`	`}`
`135`	`138`
`136`		`-func isFlex(instanceID string) bool {`
`137`		`- return instanceID == ""`
`138`		`-}`
`139`		`-`
`140`	`139`	`func (s *Service) deleteVMSSFlexVM(ctx context.Context, resourceID string) error {`
`141`	`140`	`ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.deleteVMSSFlexVM")`
`142`	`141`	`defer done()`
`143`	`142`
`144`	`143`	`defer func() {`
`145`	`144`	`if vm, err := s.VMClient.GetByID(ctx, resourceID); err == nil && vm.VirtualMachineProperties != nil {`
`146`	`145`	`log.V(4).Info("vmss vm delete in progress", "state", vm.ProvisioningState)`
`147`		`- s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm))`
	`146`	`+ s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, s.Scope.OrchestrationMode()))`
`148`	`147`	`}`
`149`	`148`	`}()`
`150`	`149`