Merge pull request #6178 from fabriziopandini/CAPD-failure-domains

k8s-ci-robot · web-flow · commit 01957390401f · 2022-03-07T04:16:54.000-08:00
✨ CAPD and E2E framework tests failure domains
diff --git a/docs/book/src/developer/providers/v1.1-to-v1.2.md b/docs/book/src/developer/providers/v1.1-to-v1.2.md
@@ -74,3 +74,10 @@ in ClusterAPI are kept in sync with the versions used by `sigs.k8s.io/controller
 
       This change has been introduced in CAPI in the following PRs: [#6072](https://github.com/kubernetes-sigs/cluster-api/pull/6072), [#6190](https://github.com/kubernetes-sigs/cluster-api/pull/6190).</br>
       **Note**: This change is not mandatory for providers, but highly recommended.
+
+- Following E2E framework functions are now checking that machines are created in the expected failure domain (if defined);
+  all E2E tests can now verify failure domains too.
+  - `ApplyClusterTemplateAndWait`
+  - `WaitForControlPlaneAndMachinesReady`
+  - `DiscoveryAndWaitForMachineDeployments`
+- The `AssertControlPlaneFailureDomains` function in the E2E test framework has been modified to allow proper failure domain testing.
diff --git a/test/e2e/cluster_upgrade.go b/test/e2e/cluster_upgrade.go
@@ -112,7 +112,7 @@ func ClusterUpgradeConformanceSpec(ctx context.Context, inputGetter func() Clust
 		clusterResources = new(clusterctl.ApplyClusterTemplateAndWaitResult)
 	})
 
-	It("Should create and upgrade a workload cluster and run kubetest", func() {
+	It("Should create and upgrade a workload cluster and eventually run kubetest", func() {
 		By("Creating a workload cluster")
 
 		clusterctl.ApplyClusterTemplateAndWait(ctx, clusterctl.ApplyClusterTemplateAndWaitInput{
diff --git a/test/e2e/data/infrastructure-docker/v1beta1/bases/cluster-with-kcp.yaml b/test/e2e/data/infrastructure-docker/v1beta1/bases/cluster-with-kcp.yaml
@@ -4,6 +4,24 @@ apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
 kind: DockerCluster
 metadata:
   name: '${CLUSTER_NAME}'
+spec:
+  failureDomains:
+    fd1:
+      controlPlane: true
+    fd2:
+      controlPlane: true
+    fd3:
+      controlPlane: true
+    fd4:
+      controlPlane: false
+    fd5:
+      controlPlane: false
+    fd6:
+      controlPlane: false
+    fd7:
+      controlPlane: false
+    fd8:
+      controlPlane: false
 ---
 # Cluster object with
 # - Reference to the KubeadmControlPlane object
diff --git a/test/e2e/data/infrastructure-docker/v1beta1/bases/cluster-with-topology.yaml b/test/e2e/data/infrastructure-docker/v1beta1/bases/cluster-with-topology.yaml
@@ -23,6 +23,7 @@ spec:
       - class: "default-worker"
         name: "md-0"
         replicas: ${WORKER_MACHINE_COUNT}
+        failureDomain: fd4
     variables:
     # The imageRepository variable is defaulted by the Cluster webhook.
     #- name: imageRepository
diff --git a/test/e2e/data/infrastructure-docker/v1beta1/bases/md.yaml b/test/e2e/data/infrastructure-docker/v1beta1/bases/md.yaml
@@ -52,3 +52,4 @@ spec:
         name: "${CLUSTER_NAME}-md-0"
         apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
         kind: DockerMachineTemplate
+      failureDomain: fd4
diff --git a/test/e2e/data/infrastructure-docker/v1beta1/bases/mp.yaml b/test/e2e/data/infrastructure-docker/v1beta1/bases/mp.yaml
@@ -20,6 +20,12 @@ spec:
         kind: DockerMachinePool
         name: "${CLUSTER_NAME}-dmp-0"
       version: "${KUBERNETES_VERSION}"
+  failureDomains:
+    - fd4
+    - fd5
+    - fd6
+    - fd7
+    - fd8
 ---
 # DockerMachinePool using default values referenced by the MachinePool
 apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
diff --git a/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml b/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml
@@ -134,7 +134,24 @@ metadata:
   name: quick-start-my-cluster
 spec:
   template:
-    spec: {}
+    spec:
+      failureDomains:
+        fd1:
+          controlPlane: true
+        fd2:
+          controlPlane: true
+        fd3:
+          controlPlane: true
+        fd4:
+          controlPlane: false
+        fd5:
+          controlPlane: false
+        fd6:
+          controlPlane: false
+        fd7:
+          controlPlane: false
+        fd8:
+          controlPlane: false
 ---
 kind: KubeadmControlPlaneTemplate
 apiVersion: controlplane.cluster.x-k8s.io/v1beta1
diff --git a/test/framework/controlplane_helpers.go b/test/framework/controlplane_helpers.go
@@ -18,13 +18,15 @@ package framework
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gstruct"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -175,45 +177,44 @@ func WaitForControlPlaneToBeReady(ctx context.Context, input WaitForControlPlane
 
 // AssertControlPlaneFailureDomainsInput is the input for AssertControlPlaneFailureDomains.
 type AssertControlPlaneFailureDomainsInput struct {
-	GetLister  GetLister
-	ClusterKey client.ObjectKey
-	// ExpectedFailureDomains is required because this function cannot (easily) infer what success looks like.
-	// In theory this field is not strictly necessary and could be replaced with enough clever logic/math.
-	ExpectedFailureDomains map[string]int
+	Lister  Lister
+	Cluster *clusterv1.Cluster
 }
 
 // AssertControlPlaneFailureDomains will look at all control plane machines and see what failure domains they were
 // placed in. If machines were placed in unexpected or wrong failure domains the expectation will fail.
 func AssertControlPlaneFailureDomains(ctx context.Context, input AssertControlPlaneFailureDomainsInput) {
-	failureDomainCounts := map[string]int{}
-
-	// Look up the cluster object to find all known failure domains.
-	cluster := &clusterv1.Cluster{}
-	Expect(input.GetLister.Get(ctx, input.ClusterKey, cluster)).To(Succeed())
-
-	for fd := range cluster.Status.FailureDomains {
-		failureDomainCounts[fd] = 0
+	Expect(ctx).NotTo(BeNil(), "ctx is required for AssertControlPlaneFailureDomains")
+	Expect(input.Lister).ToNot(BeNil(), "Invalid argument. input.Lister can't be nil when calling AssertControlPlaneFailureDomains")
+	Expect(input.Cluster).ToNot(BeNil(), "Invalid argument. input.Cluster can't be nil when calling AssertControlPlaneFailureDomains")
+
+	By("Checking all the the control plane machines are in the expected failure domains")
+	controlPlaneFailureDomains := sets.NewString()
+	for fd, fdSettings := range input.Cluster.Status.FailureDomains {
+		if fdSettings.ControlPlane {
+			controlPlaneFailureDomains.Insert(fd)
+		}
 	}
 
 	// Look up all the control plane machines.
-	inClustersNamespaceListOption := client.InNamespace(input.ClusterKey.Namespace)
+	inClustersNamespaceListOption := client.InNamespace(input.Cluster.Namespace)
 	matchClusterListOption := client.MatchingLabels{
-		clusterv1.ClusterLabelName:             input.ClusterKey.Name,
+		clusterv1.ClusterLabelName:             input.Cluster.Name,
 		clusterv1.MachineControlPlaneLabelName: "",
 	}
 
 	machineList := &clusterv1.MachineList{}
-	Expect(input.GetLister.List(ctx, machineList, inClustersNamespaceListOption, matchClusterListOption)).
-		To(Succeed(), "Couldn't list machines for the cluster %q", input.ClusterKey.Name)
+	Expect(input.Lister.List(ctx, machineList, inClustersNamespaceListOption, matchClusterListOption)).
+		To(Succeed(), "Couldn't list control-plane machines for the cluster %q", input.Cluster.Name)
 
-	// Count all control plane machine failure domains.
 	for _, machine := range machineList.Items {
-		if machine.Spec.FailureDomain == nil {
-			continue
+		if machine.Spec.FailureDomain != nil {
+			machineFD := *machine.Spec.FailureDomain
+			if !controlPlaneFailureDomains.Has(machineFD) {
+				Fail(fmt.Sprintf("Machine %s is in the %q failure domain, expecting one of the failure domain defined at cluster level", machine.Name, machineFD))
+			}
 		}
-		failureDomainCounts[*machine.Spec.FailureDomain]++
 	}
-	Expect(failureDomainCounts).To(Equal(input.ExpectedFailureDomains))
 }
 
 // DiscoveryAndWaitForControlPlaneInitializedInput is the input type for DiscoveryAndWaitForControlPlaneInitialized.
@@ -277,6 +278,11 @@ func WaitForControlPlaneAndMachinesReady(ctx context.Context, input WaitForContr
 		ControlPlane: input.ControlPlane,
 	}
 	WaitForControlPlaneToBeReady(ctx, waitForControlPlaneToBeReadyInput, intervals...)
+
+	AssertControlPlaneFailureDomains(ctx, AssertControlPlaneFailureDomainsInput{
+		Lister:  input.GetLister,
+		Cluster: input.Cluster,
+	})
 }
 
 // UpgradeControlPlaneAndWaitForUpgradeInput is the input type for UpgradeControlPlaneAndWaitForUpgrade.
diff --git a/test/framework/machinedeployment_helpers.go b/test/framework/machinedeployment_helpers.go
@@ -121,6 +121,48 @@ func WaitForMachineDeploymentNodesToExist(ctx context.Context, input WaitForMach
 	}, intervals...).Should(Equal(int(*input.MachineDeployment.Spec.Replicas)))
 }
 
+// AssertMachineDeploymentFailureDomainsInput is the input for AssertMachineDeploymentFailureDomains.
+type AssertMachineDeploymentFailureDomainsInput struct {
+	Lister            Lister
+	Cluster           *clusterv1.Cluster
+	MachineDeployment *clusterv1.MachineDeployment
+}
+
+// AssertMachineDeploymentFailureDomains will look at all MachineDeployment machines and see what failure domains they were
+// placed in. If machines were placed in unexpected or wrong failure domains the expectation will fail.
+func AssertMachineDeploymentFailureDomains(ctx context.Context, input AssertMachineDeploymentFailureDomainsInput) {
+	Expect(ctx).NotTo(BeNil(), "ctx is required for AssertMachineDeploymentFailureDomains")
+	Expect(input.Lister).ToNot(BeNil(), "Invalid argument. input.Lister can't be nil when calling AssertMachineDeploymentFailureDomains")
+	Expect(input.MachineDeployment).ToNot(BeNil(), "Invalid argument. input.MachineDeployment can't be nil when calling AssertMachineDeploymentFailureDomains")
+
+	machineDeploymentFD := pointer.StringDeref(input.MachineDeployment.Spec.Template.Spec.FailureDomain, "<None>")
+
+	By(fmt.Sprintf("Checking all the machines controlled by %s are in the %q failure domain", input.MachineDeployment.Name, machineDeploymentFD))
+	selectorMap, err := metav1.LabelSelectorAsMap(&input.MachineDeployment.Spec.Selector)
+	Expect(err).NotTo(HaveOccurred())
+
+	ms := &clusterv1.MachineSetList{}
+	err = input.Lister.List(ctx, ms, client.InNamespace(input.Cluster.Namespace), client.MatchingLabels(selectorMap))
+	Expect(err).NotTo(HaveOccurred())
+
+	for _, machineSet := range ms.Items {
+		machineSetFD := pointer.StringDeref(machineSet.Spec.Template.Spec.FailureDomain, "<None>")
+		Expect(machineSetFD).To(Equal(machineDeploymentFD), "MachineSet %s is in the %q failure domain, expecting %q", machineSet.Name, machineSetFD, machineDeploymentFD)
+
+		selectorMap, err = metav1.LabelSelectorAsMap(&machineSet.Spec.Selector)
+		Expect(err).NotTo(HaveOccurred())
+
+		machines := &clusterv1.MachineList{}
+		err = input.Lister.List(ctx, machines, client.InNamespace(machineSet.Namespace), client.MatchingLabels(selectorMap))
+		Expect(err).NotTo(HaveOccurred())
+
+		for _, machine := range machines.Items {
+			machineFD := pointer.StringDeref(machine.Spec.FailureDomain, "<None>")
+			Expect(machineFD).To(Equal(machineDeploymentFD), "Machine %s is in the %q failure domain, expecting %q", machine.Name, machineFD, machineDeploymentFD)
+		}
+	}
+}
+
 // DiscoveryAndWaitForMachineDeploymentsInput is the input type for DiscoveryAndWaitForMachineDeployments.
 type DiscoveryAndWaitForMachineDeploymentsInput struct {
 	Lister  Lister
@@ -144,6 +186,12 @@ func DiscoveryAndWaitForMachineDeployments(ctx context.Context, input DiscoveryA
 			Cluster:           input.Cluster,
 			MachineDeployment: deployment,
 		}, intervals...)
+
+		AssertMachineDeploymentFailureDomains(ctx, AssertMachineDeploymentFailureDomainsInput{
+			Lister:            input.Lister,
+			Cluster:           input.Cluster,
+			MachineDeployment: deployment,
+		})
 	}
 	return machineDeployments
 }
diff --git a/test/framework/machinepool_helpers.go b/test/framework/machinepool_helpers.go
@@ -107,6 +107,9 @@ func DiscoveryAndWaitForMachinePools(ctx context.Context, input DiscoveryAndWait
 			Getter:      input.Getter,
 			MachinePool: machinepool,
 		}, intervals...)
+
+		// TODO: check for failure domains; currently MP doesn't provide a way to check where Machine are placed
+		//  (checking infrastructure is the only alternative, but this makes test not portable)
 	}
 	return machinePools
 }
diff --git a/test/infrastructure/docker/api/v1beta1/dockercluster_types.go b/test/infrastructure/docker/api/v1beta1/dockercluster_types.go
@@ -37,7 +37,7 @@ type DockerClusterSpec struct {
 	// +optional
 	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint"`
 
-	// FailureDomains are not usulaly defined on the spec.
+	// FailureDomains are usually not defined in the spec.
 	// The docker provider is special since failure domains don't mean anything in a local docker environment.
 	// Instead, the docker cluster controller will simply copy these into the Status and allow the Cluster API
 	// controllers to do what they will with the defined failure domains.
diff --git a/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockerclusters.yaml b/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockerclusters.yaml
@@ -371,7 +371,7 @@ spec:
                         is suitable for use by control plane machines.
                       type: boolean
                   type: object
-                description: FailureDomains are not usulaly defined on the spec. The
+                description: FailureDomains are usually not defined in the spec. The
                   docker provider is special since failure domains don't mean anything
                   in a local docker environment. Instead, the docker cluster controller
                   will simply copy these into the Status and allow the Cluster API
diff --git a/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockerclustertemplates.yaml b/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockerclustertemplates.yaml
@@ -203,7 +203,7 @@ spec:
                                 domain is suitable for use by control plane machines.
                               type: boolean
                           type: object
-                        description: FailureDomains are not usulaly defined on the
+                        description: FailureDomains are usually not defined in the
                           spec. The docker provider is special since failure domains
                           don't mean anything in a local docker environment. Instead,
                           the docker cluster controller will simply copy these into
diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go
@@ -20,6 +20,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -185,7 +186,8 @@ func (r *DockerMachinePoolReconciler) reconcileNormal(ctx context.Context, clust
 	}
 
 	// Reconcile machines and updates Status.Instances
-	if res, err := pool.ReconcileMachines(ctx); err != nil || !res.IsZero() {
+	res, err := pool.ReconcileMachines(ctx)
+	if err != nil {
 		return res, err
 	}
 
@@ -207,7 +209,12 @@ func (r *DockerMachinePoolReconciler) reconcileNormal(ctx context.Context, clust
 	}
 
 	dockerMachinePool.Status.Ready = len(dockerMachinePool.Spec.ProviderIDList) == int(*machinePool.Spec.Replicas)
-	return ctrl.Result{}, nil
+
+	// if some machine is still provisioning, force reconcile in few seconds to check again infrastructure.
+	if !dockerMachinePool.Status.Ready && res.IsZero() {
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+	}
+	return res, nil
 }
 
 func getDockerMachinePoolProviderID(clusterName, dockerMachinePoolName string) string {
diff --git a/test/infrastructure/docker/exp/internal/docker/nodepool.go b/test/infrastructure/docker/exp/internal/docker/nodepool.go
diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go b/test/infrastructure/docker/internal/controllers/dockermachine_controller.go
diff --git a/test/infrastructure/docker/internal/docker/kind_manager.go b/test/infrastructure/docker/internal/docker/kind_manager.go
diff --git a/test/infrastructure/docker/internal/docker/machine.go b/test/infrastructure/docker/internal/docker/machine.go
diff --git a/test/infrastructure/docker/internal/docker/util.go b/test/infrastructure/docker/internal/docker/util.go

Original file line number	Diff line number	Diff line change
`@@ -107,6 +107,9 @@ func DiscoveryAndWaitForMachinePools(ctx context.Context, input DiscoveryAndWait`
`107`	`107`	`Getter: input.Getter,`
`108`	`108`	`MachinePool: machinepool,`
`109`	`109`	`}, intervals...)`
	`110`	`+`
	`111`	`+ // TODO: check for failure domains; currently MP doesn't provide a way to check where Machine are placed`
	`112`	`+ // (checking infrastructure is the only alternative, but this makes test not portable)`
`110`	`113`	`}`
`111`	`114`	`return machinePools`
`112`	`115`	`}`