AGENT-1488: Aggregate IRI status from MachineConfigNodes - #5841
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@bfournie: This pull request references AGENT-1488 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. |
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 InternalReleaseImage controller now watches MachineConfigNode resources, aggregates per-master-node release statuses into cluster-wide release bundles, enqueues reconciliation on MCN status changes, and updates controller wiring and tests to include the MCN informer and aggregation logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@bfournie: This pull request references AGENT-1488 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go (1)
361-363: Double controller instantiation may cause test flakiness.
newFixture()already creates a controller internally (line 259), but this test callsf.newController()again, creating a second controller instance. The second controller has a fresh queue while the first one's queue may contain stale items from informer event handlers.Consider using the controller already created by the fixture:
func TestMCNEventHandlersEnqueueIRI(t *testing.T) { f := newFixture(t, objs(iri(), cconfig(), clusterVersion(), iriCertSecret())()) - c := f.newController() + c := f.controller🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go` around lines 361 - 363, The test TestMCNEventHandlersEnqueueIRI creates two controllers which can cause flaky behavior; remove the extra controller instantiation by using the controller already created inside newFixture() instead of calling f.newController() again — locate the test function and delete the f.newController() call (and any assignment to a second controller variable), then use the existing controller instance managed by newFixture/newFixture's fields (and ensure any references to the removed variable are updated to use the fixture's controller or its queue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go`:
- Around line 361-363: The test TestMCNEventHandlersEnqueueIRI creates two
controllers which can cause flaky behavior; remove the extra controller
instantiation by using the controller already created inside newFixture()
instead of calling f.newController() again — locate the test function and delete
the f.newController() call (and any assignment to a second controller variable),
then use the existing controller instance managed by newFixture/newFixture's
fields (and ensure any references to the removed variable are updated to use the
fixture's controller or its queue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e0e8a4a-f44f-47b2-b58e-b6bafc3554d6
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gopkg/controller/internalreleaseimage/aggregation.gopkg/controller/internalreleaseimage/aggregation_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
b499a8b to
0d0ca5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go (1)
303-314: Fixture MCN wiring is only partial for sync-path coverage.
newControllernow depends on MCN informer/lister sync, but the fixture still doesn’t plumb*mcfgv1.MachineConfigNodefrominitialObjectsinto a dedicated MCN lister/indexer path. That makes MCN-backed sync tests harder and leaves aggregation integration coverage weaker than it should be.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go` around lines 303 - 314, The test fixture doesn't populate an MCN informer/lister with the MachineConfigNode objects from initialObjects, yet newController now requires the MCN informer sync (c.mcnListerSynced) for full sync-path coverage; fix by wiring the MCN objects into a dedicated indexer/lister used by the controller test: create an MCN fake informer/indexer, add any *mcfgv1.MachineConfigNode instances from initialObjects into that indexer, set c.mcnListerSynced to the informer's HasSynced and wire the informer's Lister into the controller (same pattern used for other informers like c.mcListerSynced/c.ccListerSynced), ensuring newController receives the populated MCN lister so MCN-backed sync tests exercise the aggregation path.
🤖 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_test.go`:
- Around line 387-404: The test currently only checks c.queue.Len() after
calling addMachineConfigNode and updateMachineConfigNode; change both
enqueue-path assertions in TestMCNEventHandlersEnqueueIRI to dequeue the key
(using c.queue.Get()), assert that the returned key equals
ctrlcommon.InternalReleaseImageInstanceName (casting to string if needed), and
call c.queue.Done(key) as before; update the t.Errorf messages to report the
unexpected key value so the test fails if the wrong item was enqueued rather
than just the length being correct.
---
Nitpick comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go`:
- Around line 303-314: The test fixture doesn't populate an MCN informer/lister
with the MachineConfigNode objects from initialObjects, yet newController now
requires the MCN informer sync (c.mcnListerSynced) for full sync-path coverage;
fix by wiring the MCN objects into a dedicated indexer/lister used by the
controller test: create an MCN fake informer/indexer, add any
*mcfgv1.MachineConfigNode instances from initialObjects into that indexer, set
c.mcnListerSynced to the informer's HasSynced and wire the informer's Lister
into the controller (same pattern used for other informers like
c.mcListerSynced/c.ccListerSynced), ensuring newController receives the
populated MCN lister so MCN-backed sync tests exercise the aggregation path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 089e78d1-2ef1-4fef-99ec-ca9777c9aebe
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gopkg/controller/internalreleaseimage/aggregation.gopkg/controller/internalreleaseimage/aggregation_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/internalreleaseimage/aggregation.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/machine-config-controller/start.go
- pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
0d0ca5c to
45af2c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 564-578: The status write is being forced because
reflect.DeepEqual(newIRI.Status.Releases, aggregatedReleases) sees every
aggregated condition as changed due to a freshly assigned LastTransitionTime;
change the logic so equality ignores or preserves LastTransitionTime before the
DeepEqual: either (A) when building aggregatedReleases in aggregation.go,
preserve the existing LastTransitionTime from the matching condition in
newIRI.Status.Releases (match by condition Type/Status/Reason/Message) so
unchanged conditions retain their original LastTransitionTime, or (B) perform
the equality check by comparing releases and their conditions while explicitly
excluding LastTransitionTime (i.e., compare
Type/Status/Reason/Message/ObservedGeneration but not LastTransitionTime) and
only set aggregatedReleases' LastTransitionTime when the condition truly
transitions; then use that stable aggregatedReleases in the existing
reflect.DeepEqual/newIRI.Status.Releases check before calling UpdateStatus.
- Around line 307-309: The controller currently skips calling
ctrl.enqueueAllInternalReleaseImages() when
mcn.Status.InternalReleaseImage.Releases is empty, which misses master-set and
pool-name changes; change the logic so enqueueAllInternalReleaseImages() is
invoked not only when len(Status.InternalReleaseImage.Releases)>0 but also
whenever the object represents a master (master membership changes) or
Spec.Pool.Name changes (i.e., compare old vs new Spec.Pool.Name in update
handlers); specifically update the handlers that call
ctrl.enqueueAllInternalReleaseImages() to remove the strict non-empty-only guard
and add checks for master membership changes and Spec.Pool.Name diffs so master
add/remove or pool name updates always trigger
ctrl.enqueueAllInternalReleaseImages().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfade0d3-80c5-4e53-ab5a-11c071f0752b
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gopkg/controller/internalreleaseimage/aggregation.gopkg/controller/internalreleaseimage/aggregation_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/internalreleaseimage/aggregation.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/machine-config-controller/start.go
- pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
45af2c7 to
c11b9a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/internalreleaseimage/aggregation_test.go (1)
11-129: Lock down the full aggregation contract.The suite covers part of the new API surface, but it never asserts aggregated
Reasonvalues and doesn't exercise theRemovingcondition. A regression there would still pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/aggregation_test.go` around lines 11 - 129, The tests for aggregateReleaseStatus (TestAggregateReleaseStatus_AllNodesAvailable, _PartiallyAvailable, _OneDegraded) do not assert the aggregated Condition.Reason fields and there is no test exercising the Removing condition; update these tests to assert expected Reason values for conditions returned by aggregateReleaseStatus and add a new test that constructs MCNs via createMCNWithReleaseStatus that set a Removing=true state to ensure aggregateReleaseStatus emits a Removing condition with the correct Status and Reason. Specifically, add assertions using findCondition(result.Conditions, "...").Reason for "Available", "Degraded", "Installing" in the existing tests and add a TestAggregateReleaseStatus_Removing that calls aggregateReleaseStatus and verifies the Removing condition presence, Status, and Reason (and that the Reason string contains the node name), so regressions to aggregateReleaseStatus's Reason semantics are caught.
🤖 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 456-463: The aggregation error from aggregateMCNIRIStatus is
currently only logged and not propagated, causing successful syncs with stale
Degraded=False; detect aggErr after calling aggregateMCNIRIStatus and
incorporate it into the overall sync error (e.g., set syncErr =
errors.Wrap(aggErr, "...") or return aggErr immediately) so the deferred status
update sees a non-nil error and flips the Degraded condition/causes a retry;
update references to aggregatedReleases only when aggErr is nil and ensure you
surface aggErr via the existing syncErr variable or return path so the reconcile
fails appropriately.
---
Nitpick comments:
In `@pkg/controller/internalreleaseimage/aggregation_test.go`:
- Around line 11-129: The tests for aggregateReleaseStatus
(TestAggregateReleaseStatus_AllNodesAvailable, _PartiallyAvailable,
_OneDegraded) do not assert the aggregated Condition.Reason fields and there is
no test exercising the Removing condition; update these tests to assert expected
Reason values for conditions returned by aggregateReleaseStatus and add a new
test that constructs MCNs via createMCNWithReleaseStatus that set a
Removing=true state to ensure aggregateReleaseStatus emits a Removing condition
with the correct Status and Reason. Specifically, add assertions using
findCondition(result.Conditions, "...").Reason for "Available", "Degraded",
"Installing" in the existing tests and add a TestAggregateReleaseStatus_Removing
that calls aggregateReleaseStatus and verifies the Removing condition presence,
Status, and Reason (and that the Reason string contains the node name), so
regressions to aggregateReleaseStatus's Reason semantics are caught.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a8c3428-50a4-45bf-8bcf-642c4ddb1e88
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gopkg/controller/internalreleaseimage/aggregation.gopkg/controller/internalreleaseimage/aggregation_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/machine-config-controller/start.go
- pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
- pkg/controller/internalreleaseimage/aggregation.go
c11b9a0 to
a2a0bd3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/internalreleaseimage/aggregation_test.go (1)
11-129: Add aRemovingaggregation case.This suite validates
Available,Degraded, andInstalling, but notRemoving, which is part of the new aggregated release contract. A focused case here would keep that condition from drifting untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/aggregation_test.go` around lines 11 - 129, Add a new test case in aggregation_test.go that exercises the "Removing" aggregation path: call aggregateReleaseStatus(releaseName, mcns, nil) where one MachineConfigNode created via createMCNWithReleaseStatus(...) has "Removing": true (others Available=true), then assert that findCondition(result.Conditions, "Removing") returns a condition with Status==metav1.ConditionTrue and its Message mentions the node name; also verify overall Available/Degraded expectations as appropriate to the scenario to avoid regressions.pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go (1)
406-415: Cover master pool transitions in the update-handler test.
updateMachineConfigNodealso enqueues on worker↔master pool changes, but this test only exercises IRI-status diffs. Add assertions for enter/leave-mastertransitions so the membership logic doesn’t regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go` around lines 406 - 415, The test for updateMachineConfigNode currently only asserts enqueue behavior for IRI status changes; add cases to also assert enqueue behavior when a node's pool membership changes between worker and master. Specifically, in the test around mcn/oldMCN and calls to c.updateMachineConfigNode, add two additional scenarios: one where oldMCN has pool "worker" and mcn changes to "master" (should enqueue) and one where oldMCN is "master" and mcn changes to "worker" (should enqueue), using assertQueuedIRIKey to verify the queue counts for these transitions; keep the existing IRI-diff and no-change assertions unchanged. Ensure you mutate the MachineConfigNode pool field on the DeepCopy instances (oldMCN/mcn) used in the update calls so the membership change is detected by updateMachineConfigNode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/internalreleaseimage/aggregation_test.go`:
- Around line 11-129: Add a new test case in aggregation_test.go that exercises
the "Removing" aggregation path: call aggregateReleaseStatus(releaseName, mcns,
nil) where one MachineConfigNode created via createMCNWithReleaseStatus(...) has
"Removing": true (others Available=true), then assert that
findCondition(result.Conditions, "Removing") returns a condition with
Status==metav1.ConditionTrue and its Message mentions the node name; also verify
overall Available/Degraded expectations as appropriate to the scenario to avoid
regressions.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go`:
- Around line 406-415: The test for updateMachineConfigNode currently only
asserts enqueue behavior for IRI status changes; add cases to also assert
enqueue behavior when a node's pool membership changes between worker and
master. Specifically, in the test around mcn/oldMCN and calls to
c.updateMachineConfigNode, add two additional scenarios: one where oldMCN has
pool "worker" and mcn changes to "master" (should enqueue) and one where oldMCN
is "master" and mcn changes to "worker" (should enqueue), using
assertQueuedIRIKey to verify the queue counts for these transitions; keep the
existing IRI-diff and no-change assertions unchanged. Ensure you mutate the
MachineConfigNode pool field on the DeepCopy instances (oldMCN/mcn) used in the
update calls so the membership change is detected by updateMachineConfigNode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a746903-1bc2-4116-8201-49cf23db0bea
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gopkg/controller/internalreleaseimage/aggregation.gopkg/controller/internalreleaseimage/aggregation_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/internalreleaseimage/aggregation.go
91bd602 to
8cd099b
Compare
|
/test unit |
a5315a2 to
468ac81
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
5a13738 to
0a72f68
Compare
openshift-ci-robot
added
the
verified
|
@bfournie: This PR has been marked as verified by 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. |
| } | ||
|
|
||
| // pingRegistry checks if the registry at the given URL is reachable. | ||
| func pingRegistry(registryURL string, caCert []byte) bool { |
There was a problem hiding this comment.
nit: for now it's fine (not blocking), but I'd like in future to see if we could reuse iri_registry from MCD, since CheckLocalRegistry() does exactly that, and we could avoid duplicating the logic between the controller and the daemon
There was a problem hiding this comment.
Created https://redhat.atlassian.net/browse/AGENT-1521 to track this.
|
/test ? |
|
/lgtm Let's also gather a green run with the latest changes / tests |
|
Scheduling tests matching the |
openshift-ci-robot
removed
the
verified
|
/test e2e-agent-compact-ipv4-iso-no-registry |
3e6ff2f to
3d844ad
Compare
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@bfournie: 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @bfournie |
openshift-ci-robot
added
the
verified
|
@bfournie: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, bfournie, cheesesashimi 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
AGENT-1488: Aggregate IRI status from MachineConfigNodes
5 participants
Add cluster-wide InternalReleaseImage status aggregation from all master MachineConfigNodes. For each release bundle, aggregates conditions (Available, Degraded, Installing, Removing) across nodes.
Status messages include node counts and degraded node names for operational visibility. Follows the Node Controller → MCP aggregation pattern.
- What I did
Add cluster-wide InternalReleaseImage status aggregation from all master MachineConfigNodes. For each release bundle, aggregates conditions (Available, Degraded, Installing, Removing) across nodes.
- How to verify it
oc get internalreleaseimage cluster -o yamle.g.
Check that Available=True when all master MCNs report Available=True
Scenario B: Simulate degraded node (if possible)
- Description for the changelog