Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add MachineSetReady condition to MachineDeployment #9262

Merged
merged 1 commit into from
Feb 6, 2024
Merged

✨ Add MachineSetReady condition to MachineDeployment #9262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/v1beta1/condition_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,14 @@ const (
// machines required (i.e. Spec.Replicas-MaxUnavailable when MachineDeploymentStrategyType = RollingUpdate) are up and running for at least minReadySeconds.
MachineDeploymentAvailableCondition ConditionType = "Available"

// MachineSetReadyCondition reports a summary of current status of the MachineSet owned by the MachineDeployment.
MachineSetReadyCondition ConditionType = "MachineSetReady"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be included in the MD conditions.SetSummary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, currently if the minReplicas is reached, the MachineDeployment will report Ready to be True, even if one of the machines is failing.
If we include this new condition in the summary, then the MachineDeployment will not be Ready if any machine fails.

Do we want that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, let's keep the scope to the minimal and avoid changing existing semantic.


// WaitingForMachineSetFallbackReason (Severity=Info) documents a MachineDeployment waiting for the underlying MachineSet
// to be available.
// NOTE: This reason is used only as a fallback when the MachineSet object is not reporting its own ready condition.
WaitingForMachineSetFallbackReason = "WaitingForMachineSet"

// WaitingForAvailableMachinesReason (Severity=Warning) reflects the fact that the required minimum number of machines for a machinedeployment are not available.
WaitingForAvailableMachinesReason = "WaitingForAvailableMachines"
)
Expand Down
11 changes: 11 additions & 0 deletions internal/controllers/machinedeployment/machinedeployment_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,17 @@ func (r *Reconciler) syncDeploymentStatus(allMSs []*clusterv1.MachineSet, newMS
} else {
conditions.MarkFalse(md, clusterv1.MachineDeploymentAvailableCondition, clusterv1.WaitingForAvailableMachinesReason, clusterv1.ConditionSeverityWarning, "Minimum availability requires %d replicas, current %d available", minReplicasNeeded, md.Status.AvailableReplicas)
}

if newMS != nil {
// Report a summary of current status of the MachineSet object owned by this MachineDeployment.
conditions.SetMirror(md, clusterv1.MachineSetReadyCondition,
newMS,
muraee marked this conversation as resolved.
Show resolved Hide resolved
conditions.WithFallbackValue(false, clusterv1.WaitingForMachineSetFallbackReason, clusterv1.ConditionSeverityInfo, ""),
)
} else {
conditions.MarkFalse(md, clusterv1.MachineSetReadyCondition, clusterv1.WaitingForMachineSetFallbackReason, clusterv1.ConditionSeverityInfo, "MachineSet not found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenarios would newMS be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before I added this check, one of the e2e panicked with a nil dereference error.
If I am not wrong, that was the case when the MD was deleting.

}

return nil
}

Expand Down
42 changes: 39 additions & 3 deletions internal/controllers/machinedeployment/machinedeployment_sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ func newTestMachineDeployment(pds *int32, replicas, statusReplicas, updatedRepli
}

// helper to create MS with given availableReplicas.
func newTestMachinesetWithReplicas(name string, specReplicas, statusReplicas, availableReplicas int32) *clusterv1.MachineSet {
func newTestMachinesetWithReplicas(name string, specReplicas, statusReplicas, availableReplicas int32, conditions clusterv1.Conditions) *clusterv1.MachineSet {
return &clusterv1.MachineSet{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -443,6 +443,7 @@ func newTestMachinesetWithReplicas(name string, specReplicas, statusReplicas, av
Status: clusterv1.MachineSetStatus{
AvailableReplicas: availableReplicas,
Replicas: statusReplicas,
Conditions: conditions,
},
}
}
Expand All @@ -460,7 +461,7 @@ func TestSyncDeploymentStatus(t *testing.T) {
name: "Deployment not available: MachineDeploymentAvailableCondition should exist and be false",
d: newTestMachineDeployment(&pds, 3, 2, 2, 2, clusterv1.Conditions{}),
oldMachineSets: []*clusterv1.MachineSet{},
newMachineSet: newTestMachinesetWithReplicas("foo", 3, 2, 2),
newMachineSet: newTestMachinesetWithReplicas("foo", 3, 2, 2, clusterv1.Conditions{}),
expectedConditions: []*clusterv1.Condition{
{
Type: clusterv1.MachineDeploymentAvailableCondition,
Expand All @@ -474,14 +475,49 @@ func TestSyncDeploymentStatus(t *testing.T) {
name: "Deployment Available: MachineDeploymentAvailableCondition should exist and be true",
d: newTestMachineDeployment(&pds, 3, 3, 3, 3, clusterv1.Conditions{}),
oldMachineSets: []*clusterv1.MachineSet{},
newMachineSet: newTestMachinesetWithReplicas("foo", 3, 3, 3),
newMachineSet: newTestMachinesetWithReplicas("foo", 3, 3, 3, clusterv1.Conditions{}),
expectedConditions: []*clusterv1.Condition{
{
Type: clusterv1.MachineDeploymentAvailableCondition,
Status: corev1.ConditionTrue,
},
},
},
{
name: "MachineSet exist: MachineSetReadyCondition should exist and mirror MachineSet Ready condition",
d: newTestMachineDeployment(&pds, 3, 3, 3, 3, clusterv1.Conditions{}),
oldMachineSets: []*clusterv1.MachineSet{},
newMachineSet: newTestMachinesetWithReplicas("foo", 3, 3, 3, clusterv1.Conditions{
{
Type: clusterv1.ReadyCondition,
Status: corev1.ConditionFalse,
Reason: "TestErrorResaon",
Message: "test error messsage",
},
}),
expectedConditions: []*clusterv1.Condition{
{
Type: clusterv1.MachineSetReadyCondition,
Status: corev1.ConditionFalse,
Reason: "TestErrorResaon",
Message: "test error messsage",
},
},
},
{
name: "MachineSet doesn't exist: MachineSetReadyCondition should exist and be false",
d: newTestMachineDeployment(&pds, 3, 3, 3, 3, clusterv1.Conditions{}),
oldMachineSets: []*clusterv1.MachineSet{},
newMachineSet: nil,
expectedConditions: []*clusterv1.Condition{
{
Type: clusterv1.MachineSetReadyCondition,
Status: corev1.ConditionFalse,
Severity: clusterv1.ConditionSeverityInfo,
Reason: clusterv1.WaitingForMachineSetFallbackReason,
},
},
},
}

for _, test := range tests {
Expand Down