Merge pull request #13002 from fabriziopandini/improve-topology-reconciled-condition

🌱 Improve topology reconciled condition

Author: k8s-ci-robot

internal/controllers/topology/cluster/conditions.go

@@ -201,7 +201,7 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 		reason := clusterv1.ClusterTopologyReconciledClusterUpgradingReason
 		v1Beta1Reason := clusterv1.TopologyReconciledClusterUpgradingV1Beta1Reason
 
-		cpVersion, err := contract.ControlPlane().Version().Get(s.Current.ControlPlane.Object)
+		cpVersion, err := contract.ControlPlane().Version().Get(s.Desired.ControlPlane.Object)
 		if err != nil {
 			return errors.Wrap(err, "failed to get control plane spec version")
 		}
@@ -213,21 +213,15 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 
 		// If control plane is upgrading surface it, otherwise surface the pending upgrade plan.
 		if s.UpgradeTracker.ControlPlane.IsStartingUpgrade || s.UpgradeTracker.ControlPlane.IsUpgrading {
-			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s", s.Current.ControlPlane.Object.GetKind(), *cpVersion)
-			if len(s.UpgradeTracker.ControlPlane.UpgradePlan) > 0 {
-				fmt.Fprintf(msgBuilder, " (%s pending)", strings.Join(s.UpgradeTracker.ControlPlane.UpgradePlan, ", "))
-			}
+			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s%s", s.Current.ControlPlane.Object.GetKind(), *cpVersion, pendingVersions(s.UpgradeTracker.ControlPlane.UpgradePlan, *cpVersion))
 		} else if len(s.UpgradeTracker.ControlPlane.UpgradePlan) > 0 {
 			fmt.Fprintf(msgBuilder, "\n  * %s pending upgrade to version %s", s.Current.ControlPlane.Object.GetKind(), strings.Join(s.UpgradeTracker.ControlPlane.UpgradePlan, ", "))
 		}
 
 		// If MachineDeployments are upgrading surface it, if MachineDeployments are pending upgrades then surface the upgrade plans.
 		upgradingMachineDeploymentNames, pendingMachineDeploymentNames, deferredMachineDeploymentNames := dedupNames(s.UpgradeTracker.MachineDeployments)
 		if len(upgradingMachineDeploymentNames) > 0 {
-			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s", nameList("MachineDeployment", "MachineDeployments", upgradingMachineDeploymentNames), *cpVersion)
-			if len(s.UpgradeTracker.ControlPlane.UpgradePlan) > 0 {
-				fmt.Fprintf(msgBuilder, " (%s pending)", strings.Join(s.UpgradeTracker.MachineDeployments.UpgradePlan, ", "))
-			}
+			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s%s", nameList("MachineDeployment", "MachineDeployments", upgradingMachineDeploymentNames), *cpVersion, pendingVersions(s.UpgradeTracker.MachineDeployments.UpgradePlan, *cpVersion))
 		}
 
 		if len(pendingMachineDeploymentNames) > 0 && len(s.UpgradeTracker.MachineDeployments.UpgradePlan) > 0 {
@@ -255,10 +249,7 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 		// If MachinePools are upgrading surface it, if MachinePools are pending upgrades then surface the upgrade plans.
 		upgradingMachinePoolNames, pendingMachinePoolNames, deferredMachinePoolNames := dedupNames(s.UpgradeTracker.MachinePools)
 		if len(upgradingMachinePoolNames) > 0 {
-			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s", nameList("MachinePool", "MachinePools", upgradingMachinePoolNames), *cpVersion)
-			if len(s.UpgradeTracker.ControlPlane.UpgradePlan) > 0 {
-				fmt.Fprintf(msgBuilder, " (%s pending)", strings.Join(s.UpgradeTracker.MachinePools.UpgradePlan, ", "))
-			}
+			fmt.Fprintf(msgBuilder, "\n  * %s upgrading to version %s%s", nameList("MachinePool", "MachinePools", upgradingMachinePoolNames), *cpVersion, pendingVersions(s.UpgradeTracker.MachinePools.UpgradePlan, *cpVersion))
 		}
 
 		if len(pendingMachinePoolNames) > 0 && len(s.UpgradeTracker.MachinePools.UpgradePlan) > 0 {
@@ -315,6 +306,21 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 	return nil
 }
 
+// pendingVersion return a message with pending version in the upgrad plan.
+func pendingVersions(plan []string, version string) string {
+	// clone the plan to avoid side effects on the original object.
+	planWithoutVersion := []string{}
+	for _, v := range plan {
+		if v != version {
+			planWithoutVersion = append(planWithoutVersion, v)
+		}
+	}
+	if len(planWithoutVersion) > 0 {
+		return fmt.Sprintf(" (%s pending)", strings.Join(planWithoutVersion, ", "))
+	}
+	return ""
+}
+
 // dedupNames take care of names that might exist in multiple lists.
 func dedupNames(t scope.WorkerUpgradeTracker) ([]string, []string, []string) {
 	// upgrading names are preserved

internal/controllers/topology/cluster/conditions_test.go

@@ -966,6 +966,50 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 				"  * Following hooks are blocking upgrade: AfterClusterUpgrade: msg",
 		},
 
+		//  Message edge cases
+
+		{
+			name:         "Should drop fist version from pending upgrade",
+			reconcileErr: nil,
+			s: &scope.Scope{
+				Current: &scope.ClusterState{
+					Cluster: &clusterv1.Cluster{
+						Spec: clusterv1.ClusterSpec{
+							ControlPlaneRef:   clusterv1.ContractVersionedObjectReference{Name: "controlplane1"},
+							InfrastructureRef: clusterv1.ContractVersionedObjectReference{Name: "infra1"},
+							Topology: clusterv1.Topology{
+								Version: "v1.23.0",
+							},
+						},
+					},
+					ControlPlane: &scope.ControlPlaneState{
+						Object: builder.ControlPlane("ns1", "controlplane1").WithVersion("v1.22.0").Build(),
+					},
+				},
+				UpgradeTracker: func() *scope.UpgradeTracker {
+					ut := scope.NewUpgradeTracker()
+					ut.ControlPlane.UpgradePlan = []string{}
+					ut.MachineDeployments.UpgradePlan = []string{"v1.22.0", "v1.23.0"}
+					ut.MachineDeployments.MarkUpgrading("md1")
+					ut.MachineDeployments.MarkPendingUpgrade("md2")
+					ut.MachineDeployments.MarkPendingUpgrade("md3")
+					ut.MachineDeployments.MarkPendingUpgrade("md4")
+					return ut
+				}(),
+				HookResponseTracker: scope.NewHookResponseTracker(),
+			},
+			wantV1Beta1ConditionStatus: corev1.ConditionFalse,
+			wantV1Beta1ConditionReason: clusterv1.TopologyReconciledClusterUpgradingV1Beta1Reason,
+			wantV1Beta1ConditionMessage: "Cluster is upgrading to v1.23.0\n" +
+				"  * MachineDeployment md1 upgrading to version v1.22.0 (v1.23.0 pending)\n" + // v1.22.0 dropped from the pending list
+				"  * MachineDeployments md2, md3, md4 pending upgrade to version v1.22.0, v1.23.0",
+			wantConditionStatus: metav1.ConditionFalse,
+			wantConditionReason: clusterv1.ClusterTopologyReconciledClusterUpgradingReason,
+			wantConditionMessage: "Cluster is upgrading to v1.23.0\n" +
+				"  * MachineDeployment md1 upgrading to version v1.22.0 (v1.23.0 pending)\n" + // v1.22.0 dropped from the pending list
+				"  * MachineDeployments md2, md3, md4 pending upgrade to version v1.22.0, v1.23.0",
+		},
+
 		// Hold & defer upgrade
 
 		{
@@ -1129,6 +1173,12 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 
 			objs := []client.Object{}
 			if tt.s != nil && tt.s.Current != nil {
+				tt.s.Desired = &scope.ClusterState{}
+				tt.s.Desired.Cluster = tt.s.Current.Cluster.DeepCopy()
+				if tt.s.Current.ControlPlane != nil {
+					tt.s.Desired.ControlPlane = &scope.ControlPlaneState{}
+					tt.s.Desired.ControlPlane.Object = tt.s.Current.ControlPlane.Object.DeepCopy()
+				}
 				for _, md := range tt.s.Current.MachineDeployments {
 					objs = append(objs, md.Object)
 				}