reconciler: fix inverted log message and level for UpdateNotReadyErr - #3884
reconciler: fix inverted log message and level for UpdateNotReadyErr#3884pdudley wants to merge 3 commits into
Conversation
When ensureUpdatePod returns UpdateNotReadyErr (the expected, benign signal that a new update pod has not yet reported ready), EnsureRegistryServer logged the error at level=error with the message "ensure update pod error is not of type UpdateNotReadyErr" — the exact opposite of what happened. The ok branch fires precisely when the error IS UpdateNotReadyErr, so the message was backwards. Additionally, logging a normal pod-startup wait at error level contributed to spurious error floods in environments where registry pods have long startup times. This commit: - Checks for UpdateNotReadyErr before the generic error log, so the benign case never fires at level=error. - Corrects the log message to "update pod not yet ready". - Downgrades the log to Debug, since this is an expected polling state. - Adds a regression unit test verifying EnsureRegistryServer returns UpdateNotReadyErr (unmodified) when a not-ready update pod is present.
Change message to be more clear this is not an error Co-authored-by: Todd Short <tmshort@users.noreply.github.com>
Desired registry pods use GenerateName, so GetName() is empty and made wrapped ensure-pod errors useless. Use GetGenerateName() instead, and reference the catalog source when wrapping update-pod ensure failures.
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @pdudley. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped to correcting logging/wrapping behavior for a known benign error path and includes a targeted regression test to prevent recurrence.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes misleading error-level logging in the gRPC catalog registry reconciler when an update pod is simply not ready yet (the expected UpdateNotReadyErr path), reducing noisy production logs while preserving existing control flow.
Changes:
- Fix inverted
UpdateNotReadyErrlog message and lower severity from Error to Debug for the benign “not ready yet” case. - Improve error-wrap identifiers by using the registry pod
GenerateName(sinceNameis empty for desired pods) and including the CatalogSource name for update-pod ensure failures. - Add a regression unit test covering the polling-enabled “update pod exists but not ready” behavior.
File summaries
| File | Description |
|---|---|
| pkg/controller/registry/reconciler/grpc.go | Corrects log level/message for UpdateNotReadyErr and improves wrapped error identifiers for ensure-pod/update-pod failures. |
| pkg/controller/registry/reconciler/grpc_test.go | Adds a unit test ensuring EnsureRegistryServer returns UpdateNotReadyErr unchanged when an update pod is running but not yet ready under polling. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
No jira associated. This bug was found while investigating a separate issue. If we need to make an OCPBUGS for this let us know and we can create that. |
|
@CodeRabbit review |
|
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe gRPC registry reconciler now reports pod creation and update failures with adjusted error handling. Tests cover a polling-enabled catalog source whose running update pod is not ready. ChangesCatalogSource readiness handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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
Description of the change:
When
ensureUpdatePodreturnsUpdateNotReadyErr— the expected, benign signal that a new update pod hasn't yet reported ready —EnsureRegistryServerlogged it atlevel=errorwith the message"ensure update pod error is not of type UpdateNotReadyErr". Theokbranch fires precisely when the error isUpdateNotReadyErr, so the message was the exact opposite of what happened.Also addresses review feedback from #3871:
pod.GetGenerateName()in the ensure-pod wrap (desired pods setGenerateName, soGetName()is empty).registry server update pod not yet ready).Motivation for the change:
This mislabeled error was observed flooding logs in production environments where catalog registry pods have long startup times. Every reconcile tick during pod startup hit this path, producing a steady stream of misleading
level=errormessages that appeared to indicate a failed type-assertion, when the actual state was a completely normal pod-not-ready wait. This made triage and support significantly harder.Architectural changes:
None. Control flow is unchanged —
UpdateNotReadyErris still returned as-is; only the log message/severity and error-wrap identifiers are corrected.Testing remarks:
Added a regression unit test
Grpc/PollingEnabled/UpdatePodNotReady/ReturnsUpdateNotReadyErringrpc_test.gothat verifiesEnsureRegistryServerreturnsUpdateNotReadyErr(unmodified, not wrapped) when polling is enabled and an update pod exists but has not yet reported ready. All existing tests continue to pass.Supersedes #3871 (original author unavailable; includes outstanding review feedback).
Credit: @emmahone for the original fix and test; @tmshort for the Debug message wording.
Summary by CodeRabbit
Bug Fixes
Tests