updating ImageRebuildTriggerGeneration in module status#1756
Conversation
ImageRebuildTriggerGeneration field that was intorduced in e6e0871 commit was never updated in the Module's status, even when MIC already finished to re-build the image and ImageRebuildTriggerGeneration field was updated in the MIC's status. This commit fixes it by updating ImageRebuildTriggerGeneration in the Module's status at the end of the Module's reconciliation.
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman 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 |
WalkthroughThe PR adds logic to propagate the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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)
internal/controllers/module_reconciler_test.go (1)
1208-1234: Split MIC error-path tests by error type (NotFound vs transient).At Line [1208], the test currently codifies “all MIC get errors are non-fatal.” It’s safer to assert separate behavior:
- NotFound: non-fatal + clear/keep explicit nil semantics.
- Transient API error: reconciliation should fail and retry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/module_reconciler_test.go` around lines 1208 - 1234, Split the single MIC-error test into two tests: one that simulates a NotFound error and one that simulates a transient API error. For the NotFound case, have mockMicAPI.EXPECT().Get(...) return apierrors.NewNotFound(...), call moduleUpdateWorkerPodsStatus(ctx, &mod, targetedNodes) and assert no error and the same nil/unchanged Status semantics (e.g., ImageRebuildTriggerGeneration remains nil) as currently done; for the transient case, have mockMicAPI.EXPECT().Get(...) return a regular fmt.Errorf("transient") (or similar), call moduleUpdateWorkerPodsStatus and assert it returns an error (reconciliation should fail/retry). Ensure gomock.InOrder expectations around clnt.List, helper.GetModuleSpecEntry, mockMicAPI.Get, clnt.Status().Patch(...) are adjusted per test to match the expected flow and that the NotFound test still expects the Patch call while the transient test expects no Patch and an error return from moduleUpdateWorkerPodsStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controllers/module_reconciler.go`:
- Around line 550-555: The current branch ignores all errors from mrh.micAPI.Get
so mod.Status.ImageRebuildTriggerGeneration can stay stale; change the logic in
the reconcile where mrh.micAPI.Get is called so that: if Get returns a NotFound
(use the Kubernetes apierrors.IsNotFound check) clear/zero
mod.Status.ImageRebuildTriggerGeneration, if Get returns any other error return
that error so the reconcile will be retried, and only when Get succeeds copy
micObj.Status.ImageRebuildTriggerGeneration into
mod.Status.ImageRebuildTriggerGeneration; update any log messages (logger.Info)
to reflect the NotFound vs transient error handling.
---
Nitpick comments:
In `@internal/controllers/module_reconciler_test.go`:
- Around line 1208-1234: Split the single MIC-error test into two tests: one
that simulates a NotFound error and one that simulates a transient API error.
For the NotFound case, have mockMicAPI.EXPECT().Get(...) return
apierrors.NewNotFound(...), call moduleUpdateWorkerPodsStatus(ctx, &mod,
targetedNodes) and assert no error and the same nil/unchanged Status semantics
(e.g., ImageRebuildTriggerGeneration remains nil) as currently done; for the
transient case, have mockMicAPI.EXPECT().Get(...) return a regular
fmt.Errorf("transient") (or similar), call moduleUpdateWorkerPodsStatus and
assert it returns an error (reconciliation should fail/retry). Ensure
gomock.InOrder expectations around clnt.List, helper.GetModuleSpecEntry,
mockMicAPI.Get, clnt.Status().Patch(...) are adjusted per test to match the
expected flow and that the NotFound test still expects the Patch call while the
transient test expects no Patch and an error return from
moduleUpdateWorkerPodsStatus.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/controllers/module_reconciler.gointernal/controllers/module_reconciler_test.go
| micObj, err := mrh.micAPI.Get(ctx, mod.Name, mod.Namespace) | ||
| if err != nil { | ||
| logger.Info("Could not get MIC to update ImageRebuildTriggerGeneration status, skipping", "error", err) | ||
| } else { | ||
| mod.Status.ImageRebuildTriggerGeneration = micObj.Status.ImageRebuildTriggerGeneration | ||
| } |
There was a problem hiding this comment.
Handle MIC read failures explicitly to avoid stale Module status.
At Line [551], all MIC Get errors are ignored, so mod.Status.ImageRebuildTriggerGeneration can remain stale and diverge from MIC indefinitely. Prefer: clear on NotFound, but return error for transient failures so reconcile retries.
Proposed fix
import (
"context"
"errors"
"fmt"
"reflect"
"strings"
@@
v1 "k8s.io/api/core/v1"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@
micObj, err := mrh.micAPI.Get(ctx, mod.Name, mod.Namespace)
if err != nil {
- logger.Info("Could not get MIC to update ImageRebuildTriggerGeneration status, skipping", "error", err)
+ if apierrors.IsNotFound(err) {
+ mod.Status.ImageRebuildTriggerGeneration = nil
+ } else {
+ return fmt.Errorf("failed to get MIC %s/%s: %w", mod.Namespace, mod.Name, err)
+ }
} else {
mod.Status.ImageRebuildTriggerGeneration = micObj.Status.ImageRebuildTriggerGeneration
}📝 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.
| micObj, err := mrh.micAPI.Get(ctx, mod.Name, mod.Namespace) | |
| if err != nil { | |
| logger.Info("Could not get MIC to update ImageRebuildTriggerGeneration status, skipping", "error", err) | |
| } else { | |
| mod.Status.ImageRebuildTriggerGeneration = micObj.Status.ImageRebuildTriggerGeneration | |
| } | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "reflect" | |
| "strings" | |
| v1 "k8s.io/api/core/v1" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| micObj, err := mrh.micAPI.Get(ctx, mod.Name, mod.Namespace) | |
| if err != nil { | |
| if apierrors.IsNotFound(err) { | |
| mod.Status.ImageRebuildTriggerGeneration = nil | |
| } else { | |
| return fmt.Errorf("failed to get MIC %s/%s: %w", mod.Namespace, mod.Name, err) | |
| } | |
| } else { | |
| mod.Status.ImageRebuildTriggerGeneration = micObj.Status.ImageRebuildTriggerGeneration | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controllers/module_reconciler.go` around lines 550 - 555, The
current branch ignores all errors from mrh.micAPI.Get so
mod.Status.ImageRebuildTriggerGeneration can stay stale; change the logic in the
reconcile where mrh.micAPI.Get is called so that: if Get returns a NotFound (use
the Kubernetes apierrors.IsNotFound check) clear/zero
mod.Status.ImageRebuildTriggerGeneration, if Get returns any other error return
that error so the reconcile will be retried, and only when Get succeeds copy
micObj.Status.ImageRebuildTriggerGeneration into
mod.Status.ImageRebuildTriggerGeneration; update any log messages (logger.Info)
to reflect the NotFound vs transient error handling.
|
/retest |
7 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/test ci/prow/e2e |
|
/test e2e |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| micObj, err := mrh.micAPI.Get(ctx, mod.Name, mod.Namespace) | ||
| if err != nil { | ||
| logger.Info("Could not get MIC to update ImageRebuildTriggerGeneration status, skipping", "error", err) | ||
| } else { | ||
| mod.Status.ImageRebuildTriggerGeneration = micObj.Status.ImageRebuildTriggerGeneration | ||
| } |
There was a problem hiding this comment.
Why are we doing it inside of moduleUpdateWorkerPodsStatus? This is not related to the worker pods.
I would create a new helper method called moduleUpdatePullerPodsStatus or something similar.
I know this is what we did u/s and I am OK merging this PR as is but I think we should fix it u/s and then pull this change m/s as well.
There was a problem hiding this comment.
I agree, I think also we should rename this function since its not updating worker pod status, maybe something like updateModuleLoaderStatus.
|
/retest |
|
/override ci/prow/e2e-hub |
|
@yevgeny-shnaidman: Overrode contexts on behalf of yevgeny-shnaidman: ci/prow/e2e-hub 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/operator-upgrade |
|
@yevgeny-shnaidman: Overrode contexts on behalf of yevgeny-shnaidman: ci/prow/operator-upgrade 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/operator-hub-upgrade |
|
@yevgeny-shnaidman: Overrode contexts on behalf of yevgeny-shnaidman: ci/prow/operator-hub-upgrade 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e |
|
@yevgeny-shnaidman: Overrode contexts on behalf of yevgeny-shnaidman: ci/prow/e2e 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 kubernetes-sigs/prow repository. |
|
/lgtm |
openshift-merge-bot
bot
merged commit d11e4f4
into
rh-ecosystem-edge:main
3 participants
ImageRebuildTriggerGeneration field that was introduced in #1725 commit was never updated in the Module's status, even when MIC already finished to re-build the image and ImageRebuildTriggerGeneration field was updated in the MIC's status.
This commit fixes it by updating ImageRebuildTriggerGeneration in the Module's status at the end of the Module's reconciliation.
/cc @yevgeny-shnaidman @ybettan
fixes #1755
Summary by CodeRabbit
Bug Fixes
Tests