OCPBUGS-85791: Check more errors when copying private image secret#2909
OCPBUGS-85791: Check more errors when copying private image secret#2909dlom wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@dlom: This pull request references Jira Issue OCPBUGS-85791, 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. |
📝 WalkthroughWalkthroughThe cluster deployment controller's ChangesPrivate image pull secret copy with namespace termination awareness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom 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
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dlom: This pull request references Jira Issue OCPBUGS-85791, 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. |
|
@dlom: This pull request references Jira Issue OCPBUGS-85791, 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. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/clusterdeployment/clusterdeployment_controller.go`:
- Around line 2525-2527: The log inside the err2 branch currently logs the wrong
error variable (cdLog.WithError(err).Warn(...)) which misleads debugging; change
it to log err2 (e.g., cdLog.WithError(err2).Warn("Error checking namespace
termination or discovering namespace")) so the logged error matches the returned
errors.Wrapf(err2, "failed to discover whether namespace %s is marked for
deletion", cd.Namespace); update the log message to clearly indicate this is a
namespace lookup/termination check failure and keep the existing return that
wraps err2.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48242140-2e69-4816-a312-aa977ccb4a52
📒 Files selected for processing (1)
pkg/controller/clusterdeployment/clusterdeployment_controller.go
| if err2 != nil { | ||
| cdLog.WithError(err).Warn("Error copying private image pull secret") | ||
| return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace) |
There was a problem hiding this comment.
Log the namespace lookup failure in the err2 branch.
Line 2526 logs err (copy failure) even though this branch handles err2 (namespace termination check failure). This can mislead debugging since the returned error is based on err2.
Suggested fix
- if err2 != nil {
- cdLog.WithError(err).Warn("Error copying private image pull secret")
- return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace)
- }
+ if err2 != nil {
+ cdLog.WithError(err2).Warn("failed to check whether target namespace is marked for deletion after private image pull secret copy failure")
+ return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace)
+ }📝 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.
| if err2 != nil { | |
| cdLog.WithError(err).Warn("Error copying private image pull secret") | |
| return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace) | |
| if err2 != nil { | |
| cdLog.WithError(err2).Warn("failed to check whether target namespace is marked for deletion after private image pull secret copy failure") | |
| return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace) | |
| } |
🤖 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/controller/clusterdeployment/clusterdeployment_controller.go` around
lines 2525 - 2527, The log inside the err2 branch currently logs the wrong error
variable (cdLog.WithError(err).Warn(...)) which misleads debugging; change it to
log err2 (e.g., cdLog.WithError(err2).Warn("Error checking namespace termination
or discovering namespace")) so the logged error matches the returned
errors.Wrapf(err2, "failed to discover whether namespace %s is marked for
deletion", cd.Namespace); update the log message to clearly indicate this is a
namespace lookup/termination check failure and keep the existing return that
wraps err2.
There was a problem hiding this comment.
I think the way you've got it written up makes sense, as you're bailing on the reconcile based on the namespace termination discovery failing; but it's still constructive to know why CopySecret failed.
There was a problem hiding this comment.
@2uasimojo — that's a fair point. Both failure contexts are useful: why the copy failed, and why the fallback namespace check also failed. A clean way to surface both without two separate log lines would be to attach the original copy error as a field:
if err2 != nil {
cdLog.WithError(err2).
WithField("copyError", err.Error()).
Warn("failed to check whether target namespace is marked for deletion after private image pull secret copy failure")
return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace)
}This way:
WithError(err2)surfaces the namespace-check failure (which drives the returned error),WithField("copyError", err.Error())preserves the originalCopySecretfailure for debugging.
Alternatively, two separate log calls work just as well if you prefer that style:
if err2 != nil {
cdLog.WithError(err).Warn("Error copying private image pull secret")
cdLog.WithError(err2).Warn("failed to check whether target namespace is marked for deletion")
return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace)
}Either approach is an improvement over the current code which logs err but returns/wraps err2, making the log misleading.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2909 +/- ##
==========================================
- Coverage 50.39% 50.38% -0.02%
==========================================
Files 281 281
Lines 34368 34378 +10
==========================================
Hits 17320 17320
- Misses 15696 15706 +10
Partials 1352 1352
🚀 New features to boost your workflow:
|
|
@dlom: 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. |
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
This had me a little worried: unlike where this logic was copied from, we're not in a deletion path here (yet). But I think I've convinced myself it works properly 👍
Unit test case or two? (The fact that I didn't write one for the parent code is no excuse :P )
| if err2 != nil { | ||
| cdLog.WithError(err).Warn("Error copying private image pull secret") | ||
| return false, errors.Wrapf(err2, "failed to discover whether namespace %s is marked for deletion", cd.Namespace) |
There was a problem hiding this comment.
I think the way you've got it written up makes sense, as you're bailing on the reconcile based on the namespace termination discovery failing; but it's still constructive to know why CopySecret failed.
4 participants
xref OCPBUGS-85791
/assign @suhanime
Summary by CodeRabbit