reconciler: fix inverted log message and level for UpdateNotReadyErr - #3871
reconciler: fix inverted log message and level for UpdateNotReadyErr#3871emmahone wants to merge 2 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.
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @emmahone. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR corrects misleading logging in the gRPC registry reconciler when ensureUpdatePod returns the expected UpdateNotReadyErr during catalog polling, reducing noisy/error-level logs during normal update-pod startup behavior.
Changes:
- Adjust log severity/message for
UpdateNotReadyErrfromErrorwith an inverted message to aDebugmessage indicating the update pod is not yet ready. - Add a regression unit test covering the polling-enabled “update pod exists but not ready” path and asserting
UpdateNotReadyErris returned unwrapped. - Introduce a small test helper to create a polling-enabled
CatalogSource.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controller/registry/reconciler/grpc.go | Fixes log level/message for the benign UpdateNotReadyErr path in EnsureRegistryServer. |
| pkg/controller/registry/reconciler/grpc_test.go | Adds regression coverage to ensure UpdateNotReadyErr is returned as-is when polling is enabled and the update pod isn’t ready. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if err := c.ensurePod(logger, source, sa, defaultPodSecurityConfig, overwritePod); err != nil { | ||
| logger.WithError(err).Error("error ensuring registry server: could not ensure registry pod") | ||
| return pkgerrors.Wrapf(err, "error ensuring pod: %s", pod.GetName()) |
There was a problem hiding this comment.
If GetName() returns empty, then GetGeneratedName() should be used instead.
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
@emmahone Please address the Copilot comments. |
Change message to be more clear this is not an error Co-authored-by: Todd Short <tmshort@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/controller/registry/reconciler/grpc.go:348
- The wrapped error message uses
pod.GetName()here, butpodis a desired Pod object built viasource.Pod(...)which usesGenerateName(soGetName()is typically empty until it’s created). This can produce an unhelpful message like... pod:and also refers to the registry pod rather than the update pod being ensured.
return pkgerrors.Wrapf(err, "error ensuring updated catalog source pod: %s", pod.GetName())
|
Superseded by #3884 — same change plus the outstanding review feedback (GenerateName / catalog-source wrap / Debug wording), since Evan is unavailable. |
|
@emmahone is there a Red Hat Jira ticket associated with this change? |
|
There is not an associated RH jira issue for this PR. This was something we found while investigating unrelated issues (OCPBUGS-95609). EDIT: If we need a jira tracker, that is fine. Just let me know if that is required and I can make that jira. |
|
Closing this PR as it was resubmitted under #3884 |
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.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 and severity 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.