Skip to content

Commit

Permalink
Fix Progressing and Degraded condition during workload rollouts.
Browse files Browse the repository at this point in the history 
Degraded condtion should not become False during the deployment rollout
(DeploymentProgressing condition. There can be unavailable pods during the
rollout. This is capped by a 15 minute deadline.

Progressing condition should be True until all of the old pods have terminated
as they can still accept connections.

Co-authored-by: Standa Láznička <[email protected]>
  • Loading branch information
@atiratree @stlaz
atiratree and stlaz committed Oct 22, 2024
1 parent 4c1f8b4 commit 9d0ad43
Show file tree
Hide file tree
Showing 2 changed files with 287 additions and 5 deletions.
53 changes: 51 additions & 2 deletions pkg/operator/apiserver/controller/workload/workload.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,26 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
desiredReplicas = *(workload.Spec.Replicas)
}

selector, err := metav1.LabelSelectorAsSelector(workload.Spec.Selector)
if err != nil {
return fmt.Errorf("failed to construct label selector: %v", err)
}
matchingPods, err := c.podsLister.Pods(c.targetNamespace).List(selector)
if err != nil {
return err
}
// Terminating pods don't account for any of the other status fields but
// still can exist in a state when they are accepting connections and would
// contribute to unexpected behavior if we report Progressing=False.
// The case of too many pods might occur for example if `TerminationGracePeriodSeconds` is set.
terminatingPods := countTerminatingPods(matchingPods)

// If the workload is up to date, then we are no longer progressing
workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration
workloadIsBeingUpdated := workload.Status.UpdatedReplicas < desiredReplicas
// update is done when all pods have been updated to the latest revision
// and the deployment controller has reported NewReplicaSetAvailable
workloadIsBeingUpdated := !hasDeploymentProgressed(workload.Status)
workloadHasOldTerminatingPods := workload.Status.AvailableReplicas == desiredReplicas && terminatingPods > 0
workloadIsBeingUpdatedTooLong, err := isUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type)
if !workloadAtHighestGeneration {
deploymentProgressingCondition = deploymentProgressingCondition.
Expand All @@ -300,7 +317,12 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionTrue).
WithReason("PodsUpdating").
WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas))
WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation and %d/%d pods are available", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas, workload.Status.AvailableReplicas, desiredReplicas))
} else if workloadHasOldTerminatingPods {
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionTrue).
WithReason("PreviousGenerationPodsPresent").
WithMessage(fmt.Sprintf("deployment/%s.%s: %d pod(s) from the previous generation are still present", workload.Name, c.targetNamespace, terminatingPods))
} else {
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionFalse).
Expand Down Expand Up @@ -353,6 +375,33 @@ func isUpdatingTooLong(operatorStatus *operatorv1.OperatorStatus, progressingCon
return progressing != nil && progressing.Status == operatorv1.ConditionTrue && time.Now().After(progressing.LastTransitionTime.Add(15*time.Minute)), nil
}

// hasDeploymentProgressed returns true if the deployment reports NewReplicaSetAvailable
// via the DeploymentProgressing condition
func hasDeploymentProgressed(status appsv1.DeploymentStatus) bool {
for i := range status.Conditions {
c := status.Conditions[i]
if c.Type == appsv1.DeploymentProgressing {
return c.Status == corev1.ConditionTrue && c.Reason == "NewReplicaSetAvailable"
}
}
return false
}

func countTerminatingPods(pods []*corev1.Pod) int32 {
numberOfTerminatingPods := 0
for _, p := range pods {
if isPodTerminating(p) {
numberOfTerminatingPods += 1
}
}
return int32(numberOfTerminatingPods)
}

func isPodTerminating(p *corev1.Pod) bool {
return p.Status.Phase != corev1.PodFailed && p.Status.Phase != corev1.PodSucceeded &&
p.DeletionTimestamp != nil
}

// EnsureAtMostOnePodPerNode updates the deployment spec to prevent more than
// one pod of a given replicaset from landing on a node. It accomplishes this
// by adding a label on the template and updates the pod anti-affinity term to include that label.
Expand Down
239 changes: 236 additions & 3 deletions pkg/operator/apiserver/controller/workload/workload_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
},
Status: appsv1.DeploymentStatus{
AvailableReplicas: 0,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionFalse, LastUpdateTime: metav1.NewTime(time.Now().Add(-6 * time.Minute)), LastTransitionTime: metav1.NewTime(time.Now().Add(-6 * time.Minute)), Reason: "ProgressDeadlineExceeded", Message: "timed out"},
},
},
},
pods: []*corev1.Pod{
Expand Down Expand Up @@ -192,7 +195,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing),
Status: operatorv1.ConditionTrue,
Reason: "PodsUpdating",
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation",
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available",
},
}
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
Expand All @@ -211,6 +214,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
},
Status: appsv1.DeploymentStatus{
AvailableReplicas: 0,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"},
},
},
},
pods: []*corev1.Pod{
Expand Down Expand Up @@ -262,7 +268,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing),
Status: operatorv1.ConditionTrue,
Reason: "PodsUpdating",
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation",
Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available",
},
}
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
Expand All @@ -282,6 +288,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
Status: appsv1.DeploymentStatus{
AvailableReplicas: 2,
UpdatedReplicas: 3,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"},
},
},
},
pods: []*corev1.Pod{
Expand Down Expand Up @@ -345,6 +354,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
Status: appsv1.DeploymentStatus{
AvailableReplicas: 3,
UpdatedReplicas: 3,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"},
},
},
},
validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error {
Expand Down Expand Up @@ -389,6 +401,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
Status: appsv1.DeploymentStatus{
AvailableReplicas: 3,
ObservedGeneration: 99,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"},
},
},
},
validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error {
Expand Down Expand Up @@ -469,6 +484,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
AvailableReplicas: 2,
UpdatedReplicas: 1,
ObservedGeneration: 2,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"},
},
},
},
validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error {
Expand All @@ -493,7 +511,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing),
Status: operatorv1.ConditionTrue,
Reason: "PodsUpdating",
Message: "deployment/apiserver.openshift-apiserver: 1/3 pods have been updated to the latest generation",
Message: "deployment/apiserver.openshift-apiserver: 1/3 pods have been updated to the latest generation and 2/3 pods are available",
},
}
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
Expand All @@ -512,6 +530,9 @@ func TestUpdateOperatorStatus(t *testing.T) {
Status: appsv1.DeploymentStatus{
AvailableReplicas: 3,
UpdatedReplicas: 3,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"},
},
},
},
previousConditions: []operatorv1.OperatorCondition{
Expand Down Expand Up @@ -547,6 +568,218 @@ func TestUpdateOperatorStatus(t *testing.T) {
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
},
},
{
name: "all pods rolled out but there is an old terminating pod",
workload: &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver",
Namespace: "openshift-apiserver",
},
Spec: appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
Replicas: ptr.To[int32](2),
},
Status: appsv1.DeploymentStatus{
AvailableReplicas: 2,
ReadyReplicas: 2,
UpdatedReplicas: 2,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"},
},
},
},
pods: []*corev1.Pod{
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-1-1", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
DeletionTimestamp: ptr.To(metav1.Now()),
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-2-1", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-2-2", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
},
validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error {
expectedConditions := []operatorv1.OperatorCondition{
{
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeAvailable),
Status: operatorv1.ConditionTrue,
Reason: "AsExpected",
Message: "",
},
{
Type: fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
Status: operatorv1.ConditionFalse,
},
{
Type: fmt.Sprintf("%sDeploymentDegraded", defaultControllerName),
Status: operatorv1.ConditionFalse,
Reason: "AsExpected",
Message: "",
},
{
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing),
Status: operatorv1.ConditionTrue,
Reason: "PreviousGenerationPodsPresent",
Message: "deployment/apiserver.openshift-apiserver: 1 pod(s) from the previous generation are still present",
},
}
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
},
},
{
name: "some pods rolled out and waiting for old terminating pod before we can progress further",
workload: &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver",
Namespace: "openshift-apiserver",
},
Spec: appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
Replicas: ptr.To[int32](3),
},
Status: appsv1.DeploymentStatus{
AvailableReplicas: 2,
ReadyReplicas: 2,
UpdatedReplicas: 2,
Conditions: []appsv1.DeploymentCondition{
{Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"},
},
},
},
pods: []*corev1.Pod{
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-1-1", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
DeletionTimestamp: ptr.To(metav1.Now()),
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-2-1", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "apiserver-2-2", Namespace: "openshift-apiserver",
Labels: map[string]string{"foo": "bar"},
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{
Name: "test",
Ready: true,
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
},
},
},
},
},
validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error {
expectedConditions := []operatorv1.OperatorCondition{
{
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeAvailable),
Status: operatorv1.ConditionTrue,
Reason: "AsExpected",
Message: "",
},
{
Type: fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
Status: operatorv1.ConditionFalse,
},
{
Type: fmt.Sprintf("%sDeploymentDegraded", defaultControllerName),
Status: operatorv1.ConditionFalse,
Reason: "AsExpected",
Message: "",
},
{
Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing),
Status: operatorv1.ConditionTrue,
Reason: "PodsUpdating",
Message: "deployment/apiserver.openshift-apiserver: 2/3 pods have been updated to the latest generation and 2/3 pods are available",
},
}
return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)
},
},
}

for _, scenario := range scenarios {
Expand Down

0 comments on commit 9d0ad43

Please sign in to comment.