From 7a32da2bde7eac9a72bd7ca5ed6a18de1a886f6a Mon Sep 17 00:00:00 2001 From: Steve Goodwin Date: Mon, 13 Jul 2026 17:28:32 -0400 Subject: [PATCH] OCPBUGS-93982: gate Progressing on version transition and operand rollout status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace checkDeploymentGenerationProgress with checkDeploymentRolloutStatus, which checks the deployment controller's own Progressing condition (following the 3CMO pattern from cluster-cloud-controller-manager-operator#488). Only run the check during version transitions — outside upgrades, skip entirely so annotation-driven spec updates during the MCO window cannot produce Progressing=True blips. Assisted by: Claude (Opus 4.6) --- pkg/console/operator/sync_v400.go | 36 ++++++--- pkg/console/operator/sync_v400_test.go | 104 ++++++++++++++++++------- 2 files changed, 102 insertions(+), 38 deletions(-) diff --git a/pkg/console/operator/sync_v400.go b/pkg/console/operator/sync_v400.go index 082b383aa..7f4ff0db4 100644 --- a/pkg/console/operator/sync_v400.go +++ b/pkg/console/operator/sync_v400.go @@ -218,11 +218,12 @@ func (co *consoleOperator) sync_v400(ctx context.Context, controllerContext fact statusHandler.AddCondition(status.HandleProgressing("SyncLoopRefresh", "InProgress", func() error { version := os.Getenv("OPERATOR_IMAGE_VERSION") - if err := checkDeploymentGenerationProgress(actualDeployment); err != nil { - return err - } + isUpgrading := co.versionGetter.GetVersions()["operator"] != version - if co.versionGetter.GetVersions()["operator"] != version { + if isUpgrading { + if err := checkDeploymentRolloutStatus(actualDeployment); err != nil { + return err + } co.versionGetter.SetVersion("operator", version) } return nil @@ -826,17 +827,30 @@ func (co *consoleOperator) GetAvailablePlugins(enabledPluginsNames []string) []* return availablePlugins } -// checkDeploymentGenerationProgress returns an error if the deployment controller -// has not yet processed the latest spec change (ObservedGeneration < Generation). -// This is used to determine Progressing status without relying on replica counts, -// which fluctuate during external disruptions like node reboots. -// See https://issues.redhat.com/browse/OCPBUGS-64688 -func checkDeploymentGenerationProgress(deployment *appsv1.Deployment) error { +// checkDeploymentRolloutStatus checks whether a deployment's rollout has completed +// by examining the deployment controller's own status conditions. This follows +// the 3CMO pattern (openshift/cluster-cloud-controller-manager-operator#488). +// +// See https://issues.redhat.com/browse/OCPBUGS-93982 +func checkDeploymentRolloutStatus(deployment *appsv1.Deployment) error { if deployment.Status.ObservedGeneration < deployment.Generation { return fmt.Errorf("deployment generation %d not yet observed (observed: %d)", deployment.Generation, deployment.Status.ObservedGeneration) } - return nil + + for _, condition := range deployment.Status.Conditions { + if condition.Type == appsv1.DeploymentProgressing { + if condition.Status == corev1.ConditionTrue && condition.Reason == "NewReplicaSetAvailable" { + return nil + } + if condition.Status == corev1.ConditionFalse && condition.Reason == "ProgressDeadlineExceeded" { + return fmt.Errorf("deployment rollout stalled: ProgressDeadlineExceeded") + } + return fmt.Errorf("deployment rollout in progress: %s", condition.Reason) + } + } + + return fmt.Errorf("deployment rollout status not yet available") } func getNodeComputeEnvironments(nodes []*corev1.Node) ([]string, []string) { diff --git a/pkg/console/operator/sync_v400_test.go b/pkg/console/operator/sync_v400_test.go index 985945f30..f055cd6cd 100644 --- a/pkg/console/operator/sync_v400_test.go +++ b/pkg/console/operator/sync_v400_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "sort" + "strings" "testing" "github.com/go-test/deep" @@ -224,50 +225,98 @@ func TestGetNodeComputeEnvironments(t *testing.T) { } } -// TestDeploymentProgressingByGeneration tests the ObservedGeneration-based -// Progressing check introduced in OCPBUGS-64688. The operator should only -// report Progressing=True when the deployment controller has not yet processed -// a spec change (ObservedGeneration < Generation), NOT when replica counts -// fluctuate due to external disruptions like node reboots. -func TestDeploymentProgressingByGeneration(t *testing.T) { +// TestCheckDeploymentRolloutStatus tests the deployment rollout status check +// introduced in OCPBUGS-93982 (v2). The function examines the deployment +// controller's own Progressing condition rather than just the generation gap, +// following the 3CMO pattern (openshift/cluster-cloud-controller-manager-operator#488). +func TestCheckDeploymentRolloutStatus(t *testing.T) { tests := []struct { name string generation int64 observedGeneration int64 + conditions []appsv1.DeploymentCondition wantErr bool - wantErrMsg string + wantErrContains string }{ { - name: "ObservedGeneration equals Generation: not progressing", + name: "rollout complete: NewReplicaSetAvailable", generation: 5, observedGeneration: 5, - wantErr: false, + conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: "NewReplicaSetAvailable", + }}, + wantErr: false, }, { - name: "ObservedGeneration less than Generation: progressing", - generation: 4, - observedGeneration: 3, + name: "generation gap: not yet observed", + generation: 6, + observedGeneration: 5, wantErr: true, - wantErrMsg: "deployment generation 4 not yet observed (observed: 3)", + wantErrContains: "not yet observed", }, { - name: "ObservedGeneration greater than Generation: not progressing", - generation: 2, - observedGeneration: 3, - wantErr: false, + name: "rollout in progress: ReplicaSetUpdated", + generation: 5, + observedGeneration: 5, + conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: "ReplicaSetUpdated", + }}, + wantErr: true, + wantErrContains: "rollout in progress: ReplicaSetUpdated", }, { - name: "both zero: not progressing (fresh deployment)", - generation: 0, - observedGeneration: 0, - wantErr: false, + name: "rollout in progress: NewReplicaSetCreated", + generation: 5, + observedGeneration: 5, + conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: "NewReplicaSetCreated", + }}, + wantErr: true, + wantErrContains: "rollout in progress: NewReplicaSetCreated", + }, + { + name: "rollout stalled: ProgressDeadlineExceeded", + generation: 5, + observedGeneration: 5, + conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: v1.ConditionFalse, + Reason: "ProgressDeadlineExceeded", + }}, + wantErr: true, + wantErrContains: "ProgressDeadlineExceeded", + }, + { + name: "no progressing condition yet", + generation: 5, + observedGeneration: 5, + conditions: []appsv1.DeploymentCondition{}, + wantErr: true, + wantErrContains: "not yet available", + }, + { + name: "observed greater than generation: checks conditions", + generation: 3, + observedGeneration: 4, + conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentProgressing, + Status: v1.ConditionTrue, + Reason: "NewReplicaSetAvailable", + }}, + wantErr: false, }, { - name: "Generation 1, ObservedGeneration 0: progressing (initial rollout)", + name: "initial rollout: Generation 1, ObservedGeneration 0", generation: 1, observedGeneration: 0, wantErr: true, - wantErrMsg: "deployment generation 1 not yet observed (observed: 0)", + wantErrContains: "not yet observed", }, } @@ -279,19 +328,20 @@ func TestDeploymentProgressingByGeneration(t *testing.T) { }, Status: appsv1.DeploymentStatus{ ObservedGeneration: tt.observedGeneration, + Conditions: tt.conditions, }, } - err := checkDeploymentGenerationProgress(deployment) + err := checkDeploymentRolloutStatus(deployment) if tt.wantErr && err == nil { - t.Errorf("expected error but got nil") + t.Error("expected error but got nil") } if !tt.wantErr && err != nil { t.Errorf("expected no error but got: %v", err) } - if tt.wantErr && err != nil && err.Error() != tt.wantErrMsg { - t.Errorf("error message mismatch:\n got: %q\n want: %q", err.Error(), tt.wantErrMsg) + if tt.wantErr && err != nil && !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrContains) } }) }