merge from main after failure-domain merged

wanyufe · wanyufe · commit f8d0c8029a7d · 2022-03-09T16:29:57.000-05:00
diff --git a/.gitignore b/.gitignore
@@ -8,6 +8,7 @@ bin
 testbin/*
 
 nginx.conf
+nginx.conf.bak
 
 pkg/mocks/*
 !pkg/mocks/.keep
diff --git a/api/v1beta1/cloudstackcluster_types.go b/api/v1beta1/cloudstackcluster_types.go
@@ -116,7 +116,7 @@ type CloudStackClusterStatus struct {
 	Zones ZoneStatusMap `json:"zones,omitempty"`
 
 	// CAPI recognizes failure domains as a method to spread machines.
-	// CAPC sets failure domains per to indicate functionin Zones.
+	// CAPC sets failure domains to indicate functioning Zones.
 	// +optional
 	FailureDomains clusterv1.FailureDomains `json:"failureDomains,omitempty"`
 
diff --git a/api/v1beta1/cloudstackmachine_types.go b/api/v1beta1/cloudstackmachine_types.go
@@ -63,6 +63,12 @@ type CloudStackMachineSpec struct {
 	// +optional
 	ProviderID *string `json:"providerID,omitempty"`
 
+	// Optionally settable Zone ID to land the machine in.
+	ZoneID string `json:"zoneID,omitempty"`
+
+	// Optionally settable Zone Name to land the machine in.
+	ZoneName string `json:"zoneName,omitempty"`
+
 	// IdentityRef is a reference to a identity to be used when reconciling this cluster
 	// +optional
 	// +k8s:conversion-gen=false
diff --git a/api/v1beta1/cloudstackmachine_webhook_test.go b/api/v1beta1/cloudstackmachine_webhook_test.go
@@ -83,19 +83,19 @@ var _ = Describe("CloudStackMachine webhook", func() {
 				Should(MatchError(MatchRegexp(forbiddenRegex, "details")))
 		})
 
-		It("should reject identity reference kind udpates to the CloudStackMachine", func() {
+		It("should reject identity reference kind updates to the CloudStackMachine", func() {
 			dummies.CSMachine1.Spec.IdentityRef.Kind = "ConfigMap"
 			Ω(k8sClient.Update(ctx, dummies.CSMachine1)).
 				Should(MatchError(MatchRegexp(forbiddenRegex, "identityRef\\.Kind")))
 		})
 
-		It("should reject identity reference name udpates to the CloudStackMachine", func() {
+		It("should reject identity reference name updates to the CloudStackMachine", func() {
 			dummies.CSMachine1.Spec.IdentityRef.Name = "IdentityConfigMap"
 			Ω(k8sClient.Update(ctx, dummies.CSMachine1)).
 				Should(MatchError(MatchRegexp(forbiddenRegex, "identityRef\\.Name")))
 		})
 
-		It("should reject udpates to the list of affinty groups of the CloudStackMachine", func() {
+		It("should reject updates to the list of affinty groups of the CloudStackMachine", func() {
 			dummies.CSMachine1.Spec.AffinityGroupIDs = []string{"28b907b8-75a7-4214-bd3d-6c61961fc2af"}
 			Ω(k8sClient.Update(ctx, dummies.CSMachine1)).
 				Should(MatchError(MatchRegexp(forbiddenRegex, "AffinityGroupIDs")))
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachines.yaml
@@ -123,6 +123,12 @@ spec:
                     description: Cloudstack resource Name
                     type: string
                 type: object
+              zoneID:
+                description: Optionally settable Zone ID to land the machine in.
+                type: string
+              zoneName:
+                description: Optionally settable Zone Name to land the machine in.
+                type: string
             required:
             - offering
             - template
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachinetemplates.yaml
@@ -111,6 +111,14 @@ spec:
                             description: Cloudstack resource Name
                             type: string
                         type: object
+                      zoneID:
+                        description: Optionally settable Zone ID to land the machine
+                          in.
+                        type: string
+                      zoneName:
+                        description: Optionally settable Zone Name to land the machine
+                          in.
+                        type: string
                     required:
                     - offering
                     - template
diff --git a/controllers/cloudstackmachine_controller.go b/controllers/cloudstackmachine_controller.go
@@ -166,15 +166,29 @@ func (r *CloudStackMachineReconciler) reconcile(
 	// Set ZoneID on csMachine.
 	if util.IsControlPlaneMachine(capiMachine) { // Use failure domain zone.
 		csMachine.Status.ZoneID = *capiMachine.Spec.FailureDomain
-	} else { // Random zone.
-		zones := make([]string, len(csCluster.Status.Zones))
-		zidx := 0
-		for zoneID := range csCluster.Status.Zones {
-			zones[zidx] = zoneID
-			zidx++
+	} else { // Specified by Machine Template or Random zone.
+		if csMachine.Spec.ZoneID != "" {
+			if zone, foundZone := csCluster.Status.Zones[csMachine.Spec.ZoneID]; foundZone { // ZoneID Specified.
+				csMachine.Status.ZoneID = zone.ID
+			} else {
+				return ctrl.Result{}, errors.Errorf("could not find zone by zoneID: %s", csMachine.Spec.ZoneID)
+			}
+		} else if csMachine.Spec.ZoneName != "" {
+			if zone := csCluster.Status.Zones.GetByName(csMachine.Spec.ZoneName); zone != nil { // ZoneName Specified.
+				csMachine.Status.ZoneID = zone.ID
+			} else {
+				return ctrl.Result{}, errors.Errorf("could not find zone by zoneName: %s", csMachine.Spec.ZoneName)
+			}
+		} else { // No Zone Specified, pick a Random Zone.
+			zones := make([]string, len(csCluster.Status.Zones))
+			zidx := 0
+			for zoneID := range csCluster.Status.Zones {
+				zones[zidx] = zoneID
+				zidx++
+			}
+			randNum := (rand.Int() % len(csCluster.Spec.Zones)) // #nosec G404 -- weak crypt rand doesn't matter here.
+			csMachine.Status.ZoneID = zones[randNum]
 		}
-		randNum := (rand.Int() % len(csCluster.Spec.Zones)) // #nosec G404 -- weak crypt rand doesn't matter here.
-		csMachine.Status.ZoneID = zones[randNum]
 	}
 
 	secret := &corev1.Secret{}
diff --git a/hack/add_addresses.sh b/hack/add_addresses.sh
@@ -1,5 +1,9 @@
 #!/bin/bash
 
+# This is a very simple script that can be used to launch a load balancer and add addresses to it.
+
+# It does so by querying kubernetes and parsing the output via jq.
+
 while true; do
     ADDRESSES=$(kubectl get machine -o json | jq -r '.items[] | select(.metadata.labels."cluster.x-k8s.io/control-plane" != null) | .status | select(.addresses!=null) | .addresses[].address')
     if [[ $ADDRESSES != $OLD_ADDRESSES ]]; then
@@ -10,8 +14,8 @@ while true; do
             echo $ADDRESS
             sed -i.bak '/upstream kubeendpoints/a\'$'\n'$'\t''server '$ADDRESS':6443 max_fails=3 fail_timeout=10s;'$'\n' nginx.conf
         done
-        docker stop nginx-container || echo
-        docker rm nginx-container || echo
+        docker stop nginx-container &> /dev/null || echo
+        docker rm nginx-container &> /dev/null || echo
         docker run --name=nginx-container --rm -p 8082:8082 -v $(pwd)/nginx.conf:/etc/nginx/nginx.conf nginx &
     fi
     OLD_ADDRESSES=$ADDRESSES 
diff --git a/hack/nginx.conf b/hack/nginx.conf
@@ -44,6 +44,7 @@ http {
 stream {
     upstream kubeendpoints {
 
+        # The following is the format for adding a kubernetes upstream endpoint.
         # server <server1 IP>:6443 max_fails=3 fail_timeout=10s;
         # server <server2 IP>:6443 max_fails=3 fail_timeout=10s;
     }
diff --git a/hack/setup_for_dev.sh b/hack/setup_for_dev.sh
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+# Source this file for local dev.
+
+export IMG=localhost:5000/cluster-api-provider-cloudstack:latest
+export PROJECT_DIR=`pwd`
+export KUBEBUILDER_ASSETS=$PROJECT_DIR/bin
+export PATH=$PROJECT_DIR/bin:$PATH
+export ACK_GINKGO_DEPRECATIONS=1.16.4
diff --git a/pkg/cloud/instance.go b/pkg/cloud/instance.go
@@ -214,29 +214,18 @@ func (c *client) GetOrCreateVMInstance(
 
 }
 
-// DestroyVMInstance Destroy a VM instance. Assumes machine has been fetched prior and has an instance ID.
+// DestroyVMInstance Destroys a VM instance. Assumes machine has been fetched prior and has an instance ID.
 func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
 
-	if err := c.ResolveVMInstanceDetails(csMachine); err == nil && csMachine.Status.InstanceState != "Running" {
-		if csMachine.Status.InstanceState == "Stopping" ||
-			csMachine.Status.InstanceState == "Stopped" {
-			return errors.New("VM deletion in progress")
-		} else if csMachine.Status.InstanceState == "Expunging" ||
-			csMachine.Status.InstanceState == "Expunged" {
-			// VM is stopped and getting expunged.  So the desired state is getting satisfied.  Let's move on.
-			return nil
-		}
-	} else if err != nil && strings.Contains(strings.ToLower(err.Error()), "no match found") {
-		// VM doesn't exist.  So the desired state is in effect.  Our work is done here.
-		return nil
-	}
-
+	// Attempt deletion regardless of machine state.
 	p := c.cs.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
 	p.SetExpunge(true)
-	_, err := c.csAsync.VirtualMachine.DestroyVirtualMachine(p)
-	if err != nil && strings.Contains(err.Error(), "unable to find UUID for id") {
-		// VM doesn't exist.  So the desired state is in effect.  Our work is done here.
+	if _, err := c.csAsync.VirtualMachine.DestroyVirtualMachine(p); err != nil &&
+		strings.Contains(strings.ToLower(err.Error()), "unable to find uuid for id") {
+		// VM doesn't exist. Success...
 		return nil
+	} else if err != nil {
+		return err
 	}
 	return errors.New("VM deletion in progress")
 }
diff --git a/pkg/cloud/network_test.go b/pkg/cloud/network_test.go
@@ -99,15 +99,6 @@ var _ = Describe("Network", func() {
 		It("correctly identifies an existing network from a network status", func() {
 			Ω(cloud.NetworkExists(dummies.CSCluster.Status.Zones.GetOne().Network)).Should(BeTrue())
 		})
-
-		// It("resolves network details with network ID instead of network name", func() {
-		// 	ns.EXPECT().GetNetworkID(gomock.Any()).Return("", -1, errors.New("no match found for blah"))
-		// 	ns.EXPECT().GetNetworkByID(fakeNetID).Return(&cloudstack.Network{Type: isolatedNetworkType}, 1, nil)
-		// 	expectNetworkTags(fakeNetID, false)
-
-		// 	csCluster.Spec.Network = fakeNetID
-		// 	Ω(client.GetOrCreateNetwork(csCluster)).Should(Succeed())
-		// })
 	})
 
 	Context("for a non-existent network", func() {

Original file line number	Diff line number	Diff line change
`@@ -44,6 +44,7 @@ http {`
`44`	`44`	`stream {`
`45`	`45`	`upstream kubeendpoints {`
`46`	`46`
	`47`	`+ # The following is the format for adding a kubernetes upstream endpoint.`
`47`	`48`	`# server <server1 IP>:6443 max_fails=3 fail_timeout=10s;`
`48`	`49`	`# server <server2 IP>:6443 max_fails=3 fail_timeout=10s;`
`49`	`50`	`}`