refactor

internal/controllers/topology/cluster/desired_state.go

@@ -89,7 +89,7 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (*
 	// If required, compute the desired state of the MachineDeployments from the list of MachineDeploymentTopologies
 	// defined in the cluster.
 	if s.Blueprint.HasMachineDeployments() {
-		desiredState.MachineDeployments, err = r.computeMachineDeployments(ctx, s, desiredState.ControlPlane)
+		desiredState.MachineDeployments, err = r.computeMachineDeployments(ctx, s)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to compute MachineDeployments")
 		}
@@ -461,6 +461,7 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 	// Control plane and machine deployments are stable. All the required hook are called.
 	// Ready to pick up the topology version.
 	s.UpgradeTracker.ControlPlane.PendingUpgrade = false
+	s.UpgradeTracker.ControlPlane.IsStartingUpgrade = true
 	return desiredVersion, nil
 }
 
@@ -524,7 +525,7 @@ 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) {
+func (r *Reconciler) computeMachineDeployments(ctx context.Context, s *scope.Scope) (scope.MachineDeploymentsStateMap, error) {
 	// Mark all the MachineDeployments that are currently upgrading.
 	// This captured information is used for:
 	// - Building the TopologyReconciled condition.
@@ -542,7 +543,7 @@ func (r *Reconciler) computeMachineDeployments(ctx context.Context, s *scope.Sco
 
 	machineDeploymentsStateMap := make(scope.MachineDeploymentsStateMap)
 	for _, mdTopology := range s.Blueprint.Topology.Workers.MachineDeployments {
-		desiredMachineDeployment, err := computeMachineDeployment(ctx, s, desiredControlPlaneState, mdTopology)
+		desiredMachineDeployment, err := computeMachineDeployment(ctx, s, mdTopology)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to compute MachineDepoyment for topology %q", mdTopology.Name)
 		}
@@ -554,7 +555,7 @@ func (r *Reconciler) computeMachineDeployments(ctx context.Context, s *scope.Sco
 // computeMachineDeployment computes the desired state for a MachineDeploymentTopology.
 // The generated machineDeployment object is calculated using the values from the machineDeploymentTopology and
 // the machineDeployment class.
-func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlPlaneState *scope.ControlPlaneState, machineDeploymentTopology clusterv1.MachineDeploymentTopology) (*scope.MachineDeploymentState, error) {
+func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeploymentTopology clusterv1.MachineDeploymentTopology) (*scope.MachineDeploymentState, error) {
 	desiredMachineDeployment := &scope.MachineDeploymentState{}
 
 	// Gets the blueprint for the MachineDeployment class.
@@ -701,10 +702,7 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 		desiredMachineDeploymentObj.SetName(currentMachineDeployment.Object.Name)
 	}
 
-	version, err := computeMachineDeploymentVersion(s, desiredMachineDeploymentObj.Name, machineDeploymentTopology, desiredControlPlaneState, currentMachineDeployment)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to compute version for %s", machineDeploymentTopology.Name)
-	}
+	version := computeMachineDeploymentVersion(s, desiredMachineDeploymentObj.Name, machineDeploymentTopology, currentMachineDeployment)
 	desiredMachineDeploymentObj.Spec.Template.Spec.Version = pointer.String(version)
 
 	// Apply annotations
@@ -760,16 +758,23 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 // computeMachineDeploymentVersion calculates the version of the desired machine deployment.
 // The version is calculated using the state of the current machine deployments,
 // the current control plane and the version defined in the topology.
-// Nb: No MachineDeployment upgrades will be triggered while any MachineDeployment is in the middle
-// of an upgrade. Even if the number of MachineDeployments that are being upgraded is less
-// than the number of allowed concurrent upgrades.
-func computeMachineDeploymentVersion(s *scope.Scope, desiredMDName string, machineDeploymentTopology clusterv1.MachineDeploymentTopology, desiredControlPlaneState *scope.ControlPlaneState, currentMDState *scope.MachineDeploymentState) (string, error) {
+func computeMachineDeploymentVersion(s *scope.Scope, desiredMDName string, machineDeploymentTopology clusterv1.MachineDeploymentTopology, currentMDState *scope.MachineDeploymentState) string {
 	desiredVersion := s.Blueprint.Topology.Version
-	// If creating a new machine deployment, we can pick up the desired version
-	// Note: We are not blocking the creation of new machine deployments when
-	// the control plane or any of the machine deployments are upgrading/scaling.
+	// If creating a new machine deployment mark it as pending if the control plane is not
+	// yet stable. Creating a new MD while the control plane is upgrading is unsafe.
 	if currentMDState == nil || currentMDState.Object == nil {
-		return desiredVersion, nil
+		if s.UpgradeTracker.ControlPlane.IsStartingUpgrade ||
+			s.UpgradeTracker.ControlPlane.IsUpgrading ||
+			s.UpgradeTracker.ControlPlane.IsScaling ||
+			s.UpgradeTracker.ControlPlane.IsProvisioning {
+			// TODO(ykakarap): Here we are marking as pending upgrade so that the reconciliation of this MD does not
+			// occur and therefore not create the new MD. Find a better name.
+			// TODO(ykakarap): Reusing MarkPendingUpgrade is a problem because then MD creations will be counted
+			// against upgrade concurrency which should not be the case, at least according to the current concurrency
+			// definitions.
+			s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
+		}
+		return desiredVersion
 	}
 
 	// Get the current version of the machine deployment.
@@ -778,19 +783,20 @@ func computeMachineDeploymentVersion(s *scope.Scope, desiredMDName string, machi
 	// Return early if the currentVersion is already equal to the desiredVersion
 	// no further checks required.
 	if currentVersion == desiredVersion {
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// Return early if the upgrade for the MachineDeployment is deferred.
 	if isMachineDeploymentDeferred(s.Blueprint.Topology, machineDeploymentTopology) {
 		s.UpgradeTracker.MachineDeployments.MarkDeferredUpgrade(desiredMDName)
-		return currentVersion, nil
+		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
+		return currentVersion
 	}
 
 	// Return early if we are not allowed to upgrade the machine deployment.
 	if !s.UpgradeTracker.MachineDeployments.AllowUpgrade() {
 		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// If the control plane is being created (current control plane is nil), do not perform
@@ -800,69 +806,48 @@ func computeMachineDeploymentVersion(s *scope.Scope, desiredMDName string, machi
 	// but we are implementing this check for extra safety.
 	if s.Current.ControlPlane == nil || s.Current.ControlPlane.Object == nil {
 		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-		// TODO(ykakarap): Hold out creating new MD.
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// If the current control plane is upgrading, then do not pick up the desiredVersion yet.
 	// Return the current version of the machine deployment. We will pick up the new version after the control
 	// plane is stable.
-	cpUpgrading, err := contract.ControlPlane().IsUpgrading(s.Current.ControlPlane.Object)
-	if err != nil {
-		return "", errors.Wrap(err, "failed to check if control plane is upgrading")
-	}
-	if cpUpgrading {
+	if s.UpgradeTracker.ControlPlane.IsUpgrading {
 		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-		// TODO(ykakarap): Hold out creating new MD.
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// 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 do not pick up the desiredVersion yet.
 	// Return the current version of the machine deployment. We will pick up the new version after the control
 	// plane is stable.
 	if s.Blueprint.Topology.ControlPlane.Replicas != nil {
-		cpScaling, err := contract.ControlPlane().IsScaling(s.Current.ControlPlane.Object)
-		if err != nil {
-			return "", errors.Wrap(err, "failed to check if the control plane is scaling")
-		}
-		if cpScaling {
+		if s.UpgradeTracker.ControlPlane.IsScaling {
 			s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-			// TODO(ykakarap): Hold out creating new MD.
-			return currentVersion, nil
+			return currentVersion
 		}
 	}
 
 	// Check if we are about to upgrade the control plane. In that case, do not upgrade the machine deployment yet.
 	// Wait for the new upgrade operation on the control plane to finish before picking up the new version for the
 	// machine deployment.
-	currentCPVersion, err := contract.ControlPlane().Version().Get(s.Current.ControlPlane.Object)
-	if err != nil {
-		return "", errors.Wrap(err, "failed to get version of current control plane")
-	}
-	desiredCPVersion, err := contract.ControlPlane().Version().Get(desiredControlPlaneState.Object)
-	if err != nil {
-		return "", errors.Wrap(err, "failed to get version of desired control plane")
-	}
-	if *currentCPVersion != *desiredCPVersion {
+	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.
 		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-		// TODO(ykakarap): Hold out creating new MD.
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// If the ControlPlane is pending picking up an upgrade then do not pick up the new version yet.
 	if s.UpgradeTracker.ControlPlane.PendingUpgrade {
 		s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(desiredMDName)
-		// TODO(ykakarap): Hold out creating new MD.
-		return currentVersion, nil
+		return currentVersion
 	}
 
 	// Control plane and machine deployments are stable.
 	// Ready to pick up the topology version.
 	s.UpgradeTracker.MachineDeployments.MarkUpgradingAndRollingOut(desiredMDName)
-	return desiredVersion, nil
+	return desiredVersion
 }
 
 // isMachineDeploymentDeferred returns true if the upgrade for the mdTopology is deferred.