upgrading control plane should be blocked if MD are upgrading

Author: Yuvaraj Kakaraparthi

internal/controllers/topology/cluster/conditions.go

@@ -137,11 +137,6 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 			fmt.Fprintf(msgBuilder, " MachineDeployment(s) %s are upgrading",
 				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.UpgradingNames()),
 			)
-
-		case s.Current.MachineDeployments.IsAnyRollingOut():
-			fmt.Fprintf(msgBuilder, " MachineDeployment(s) %s are rolling out",
-				computeMachineDeploymentNameList(s.UpgradeTracker.MachineDeployments.RolloutNames()),
-			)
 		}
 
 		conditions.Set(

internal/controllers/topology/cluster/conditions_test.go

@@ -195,7 +195,7 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 			wantConditionMessage: "Control plane upgrade to v1.22.0 on hold. Control plane is reconciling desired replicas",
 		},
 		{
-			name:         "should set the condition to false if new version is not picked up because at least one of the machine deployment is rolling out",
+			name:         "should set the condition to false if new version is not picked up because at least one of the machine deployment is upgrading",
 			reconcileErr: nil,
 			cluster:      &clusterv1.Cluster{},
 			s: &scope.Scope{
@@ -230,14 +230,14 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 				UpgradeTracker: func() *scope.UpgradeTracker {
 					ut := scope.NewUpgradeTracker()
 					ut.ControlPlane.PendingUpgrade = true
-					ut.MachineDeployments.MarkRollingOut("md0-abc123")
+					ut.MachineDeployments.MarkUpgrading("md0-abc123")
 					return ut
 				}(),
 				HookResponseTracker: scope.NewHookResponseTracker(),
 			},
 			wantConditionStatus:  corev1.ConditionFalse,
 			wantConditionReason:  clusterv1.TopologyReconciledControlPlaneUpgradePendingReason,
-			wantConditionMessage: "Control plane upgrade to v1.22.0 on hold. MachineDeployment(s) md0-abc123 are rolling out",
+			wantConditionMessage: "Control plane upgrade to v1.22.0 on hold. MachineDeployment(s) md0-abc123 are upgrading",
 		},
 		{
 			name:         "should set the condition to false if control plane picked the new version but machine deployments did not because control plane is upgrading",
@@ -450,7 +450,7 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 				UpgradeTracker: func() *scope.UpgradeTracker {
 					ut := scope.NewUpgradeTracker()
 					ut.ControlPlane.PendingUpgrade = false
-					ut.MachineDeployments.MarkUpgradingAndRollingOut("md0-abc123")
+					ut.MachineDeployments.MarkUpgrading("md0-abc123")
 					ut.MachineDeployments.MarkPendingUpgrade("md1-abc123")
 					return ut
 				}(),

internal/controllers/topology/cluster/desired_state.go

@@ -63,6 +63,17 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (*
 		}
 	}
 
+	// Mark all the MachineDeployments that are currently upgrading.
+	// This captured information is used for:
+	// - Building the TopologyReconciled condition.
+	// - Make upgrade decisions on the control plane.
+	// - Making upgrade decisions on machine deployments.
+	mdUpgradingNames, err := s.Current.MachineDeployments.Upgrading(ctx, r.Client)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to check if any MachineDeployment is upgrading")
+	}
+	s.UpgradeTracker.MachineDeployments.MarkUpgrading(mdUpgradingNames...)
+
 	// Compute the desired state of the ControlPlane object, eventually adding a reference to the
 	// InfrastructureMachineTemplate generated by the previous step.
 	if desiredState.ControlPlane.Object, err = r.computeControlPlane(ctx, s, desiredState.ControlPlane.InfrastructureMachineTemplate); err != nil {
@@ -424,10 +435,10 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 	}
 
 	// If the control plane is not upgrading or scaling, we can assume the control plane is stable.
-	// However, we should also check for the MachineDeployments to be stable.
-	// If the MachineDeployments are rolling out (still completing a previous upgrade), then do not pick
-	// up the desiredVersion yet. We will pick up the new version after the MachineDeployments are stable.
-	if s.Current.MachineDeployments.IsAnyRollingOut() {
+	// However, we should also check for the MachineDeployments upgrading.
+	// If the MachineDeployments are upgrading, then do not pick up the desiredVersion yet.
+	// We will pick up the new version after the MachineDeployments finish upgrading.
+	if len(s.UpgradeTracker.MachineDeployments.UpgradingNames()) > 0 {
 		return *currentVersion, nil
 	}
 
@@ -524,21 +535,6 @@ func calculateRefDesiredAPIVersion(currentRef *corev1.ObjectReference, desiredRe
 
 // computeMachineDeployments computes the desired state of the list of MachineDeployments.
 func (r *Reconciler) computeMachineDeployments(ctx context.Context, s *scope.Scope, desiredControlPlaneState *scope.ControlPlaneState) (scope.MachineDeploymentsStateMap, error) {
-	// Mark all the MachineDeployments that are currently upgrading.
-	// This captured information is used for:
-	// - Building the TopologyReconciled condition.
-	// - Making upgrade decisions on machine deployments.
-	upgradingMDs, err := s.Current.MachineDeployments.Upgrading(ctx, r.Client)
-	if err != nil {
-		return nil, err
-	}
-	s.UpgradeTracker.MachineDeployments.MarkUpgradingAndRollingOut(upgradingMDs...)
-
-	// Mark all MachineDeployments that are currently rolling out.
-	// This captured information is used for:
-	// - Building the TopologyReconciled condition (when control plane upgrade is pending)
-	s.UpgradeTracker.MachineDeployments.MarkRollingOut(s.Current.MachineDeployments.RollingOut()...)
-
 	machineDeploymentsStateMap := make(scope.MachineDeploymentsStateMap)
 	for _, mdTopology := range s.Blueprint.Topology.Workers.MachineDeployments {
 		desiredMachineDeployment, err := computeMachineDeployment(ctx, s, desiredControlPlaneState, mdTopology)
@@ -854,7 +850,7 @@ func computeMachineDeploymentVersion(s *scope.Scope, machineDeploymentTopology c
 
 	// Control plane and machine deployments are stable.
 	// Ready to pick up the topology version.
-	s.UpgradeTracker.MachineDeployments.MarkUpgradingAndRollingOut(currentMDState.Object.Name)
+	s.UpgradeTracker.MachineDeployments.MarkUpgrading(currentMDState.Object.Name)
 	return desiredVersion, nil
 }
 

internal/controllers/topology/cluster/desired_state_test.go

@@ -637,43 +637,6 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 	t.Run("Compute control plane version under various circumstances", func(t *testing.T) {
 		defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
 
-		// Note: the version used by the machine deployments does
-		// not affect how we're determining the control plane version.
-		// We only want to know if the machine deployments are stable.
-		//
-		// A machine deployment is considered stable if all the following are true:
-		// - md.spec.replicas == md.status.replicas
-		// - md.spec.replicas == md.status.updatedReplicas
-		// - md.spec.replicas == md.status.readyReplicas
-		// - md.Generation < md.status.observedGeneration
-		//
-		// A machine deployment is considered upgrading if any of the above conditions
-		// is false.
-		machineDeploymentStable := builder.MachineDeployment("test-namespace", "md1").
-			WithGeneration(int64(1)).
-			WithReplicas(int32(2)).
-			WithStatus(clusterv1.MachineDeploymentStatus{
-				ObservedGeneration:  2,
-				Replicas:            2,
-				UpdatedReplicas:     2,
-				AvailableReplicas:   2,
-				ReadyReplicas:       2,
-				UnavailableReplicas: 0,
-			}).
-			Build()
-		machineDeploymentRollingOut := builder.MachineDeployment("test-namespace", "md2").
-			WithGeneration(int64(1)).
-			WithReplicas(int32(2)).
-			WithStatus(clusterv1.MachineDeploymentStatus{
-				ObservedGeneration:  2,
-				Replicas:            1,
-				UpdatedReplicas:     1,
-				AvailableReplicas:   1,
-				ReadyReplicas:       1,
-				UnavailableReplicas: 0,
-			}).
-			Build()
-
 		nonBlockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
 			CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
 				CommonResponse: runtimehooksv1.CommonResponse{
@@ -708,13 +671,13 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 		}
 
 		tests := []struct {
-			name                    string
-			hookResponse            *runtimehooksv1.BeforeClusterUpgradeResponse
-			topologyVersion         string
-			controlPlaneObj         *unstructured.Unstructured
-			machineDeploymentsState scope.MachineDeploymentsStateMap
-			expectedVersion         string
-			wantErr                 bool
+			name                        string
+			hookResponse                *runtimehooksv1.BeforeClusterUpgradeResponse
+			topologyVersion             string
+			controlPlaneObj             *unstructured.Unstructured
+			upgradingMachineDeployments []string
+			expectedVersion             string
+			wantErr                     bool
 		}{
 			{
 				name:            "should return cluster.spec.topology.version if creating a new control plane",
@@ -779,7 +742,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 				expectedVersion: "v1.2.2",
 			},
 			{
-				name:            "should return controlplane.spec.version if control plane is not upgrading and not scaling and one of the machine deployments is rolling out",
+				name:            "should return controlplane.spec.version if control plane is not upgrading and not scaling and one of the machine deployments is upgrading",
 				topologyVersion: "v1.2.3",
 				controlPlaneObj: builder.ControlPlane("test1", "cp1").
 					WithSpecFields(map[string]interface{}{
@@ -794,14 +757,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 						"status.unavailableReplicas": int64(0),
 					}).
 					Build(),
-				machineDeploymentsState: scope.MachineDeploymentsStateMap{
-					"md1": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-					"md2": &scope.MachineDeploymentState{Object: machineDeploymentRollingOut},
-				},
-				expectedVersion: "v1.2.2",
+				upgradingMachineDeployments: []string{"md1"},
+				expectedVersion:             "v1.2.2",
 			},
 			{
-				name:            "should return cluster.spec.topology.version if control plane is not upgrading and not scaling and none of the machine deployments are rolling out - hook returns non blocking response",
+				name:            "should return cluster.spec.topology.version if control plane is not upgrading and not scaling and none of the machine deployments are upgrading - hook returns non blocking response",
 				hookResponse:    nonBlockingBeforeClusterUpgradeResponse,
 				topologyVersion: "v1.2.3",
 				controlPlaneObj: builder.ControlPlane("test1", "cp1").
@@ -817,11 +777,8 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 						"status.unavailableReplicas": int64(0),
 					}).
 					Build(),
-				machineDeploymentsState: scope.MachineDeploymentsStateMap{
-					"md1": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-					"md2": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-				},
-				expectedVersion: "v1.2.3",
+				upgradingMachineDeployments: []string{},
+				expectedVersion:             "v1.2.3",
 			},
 			{
 				name:            "should return the controlplane.spec.version if the BeforeClusterUpgrade hooks returns a blocking response",
@@ -840,10 +797,6 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 						"status.unavailableReplicas": int64(0),
 					}).
 					Build(),
-				machineDeploymentsState: scope.MachineDeploymentsStateMap{
-					"md1": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-					"md2": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-				},
 				expectedVersion: "v1.2.2",
 			},
 			{
@@ -863,10 +816,6 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 						"status.unavailableReplicas": int64(0),
 					}).
 					Build(),
-				machineDeploymentsState: scope.MachineDeploymentsStateMap{
-					"md1": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-					"md2": &scope.MachineDeploymentState{Object: machineDeploymentStable},
-				},
 				expectedVersion: "v1.2.2",
 				wantErr:         true,
 			},
@@ -889,12 +838,14 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 								Namespace: "test-ns",
 							},
 						},
-						ControlPlane:       &scope.ControlPlaneState{Object: tt.controlPlaneObj},
-						MachineDeployments: tt.machineDeploymentsState,
+						ControlPlane: &scope.ControlPlaneState{Object: tt.controlPlaneObj},
 					},
 					UpgradeTracker:      scope.NewUpgradeTracker(),
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				}
+				if len(tt.upgradingMachineDeployments) > 0 {
+					s.UpgradeTracker.MachineDeployments.MarkUpgrading(tt.upgradingMachineDeployments...)
+				}
 
 				runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
 					WithCatalog(catalog).
@@ -1723,7 +1674,6 @@ func TestComputeMachineDeployment(t *testing.T) {
 				s.Current.ControlPlane = &scope.ControlPlaneState{
 					Object: controlPlaneStable123,
 				}
-				s.UpgradeTracker.MachineDeployments.MarkRollingOut(s.Current.MachineDeployments.RollingOut()...)
 				desiredControlPlaneState := &scope.ControlPlaneState{
 					Object: controlPlaneStable123,
 				}
@@ -1733,7 +1683,7 @@ func TestComputeMachineDeployment(t *testing.T) {
 					Name:     "big-pool-of-machines",
 					Replicas: pointer.Int32(2),
 				}
-				s.UpgradeTracker.MachineDeployments.MarkUpgradingAndRollingOut(tt.upgradingMachineDeployments...)
+				s.UpgradeTracker.MachineDeployments.MarkUpgrading(tt.upgradingMachineDeployments...)
 				obj, err := computeMachineDeployment(ctx, s, desiredControlPlaneState, mdTopology)
 				g.Expect(err).NotTo(HaveOccurred())
 				g.Expect(*obj.Object.Spec.Template.Spec.Version).To(Equal(tt.expectedVersion))
@@ -1883,7 +1833,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
 			expectedVersion:               "v1.2.2",
 		},
 		{
-			name:                          "should return cluster.spec.topology.version if the control plane is not upgrading, not scaling, not ready to upgrade and none of the machine deployments are rolling out",
+			name:                          "should return cluster.spec.topology.version if the control plane is not upgrading, not scaling, not ready to upgrade and none of the machine deployments are upgrading",
 			currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()},
 			upgradingMachineDeployments:   []string{},
 			currentControlPlane:           controlPlaneStable123,
@@ -1892,7 +1842,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
 			expectedVersion:               "v1.2.3",
 		},
 		{
-			name:                          "should return cluster.spec.topology.version if control plane is stable, other machine deployments are rolling out, concurrency limit not reached",
+			name:                          "should return cluster.spec.topology.version if control plane is stable, other machine deployments are upgrading, concurrency limit not reached",
 			currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()},
 			upgradingMachineDeployments:   []string{"upgrading-md1"},
 			upgradeConcurrency:            2,
@@ -1902,7 +1852,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
 			expectedVersion:               "v1.2.3",
 		},
 		{
-			name:                          "should return machine deployment's spec.template.spec.version if control plane is stable, other machine deployments are rolling out, concurrency limit reached",
+			name:                          "should return machine deployment's spec.template.spec.version if control plane is stable, other machine deployments are upgrading, concurrency limit reached",
 			currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()},
 			upgradingMachineDeployments:   []string{"upgrading-md1", "upgrading-md2"},
 			upgradeConcurrency:            2,
@@ -1931,7 +1881,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
 				UpgradeTracker: scope.NewUpgradeTracker(scope.MaxMDUpgradeConcurrency(tt.upgradeConcurrency)),
 			}
 			desiredControlPlaneState := &scope.ControlPlaneState{Object: tt.desiredControlPlane}
-			s.UpgradeTracker.MachineDeployments.MarkUpgradingAndRollingOut(tt.upgradingMachineDeployments...)
+			s.UpgradeTracker.MachineDeployments.MarkUpgrading(tt.upgradingMachineDeployments...)
 			version, err := computeMachineDeploymentVersion(s, tt.machineDeploymentTopology, desiredControlPlaneState, tt.currentMachineDeploymentState)
 			g.Expect(err).NotTo(HaveOccurred())
 			g.Expect(version).To(Equal(tt.expectedVersion))

internal/controllers/topology/cluster/reconcile_state.go

@@ -264,7 +264,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
 		}
 
 		if !cpUpgrading && !cpScaling && !s.UpgradeTracker.ControlPlane.PendingUpgrade && // Control Plane checks
-			len(s.UpgradeTracker.MachineDeployments.RolloutNames()) == 0 && // Machine deployments are not rollout out or not about to roll out
+			len(s.UpgradeTracker.MachineDeployments.UpgradingNames()) == 0 && // Machine deployments are not upgrading or not about to upgrade
 			!s.UpgradeTracker.MachineDeployments.PendingUpgrade() && // No MachineDeployments have an upgrade pending
 			!s.UpgradeTracker.MachineDeployments.DeferredUpgrade() { // No MachineDeployments have an upgrade deferred
 			// Everything is stable and the cluster can be considered fully upgraded.

internal/controllers/topology/cluster/reconcile_state_test.go

@@ -685,7 +685,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 			wantError:          false,
 		},
 		{
-			name: "hook should not be called if the control plane is stable at desired version but MDs are rolling out - hook is marked",
+			name: "hook should not be called if the control plane is stable at desired version but MDs are upgrading - hook is marked",
 			s: &scope.Scope{
 				Blueprint: &scope.ClusterBlueprint{
 					Topology: &clusterv1.Topology{
@@ -713,7 +713,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 				UpgradeTracker: func() *scope.UpgradeTracker {
 					ut := scope.NewUpgradeTracker()
 					ut.ControlPlane.PendingUpgrade = false
-					ut.MachineDeployments.MarkRollingOut("md1")
+					ut.MachineDeployments.MarkUpgrading("md1")
 					return ut
 				}(),
 			},

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

@@ -26,7 +26,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil"
 )
 
 // ClusterState holds all the objects representing the state of a managed Cluster topology.
@@ -62,24 +61,6 @@ type ControlPlaneState struct {
 // MachineDeploymentsStateMap holds a collection of MachineDeployment states.
 type MachineDeploymentsStateMap map[string]*MachineDeploymentState
 
-// RollingOut returns the list of the machine deployments
-// that are rolling out.
-func (mds MachineDeploymentsStateMap) RollingOut() []string {
-	names := []string{}
-	for _, md := range mds {
-		if md.IsRollingOut() {
-			names = append(names, md.Object.Name)
-		}
-	}
-	return names
-}
-
-// IsAnyRollingOut returns true if at least one of the
-// machine deployments is rolling out. False, otherwise.
-func (mds MachineDeploymentsStateMap) IsAnyRollingOut() bool {
-	return len(mds.RollingOut()) != 0
-}
-
 // Upgrading returns the list of the machine deployments
 // that are upgrading.
 func (mds MachineDeploymentsStateMap) Upgrading(ctx context.Context, c client.Client) ([]string, error) {
@@ -112,15 +93,6 @@ type MachineDeploymentState struct {
 	MachineHealthCheck *clusterv1.MachineHealthCheck
 }
 
-// IsRollingOut determines if the machine deployment is rolling out.
-// A machine deployment is considered upgrading if:
-// - if any of the replicas of the machine deployment is not ready.
-func (md *MachineDeploymentState) IsRollingOut() bool {
-	return !mdutil.DeploymentComplete(md.Object, &md.Object.Status) ||
-		*md.Object.Spec.Replicas != md.Object.Status.ReadyReplicas ||
-		md.Object.Status.UnavailableReplicas > 0
-}
-
 // IsUpgrading determines if the MachineDeployment is upgrading.
 // A machine deployment is considered upgrading if at least one of the Machines of this
 // MachineDeployment has a different version.