Bug fix: failure to clean-up during CS Machine provisioning if VM provisioned to error state. (kubernetes-sigs#58)

* Deleting capi machine (instead of cs machine) upon VM detected in error state.

* Added rbac permissions to enable the CloudStackMachine controller to delete/patch CAPI machines.  Fixed race condition that occurs when VMs create in the error state by a) detecting this, and b) adding a finalizer when detected.  Enabled the use of klog parameters on the command line.

* Defensively narrowed the scope of VM looked up after provisioning error.

* Fixed broken unit test setup, to accomodate new calls made in the last commit.

Author: jweite-amazon

config/rbac/role.yaml

@@ -40,8 +40,10 @@ rules:
   - machines
   - machines/status
   verbs:
+  - delete
   - get
   - list
+  - patch
   - watch
 - apiGroups:
   - cluster.x-k8s.io

controllers/cloudstackmachine_controller.go

@@ -61,7 +61,7 @@ const destoryVMRequeueInterval = 10 * time.Second
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=cloudstackmachines/finalizers,verbs=update
-// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch;patch;delete
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets,verbs=get;list;watch
 // +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=kubeadmcontrolplanes,verbs=get;list;watch
 
@@ -71,7 +71,7 @@ const destoryVMRequeueInterval = 10 * time.Second
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
 func (r *CloudStackMachineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ctrl.Result, retErr error) {
-	log := r.Log.WithValues("machine", req.Name, "namespace", req.Namespace)
+	log := r.Log.WithValues("cloudstackmachine", req.Name, "namespace", req.Namespace)
 	log.V(1).Info("Reconcile CloudStackMachine")
 
 	// Fetch the CloudStackMachine.
@@ -210,6 +210,10 @@ func (r *CloudStackMachineReconciler) reconcile(
 				controllerutil.AddFinalizer(csMachine, infrav1.MachineFinalizer)
 			}
 		} else if err != nil {
+			if csMachine.Spec.InstanceID != nil {
+				// Despite the failed VM Creation call, a VM has been created, and will need to be properly destroyed.
+				controllerutil.AddFinalizer(csMachine, infrav1.MachineFinalizer)
+			}
 			return ctrl.Result{}, err
 		}
 	} else {
@@ -221,11 +225,12 @@ func (r *CloudStackMachineReconciler) reconcile(
 		log.Info("Machine instance is Running...")
 		csMachine.Status.Ready = true
 	} else if csMachine.Status.InstanceState == "Error" {
-		log.Info("CloudStackMachine VM in error state. Deleting associated Machine.", "csMachine", csMachine)
-		if err := r.Client.Delete(ctx, csMachine); err != nil {
+		log.Info("CloudStackMachine VM in error state. Deleting associated Machine.", "capiMachine", capiMachine, "csMachine", csMachine)
+		if err := r.Client.Delete(ctx, capiMachine); err != nil {
 			return ctrl.Result{}, err
 		}
-		return ctrl.Result{RequeueAfter: requeueTimeout}, nil
+		log.Info("CAPI machine deletion requested.")
+		return ctrl.Result{}, nil // We're done.  CAPI Machine will take it from here.
 	} else {
 		log.Info(fmt.Sprintf("Instance not ready, is %s.", csMachine.Status.InstanceState))
 		return ctrl.Result{RequeueAfter: requeueTimeout}, nil

main.go

@@ -27,6 +27,7 @@ import (
 	flag "github.com/spf13/pflag"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 
+	goflag "flag"
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 	"k8s.io/klog"
 	"k8s.io/klog/klogr"
@@ -112,8 +113,9 @@ func setFlags() *managerOpts {
 }
 
 func main() {
-	opts := setFlags()  // Add our options to flag set.
-	klog.InitFlags(nil) // Add klog options to flag set.
+	opts := setFlags()                                // Add our options to flag set.
+	klog.InitFlags(nil)                               // Add klog options to flag set.
+	flag.CommandLine.AddGoFlagSet(goflag.CommandLine) // Merge klog's gofloag flags into the pflags.
 	flag.Parse()
 
 	ctrl.SetLogger(klogr.New())

pkg/cloud/instance.go

@@ -212,6 +212,19 @@ func (c *client) GetOrCreateVMInstance(
 
 	deployVMResp, err := c.cs.VirtualMachine.DeployVirtualMachine(p)
 	if err != nil {
+		// Just because an error was returned doesn't mean a (failed) VM wasn't created and will need to be dealt with.
+		// Regretfully the deployVMResp may be nil, so we need to get the VM ID with a separate query, so we
+		// can return it to the caller, so they can clean it up.
+		listVirtualMachineParams := c.cs.VirtualMachine.NewListVirtualMachinesParams()
+		listVirtualMachineParams.SetTemplateid(templateID)
+		listVirtualMachineParams.SetZoneid(csMachine.Status.ZoneID)
+		listVirtualMachineParams.SetNetworkid(zone.Network.ID)
+		listVirtualMachineParams.SetName(csMachine.Name)
+		setIfNotEmpty(csCluster.Status.DomainID, listVirtualMachineParams.SetDomainid)
+		setIfNotEmpty(csCluster.Spec.Account, listVirtualMachineParams.SetAccount)
+		if listVirtualMachinesResponse, err2 := c.cs.VirtualMachine.ListVirtualMachines(listVirtualMachineParams); err2 == nil && listVirtualMachinesResponse.Count > 0 {
+			csMachine.Spec.InstanceID = pointer.StringPtr(listVirtualMachinesResponse.VirtualMachines[0].Id)
+		}
 		return err
 	}
 	csMachine.Spec.InstanceID = pointer.StringPtr(deployVMResp.Id)

pkg/cloud/instance_test.go

@@ -174,7 +174,8 @@ var _ = Describe("Instance", func() {
 			vms.EXPECT().NewDeployVirtualMachineParams(offeringFakeID, templateFakeID, dummies.Zone1.ID).
 				Return(&cloudstack.DeployVirtualMachineParams{})
 			vms.EXPECT().DeployVirtualMachine(gomock.Any()).Return(nil, unknownError)
-
+			vms.EXPECT().NewListVirtualMachinesParams().Return(&cloudstack.ListVirtualMachinesParams{})
+			vms.EXPECT().ListVirtualMachines(gomock.Any()).Return(&cloudstack.ListVirtualMachinesResponse{}, nil)
 			Ω(client.GetOrCreateVMInstance(dummies.CSMachine1, dummies.CAPIMachine, dummies.CSCluster, "")).
 				Should(MatchError(unknownErrorMessage))
 		})