Skip to content

OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions #2128

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

Open
tchap wants to merge 1 commit into
base: master
Choose a base branch
from
+268 −90
Open

OCPBUGS-65896: controller/workload: Rework Degraded/Progressing conditions #2128

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
77 changes: 45 additions & 32 deletions pkg/operator/apiserver/controller/workload/workload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"strings"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand All @@ -19,7 +18,6 @@ import (

operatorv1 "github.com/openshift/api/operator/v1"
applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
"github.com/openshift/library-go/pkg/apps/deployment"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/status"
Expand Down Expand Up @@ -291,7 +289,7 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
deploymentAvailableCondition = deploymentAvailableCondition.
WithStatus(operatorv1.ConditionFalse).
WithReason("NoPod").
WithMessage(fmt.Sprintf("no %s.%s pods available on any node.", workload.Name, c.targetNamespace))
WithMessage(fmt.Sprintf("no %s.%s pods available on any node", workload.Name, c.targetNamespace))
} else {
deploymentAvailableCondition = deploymentAvailableCondition.
WithStatus(operatorv1.ConditionTrue).
Expand All @@ -303,23 +301,24 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
desiredReplicas = *(workload.Spec.Replicas)
}

// If the workload is up to date, then we are no longer progressing
workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration
// Update is done when all pods have been updated to the latest revision
// and the deployment controller has reported NewReplicaSetAvailable
workloadIsBeingUpdated := !workloadAtHighestGeneration || !hasDeploymentProgressed(workload.Status)
workloadIsBeingUpdatedTooLong := v1helpers.IsUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type)
Copy link
Contributor Author

@tchap tchap Feb 18, 2026

Choose a reason for hiding this comment

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

We don't use IsUpdatingTooLong any more, we based the check on ProgressDeadlineExceeded from the deployment controller.

Copy link
Member

@atiratree atiratree Feb 18, 2026

Choose a reason for hiding this comment

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

let's also set .spec.progressDeadlineSeconds to 15m on the consumer workloads to honor the superseded progressingConditionTimeout value

Copy link
Contributor Author

@tchap tchap Feb 18, 2026

Choose a reason for hiding this comment

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

I don't think we can do that. Sync from the delegate returns the synchronized deployment already. So we would have to send another update request, I guess.

Copy link
Contributor Author

@tchap tchap Feb 18, 2026

Choose a reason for hiding this comment

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

We can align this in auth when updating library-go. I will actually open a PR there once this is accepted to run CI anyway.

Copy link
Member

@atiratree atiratree Feb 18, 2026

Choose a reason for hiding this comment

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

Yup, I meant that we should update it in the consumer repos like auth.

Copy link
Member

@atiratree atiratree Feb 18, 2026

Choose a reason for hiding this comment

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

Might be better to open a proof PR before the library-go changes are merged.

Copy link
Contributor Author

@tchap tchap Feb 18, 2026

Choose a reason for hiding this comment

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

Opened openshift/cluster-authentication-operator#843

if !workloadAtHighestGeneration {
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionTrue).
WithReason("NewGeneration").
WithMessage(fmt.Sprintf("deployment/%s.%s: observed generation is %d, desired generation is %d.", workload.Name, c.targetNamespace, workload.Status.ObservedGeneration, workload.ObjectMeta.Generation))
} else if workloadIsBeingUpdated {
// Update is done when the deployment controller has reported NewReplicaSetAvailable.
// Checking the current vs. observed generation here is not possible since we don't want to be Progressing on scaling.
progressTimedOutMessage, workloadIsBeingUpdatedTooLong := hasDeploymentTimedOutProgressing(workload.Status)
workloadIsBeingUpdated := !hasDeploymentProgressed(workload.Status) && !workloadIsBeingUpdatedTooLong
switch {
case workloadIsBeingUpdated:
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionTrue).
WithReason("PodsUpdating").
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 {
WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest revision and %d/%d pods are available", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas, workload.Status.AvailableReplicas, desiredReplicas))

case workloadIsBeingUpdatedTooLong:
deploymentProgressingCondition = deploymentProgressingCondition.
WithStatus(operatorv1.ConditionFalse).
WithReason("ProgressDeadlineExceeded").
WithMessage(fmt.Sprintf("deployment/%s.%s has timed out progressing: %s", workload.Name, c.targetNamespace, progressTimedOutMessage))

default:
// 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 when we report Progressing=False.
Expand All @@ -332,22 +331,22 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
WithReason("AsExpected")
}

// During a rollout the default maxSurge (25%) will allow the available
// replicas to temporarily exceed the desired replica count. If this were
// to occur, the operator should not report degraded.
workloadHasAllPodsAvailable := workload.Status.AvailableReplicas >= desiredReplicas
if !workloadHasAllPodsAvailable && (!workloadIsBeingUpdated || workloadIsBeingUpdatedTooLong) {
numNonAvailablePods := desiredReplicas - workload.Status.AvailableReplicas
// Degraded is set when the deployment is not Available or timed out progressing.
// This will cause the operator to go Degraded during the initial rollout.
switch {
case workload.Status.AvailableReplicas == 0:
deploymentDegradedCondition = deploymentDegradedCondition.
WithStatus(operatorv1.ConditionTrue).
WithReason("UnavailablePod")
podContainersStatus, err := deployment.PodContainersStatus(workload, c.podsLister)
if err != nil {
podContainersStatus = []string{fmt.Sprintf("failed to get pod containers details: %v", err)}
}
WithReason("Unavailable").
WithMessage(fmt.Sprintf("no %s.%s pods available on any node", workload.Name, c.targetNamespace))

case workloadIsBeingUpdatedTooLong:
deploymentDegradedCondition = deploymentDegradedCondition.
WithMessage(fmt.Sprintf("%v of %v requested instances are unavailable for %s.%s (%s)", numNonAvailablePods, desiredReplicas, workload.Name, c.targetNamespace, strings.Join(podContainersStatus, ", ")))
} else {
WithStatus(operatorv1.ConditionTrue).
WithReason("ProgressDeadlineExceeded").
WithMessage(fmt.Sprintf("deployment/%s.%s has timed out progressing: %s", workload.Name, c.targetNamespace, progressTimedOutMessage))

default:
deploymentDegradedCondition = deploymentDegradedCondition.
WithStatus(operatorv1.ConditionFalse).
WithReason("AsExpected")
Expand All @@ -356,8 +355,11 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
// if the deployment is all available and at the expected generation, then update the version to the latest
// when we update, the image pull spec should immediately be different, which should immediately cause a deployment rollout
// which should immediately result in a deployment generation diff, which should cause this block to be skipped until it is ready.
workloadHasAllPodsUpdated := workload.Status.UpdatedReplicas == desiredReplicas
if workloadAtHighestGeneration && workloadHasAllPodsAvailable && workloadHasAllPodsUpdated && operatorConfigAtHighestGeneration {
if operatorConfigAtHighestGeneration &&
workload.ObjectMeta.Generation == workload.Status.ObservedGeneration &&
workload.Status.AvailableReplicas == desiredReplicas &&
workload.Status.UpdatedReplicas == desiredReplicas {

c.versionRecorder.SetVersion(c.constructOperandNameFor(workload.Name), c.targetOperandVersion)
}

Expand Down Expand Up @@ -386,6 +388,17 @@ func hasDeploymentProgressed(status appsv1.DeploymentStatus) bool {
return false
}

// hasDeploymentTimedOutProgressing returns true if the deployment reports ProgressDeadlineExceeded.
// The function returns the Progressing condition message as the first return value.
func hasDeploymentTimedOutProgressing(status appsv1.DeploymentStatus) (string, bool) {
for _, cond := range status.Conditions {
if cond.Type == appsv1.DeploymentProgressing {
return cond.Message, cond.Status == corev1.ConditionFalse && cond.Reason == "ProgressDeadlineExceeded"
}
}
return "", false
}

// 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
Loading