AGENT-1366: Report InternalReleaseImageController errors in IRI status#5803
AGENT-1366: Report InternalReleaseImageController errors in IRI status#5803bfournie wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@bfournie: This pull request references AGENT-1366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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
added
the
jira/valid-reference
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe controller's sync now captures a named Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Line 340: The error construction assigning to syncErr uses fmt.Errorf("could
not get ControllerConfig %w", err) and is missing the colon before the wrapped
error; update that call to include the colon so it matches the other message
(e.g., change to fmt.Errorf("could not get ControllerConfig: %w", err)) to keep
formatting consistent for the ControllerConfig error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2303d8c9-1670-4b55-8275-8b77035f94ff
📒 Files selected for processing (2)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
Outdated
Show resolved
Hide resolved
bfournie
force-pushed
the
iri-controller
branch
from
3e22ff1 to
a7f10da
Compare
|
/cc @andfasano |
|
/jira refresh |
|
@bfournie: This pull request references AGENT-1366 which is a valid jira issue. 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go (1)
340-394:⚠️ Potential issue | 🟠 MajorHandle status-update failures in error paths instead of dropping them silently
In Lines 341, 348, 359, 366, 374, 380, 385, and 393,
updateInternalReleaseImageStatus(...)errors are ignored. If those calls fail, the controller returnssyncErrwith no signal that status reporting also failed, which undermines this PR’s core behavior.Suggested refactor
func (ctrl *Controller) syncInternalReleaseImage(key string) error { @@ - // Track sync error to update status at the end - var syncErr error + reportSyncError := func(syncErr error) error { + if statusErr := ctrl.updateInternalReleaseImageStatus(iri, syncErr); statusErr != nil { + klog.Warningf("Failed to update InternalReleaseImage status after sync error: %v (sync error: %v)", statusErr, syncErr) + } + return syncErr + } @@ cconfig, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) if err != nil { - syncErr = fmt.Errorf("could not get ControllerConfig: %w", err) - ctrl.updateInternalReleaseImageStatus(iri, syncErr) - return syncErr + return reportSyncError(fmt.Errorf("could not get ControllerConfig: %w", err)) } @@ iriSecret, err := ctrl.secretLister.Secrets(ctrlcommon.MCONamespace).Get(ctrlcommon.InternalReleaseImageTLSSecretName) if err != nil { - syncErr = fmt.Errorf("could not get Secret %s: %w", ctrlcommon.InternalReleaseImageTLSSecretName, err) - ctrl.updateInternalReleaseImageStatus(iri, syncErr) - return syncErr + return reportSyncError(fmt.Errorf("could not get Secret %s: %w", ctrlcommon.InternalReleaseImageTLSSecretName, err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go` around lines 340 - 394, The status-update calls like ctrl.updateInternalReleaseImageStatus(...) (and the initializeInternalReleaseImageStatus/ addFinalizerToInternalReleaseImage paths) currently ignore their returned error; change each to capture the statusErr and, if non-nil, combine it with the original syncErr (or wrap both) and return the combined error so callers know both the sync failure and the status-update failure occurred. Concretely, after setting syncErr (e.g., syncErr = fmt.Errorf("...: %w", err)), call statusErr := ctrl.updateInternalReleaseImageStatus(iri, syncErr); if statusErr != nil { return fmt.Errorf("%v; status update failed: %w", syncErr, statusErr) } (apply this pattern to every place that calls ctrl.updateInternalReleaseImageStatus, plus the calls to ctrl.initializeInternalReleaseImageStatus and ctrl.addFinalizerToInternalReleaseImage so their errors are checked and propagated).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 340-394: The status-update calls like
ctrl.updateInternalReleaseImageStatus(...) (and the
initializeInternalReleaseImageStatus/ addFinalizerToInternalReleaseImage paths)
currently ignore their returned error; change each to capture the statusErr and,
if non-nil, combine it with the original syncErr (or wrap both) and return the
combined error so callers know both the sync failure and the status-update
failure occurred. Concretely, after setting syncErr (e.g., syncErr =
fmt.Errorf("...: %w", err)), call statusErr :=
ctrl.updateInternalReleaseImageStatus(iri, syncErr); if statusErr != nil {
return fmt.Errorf("%v; status update failed: %w", syncErr, statusErr) } (apply
this pattern to every place that calls ctrl.updateInternalReleaseImageStatus,
plus the calls to ctrl.initializeInternalReleaseImageStatus and
ctrl.addFinalizerToInternalReleaseImage so their errors are checked and
propagated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78822c2d-cb45-4f29-b47e-2d08c78d3c53
📒 Files selected for processing (2)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
bfournie
force-pushed
the
iri-controller
branch
from
a7f10da to
66a0fed
Compare
|
/test unit |
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 413-415: The status-update failures are currently only logged (at
the success path and where statusErr is set) which hides transient write errors;
change the reconcile code to return the error instead of klog.Warningf so
failures bubble up and trigger requeue, and update
updateInternalReleaseImageStatus to avoid reading from the cached ctrl.iriLister
inside its retry loop—use the API server via ctrl.client.Get to fetch the latest
InternalReleaseImage prior to each UpdateStatus attempt to avoid stale
resourceVersion conflicts; also propagate errors from the earlier statusErr site
in the same manner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46960fb6-ce53-4623-9be0-e3000343918b
📒 Files selected for processing (2)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
Outdated
Show resolved
Hide resolved
bfournie
force-pushed
the
iri-controller
branch
from
66a0fed to
d82431e
Compare
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
Outdated
Show resolved
Hide resolved
bfournie
force-pushed
the
iri-controller
branch
from
d82431e to
3c54986
Compare
|
/retest-required |
Implemented status condition reporting for the InternalReleaseImageController to report controller errors/issues in the InternalReleaseImage (IRI) status.
bfournie
force-pushed
the
iri-controller
branch
from
3c54986 to
a3564a1
Compare
|
/retest |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@bfournie: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, bfournie 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 |
5 participants
Implemented status condition reporting for the InternalReleaseImageController to report controller errors/issues in the InternalReleaseImage (IRI) status.
- What I did
- How to verify it
- Description for the changelog