OCPBUGS-104854: Cache deployment generation to prevent echo events - #1206
OCPBUGS-104854: Cache deployment generation to prevent echo events#1206TheRealJon wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-104854, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThe operator now tracks the last applied deployment generation. ChangesDeployment generation caching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/console/operator/sync_v400.go (1)
352-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the cached-generation path.
The supplied
TestDeploymentProgressingByGenerationtest does not exerciseSyncDeploymentor the new cache. Add a focused test that performs two reconciliations:
- The first successful apply records generation
N.- The second reconciliation has stale operator status but uses cached generation
N.- A failed apply does not update
lastAppliedDeploymentGeneration.Also cover Deployment replacement if the identity check is added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/console/operator/sync_v400.go` around lines 352 - 360, Extend the test coverage around SyncDeployment and lastAppliedDeploymentGeneration with a focused two-reconciliation scenario: verify a successful apply caches generation N, a subsequent reconciliation with stale operator status reuses cached generation N, and a failed apply leaves the cache unchanged. If SyncDeployment validates deployment identity, add coverage for replacing the Deployment and ensure the cache is not incorrectly reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/console/operator/sync_v400.go`:
- Around line 334-343: Scope co.lastAppliedDeploymentGeneration to the current
Deployment UID in the expected-generation logic. Track the UID alongside the
cached generation or reset the cached generation whenever the Deployment UID
changes, ensuring a recreated Deployment cannot reuse a stale higher generation
before resourceapply.ApplyDeployment.
---
Nitpick comments:
In `@pkg/console/operator/sync_v400.go`:
- Around line 352-360: Extend the test coverage around SyncDeployment and
lastAppliedDeploymentGeneration with a focused two-reconciliation scenario:
verify a successful apply caches generation N, a subsequent reconciliation with
stale operator status reuses cached generation N, and a failed apply leaves the
cache unchanged. If SyncDeployment validates deployment identity, add coverage
for replacing the Deployment and ensure the cache is not incorrectly reused.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 89477911-1278-4593-a161-962c50131366
📒 Files selected for processing (2)
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack contextFlag MD5, SHA1, DES, RC4, 3DES, Blowfish, and ECB mode cryptographic usage. Also flag custom crypto implementations and non-constant-time comparison of secrets or tokens.
**/*.go: Do not use deprecated Go APIs such asioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll, ornet.DialinDialcallbacks; useos.ReadFile,os.WriteFile,io.ReadAll, andDialContextinstead.
When returning errors in Go, wrap them with%wand include meaningful context instead of returning the raw error or using%v.
Use specific error checks such asapierrors.IsNotFound(err)instead of matching error strings withstrings.Contains(err.Error(), ...).
Propagate the caller’scontext.Contextthrough operations and avoid replacing it withcontext.Background()inside request/controller code.
Usedeferto release acquired resources so cleanup happens on all return paths.
Avoid god functions: keep Go functions to roughly under 100 lines and split code with too many responsibilities into smaller...
Files:
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.Refer to the following skills based on CODE PATTERNS, not just file paths:
Refer to /controller-review when code contains:
- Controller struct types (e.g.,
type *Controller struct)func New*Controller(factory functionsfactory.New().WithFilteredEventsInformers(pattern.ToController(method callsSync(ctx context.Context, controllerContext factory.SyncContext)methodsoperatorConfig.Spec.ManagementStatechecksstatus.NewStatusHandlerorstatus.Handle*functionsRefer to /sync-handler-review when code contains:
- Main operator sync functions (e.g.,
sync_v400.gocontent)- Sequential resource syncing with early returns
- Incremental reconciliation loops
- Multiple
resourceapply.Apply*()calls in sequence- Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes
- Feature gate conditional logic
Refer to /go-quality-review for all Go code to check:
- Deprecated imports:
ioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll- Deprecated patterns:
DialwithoutDialContext- Error handling: missing
%win fmt.Errorf- Code smells: deep nesting (4+ levels), functions >100 lines
- Magic values: unexplained numbers/strings
- Context propagation:
context.Background()instead of passed ctx- Missing godoc on exported functions
Files:
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/operator/operator.gopkg/console/operator/sync_v400.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400.go
**/sync_v400.go
📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)
Incremental sync pattern: each sync loop should stop on the first error and resume from the next step on the next reconciliation instead of collecting and joining all errors.
Files:
pkg/console/operator/sync_v400.go
🔇 Additional comments (1)
pkg/console/operator/operator.go (1)
98-99: LGTM!
| 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 | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
module_dir="$(go list -m -f '{{.Dir}}' github.com/openshift/library-go)"
module_version="$(go list -m -f '{{.Version}}' github.com/openshift/library-go)"
printf 'library-go %s (%s)\n' "$module_version" "$module_dir"
rg -n -C 6 \
'existingCopy\.ObjectMeta\.Generation == expectedGeneration|ApplyDeploymentWithForce' \
"$module_dir/pkg/operator/resource/resourceapply/apps.go"Repository: openshift/console-operator
Length of output: 374
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- module declaration and replace directives ---'
sed -n '1,120p' go.mod
printf '%s\n' '--- cached-generation declarations and uses ---'
rg -n -C 8 'lastAppliedDeploymentGeneration|ExpectedDeploymentGeneration|ApplyDeployment' pkg/console/operator
printf '%s\n' '--- local module-cache locations ---'
go env GOMODCACHE GOPATH
find "$(go env GOMODCACHE)" -path '*/github.com/openshift/library-go*' -maxdepth 8 -type f \( -path '*/pkg/operator/resource/resourceapply/apps.go' -o -name 'apps.go' \) -print 2>/dev/null | head -20 || trueRepository: openshift/console-operator
Length of output: 10866
🏁 Script executed:
set -euo pipefail
library_dir="$(find "$(go env GOMODCACHE)" -path '*/github.com/openshift/library-go@v0.0.0-20260713084045-a99049bdd190' -type d -print -quit)"
printf '%s\n' "--- library-go source: $library_dir/pkg/operator/resource/resourceapply/apps.go ---"
sed -n '1,260p' "$library_dir/pkg/operator/resource/resourceapply/apps.go"
printf '%s\n' "--- expected-generation helper ---"
rg -n -C 12 'func ExpectedDeploymentGeneration|ExpectedDeploymentGeneration' \
"$library_dir/pkg/operator/resource/resourcemerge/apps.go" \
"$library_dir/pkg/operator/resource"
printf '%s\n' '--- surrounding sync and status flow ---'
sed -n '280,390p' pkg/console/operator/sync_v400.go
rg -n -C 10 'Generations|ObservedGeneration|lastAppliedDeploymentGeneration|ObjectMeta.UID|UID' \
pkg/console/operatorRepository: openshift/console-operator
Length of output: 40272
🏁 Script executed:
set -euo pipefail
library_dir="$(find "$(go env GOMODCACHE)" -path '*/github.com/openshift/library-go@v0.0.0-20260713084045-a99049bdd190' -type d -print -quit)"
printf '%s\n' '--- metadata merge and update-event behavior ---'
rg -n -C 12 'func EnsureObjectMeta|func ReportUpdateEvent|DeploymentUpdated|Updated' \
"$library_dir/pkg/operator/resource" "$library_dir/pkg/operator/events"
printf '%s\n' '--- tests for ApplyDeployment generation mismatch ---'
rg -n -C 10 'expectedGeneration|generation mismatch|DeploymentUpdated|ApplyDeployment' \
"$library_dir/pkg/operator/resource/resourceapply" | head -240
printf '%s\n' '--- deployment informer/event consumers in this repository ---'
rg -n -C 8 'DeploymentUpdated|Deployment.*Updated|deployment.*event|deploymentInformer|WithFilteredEventsInformers|deployment.*resync' \
pkg cmdRepository: openshift/console-operator
Length of output: 50383
🏁 Script executed:
set -euo pipefail
library_dir="$(find "$(go env GOMODCACHE)" -path '*/github.com/openshift/library-go@v0.0.0-20260713084045-a99049bdd190' -type d -print -quit)"
python3 - <<'PY'
from pathlib import Path
import re
apps = Path("""REPLACE_APPS""")
merger = Path("""REPLACE_MERGER""")
event = Path("""REPLACE_EVENT""")
PYRepository: openshift/console-operator
Length of output: 164
Scope lastAppliedDeploymentGeneration to the Deployment UID.
If the Deployment is recreated, its new UID can have a lower generation than the cached value. resourceapply.ApplyDeployment then performs an unnecessary update and emits DeploymentUpdated. Store the UID with the generation, or reset the cached generation when the UID changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/console/operator/sync_v400.go` around lines 334 - 343, Scope
co.lastAppliedDeploymentGeneration to the current Deployment UID in the
expected-generation logic. Track the UID alongside the cached generation or
reset the cached generation whenever the Deployment UID changes, ensuring a
recreated Deployment cannot reuse a stale higher generation before
resourceapply.ApplyDeployment.
Source: MCP tools
There was a problem hiding this comment.
LGTM 👍 @TheRealJon
The max(cachedGen, statusGen) approach is correct since deployment generations are monotonic, and the cache self-corrects even in edge cases like external deployment recreation. CodeRabbit's UID-scoping suggestion is overkill here.
One ask before merging:
Missing test for lastAppliedDeploymentGeneration - the cache logic is simple but it's on a correctness-critical path. Would be good to add a focused test covering:
- First apply - cache starts at 0,
expectedGencomes from operator status, cache updates after success - Stale informer (the echo case) - second sync where operator status still has the old generation but the cache bridges the gap, preventing the no-op write
- Failed apply - cache stays unchanged
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/console/operator/sync_v400_test.go`:
- Around line 785-786: Update the deployment lookup in the test around existing
to capture and assert or fail on the Get error before accessing
existing.Generation. Preserve the subsequent generation setup only after
confirming the deployment was retrieved successfully.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7e9e4aeb-5e71-4f8a-8e4c-e16bd33d1cc6
📒 Files selected for processing (1)
pkg/console/operator/sync_v400_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack contextFlag MD5, SHA1, DES, RC4, 3DES, Blowfish, and ECB mode cryptographic usage. Also flag custom crypto implementations and non-constant-time comparison of secrets or tokens.
**/*.go: Do not use deprecated Go APIs such asioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll, ornet.DialinDialcallbacks; useos.ReadFile,os.WriteFile,io.ReadAll, andDialContextinstead.
When returning errors in Go, wrap them with%wand include meaningful context instead of returning the raw error or using%v.
Use specific error checks such asapierrors.IsNotFound(err)instead of matching error strings withstrings.Contains(err.Error(), ...).
Propagate the caller’scontext.Contextthrough operations and avoid replacing it withcontext.Background()inside request/controller code.
Usedeferto release acquired resources so cleanup happens on all return paths.
Avoid god functions: keep Go functions to roughly under 100 lines and split code with too many responsibilities into smaller...
Files:
pkg/console/operator/sync_v400_test.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.Refer to the following skills based on CODE PATTERNS, not just file paths:
Refer to /controller-review when code contains:
- Controller struct types (e.g.,
type *Controller struct)func New*Controller(factory functionsfactory.New().WithFilteredEventsInformers(pattern.ToController(method callsSync(ctx context.Context, controllerContext factory.SyncContext)methodsoperatorConfig.Spec.ManagementStatechecksstatus.NewStatusHandlerorstatus.Handle*functionsRefer to /sync-handler-review when code contains:
- Main operator sync functions (e.g.,
sync_v400.gocontent)- Sequential resource syncing with early returns
- Incremental reconciliation loops
- Multiple
resourceapply.Apply*()calls in sequence- Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes
- Feature gate conditional logic
Refer to /go-quality-review for all Go code to check:
- Deprecated imports:
ioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll- Deprecated patterns:
DialwithoutDialContext- Error handling: missing
%win fmt.Errorf- Code smells: deep nesting (4+ levels), functions >100 lines
- Magic values: unexplained numbers/strings
- Context propagation:
context.Background()instead of passed ctx- Missing godoc on exported functions
Files:
pkg/console/operator/sync_v400_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow testing patterns and commands documented in TESTING.md
Follow testing patterns and commands as documented in TESTING.md, including running unit tests with 'make test-unit' and checks with 'make check'
**/*_test.go: Use table-driven tests for comprehensive coverage
Usehttptestfor HTTP handler testing in Go
Include proper cleanup functions in tests
Test both success and failure pathsIn Go tests, do not ignore returned errors; check
errand fail the test witht.Fatalfort.Errorfas appropriate.
Files:
pkg/console/operator/sync_v400_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Review test code for quality and patterns.Refer to /unit-test-review when test is in pkg//*_test.go:**
- Table-driven test structure with test cases
- Use of
go-test/deepfor struct comparisons- Test naming conventions (TestFunctionName)
- Error handling with
wantErrpattern- Edge case coverage (nil, empty, boundary values)
- Proper assertions with helpful error messages
- Test isolation (no shared mutable state)
Refer to /e2e-test-review when test contains:
framework.MustNewClientset(t, nil)or similar e2e framework usagewait.Pollorwait.PollImmediatepatternsretry.RetryOnConflictfor updates- Cleanup via
deferfunctions- Console/operator CR manipulations
- Test assertions on cluster state
Suggest to use /e2e-test-review when:
- PR adds new feature requiring e2e coverage
- Test file is empty or skeleton
- Comments indicate "TODO: add test"
Review for common issues:
- Missing cleanup (defer statements)
- Using
time.Sleepinstead ofwait.Poll- Missing context timeouts
- Vague error messages in assertions
- Tests without table-driven structure when testing multiple cases
- Ignoring errors with
_- Tests without assertions
Files:
pkg/console/operator/sync_v400_test.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/operator/sync_v400_test.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400_test.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (.claude/skills/unit-test-review.md)
pkg/**/*_test.go: Most unit tests should use the table-driven test pattern, including atests := []struct{...}table andt.Run(tt.name, ...)subtests for scenarios with multiple cases.
Test function names and subtest case names should be descriptive of the behavior or scenario being tested (for example,TestGetNodeComputeEnvironmentsor"Custom hostname and TLS secret set").
Usegithub.com/go-test/deep(deep.Equal) for struct comparisons instead of==or manual field-by-field checks.
Cover both success and failure paths in unit tests, including edge cases such as empty inputs, boundary values, missing fields, duplicates, and large inputs.
Structure tests using Arrange-Act-Assert so setup, execution, and verification are clearly separated.
When testing error-returning functions, assert error presence correctly and, when relevant, validate the error message substring instead of ignoring the error or discarding it with_.
Prefer dependency injection via interfaces for testability, and keep tests isolated so they do not depend on execution order or shared mutable state.
Extract repeated setup into helper functions when common test fixtures are reused across multiple tests.
Write specific, informative assertions that explain what failed instead of vague or silent failures.
Inline simple test data, but move complex fixtures to helper functions ortestdata/files.
Avoid tests that rely on execution order, share global mutable state, use hardcoded sleeps, omit assertions, or verify implementation details instead of behavior.
Files:
pkg/console/operator/sync_v400_test.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/operator/sync_v400_test.go
🔇 Additional comments (1)
pkg/console/operator/sync_v400_test.go (1)
6-6: LGTM!Also applies to: 16-28, 691-746, 748-784, 787-850
| existing, _ := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{}) | ||
| existing.Generation = 10 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the deployment lookup error before using existing.
Line 785 discards the Get error. If the fake client returns an error, line 786 can dereference a nil deployment and hide the actual test failure.
Proposed fix
- existing, _ := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{})
+ existing, err := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{})
+ if err != nil {
+ t.Fatalf("get deployment for generation bump: %v", err)
+ }
existing.Generation = 10As per coding guidelines, **/*_test.go: “In Go tests, do not ignore returned errors.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| existing, _ := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{}) | |
| existing.Generation = 10 | |
| existing, err := fakeClient.AppsV1().Deployments("openshift-console").Get(context.Background(), "console", metav1.GetOptions{}) | |
| if err != nil { | |
| t.Fatalf("get deployment for generation bump: %v", err) | |
| } | |
| existing.Generation = 10 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/console/operator/sync_v400_test.go` around lines 785 - 786, Update the
deployment lookup in the test around existing to capture and assert or fail on
the Get error before accessing existing.Generation. Preserve the subsequent
generation setup only after confirming the deployment was retrieved
successfully.
Source: Coding guidelines
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
@TheRealJon: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-104854, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
3 participants
Analysis / Root cause:
The console-operator triggers excessive
DeploymentUpdatedevents (21-31 per test run, avg 24) on vsphere techpreview serial clusters, failing the[Monitor:legacy-test-framework-invariants-pathological]event invariant test. Pass rate dropped from 100% to 32.81% since July 24, 2026.Must-gather analysis of a failed run (31 events) reveals two compounding issues:
CONSOLE-5122 e2e tests (direct regression trigger, merged July 15): Serial tests create/delete
console-label-test-NcomponentRoutes, each producing configmap content changes (additionalConsoleBaseAddresses). 5 test iterations × 2 changes = 10 real configmap changes → 10 deployment updates.Generation echo amplifier (pre-existing): After
ApplyDeploymentupdates the console deployment, the deployment informer triggers a re-sync before the operator config informer has processed the corresponding status write fromFlushAndReturn. The stale expected generation causes a no-op deployment update and a spuriousDeploymentUpdatedevent — effectively doubling every legitimate update.Event breakdown from must-gather:
Solution description:
Cache the last-applied deployment generation in-memory on the
consoleOperatorstruct. InSyncDeployment, usemax(cachedGen, statusGen)as the expected generation passed toApplyDeployment. This bridges the informer cache gap: even if the operator config informer hasn't yet processed the status write, the in-memory cache holds the correct generation, preventing the no-op update.The cache starts at zero on operator restart, which is safe — the first sync reads the correct generation from the operator config status (which the informer has had time to populate). Subsequent syncs benefit from the cache when the informer lags.
Test setup:
Deploy the operator to any cluster and trigger a configmap change (e.g., add a componentRoute to the ingress config).
Test cases:
DeploymentUpdatedevents (real + echo ~2s later)Verified manually on a dev cluster:
Expected CI impact: 31 events → ~18 events (13 echo events eliminated), under the pathological threshold of 20.
Browser conformance:
N/A — backend-only change, no UI impact.
Additional info:
This is a targeted fix for the echo amplifier only. The underlying configmap churn during cluster initialization (plugins registering, monitoring coming online) and from CONSOLE-5122 e2e tests is legitimate operator behavior. A secondary optimization to remove redundant infrastructure/proxy config RV annotations from the deployment could further reduce event count but is not included in this PR to keep scope minimal.
🤖 Generated with Claude Code
Summary by CodeRabbit