update

Author: sbueringer

test/e2e/clusterclass_rollout.go

@@ -49,6 +49,8 @@ import (
 )
 
 // ClusterClassRolloutSpecInput is the input for ClusterClassRolloutSpec.
+// FIXME(sbueringer):doc: review the entire PR and add more godoc
+// FIXME(sbueringer): review the entire PR and check if we can move more to util funcs, split up more funcs
 type ClusterClassRolloutSpecInput struct {
 	E2EConfig             *clusterctl.E2EConfig
 	ClusterctlConfigPath  string
@@ -59,7 +61,7 @@ type ClusterClassRolloutSpecInput struct {
 
 	// Flavor is the cluster-template flavor used to create the Cluster for testing.
 	// NOTE: The template must be using a ClusterClass.
-	// FIXME(sbueringer): add more requirements
+	// FIXME(sbueringer):doc: add more requirements
 	Flavor string
 
 	// ModifyControlPlaneFields are the ControlPlane fields which will be set on the
@@ -69,7 +71,7 @@ type ClusterClassRolloutSpecInput struct {
 	// map[string]interface{}{
 	//   "spec.path.to.field": <value>,
 	// }
-	// FIXME(sbueringer): should be a field which triggers a Machine rollout
+	// FIXME(sbueringer):doc: should be a field which triggers a Machine rollout
 	ModifyControlPlaneFields map[string]interface{}
 
 	// ModifyMachineDeploymentBootstrapConfigTemplateFields are the fields which will be set on the
@@ -92,7 +94,7 @@ type ClusterClassRolloutSpecInput struct {
 }
 
 // ClusterClassRolloutSpec implements a test that verifies that ClusterClass changes are rolled out successfully.
-// FIXME(sbueringer)
+// FIXME(sbueringer):doc document this test is specifically for KCP + CABPK + MD/MS rollouts
 // Thus, the test consists of the following steps:
 //   - Deploy Cluster using a ClusterClass and wait until it is fully provisioned.
 //   - Modify the ControlPlaneTemplate of the ClusterClass by setting ModifyControlPlaneFields
@@ -162,8 +164,6 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 
 		assertMetadataOnClusterObjects(ctx, input.BootstrapClusterProxy, clusterResources.Cluster, clusterResources.ClusterClass)
 
-		// FIXME: Verify all labels/annotations exactly on all objects (including nodes, inframachine, bootstrapconfig?) (fail on CAPI internal labels/annotations)
-
 		By("Rolling out changes to control plane and MachineDeployment (in-place)")
 
 		By("Modifying the control plane configuration in Cluster and wait for changes to be applied to the control plane object (in-place)")
@@ -184,10 +184,6 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 			},
 			WaitForControlPlane: input.E2EConfig.GetIntervals(specName, "wait-control-plane"),
 		})
-
-		// FIXME: Verify all labels/annotations exactly on all objects (including nodes, inframachine, bootstrapconfig?) (fail on CAPI internal labels/annotations)
-		// FIXME: Verify Machines are still the same
-
 		By("Modifying the MachineDeployment configuration in ClusterClass and wait for changes to be applied to the MachineDeployment objects (in-place")
 		modifyMachineDeploymentViaClusterAndWait(ctx, modifyMachineDeploymentViaClusterAndWaitInput{
 			ClusterProxy: input.BootstrapClusterProxy,
@@ -219,8 +215,9 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
 		})
 
-		// FIXME: Verify all labels/annotations exactly on all objects (including nodes, inframachine, bootstrapconfig?) (fail on CAPI internal labels/annotations)
-		// * Wait for machines & they are still the same
+		assertMetadataOnClusterObjects(ctx, input.BootstrapClusterProxy, clusterResources.Cluster, clusterResources.ClusterClass)
+
+		// FIXME: Verify machines are still the same (assert already waited for the in-place rollout)
 
 		By("Rolling out changes to control plane and MachineDeployment (rollout)")
 
@@ -232,10 +229,6 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 			ModifyControlPlaneFields: input.ModifyControlPlaneFields,
 			WaitForControlPlane:      input.E2EConfig.GetIntervals(specName, "wait-control-plane"),
 		})
-
-		// FIXME: Verify all labels/annotations exactly on all objects (including nodes, inframachine, bootstrapconfig?) (fail on CAPI internal labels/annotations)
-		// * Wait for machines & they are new
-
 		By("Modifying the MachineDeployment configuration in ClusterClass and wait for changes to be applied to the MachineDeployment objects (rollout")
 		modifyMachineDeploymentViaClusterClassAndWait(ctx, modifyMachineDeploymentViaClusterClassAndWaitInput{
 			ClusterProxy:                        input.BootstrapClusterProxy,
@@ -246,9 +239,9 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 			WaitForMachineDeployments:                 input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
 		})
 
-		// FIXME: Verify all labels/annotations exactly on all objects (including nodes, inframachine, bootstrapconfig?) (fail on CAPI internal labels/annotations)
-		// * Wait for machines & they are new
-		// * Verify old MachineSet is not changed
+		assertMetadataOnClusterObjects(ctx, input.BootstrapClusterProxy, clusterResources.Cluster, clusterResources.ClusterClass)
+
+		// FIXME: Verify machines are all rotated (assert already waited for the full rollout)
 
 		By("PASSED!")
 	})
@@ -259,10 +252,11 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas
 	})
 }
 
-// FIXME: ensure we wait until everything is reconciled enough before we call this func
-// FIXME: extract util stuff into util funcs, also the whole get all objects thing
-// FIXME: check assertions have the right semantics (i.e. they use the right labels/annotations => based on picture)
-// FIXME: double check ordering vs code
+// FIXME: Ensure this func gracefully retries if the rollout is not complete (e.g. node might not exist)
+// FIXME: Check assertions have the right semantics (i.e. they use the right labels/annotations => based on picture)
+// FIXME: Double check ordering of labels/annotations vs our code paths
+// FIXME: Split up func in smaller assertion funcs
+// FIXME: Maybe extract util stuff into util funcs
 func assertMetadataOnClusterObjects(ctx context.Context, clusterProxy framework.ClusterProxy, cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) {
 	By("Checking cluster objects have the right labels, annotations and selectors")
 
@@ -323,15 +317,15 @@ func assertMetadataOnClusterObjects(ctx context.Context, clusterProxy framework.
 				},
 				cluster.Spec.Topology.ControlPlane.Metadata.Labels,
 				clusterClass.Spec.ControlPlane.Metadata.Labels,
-				//machineTemplateMetadata(clusterClassObjects.ControlPlaneTemplate).Labels, // FIXME: shouldn't work atm (fix CC reconciler)
+				//machineTemplateMetadata(clusterClassObjects.ControlPlaneTemplate).Labels, // FIXME: doesn't work atm (fix CC reconciler)
 			),
 		))
 
 		g.Expect(controlPlaneMachineTemplateMetadata.Annotations).To(BeEquivalentTo(
 			union(
 				cluster.Spec.Topology.ControlPlane.Metadata.Annotations,
 				clusterClass.Spec.ControlPlane.Metadata.Annotations,
-				//machineTemplateMetadata(clusterClassObjects.ControlPlaneTemplate).Annotations, // FIXME: shouldn't work atm (fix CC reconciler)
+				//machineTemplateMetadata(clusterClassObjects.ControlPlaneTemplate).Annotations, // FIXME: doesn't work atm (fix CC reconciler)
 			),
 		))
 
@@ -560,6 +554,7 @@ func assertMetadataOnClusterObjects(ctx context.Context, clusterProxy framework.
 
 			// MachineDeployment MachineSet
 			for _, machineSet := range clusterObjects.MachineSetsByMachineDeployment[machineDeployment.Name] {
+				// FIXME: skip old MachineSet
 				machineTemplateHash := machineSet.Labels[clusterv1.MachineDeploymentUniqueLabel]
 
 				g.Expect(machineSet.Labels).To(BeEquivalentTo(
@@ -676,7 +671,7 @@ func assertMetadataOnClusterObjects(ctx context.Context, clusterProxy framework.
 			}
 		}
 
-		fmt.Println("FIXME: breakpoint")
+		By("All cluster objects have the right labels, annotations and selectors")
 	}, 10*time.Minute, 1*time.Second).Should(Succeed()) // FIXME: timeouts, look at actual rollout times
 }
 
@@ -713,7 +708,8 @@ func groupKind(ref *corev1.ObjectReference) string {
 	return gk.String()
 }
 
-// FIXME(sbueringer) move to contract (+ use must func)
+// FIXME(sbueringer) move to contract (+ use must func), also for other usages of contract + check if we can optimize contract call length
+// e.g. contract.ControlPlane().MachineTemplate() => controlPlaneMachineTemplate := contract.ControlPlane().MachineTemplate()
 func templateMetadata(obj *unstructured.Unstructured) *clusterv1.ObjectMeta {
 	labelsValue, ok, err := unstructured.NestedStringMap(obj.UnstructuredContent(), "spec", "template", "metadata", "labels")
 	Expect(err).ToNot(HaveOccurred())
@@ -733,26 +729,6 @@ func templateMetadata(obj *unstructured.Unstructured) *clusterv1.ObjectMeta {
 	}
 }
 
-// FIXME(sbueringer) move to contract (+ use must func)
-func machineTemplateMetadata(obj *unstructured.Unstructured) *clusterv1.ObjectMeta {
-	labelsValue, ok, err := unstructured.NestedStringMap(obj.UnstructuredContent(), "spec", "template", "spec", "machineTemplate", "metadata", "labels")
-	Expect(err).ToNot(HaveOccurred())
-	if !ok {
-		labelsValue = map[string]string{}
-	}
-
-	annotationsValue, ok, err := unstructured.NestedStringMap(obj.UnstructuredContent(), "spec", "template", "spec", "machineTemplate", "metadata", "annotations")
-	Expect(err).ToNot(HaveOccurred())
-	if !ok {
-		annotationsValue = map[string]string{}
-	}
-
-	return &clusterv1.ObjectMeta{
-		Labels:      labelsValue,
-		Annotations: annotationsValue,
-	}
-}
-
 type unionMap map[string]string
 
 // union merges maps. keys from earlier maps are preserved over keys from later maps.
@@ -811,7 +787,6 @@ type clusterClassObjects struct {
 	BootstrapConfigTemplateByMachineDeploymentClass       map[string]*unstructured.Unstructured
 }
 
-// FIXME: ensure bootstrap is optional everywhere
 func getClusterClassObjects(ctx context.Context, g Gomega, clusterProxy framework.ClusterProxy, clusterClass *clusterv1.ClusterClass) clusterClassObjects {
 	mgmtClient := clusterProxy.GetClient()
 
@@ -835,11 +810,9 @@ func getClusterClassObjects(ctx context.Context, g Gomega, clusterProxy framewor
 		g.Expect(err).ToNot(HaveOccurred())
 		res.InfrastructureMachineTemplateByMachineDeploymentClass[mdClass.Class] = infrastructureMachineTemplate
 
-		if mdClass.Template.Bootstrap.Ref != nil {
-			bootstrapConfigTemplate, err := external.Get(ctx, mgmtClient, mdClass.Template.Bootstrap.Ref, clusterClass.Namespace)
-			g.Expect(err).ToNot(HaveOccurred())
-			res.BootstrapConfigTemplateByMachineDeploymentClass[mdClass.Class] = bootstrapConfigTemplate
-		}
+		bootstrapConfigTemplate, err := external.Get(ctx, mgmtClient, mdClass.Template.Bootstrap.Ref, clusterClass.Namespace)
+		g.Expect(err).ToNot(HaveOccurred())
+		res.BootstrapConfigTemplateByMachineDeploymentClass[mdClass.Class] = bootstrapConfigTemplate
 	}
 
 	return res
@@ -864,7 +837,7 @@ type clusterObjects struct {
 	BootstrapConfigByMachine       map[string]*unstructured.Unstructured
 }
 
-// FIXME(sbueringer) add godoc (which should help with structure
+// FIXME(sbueringer):doc add godoc (which should help with structure)
 func getClusterObjects(ctx context.Context, g Gomega, clusterProxy framework.ClusterProxy, cluster *clusterv1.Cluster) clusterObjects {
 	mgmtClient := clusterProxy.GetClient()
 	workloadClient := clusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name).GetClient()
@@ -912,11 +885,9 @@ func getClusterObjects(ctx context.Context, g Gomega, clusterProxy framework.Clu
 		md := mdList.Items[0]
 		res.MachineDeployments = append(res.MachineDeployments, &md)
 
-		if md.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
-			bootstrapConfigTemplate, err := external.Get(ctx, mgmtClient, md.Spec.Template.Spec.Bootstrap.ConfigRef, cluster.Namespace)
-			g.Expect(err).ToNot(HaveOccurred())
-			res.BootstrapConfigTemplateByMachineDeployment[md.Name] = bootstrapConfigTemplate
-		}
+		bootstrapConfigTemplate, err := external.Get(ctx, mgmtClient, md.Spec.Template.Spec.Bootstrap.ConfigRef, cluster.Namespace)
+		g.Expect(err).ToNot(HaveOccurred())
+		res.BootstrapConfigTemplateByMachineDeployment[md.Name] = bootstrapConfigTemplate
 
 		infrastructureMachineTemplate, err := external.Get(ctx, mgmtClient, &md.Spec.Template.Spec.InfrastructureRef, cluster.Namespace)
 		g.Expect(err).ToNot(HaveOccurred())
@@ -944,11 +915,9 @@ func getClusterObjects(ctx context.Context, g Gomega, clusterProxy framework.Clu
 }
 
 func addMachineObjects(ctx context.Context, g Gomega, res clusterObjects, mgmtClient, workloadClient client.Client, cluster *clusterv1.Cluster, machine *clusterv1.Machine) {
-	if machine.Spec.Bootstrap.ConfigRef != nil {
-		bootstrapConfig, err := external.Get(ctx, mgmtClient, machine.Spec.Bootstrap.ConfigRef, cluster.Namespace)
-		g.Expect(err).ToNot(HaveOccurred())
-		res.BootstrapConfigByMachine[machine.Name] = bootstrapConfig
-	}
+	bootstrapConfig, err := external.Get(ctx, mgmtClient, machine.Spec.Bootstrap.ConfigRef, cluster.Namespace)
+	g.Expect(err).ToNot(HaveOccurred())
+	res.BootstrapConfigByMachine[machine.Name] = bootstrapConfig
 
 	infrastructureMachine, err := external.Get(ctx, mgmtClient, &machine.Spec.InfrastructureRef, cluster.Namespace)
 	g.Expect(err).ToNot(HaveOccurred())
@@ -971,7 +940,6 @@ type modifyControlPlaneViaClusterAndWaitInput struct {
 // modifyControlPlaneViaClusterAndWait modifies the ControlPlaneTemplate of a ClusterClass by setting ModifyControlPlaneFields
 // and waits until the changes are rolled out to the ControlPlane of the Cluster.
 // NOTE: This helper is really specific to this test, so we are keeping this private vs. adding it to the framework.
-// FIXME(sbueringer)
 func modifyControlPlaneViaClusterAndWait(ctx context.Context, input modifyControlPlaneViaClusterAndWaitInput) {
 	Expect(ctx).NotTo(BeNil(), "ctx is required for modifyControlPlaneViaClusterAndWait")
 	Expect(input.ClusterProxy).ToNot(BeNil(), "Invalid argument. input.ClusterProxy can't be nil when calling modifyControlPlaneViaClusterAndWait")
@@ -1013,7 +981,6 @@ type modifyMachineDeploymentViaClusterAndWaitInput struct {
 // modifyMachineDeploymentViaClusterAndWait modifies the BootstrapConfigTemplate of MachineDeploymentClasses of a ClusterClass
 // by setting ModifyBootstrapConfigTemplateFields and waits until the changes are rolled out to the MachineDeployments of the Cluster.
 // NOTE: This helper is really specific to this test, so we are keeping this private vs. adding it to the framework.
-// FIXME(sbueringer)
 func modifyMachineDeploymentViaClusterAndWait(ctx context.Context, input modifyMachineDeploymentViaClusterAndWaitInput) {
 	Expect(ctx).NotTo(BeNil(), "ctx is required for modifyMachineDeploymentViaClusterAndWait")
 	Expect(input.ClusterProxy).ToNot(BeNil(), "Invalid argument. input.ClusterProxy can't be nil when calling modifyMachineDeploymentViaClusterAndWait")