Skip to content

Commit

Permalink
ClusterClass: also consider MD unavailableReplicas for rollout
Browse files Browse the repository at this point in the history 
Signed-off-by: Stefan Büringer [email protected]
  • Loading branch information
@sbueringer
sbueringer committed Feb 9, 2023
1 parent 46c3438 commit 9d20ebf
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 50 deletions.
125 changes: 96 additions & 29 deletions internal/controllers/topology/cluster/conditions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
Expand Down Expand Up @@ -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(),
},
Expand Down Expand Up @@ -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(),
},
Expand Down Expand Up @@ -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{
Expand All @@ -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(),
},
Expand All @@ -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(),
},
Expand Down Expand Up @@ -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(),
},
Expand All @@ -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(),
},
Expand Down
45 changes: 25 additions & 20 deletions internal/controllers/topology/cluster/desired_state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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
Expand All @@ -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()

Expand Down
4 changes: 3 additions & 1 deletion internal/controllers/topology/cluster/scope/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit 9d20ebf

Please sign in to comment.