From 9d20ebf0d29ea9fe12868e70c3a2966e4e363f07 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 6 Feb 2023 09:44:01 +0100 Subject: [PATCH] ClusterClass: also consider MD unavailableReplicas for rollout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../topology/cluster/conditions_test.go | 125 ++++++++++++++---- .../topology/cluster/desired_state_test.go | 45 ++++--- .../topology/cluster/scope/state.go | 4 +- 3 files changed, 124 insertions(+), 50 deletions(-) diff --git a/internal/controllers/topology/cluster/conditions_test.go b/internal/controllers/topology/cluster/conditions_test.go index dbd6d9cce998..aefadf907073 100644 --- a/internal/controllers/topology/cluster/conditions_test.go +++ b/internal/controllers/topology/cluster/conditions_test.go @@ -178,10 +178,11 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { Object: builder.MachineDeployment("ns1", "md0-abc123"). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(1), - UpdatedReplicas: int32(1), - ReadyReplicas: int32(1), - AvailableReplicas: int32(1), + Replicas: int32(1), + UpdatedReplicas: int32(1), + ReadyReplicas: int32(1), + AvailableReplicas: int32(1), + UnavailableReplicas: int32(0), }). Build(), }, @@ -220,10 +221,11 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { Object: builder.MachineDeployment("ns1", "md0-abc123"). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(2), - UpdatedReplicas: int32(2), - ReadyReplicas: int32(2), - AvailableReplicas: int32(2), + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(0), }). Build(), }, @@ -264,10 +266,11 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { Object: builder.MachineDeployment("ns1", "md0-abc123"). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(2), - UpdatedReplicas: int32(2), - ReadyReplicas: int32(2), - AvailableReplicas: int32(2), + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(0), }). Build(), }, @@ -344,7 +347,7 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { wantConditionStatus: corev1.ConditionTrue, }, { - name: "should set the condition to false is some machine deployments have not picked the new version because other machine deployments are rolling out", + name: "should set the condition to false is some machine deployments have not picked the new version because other machine deployments are rolling out (not all replicas ready)", reconcileErr: nil, cluster: &clusterv1.Cluster{}, s: &scope.Scope{ @@ -367,10 +370,12 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { WithReplicas(2). WithVersion("v1.22.0"). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(1), - UpdatedReplicas: int32(1), - ReadyReplicas: int32(1), - AvailableReplicas: int32(1), + // MD is not ready because we don't have 2 updated, ready and available replicas. + Replicas: int32(2), + UpdatedReplicas: int32(1), + ReadyReplicas: int32(1), + AvailableReplicas: int32(1), + UnavailableReplicas: int32(0), }). Build(), }, @@ -379,10 +384,70 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { WithReplicas(2). WithVersion("v1.21.2"). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(2), - UpdatedReplicas: int32(2), - ReadyReplicas: int32(2), - AvailableReplicas: int32(2), + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(0), + }). + Build(), + }, + }, + }, + UpgradeTracker: func() *scope.UpgradeTracker { + ut := scope.NewUpgradeTracker() + ut.ControlPlane.PendingUpgrade = false + ut.MachineDeployments.MarkPendingUpgrade("md1-abc123") + return ut + }(), + HookResponseTracker: scope.NewHookResponseTracker(), + }, + wantConditionStatus: corev1.ConditionFalse, + wantConditionReason: clusterv1.TopologyReconciledMachineDeploymentsUpgradePendingReason, + }, + { + name: "should set the condition to false is some machine deployments have not picked the new version because other machine deployments are rolling out (unavailable replica)", + reconcileErr: nil, + cluster: &clusterv1.Cluster{}, + s: &scope.Scope{ + Blueprint: &scope.ClusterBlueprint{ + Topology: &clusterv1.Topology{ + Version: "v1.22.0", + }, + }, + Current: &scope.ClusterState{ + Cluster: &clusterv1.Cluster{}, + ControlPlane: &scope.ControlPlaneState{ + Object: builder.ControlPlane("ns1", "controlplane1"). + WithVersion("v1.22.0"). + WithReplicas(3). + Build(), + }, + MachineDeployments: scope.MachineDeploymentsStateMap{ + "md0": &scope.MachineDeploymentState{ + Object: builder.MachineDeployment("ns1", "md0-abc123"). + WithReplicas(2). + WithVersion("v1.22.0"). + WithStatus(clusterv1.MachineDeploymentStatus{ + // MD is not ready because we still have an unavailable replica. + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(1), + }). + Build(), + }, + "md1": &scope.MachineDeploymentState{ + Object: builder.MachineDeployment("ns1", "md1-abc123"). + WithReplicas(2). + WithVersion("v1.21.2"). + WithStatus(clusterv1.MachineDeploymentStatus{ + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(0), }). Build(), }, @@ -423,10 +488,11 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { WithReplicas(2). WithVersion("v1.22.0"). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(1), - UpdatedReplicas: int32(1), - ReadyReplicas: int32(1), - AvailableReplicas: int32(1), + Replicas: int32(1), + UpdatedReplicas: int32(1), + ReadyReplicas: int32(1), + AvailableReplicas: int32(1), + UnavailableReplicas: int32(0), }). Build(), }, @@ -435,10 +501,11 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) { WithReplicas(2). WithVersion("v1.22.0"). WithStatus(clusterv1.MachineDeploymentStatus{ - Replicas: int32(2), - UpdatedReplicas: int32(2), - ReadyReplicas: int32(2), - AvailableReplicas: int32(2), + Replicas: int32(2), + UpdatedReplicas: int32(2), + ReadyReplicas: int32(2), + AvailableReplicas: int32(2), + UnavailableReplicas: int32(0), }). Build(), }, diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 8ff7bf21c836..f96a84aef3fe 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -634,22 +634,24 @@ func TestComputeControlPlaneVersion(t *testing.T) { WithGeneration(int64(1)). WithReplicas(int32(2)). WithStatus(clusterv1.MachineDeploymentStatus{ - ObservedGeneration: 2, - Replicas: 2, - UpdatedReplicas: 2, - AvailableReplicas: 2, - ReadyReplicas: 2, + 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, + ObservedGeneration: 2, + Replicas: 1, + UpdatedReplicas: 1, + AvailableReplicas: 1, + ReadyReplicas: 1, + UnavailableReplicas: 0, }). Build() @@ -1769,6 +1771,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { // - md.spec.replicas == md.status.replicas // - md.spec.replicas == md.status.updatedReplicas // - md.spec.replicas == md.status.readyReplicas + // - md.status.unavailableReplicas == 0 // - md.Generation < md.status.observedGeneration // // A machine deployment is considered upgrading if any of the above conditions @@ -1777,22 +1780,24 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { WithGeneration(1). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ - ObservedGeneration: 2, - Replicas: 2, - UpdatedReplicas: 2, - AvailableReplicas: 2, - ReadyReplicas: 2, + ObservedGeneration: 2, + Replicas: 2, + UpdatedReplicas: 2, + AvailableReplicas: 2, + ReadyReplicas: 2, + UnavailableReplicas: 0, }). Build() machineDeploymentRollingOut := builder.MachineDeployment("test-namespace", "md-2"). WithGeneration(1). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ - ObservedGeneration: 2, - Replicas: 1, - UpdatedReplicas: 1, - AvailableReplicas: 1, - ReadyReplicas: 1, + ObservedGeneration: 2, + Replicas: 1, + UpdatedReplicas: 1, + AvailableReplicas: 1, + ReadyReplicas: 1, + UnavailableReplicas: 1, }). Build() diff --git a/internal/controllers/topology/cluster/scope/state.go b/internal/controllers/topology/cluster/scope/state.go index 6c41b4b3299e..220df1ba6608 100644 --- a/internal/controllers/topology/cluster/scope/state.go +++ b/internal/controllers/topology/cluster/scope/state.go @@ -94,5 +94,7 @@ type MachineDeploymentState struct { // 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 + return !mdutil.DeploymentComplete(md.Object, &md.Object.Status) || + *md.Object.Spec.Replicas != md.Object.Status.ReadyReplicas || + md.Object.Status.UnavailableReplicas > 0 }