ClusterClass: use namePrefix func consistently, fix MachineDeployment…

… template rotation, fix unit test panic

Signed-off-by: Stefan Büringer [email protected]

controllers/topology/desired_state.go

@@ -124,7 +124,7 @@ func computeControlPlaneInfrastructureMachineTemplate(_ context.Context, s *scop
 		template:              template,
 		templateClonedFromRef: templateClonedFromref,
 		cluster:               cluster,
-		namePrefix:            fmt.Sprintf("%s-controlplane-", cluster.Name),
+		namePrefix:            controlPlaneInfrastructureMachineTemplateNamePrefix(cluster.Name),
 		currentObjectRef:      currentRef,
 		labels:                mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels),
 		annotations:           mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations),
@@ -264,7 +264,9 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
 			ClusterName: s.Current.Cluster.Name,
 			Template: clusterv1.MachineTemplateSpec{
 				Spec: clusterv1.MachineSpec{
-					ClusterName:       s.Current.Cluster.Name,
+					ClusterName: s.Current.Cluster.Name,
+					// Sets the desired Kubernetes version for the MachineDeployment.
+					// TODO: improve this logic by adding support for version upgrade component by component
 					Version:           pointer.String(s.Blueprint.Topology.Version),
 					Bootstrap:         clusterv1.Bootstrap{ConfigRef: contract.ObjToRef(desiredMachineDeployment.BootstrapTemplate)},
 					InfrastructureRef: *contract.ObjToRef(desiredMachineDeployment.InfrastructureMachineTemplate),
@@ -293,8 +295,7 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
 	// NOTE: Topology label takes precedence on labels defined in the topology/in the ClusterClass.
 	desiredMachineDeploymentObj.Annotations = mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations)
 
-	// Sets the desired Kubernetes version for the control plane.
-	// TODO: improve this logic by adding support for version upgrade component by component
+	// Set the desired replicas.
 	desiredMachineDeploymentObj.Spec.Replicas = machineDeploymentTopology.Replicas
 
 	desiredMachineDeployment.Object = desiredMachineDeploymentObj

controllers/topology/reconcile_state.go

@@ -147,7 +147,7 @@ func (r *ClusterReconciler) reconcileMachineDeployments(ctx context.Context, s *
 	for _, mdTopologyName := range diff.toUpdate {
 		currentMD := s.Current.MachineDeployments[mdTopologyName]
 		desiredMD := s.Desired.MachineDeployments[mdTopologyName]
-		if err := r.updateMachineDeployment(ctx, s.Current.Cluster.Name, currentMD, desiredMD); err != nil {
+		if err := r.updateMachineDeployment(ctx, s.Current.Cluster.Name, mdTopologyName, currentMD, desiredMD); err != nil {
 			return err
 		}
 	}
@@ -187,15 +187,15 @@ func (r *ClusterReconciler) createMachineDeployment(ctx context.Context, md *sco
 }
 
 // updateMachineDeployment updates a MachineDeployment. Also rotates the corresponding Templates if necessary.
-func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, clusterName string, currentMD, desiredMD *scope.MachineDeploymentState) error {
+func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, clusterName string, mdTopologyName string, currentMD, desiredMD *scope.MachineDeploymentState) error {
 	log := ctrl.LoggerFrom(ctx)
 
 	cleanupOldInfrastructureTemplate, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		ref:     &desiredMD.Object.Spec.Template.Spec.InfrastructureRef,
 		current: currentMD.InfrastructureMachineTemplate,
 		desired: desiredMD.InfrastructureMachineTemplate,
 		templateNamer: func() string {
-			return infrastructureMachineTemplateNamePrefix(clusterName, desiredMD.Object.Name)
+			return infrastructureMachineTemplateNamePrefix(clusterName, mdTopologyName)
 		},
 		compatibilityChecker: check.ReferencedObjectsAreCompatible,
 	})
@@ -208,7 +208,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
 		current: currentMD.BootstrapTemplate,
 		desired: desiredMD.BootstrapTemplate,
 		templateNamer: func() string {
-			return bootstrapTemplateNamePrefix(clusterName, desiredMD.Object.Name)
+			return bootstrapTemplateNamePrefix(clusterName, mdTopologyName)
 		},
 		compatibilityChecker: check.ObjectsAreInTheSameNamespace,
 	})

controllers/topology/reconcile_state_test.go

@@ -267,14 +267,6 @@ func TestReconcileControlPlaneObject(t *testing.T) {
 			want:    &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
 			wantErr: false,
 		},
-		{
-			// Will panic due to the design of logging.
-			name:    "Attempt to update controlPlane on controlPlaneState with no infrastructureMachineTemplate",
-			class:   ccWithControlPlaneInfrastructure,
-			current: &scope.ControlPlaneState{Object: controlPlane1},
-			desired: &scope.ControlPlaneState{Object: controlPlane3},
-			wantErr: true,
-		},
 		{
 			name:    "Update to ControlPlaneObject with no underlying infrastructure",
 			class:   ccWithoutControlPlaneInfrastructure,
@@ -294,19 +286,19 @@ func TestReconcileControlPlaneObject(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// this panic catcher catches the case when there is some issue with the clusterClass controlPlaneInfrastructureCheck that causes it to falsely proceed
-			// the test case that throws this panic shows that the structure of our logs is prone to panic if some of our assumptions are off.
-			defer func() {
-				if r := recover(); r != nil {
-					if tt.wantErr {
-						err := fmt.Errorf("panic occurred during testing")
-						g.Expect(err).To(HaveOccurred())
-					}
-				}
-			}()
-
 			fakeObjs := make([]client.Object, 0)
 			s := scope.New(nil)
+
+			s.Blueprint = &scope.ClusterBlueprint{
+				ClusterClass: &clusterv1.ClusterClass{},
+			}
+			if tt.class.InfrastructureMachineTemplate != nil {
+				s.Blueprint.ClusterClass.Spec.ControlPlane.MachineInfrastructure = &clusterv1.LocalObjectTemplate{
+					Ref: contract.ObjToRef(tt.class.InfrastructureMachineTemplate),
+				}
+			}
+
+			s.Current.ControlPlane = &scope.ControlPlaneState{}
 			if tt.current != nil {
 				s.Current.ControlPlane = tt.current
 				if tt.current.Object != nil {
@@ -481,8 +473,7 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
 			if tt.current.InfrastructureMachineTemplate != nil {
 				item, err := contract.ControlPlane().InfrastructureMachineTemplate().Get(gotControlPlaneObject)
 				g.Expect(err).ToNot(HaveOccurred())
-				// This pattern should match return value in controlPlaneinfrastructureMachineTemplateNamePrefix
-				pattern := fmt.Sprintf("%s-controlplane-.*", s.Current.Cluster.Name)
+				pattern := fmt.Sprintf("%s.*", controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name))
 				fmt.Println(pattern, item.Name)
 				ok, err := regexp.Match(pattern, []byte(item.Name))
 				g.Expect(err).NotTo(HaveOccurred())

controllers/topology/util.go

@@ -39,7 +39,7 @@ func infrastructureMachineTemplateNamePrefix(clusterName, machineDeploymentTopol
 
 // infrastructureMachineTemplateNamePrefix calculates the name prefix for a InfrastructureMachineTemplate.
 func controlPlaneInfrastructureMachineTemplateNamePrefix(clusterName string) string {
-	return fmt.Sprintf("%s-controlplane-", clusterName)
+	return fmt.Sprintf("%s-control-plane-", clusterName)
 }
 
 // getReference gets the object referenced in ref.