Merge pull request #5292 from helio/fix-vmss-update-state-on-get

MachinePool always update vmssState

Author: k8s-ci-robot

azure/services/scalesets/scalesets.go

@@ -97,7 +97,7 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) {
 		return errors.Errorf("%T is not of type ScaleSetSpec", spec)
 	}
 
-	_, err := s.Client.Get(ctx, spec)
+	result, err := s.Client.Get(ctx, spec)
 	if err == nil {
 		// We can only get the existing instances if the VMSS already exists
 		scaleSetSpec.VMSSInstances, err = s.Client.ListInstances(ctx, spec.ResourceGroupName(), spec.ResourceName())
@@ -106,34 +106,51 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) {
 			s.Scope.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, err)
 			return err
 		}
+		if result != nil {
+			if err := s.updateScopeState(ctx, result, scaleSetSpec); err != nil {
+				return err
+			}
+		}
 	} else if !azure.ResourceNotFound(err) {
 		return errors.Wrapf(err, "failed to get existing VMSS")
 	}
 
-	result, err := s.CreateOrUpdateResource(ctx, scaleSetSpec, serviceName)
+	result, err = s.CreateOrUpdateResource(ctx, scaleSetSpec, serviceName)
 	s.Scope.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, err)
 
 	if err == nil && result != nil {
-		vmss, ok := result.(armcompute.VirtualMachineScaleSet)
-		if !ok {
-			return errors.Errorf("%T is not an armcompute.VirtualMachineScaleSet", result)
+		if err := s.updateScopeState(ctx, result, scaleSetSpec); err != nil {
+			return err
 		}
+	}
 
-		fetchedVMSS := converters.SDKToVMSS(vmss, scaleSetSpec.VMSSInstances)
-		if err := s.Scope.ReconcileReplicas(ctx, &fetchedVMSS); err != nil {
-			return errors.Wrap(err, "unable to reconcile VMSS replicas")
-		}
+	return err
+}
 
-		// Transform the VMSS resource representation to conform to the cloud-provider-azure representation
-		providerID, err := azprovider.ConvertResourceGroupNameToLower(azureutil.ProviderIDPrefix + fetchedVMSS.ID)
-		if err != nil {
-			return errors.Wrapf(err, "failed to parse VMSS ID %s", fetchedVMSS.ID)
-		}
-		s.Scope.SetProviderID(providerID)
-		s.Scope.SetVMSSState(&fetchedVMSS)
+// updateScopeState updates the scope's VMSS state and provider ID
+//
+// Code later in the reconciler uses scope's VMSS state for determining scale status and whether to create/delete
+// AzureMachinePoolMachines.
+// N.B.: before calling this function, make sure scaleSetSpec.VMSSInstances is updated to the latest state.
+func (s *Service) updateScopeState(ctx context.Context, result interface{}, scaleSetSpec *ScaleSetSpec) error {
+	vmss, ok := result.(armcompute.VirtualMachineScaleSet)
+	if !ok {
+		return errors.Errorf("%T is not an armcompute.VirtualMachineScaleSet", result)
 	}
 
-	return err
+	fetchedVMSS := converters.SDKToVMSS(vmss, scaleSetSpec.VMSSInstances)
+	if err := s.Scope.ReconcileReplicas(ctx, &fetchedVMSS); err != nil {
+		return errors.Wrap(err, "unable to reconcile VMSS replicas")
+	}
+
+	// Transform the VMSS resource representation to conform to the cloud-provider-azure representation
+	providerID, err := azprovider.ConvertResourceGroupNameToLower(azureutil.ProviderIDPrefix + fetchedVMSS.ID)
+	if err != nil {
+		return errors.Wrapf(err, "failed to parse VMSS ID %s", fetchedVMSS.ID)
+	}
+	s.Scope.SetProviderID(providerID)
+	s.Scope.SetVMSSState(&fetchedVMSS)
+	return nil
 }
 
 // Delete deletes a scale set asynchronously. Delete sends a DELETE request to Azure and if accepted without error,

azure/services/scalesets/scalesets_test.go

@@ -120,14 +120,14 @@ func TestReconcileVMSS(t *testing.T) {
 				spec := getDefaultVMSSSpec()
 				// Validate spec
 				s.ScaleSetSpec(gomockinternal.AContext()).Return(spec).AnyTimes()
-				m.Get(gomockinternal.AContext(), &defaultSpec).Return(&resultVMSS, nil)
+				m.Get(gomockinternal.AContext(), &defaultSpec).Return(resultVMSS, nil)
 				m.ListInstances(gomockinternal.AContext(), defaultSpec.ResourceGroup, defaultSpec.Name).Return(defaultInstances, nil)
 				r.CreateOrUpdateResource(gomockinternal.AContext(), spec, serviceName).Return(getResultVMSS(), nil)
 				s.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, nil)
 
-				s.ReconcileReplicas(gomockinternal.AContext(), &fetchedVMSS).Return(nil)
-				s.SetProviderID(azureutil.ProviderIDPrefix + defaultVMSSID)
-				s.SetVMSSState(&fetchedVMSS)
+				s.ReconcileReplicas(gomockinternal.AContext(), &fetchedVMSS).Return(nil).Times(2)
+				s.SetProviderID(azureutil.ProviderIDPrefix + defaultVMSSID).Times(2)
+				s.SetVMSSState(&fetchedVMSS).Times(2)
 			},
 		},
 		{
@@ -178,28 +178,16 @@ func TestReconcileVMSS(t *testing.T) {
 				s.DefaultedAzureServiceReconcileTimeout().Return(reconciler.DefaultAzureServiceReconcileTimeout)
 				spec := getDefaultVMSSSpec()
 				s.ScaleSetSpec(gomockinternal.AContext()).Return(spec).AnyTimes()
-				m.Get(gomockinternal.AContext(), &defaultSpec).Return(&resultVMSS, nil)
+				m.Get(gomockinternal.AContext(), &defaultSpec).Return(resultVMSS, nil)
 				m.ListInstances(gomockinternal.AContext(), defaultSpec.ResourceGroup, defaultSpec.Name).Return(defaultInstances, nil)
 
 				r.CreateOrUpdateResource(gomockinternal.AContext(), spec, serviceName).
 					Return(nil, internalError())
 				s.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, internalError())
-			},
-		},
-		{
-			name:          "failed to reconcile replicas",
-			expectedError: "unable to reconcile VMSS replicas:.*#: Internal Server Error: StatusCode=500",
-			expect: func(g *WithT, s *mock_scalesets.MockScaleSetScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, m *mock_scalesets.MockClientMockRecorder) {
-				s.DefaultedAzureServiceReconcileTimeout().Return(reconciler.DefaultAzureServiceReconcileTimeout)
-				spec := getDefaultVMSSSpec()
-				s.ScaleSetSpec(gomockinternal.AContext()).Return(spec).AnyTimes()
-				m.Get(gomockinternal.AContext(), &defaultSpec).Return(&resultVMSS, nil)
-				m.ListInstances(gomockinternal.AContext(), defaultSpec.ResourceGroup, defaultSpec.Name).Return(defaultInstances, nil)
 
-				r.CreateOrUpdateResource(gomockinternal.AContext(), spec, serviceName).Return(getResultVMSS(), nil)
-				s.UpdatePutStatus(infrav1.BootstrapSucceededCondition, serviceName, nil)
-
-				s.ReconcileReplicas(gomockinternal.AContext(), &fetchedVMSS).Return(internalError())
+				s.ReconcileReplicas(gomockinternal.AContext(), &fetchedVMSS).Return(nil)
+				s.SetProviderID(azureutil.ProviderIDPrefix + defaultVMSSID)
+				s.SetVMSSState(&fetchedVMSS)
 			},
 		},
 		{