diff --git a/internal/controllers/topology/cluster/conditions.go b/internal/controllers/topology/cluster/conditions.go index 2bb6463829c0..ed0140ed66d6 100644 --- a/internal/controllers/topology/cluster/conditions.go +++ b/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, ", ") } diff --git a/internal/controllers/topology/cluster/conditions_test.go b/internal/controllers/topology/cluster/conditions_test.go index ce2d110462ea..d2b21aa3af55 100644 --- a/internal/controllers/topology/cluster/conditions_test.go +++ b/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)) }) } } diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 4fcc316a3b56..ce222170c28a 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/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 } diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 65fbc60a72a5..684d13b0f72c 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/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", diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index acfb0f6cf63b..ef4b11a39d14 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/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) { diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 50e8eb1f7fa1..32363fc62abd 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/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{ diff --git a/internal/controllers/topology/cluster/scope/upgradetracker.go b/internal/controllers/topology/cluster/scope/upgradetracker.go index 55f7c37c86f6..3ac1590365a9 100644 --- a/internal/controllers/topology/cluster/scope/upgradetracker.go +++ b/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