address review comments

internal/controllers/topology/cluster/conditions.go

@@ -128,18 +128,18 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 			reason = clusterv1.TopologyReconciledControlPlaneUpgradePendingReason
 		case s.UpgradeTracker.MachineDeployments.IsAnyPendingUpgrade():
 			fmt.Fprintf(msgBuilder, "MachineDeployment(s) %s rollout and upgrade to version %s on hold.",
-				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.PendingUpgradeNames()),
+				computeNameList(s.UpgradeTracker.MachineDeployments.PendingUpgradeNames()),
 				s.Blueprint.Topology.Version,
 			)
 			reason = clusterv1.TopologyReconciledMachineDeploymentsUpgradePendingReason
 		case s.UpgradeTracker.MachineDeployments.IsAnyPendingCreate():
-			fmt.Fprintf(msgBuilder, "MachineDeployments for Topologies %s creation on hold.",
-				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.PendingCreateTopologyNames()),
+			fmt.Fprintf(msgBuilder, "MachineDeployment(s) for Topologies %s creation on hold.",
+				computeNameList(s.UpgradeTracker.MachineDeployments.PendingCreateTopologyNames()),
 			)
 			reason = clusterv1.TopologyReconciledMachineDeploymentsCreatePendingReason
 		case s.UpgradeTracker.MachineDeployments.DeferredUpgrade():
 			fmt.Fprintf(msgBuilder, "MachineDeployment(s) %s rollout and upgrade to version %s deferred.",
-				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.DeferredUpgradeNames()),
+				computeNameList(s.UpgradeTracker.MachineDeployments.DeferredUpgradeNames()),
 				s.Blueprint.Topology.Version,
 			)
 			reason = clusterv1.TopologyReconciledMachineDeploymentsUpgradeDeferredReason
@@ -161,7 +161,7 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 
 		case len(s.UpgradeTracker.MachineDeployments.UpgradingNames()) > 0:
 			fmt.Fprintf(msgBuilder, " MachineDeployment(s) %s are upgrading",
-				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.UpgradingNames()),
+				computeNameList(s.UpgradeTracker.MachineDeployments.UpgradingNames()),
 			)
 		}
 
@@ -188,12 +188,13 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 	return nil
 }
 
-// computeMachineDeploymentNameList computes the MD name list to be shown in conditions.
-// It shortens the list to at most 5 MachineDeployment names.
-func computeMachineDeploymentNameList(mdList []string) any {
-	if len(mdList) > 5 {
-		mdList = append(mdList[:5], "...")
+// computeNameList computes list of names form the given list to be shown in conditions.
+// It shortens the list to at most 5 names and adds an ellipsis at the end if the list
+// has more than 5 elements.
+func computeNameList(list []string) any {
+	if len(list) > 5 {
+		list = append(list[:5], "...")
 	}
 
-	return strings.Join(mdList, ", ")
+	return strings.Join(list, ", ")
 }

internal/controllers/topology/cluster/conditions_test.go

@@ -362,7 +362,7 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 			},
 			wantConditionStatus:  corev1.ConditionFalse,
 			wantConditionReason:  clusterv1.TopologyReconciledMachineDeploymentsCreatePendingReason,
-			wantConditionMessage: "MachineDeployments for Topologies md0 creation on hold. Control plane is reconciling desired replicas",
+			wantConditionMessage: "MachineDeployment(s) for Topologies md0 creation on hold. Control plane is reconciling desired replicas",
 		},
 		{
 			name:         "should set the condition to true if control plane picked the new version and is upgrading but there are no machine deployments",
@@ -688,7 +688,7 @@ func TestComputeMachineDeploymentNameList(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
-			g.Expect(computeMachineDeploymentNameList(tt.mdList)).To(Equal(tt.expected))
+			g.Expect(computeNameList(tt.mdList)).To(Equal(tt.expected))
 		})
 	}
 }

internal/controllers/topology/cluster/desired_state.go

@@ -816,17 +816,13 @@ func isControlPlaneStable(s *scope.Scope) bool {
 
 	// If control plane supports replicas, check if the control plane is in the middle of a scale operation.
 	// If the current control plane is scaling then it is not considered stable.
-	if s.Blueprint.Topology.ControlPlane.Replicas != nil {
-		if s.UpgradeTracker.ControlPlane.IsScaling {
-			return false
-		}
+	if s.UpgradeTracker.ControlPlane.IsScaling {
+		return false
 	}
 
 	// Check if we are about to upgrade the control plane. Since the control plane is about to start its upgrade process
 	// it cannot be considered stable.
 	if s.UpgradeTracker.ControlPlane.IsStartingUpgrade {
-		// The versions of the current and desired control planes do no match,
-		// implies we are about to upgrade the control plane.
 		return false
 	}
 

internal/controllers/topology/cluster/desired_state_test.go

@@ -954,6 +954,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 			hookResponse       *runtimehooksv1.AfterControlPlaneUpgradeResponse
 			wantIntentToCall   bool
 			wantHookToBeCalled bool
+			wantHookToBlock    bool
 			wantErr            bool
 		}{
 			{
@@ -1076,6 +1077,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 				hookResponse:       nonBlockingResponse,
 				wantIntentToCall:   false,
 				wantHookToBeCalled: true,
+				wantHookToBlock:    false,
 				wantErr:            false,
 			},
 			{
@@ -1108,6 +1110,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 				hookResponse:       blockingResponse,
 				wantIntentToCall:   true,
 				wantHookToBeCalled: true,
+				wantHookToBlock:    true,
 				wantErr:            false,
 			},
 			{
@@ -1167,6 +1170,9 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 				g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.AfterControlPlaneUpgrade) == 1).To(Equal(tt.wantHookToBeCalled))
 				g.Expect(hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, tt.s.Current.Cluster)).To(Equal(tt.wantIntentToCall))
 				g.Expect(err != nil).To(Equal(tt.wantErr))
+				if tt.wantHookToBeCalled && !tt.wantErr {
+					g.Expect(tt.s.HookResponseTracker.IsBlocking(runtimehooksv1.AfterControlPlaneUpgrade)).To(Equal(tt.wantHookToBlock))
+				}
 			})
 		}
 	})
@@ -1735,11 +1741,14 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
 		expectPendingUpgrade                 bool
 	}{
 		{
-			name:                          "should return cluster.spec.topology.version if creating a new machine deployment",
+			name:                          "should return cluster.spec.topology.version if creating a new machine deployment and if control plane is stable",
 			currentMachineDeploymentState: nil,
-			topologyVersion:               "v1.2.3",
-			expectedVersion:               "v1.2.3",
-			expectPendingCreate:           false,
+			machineDeploymentTopology: clusterv1.MachineDeploymentTopology{
+				Name: "md-topology-1",
+			},
+			topologyVersion:     "v1.2.3",
+			expectedVersion:     "v1.2.3",
+			expectPendingCreate: false,
 		},
 		{
 			name:                "should return cluster.spec.topology.version if creating a new machine deployment and if control plane is not stable - marked as pending create",

internal/controllers/topology/cluster/reconcile_state.go

@@ -481,7 +481,7 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, s *scope.Scope
 	if !ok || mdTopologyName == "" {
 		// Note: This is only an additional safety check and should not happen. The label will always be added when computing
 		// the desired MachineDeployment.
-		return fmt.Errorf("new MachineDeployment is missing the %q label", clusterv1.ClusterTopologyMachineDeploymentNameLabel)
+		return errors.Errorf("new MachineDeployment is missing the %q label", clusterv1.ClusterTopologyMachineDeploymentNameLabel)
 	}
 	// Return early if the MachineDeployment is pending create.
 	if s.UpgradeTracker.MachineDeployments.IsPendingCreate(mdTopologyName) {

internal/controllers/topology/cluster/reconcile_state_test.go

@@ -533,6 +533,43 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 			wantHookToBeCalled: false,
 			wantError:          false,
 		},
+		{
+			name: "hook should not be called if the control plane is starting a new upgrade - hook is marked",
+			s: &scope.Scope{
+				Blueprint: &scope.ClusterBlueprint{
+					Topology: &clusterv1.Topology{
+						ControlPlane: clusterv1.ControlPlaneTopology{
+							Replicas: pointer.Int32(2),
+						},
+					},
+				},
+				Current: &scope.ClusterState{
+					Cluster: &clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-cluster",
+							Namespace: "test-ns",
+							Annotations: map[string]string{
+								runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
+							},
+						},
+						Spec: clusterv1.ClusterSpec{},
+					},
+					ControlPlane: &scope.ControlPlaneState{
+						Object: controlPlaneObj,
+					},
+				},
+				HookResponseTracker: scope.NewHookResponseTracker(),
+				UpgradeTracker: func() *scope.UpgradeTracker {
+					ut := scope.NewUpgradeTracker()
+					ut.ControlPlane.IsStartingUpgrade = true
+					return ut
+				}(),
+			},
+			wantMarked:         true,
+			hookResponse:       successResponse,
+			wantHookToBeCalled: false,
+			wantError:          false,
+		},
 		{
 			name: "hook should not be called if the control plane is upgrading - hook is marked",
 			s: &scope.Scope{

internal/controllers/topology/cluster/scope/upgradetracker.go

@@ -60,6 +60,7 @@ type ControlPlaneUpgradeTracker struct {
 	// - IsScaling will be true if the current ControlPlane is scaling.
 	// - IsScaling will not be true if the current Control Plane is stable and the reconcile loop is going to scale the Control Plane.
 	// Note: Refer to control plane contract for definition of scaling.
+	// Note: IsScaling will be false if the Control Plane does not support replicas.
 	IsScaling bool
 }
 
@@ -164,8 +165,8 @@ func (m *MachineDeploymentUpgradeTracker) MarkPendingCreate(mdTopologyName strin
 }
 
 // IsPendingCreate returns true is the MachineDeployment topology is marked as pending create.
-func (m *MachineDeploymentUpgradeTracker) IsPendingCreate(name string) bool {
-	return m.pendingCreateTopologyNames.Has(name)
+func (m *MachineDeploymentUpgradeTracker) IsPendingCreate(mdTopologyName string) bool {
+	return m.pendingCreateTopologyNames.Has(mdTopologyName)
 }
 
 // IsAnyPendingCreate returns true if any of the machine deployments are pending