From 99ca10096a708f975825cbca1c3cde7290c325b6 Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Wed, 5 Aug 2026 16:39:30 -0400 Subject: [PATCH 1/2] OCPBUGS-104854: Cache deployment generation to prevent echo events After ApplyDeployment updates the console deployment, the deployment informer triggers a re-sync before the operator config informer has processed the corresponding status write. The stale expected generation causes a no-op deployment update and a spurious DeploymentUpdated event, effectively doubling every legitimate deployment update. Cache the last-applied deployment generation in-memory and use max(cached, status) as the expected generation to bridge the informer cache gap. This eliminates ~13 echo events per test run, bringing the total from ~31 to ~18 on vsphere techpreview serial clusters. Co-Authored-By: Claude Opus 4.6 --- pkg/console/operator/operator.go | 3 ++- pkg/console/operator/sync_v400.go | 13 ++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/console/operator/operator.go b/pkg/console/operator/operator.go index c1c62cefae..0676eb6f8a 100644 --- a/pkg/console/operator/operator.go +++ b/pkg/console/operator/operator.go @@ -95,7 +95,8 @@ type consoleOperator struct { monitoringDeploymentLister appsv1listers.DeploymentLister - lastDeploymentAvailableTime time.Time + lastDeploymentAvailableTime time.Time + lastAppliedDeploymentGeneration int64 } type trackables struct { diff --git a/pkg/console/operator/sync_v400.go b/pkg/console/operator/sync_v400.go index 0a193666c4..b8acc86cbe 100644 --- a/pkg/console/operator/sync_v400.go +++ b/pkg/console/operator/sync_v400.go @@ -331,6 +331,16 @@ func (co *consoleOperator) SyncDeployment( } deploymentsub.LogDeploymentAnnotationChanges(co.deploymentClient, requiredDeployment, ctx) + expectedGen := resourcemerge.ExpectedDeploymentGeneration(requiredDeployment, updatedOperatorConfig.Status.Generations) + // After the operator updates the deployment, the deployment informer + // triggers a re-sync before the operator config informer has processed + // the corresponding status write. The stale expected generation causes + // a no-op update and a spurious DeploymentUpdated event. Use the + // in-memory cached generation to bridge the informer cache gap. + if co.lastAppliedDeploymentGeneration > expectedGen { + expectedGen = co.lastAppliedDeploymentGeneration + } + var deployment *appsv1.Deployment applyDepErr := controllersutil.RetryOnTransientError(func() error { var e error @@ -339,7 +349,7 @@ func (co *consoleOperator) SyncDeployment( co.deploymentClient, recorder, requiredDeployment, - resourcemerge.ExpectedDeploymentGeneration(requiredDeployment, updatedOperatorConfig.Status.Generations), + expectedGen, ) return e }) @@ -347,6 +357,7 @@ func (co *consoleOperator) SyncDeployment( if applyDepErr != nil { return nil, "FailedApply", applyDepErr } + co.lastAppliedDeploymentGeneration = deployment.Generation return deployment, "", nil } From fe90db74ee4a413dab35a46de092f7a14e4a9b83 Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Thu, 6 Aug 2026 09:09:40 -0400 Subject: [PATCH 2/2] Add unit tests for deployment generation cache Cover three scenarios for lastAppliedDeploymentGeneration: - First apply: cache starts at 0, updates to returned generation - Stale informer (echo case): cached generation bridges the informer gap, preventing no-op deployment update - Failed apply: cache remains unchanged Co-Authored-By: Claude Opus 4.6 --- pkg/console/operator/sync_v400_test.go | 170 +++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/pkg/console/operator/sync_v400_test.go b/pkg/console/operator/sync_v400_test.go index de67ed9233..e349db34ad 100644 --- a/pkg/console/operator/sync_v400_test.go +++ b/pkg/console/operator/sync_v400_test.go @@ -3,6 +3,7 @@ package operator import ( "context" "encoding/json" + "fmt" "sort" "testing" "time" @@ -12,12 +13,19 @@ import ( configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" + "github.com/openshift/library-go/pkg/operator/events" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubefake "k8s.io/client-go/kubernetes/fake" appsv1listers "k8s.io/client-go/listers/apps/v1" corev1listers "k8s.io/client-go/listers/core/v1" + clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" + clocktesting "k8s.io/utils/clock/testing" "github.com/openshift/console-operator/pkg/api" "github.com/openshift/console-operator/pkg/console/telemetry" @@ -679,3 +687,165 @@ func TestEvaluateDeploymentAvailability(t *testing.T) { } }) } + +// syncDeploymentInputs holds the minimal inputs needed to call SyncDeployment. +type syncDeploymentInputs struct { + operatorConfig *operatorv1.Console + cm *v1.ConfigMap + serviceCAConfigMap *v1.ConfigMap + oauthServingCertCM *v1.ConfigMap + authServerCACM *v1.ConfigMap + trustedCACM *v1.ConfigMap + oauthSecret *v1.Secret + sessionSecret *v1.Secret + servingCertSecret *v1.Secret + proxyConfig *configv1.Proxy + infrastructureConfig *configv1.Infrastructure +} + +func newSyncDeploymentInputs() syncDeploymentInputs { + return syncDeploymentInputs{ + operatorConfig: &operatorv1.Console{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", UID: "test-uid"}, + Spec: operatorv1.ConsoleSpec{}, + }, + cm: &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "console-config", Namespace: "openshift-console", ResourceVersion: "100"}}, + serviceCAConfigMap: &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "service-ca", Namespace: "openshift-console", ResourceVersion: "200"}}, + oauthServingCertCM: &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "oauth-serving-cert", Namespace: "openshift-console", ResourceVersion: "300"}}, + authServerCACM: nil, + trustedCACM: &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "trusted-ca", Namespace: "openshift-console", ResourceVersion: "400"}}, + oauthSecret: &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "console-oauth-config", Namespace: "openshift-console", ResourceVersion: "500"}}, + sessionSecret: nil, + servingCertSecret: &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "console-serving-cert", Namespace: "openshift-console", ResourceVersion: "600"}}, + proxyConfig: &configv1.Proxy{ObjectMeta: metav1.ObjectMeta{Name: "cluster", ResourceVersion: "700"}}, + infrastructureConfig: &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", ResourceVersion: "800"}, + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.HighlyAvailableTopologyMode, + }, + }, + } +} + +func (in syncDeploymentInputs) callSyncDeployment(co *consoleOperator, recorder events.Recorder) (*appsv1.Deployment, string, error) { + return co.SyncDeployment( + context.Background(), + in.operatorConfig, + in.cm, + in.serviceCAConfigMap, + in.oauthServingCertCM, + in.authServerCACM, + in.trustedCACM, + in.oauthSecret, + in.sessionSecret, + in.servingCertSecret, + in.proxyConfig, + in.infrastructureConfig, + recorder, + ) +} + +func TestSyncDeploymentGenerationCache(t *testing.T) { + newRecorder := func() events.Recorder { + return events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())) + } + + t.Run("first apply caches generation", func(t *testing.T) { + fakeClient := kubefake.NewSimpleClientset() + co := &consoleOperator{ + deploymentClient: fakeClient.AppsV1(), + } + inputs := newSyncDeploymentInputs() + + dep, _, err := inputs.callSyncDeployment(co, newRecorder()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if co.lastAppliedDeploymentGeneration != dep.Generation { + t.Errorf("expected cache=%d, got=%d", dep.Generation, co.lastAppliedDeploymentGeneration) + } + }) + + t.Run("cached generation prevents echo update", func(t *testing.T) { + fakeClient := kubefake.NewSimpleClientset() + co := &consoleOperator{ + deploymentClient: fakeClient.AppsV1(), + } + inputs := newSyncDeploymentInputs() + recorder := newRecorder() + + // First apply creates the deployment and populates all annotations + // including the specHash. + _, _, err := inputs.callSyncDeployment(co, recorder) + if err != nil { + t.Fatalf("first apply: %v", err) + } + + // Simulate the API server incrementing generation after the update. + existing, _ := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{}) + existing.Generation = 10 + _, err = fakeClient.AppsV1().Deployments("openshift-console").Update(context.Background(), existing, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("simulating generation bump: %v", err) + } + + // Set cache to match (as if previous SyncDeployment returned this). + co.lastAppliedDeploymentGeneration = 10 + + // Simulate stale informer: operator status still has gen 9. + inputs.operatorConfig = inputs.operatorConfig.DeepCopy() + inputs.operatorConfig.Status.Generations = []operatorv1.GenerationStatus{{ + Group: "apps", + Resource: "deployments", + Namespace: "openshift-console", + Name: "console", + LastGeneration: 9, + }} + + // Track whether ApplyDeployment calls Update. + updateCalls := 0 + fakeClient.PrependReactor("update", "deployments", func(action clienttesting.Action) (bool, runtime.Object, error) { + updateCalls++ + return false, nil, nil + }) + + // Second apply with identical inputs — cache should bridge the gap. + _, _, err = inputs.callSyncDeployment(co, recorder) + if err != nil { + t.Fatalf("second apply: %v", err) + } + if updateCalls != 0 { + t.Errorf("expected no deployment update (echo prevented), got %d update(s)", updateCalls) + } + if co.lastAppliedDeploymentGeneration != 10 { + t.Errorf("expected cache to remain 10, got %d", co.lastAppliedDeploymentGeneration) + } + }) + + t.Run("failed apply preserves cached generation", func(t *testing.T) { + fakeClient := kubefake.NewSimpleClientset() + co := &consoleOperator{ + deploymentClient: fakeClient.AppsV1(), + lastAppliedDeploymentGeneration: 5, + } + inputs := newSyncDeploymentInputs() + + // Make Get return a non-retryable error so RetryOnTransientError + // gives up immediately. + fakeClient.PrependReactor("get", "deployments", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden( + schema.GroupResource{Group: "apps", Resource: "deployments"}, + "console", + fmt.Errorf("synthetic test error"), + ) + }) + + _, _, err := inputs.callSyncDeployment(co, newRecorder()) + if err == nil { + t.Fatal("expected error, got nil") + } + if co.lastAppliedDeploymentGeneration != 5 { + t.Errorf("expected cache to remain 5 after failure, got %d", co.lastAppliedDeploymentGeneration) + } + }) +}