Improve VM provisioning State handling

Author: Cecile Robert-Michon

api/v1alpha3/types.go

@@ -338,19 +338,19 @@ type LoadBalancerHealthCheck struct {
 // VMState describes the state of an Azure virtual machine.
 type VMState string
 
-var (
+const (
 	// VMStateCreating ...
-	VMStateCreating = VMState("Creating")
+	VMStateCreating VMState = "Creating"
 	// VMStateDeleting ...
-	VMStateDeleting = VMState("Deleting")
+	VMStateDeleting VMState = "Deleting"
 	// VMStateFailed ...
-	VMStateFailed = VMState("Failed")
+	VMStateFailed VMState = "Failed"
 	// VMStateMigrating ...
-	VMStateMigrating = VMState("Migrating")
+	VMStateMigrating VMState = "Migrating"
 	// VMStateSucceeded ...
-	VMStateSucceeded = VMState("Succeeded")
+	VMStateSucceeded VMState = "Succeeded"
 	// VMStateUpdating ...
-	VMStateUpdating = VMState("Updating")
+	VMStateUpdating VMState = "Updating"
 )
 
 // VM describes an Azure virtual machine.

cloud/scope/machine.go

@@ -161,11 +161,16 @@ func (m *MachineScope) SetVMState(v infrav1.VMState) {
 	m.AzureMachine.Status.VMState = &v
 }
 
-// SetReady sets the AzureMachine Ready Status
+// SetReady sets the AzureMachine Ready Status to true.
 func (m *MachineScope) SetReady() {
 	m.AzureMachine.Status.Ready = true
 }
 
+// SetNotReady sets the AzureMachine Ready Status to false.
+func (m *MachineScope) SetNotReady() {
+	m.AzureMachine.Status.Ready = false
+}
+
 // SetFailureMessage sets the AzureMachine status failure message.
 func (m *MachineScope) SetFailureMessage(v error) {
 	m.AzureMachine.Status.FailureMessage = pointer.StringPtr(v.Error())

cloud/services/virtualmachines/virtualmachines.go

@@ -51,11 +51,11 @@ type Spec struct {
 func (s *Service) Get(ctx context.Context, spec interface{}) (interface{}, error) {
 	vmSpec, ok := spec.(*Spec)
 	if !ok {
-		return compute.VirtualMachine{}, errors.New("invalid vm specification")
+		return compute.VirtualMachine{}, errors.New("invalid VM specification")
 	}
 	vm, err := s.Client.Get(ctx, s.Scope.ResourceGroup(), vmSpec.Name)
 	if err != nil && azure.ResourceNotFound(err) {
-		return nil, errors.Wrapf(err, "vm %s not found", vmSpec.Name)
+		return nil, errors.Wrapf(err, "VM %s not found", vmSpec.Name)
 	} else if err != nil {
 		return vm, err
 	}
@@ -79,22 +79,22 @@ func (s *Service) Get(ctx context.Context, spec interface{}) (interface{}, error
 func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 	vmSpec, ok := spec.(*Spec)
 	if !ok {
-		return errors.New("invalid vm specification")
+		return errors.New("invalid VM specification")
 	}
 
 	storageProfile, err := generateStorageProfile(*vmSpec)
 	if err != nil {
 		return err
 	}
 
-	klog.V(2).Infof("getting nic %s", vmSpec.NICName)
+	klog.V(2).Infof("getting NIC %s", vmSpec.NICName)
 	nic, err := s.InterfacesClient.Get(ctx, s.Scope.ResourceGroup(), vmSpec.NICName)
 	if err != nil {
 		return err
 	}
-	klog.V(2).Infof("got nic %s", vmSpec.NICName)
+	klog.V(2).Infof("got NIC %s", vmSpec.NICName)
 
-	klog.V(2).Infof("creating vm %s ", vmSpec.Name)
+	klog.V(2).Infof("creating VM %s ", vmSpec.Name)
 
 	sshKeyData := vmSpec.SSHKeyData
 	if sshKeyData == "" {
@@ -174,27 +174,27 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 		return errors.Wrapf(err, "cannot create vm")
 	}
 
-	klog.V(2).Infof("successfully created vm %s ", vmSpec.Name)
+	klog.V(2).Infof("successfully created VM %s ", vmSpec.Name)
 	return nil
 }
 
 // Delete deletes the virtual machine with the provided name.
 func (s *Service) Delete(ctx context.Context, spec interface{}) error {
 	vmSpec, ok := spec.(*Spec)
 	if !ok {
-		return errors.New("invalid vm specification")
+		return errors.New("invalid VM specification")
 	}
-	klog.V(2).Infof("deleting vm %s ", vmSpec.Name)
+	klog.V(2).Infof("deleting VM %s ", vmSpec.Name)
 	err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), vmSpec.Name)
 	if err != nil && azure.ResourceNotFound(err) {
 		// already deleted
 		return nil
 	}
 	if err != nil {
-		return errors.Wrapf(err, "failed to delete vm %s in resource group %s", vmSpec.Name, s.Scope.ResourceGroup())
+		return errors.Wrapf(err, "failed to delete VM %s in resource group %s", vmSpec.Name, s.Scope.ResourceGroup())
 	}
 
-	klog.V(2).Infof("successfully deleted vm %s ", vmSpec.Name)
+	klog.V(2).Infof("successfully deleted VM %s ", vmSpec.Name)
 	return nil
 }
 

cloud/services/virtualmachines/virtualmachines_test.go

@@ -41,7 +41,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-const expectedInvalidSpec = "invalid vm specification"
+const expectedInvalidSpec = "invalid VM specification"
 
 func init() {
 	clusterv1.AddToScheme(scheme.Scheme)
@@ -167,7 +167,7 @@ func TestGetVM(t *testing.T) {
 			vmSpec: Spec{
 				Name: "my-vm",
 			},
-			expectedError: "vm my-vm not found: #: Not found: StatusCode=404",
+			expectedError: "VM my-vm not found: #: Not found: StatusCode=404",
 			expect: func(m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) {
 				mpip.Get(context.TODO(), "my-rg", "my-publicIP-id").Return(network.PublicIPAddress{}, nil)
 				mnic.Get(context.TODO(), "my-rg", gomock.Any()).Return(network.Interface{}, nil)
@@ -604,7 +604,7 @@ func TestDeleteVM(t *testing.T) {
 			vmSpec: Spec{
 				Name: "my-vm",
 			},
-			expectedError: "failed to delete vm my-vm in resource group my-rg: #: Internal Server Error: StatusCode=500",
+			expectedError: "failed to delete VM my-vm in resource group my-rg: #: Internal Server Error: StatusCode=500",
 			expect: func(m *mock_virtualmachines.MockClientMockRecorder) {
 				m.Delete(context.TODO(), "my-rg", "my-vm").
 					Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))

controllers/azuremachine_controller.go

@@ -217,17 +217,6 @@ func (r *AzureMachineReconciler) reconcileNormal(ctx context.Context, machineSco
 		return reconcile.Result{}, err
 	}
 
-	// Set an error message if we couldn't find the VM.
-	// TODO: Nodes are getting marked failed. Need to debug this.
-	//       Should we set a requeue above? CAPA doesn't seem to need the requeue.
-	/*
-		if vm == nil {
-			machineScope.SetFailureReason(capierrors.UpdateMachineError)
-			machineScope.SetFailureMessage(errors.New("Azure virtual machine cannot be found"))
-			return reconcile.Result{}, nil
-		}
-	*/
-
 	// TODO(ncdc): move this validation logic into a validating webhook
 	if errs := r.validateUpdate(&machineScope.AzureMachine.Spec, vm); len(errs) > 0 {
 		agg := kerrors.NewAggregate(errs)
@@ -248,13 +237,30 @@ func (r *AzureMachineReconciler) reconcileNormal(ctx context.Context, machineSco
 
 	switch vm.State {
 	case infrav1.VMStateSucceeded:
-		machineScope.Info("Machine VM is running", "instance-id", *machineScope.GetVMID())
+		machineScope.V(2).Info("VM is running", "id", *machineScope.GetVMID())
 		machineScope.SetReady()
+	case infrav1.VMStateCreating:
+		machineScope.V(2).Info("VM is creating", "id", *machineScope.GetVMID())
+		machineScope.SetNotReady()
 	case infrav1.VMStateUpdating:
-		machineScope.Info("Machine VM is updating", "instance-id", *machineScope.GetVMID())
+		machineScope.V(2).Info("VM is updating", "id", *machineScope.GetVMID())
+		machineScope.SetNotReady()
+	case infrav1.VMStateDeleting:
+		machineScope.Info("Unexpected VM deletion", "state", vm.State, "instance-id", *machineScope.GetVMID())
+		r.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "UnexpectedVMDeletion", "Unexpected Azure VM deletion")
+		machineScope.SetNotReady()
+	case infrav1.VMStateFailed:
+		machineScope.SetNotReady()
+		machineScope.Error(errors.New("Failed to create or update VM"), "VM is in failed state", "id", *machineScope.GetVMID())
+		r.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "FailedVMState", "Azure VM is in failed state")
+		machineScope.SetFailureReason(capierrors.UpdateMachineError)
+		machineScope.SetFailureMessage(errors.Errorf("Azure VM state is %s", vm.State))
 	default:
+		machineScope.SetNotReady()
+		machineScope.Info("VM state is undefined", "state", vm.State, "instance-id", *machineScope.GetVMID())
+		r.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "UnhandledVMState", "Azure VM state is undefined")
 		machineScope.SetFailureReason(capierrors.UpdateMachineError)
-		machineScope.SetFailureMessage(errors.Errorf("Azure VM state %q is unexpected", vm.State))
+		machineScope.SetFailureMessage(errors.Errorf("Azure VM state %q is undefined", vm.State))
 	}
 
 	// Ensure that the tags are correct.

controllers/azuremachine_reconciler.go

@@ -73,12 +73,12 @@ func (s *azureMachineService) Create() (*infrav1.VM, error) {
 	nicName := azure.GenerateNICName(s.machineScope.Name())
 	nicErr := s.reconcileNetworkInterface(nicName)
 	if nicErr != nil {
-		return nil, errors.Wrapf(nicErr, "failed to create nic %s for machine %s", nicName, s.machineScope.Name())
+		return nil, errors.Wrapf(nicErr, "failed to create NIC %s for machine %s", nicName, s.machineScope.Name())
 	}
 
 	vm, vmErr := s.createVirtualMachine(nicName)
 	if vmErr != nil {
-		return nil, errors.Wrapf(vmErr, "failed to create vm %s ", s.machineScope.Name())
+		return nil, errors.Wrapf(vmErr, "failed to create VM %s ", s.machineScope.Name())
 	}
 
 	return vm, nil
@@ -131,7 +131,7 @@ func (s *azureMachineService) Delete() error {
 
 func (s *azureMachineService) VMIfExists(id *string) (*infrav1.VM, error) {
 	if id == nil {
-		s.clusterScope.Info("VM does not have an id")
+		s.clusterScope.Info("VM does not have an ID")
 		return nil, nil
 	}
 
@@ -145,15 +145,15 @@ func (s *azureMachineService) VMIfExists(id *string) (*infrav1.VM, error) {
 	}
 
 	if err != nil {
-		return nil, errors.Wrap(err, "Failed to get vm")
+		return nil, errors.Wrap(err, "Failed to get VM")
 	}
 
 	vm, ok := vmInterface.(*infrav1.VM)
 	if !ok {
 		return nil, errors.New("returned incorrect vm interface")
 	}
 
-	klog.Infof("Found vm for machine %s", s.machineScope.Name())
+	klog.V(2).Infof("Found VM for AzureMachine %s", s.machineScope.Name())
 
 	return vm, nil
 }