CNF-23239: controller: nrop: per-tree processing and progress

[ffromani]

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits which are intentionally shaped to make the backport down to the distant 4.18 as low risk as possible.
Compatibility layers are added and then removed in the context of the same PR explicitly and only in order
to enable gradual testing, not because their own merit.

review notice

Adding and removing code in the same PR makes the review harder, even though the final tree state is identical.
It may be useful to review the final tree state (e.g. git diff ..HEAD) in addition to individual commits, since the end result is identical to a cleaner, main-only series.
The most independent commits are intentionally pushed towards the beginning of the series to enable this review mode as much as possible.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	minimal	Essentially clean cherry-picks (after PR #4025 lands)
4.20	very low	Minor controller conflicts (after PR #3801 backport lands)
4.19	low	Minor conflicts; rte.go diverges more (after PR #3801 backport lands)
4.18	high	Path renames, no MachineConfigsState, machineconfigpool.go heavily diverged (see below)

4.18 effort breakdown

Commits	Effort	Nature
1 (Name() helper)	very low	Path adjustment: api/v1/helper/ → api/numaresourcesoperator/v1/helper/
2-3 (conditioninfo, step helpers)	minimal	step.go identical; conditioninfo.go nearly identical
4 (move helper around)	low	Path adjustment: internal/controller/ → controllers/
5 (rte + compat wrappers)	hard/very hard	Key refactoring; machineconfigpool.go is 134 lines (vs 244 on main), no MachineConfigObjectState type, no MachineConfigsState method; rte.go is 436 lines (vs 379) with 288 diff lines
6 (signature refactoring)	low	Path adjustment only
7 (add per-tree functions)	mid/hard	New code, but depends on compat wrappers from commit 5 being adapted first
8 (wire per-tree functions)	hard	Controller rewrite; building blocks from 5-7 must be proven first
9-10 (narrow helpers, rename)	very low	Mechanical signature changes
11 (remove compat wrappers)	mid	Adaptation for different machineconfigpool.go baseline
12 (vertical processing)	mid	Controller change; same pattern as main but different baseline

4.21/4.20/4.19 prerequisite

These branches require PR #3801 ("OCPBUGS-84226: per-pool MachineConfig state
with paused MCP awareness") to be backported first. PR #4025 covers 4.21;
4.20 and 4.19 need equivalent backports. Once that prerequisite lands,
these branches have the same MachineConfigObjectState type and
MachineConfigsState signature as main, making the cherry-picks
straightforward.

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-3 (Name helper, UpdateMessage, step helpers) are trivial
   cherry-picks on all branches. Done and verified quickly.
2. Commit 4 (move helper) is a pure reorder, low risk.
3. Commit 5 (rte compat wrappers) is the key risk point — it refactors
   the objectstate layer but adds compat wrappers so the controller continues
   working unchanged. Existing controller tests validate the compat wrappers
   without any controller changes.
4. Commit 6 (signature refactoring) is mechanical prep work.
5. Commit 7 (add per-tree functions) adds dead code. If it compiles,
   it is correct.
6. Commit 8 (wire per-tree functions) is the core behavioral change.
   By this point all building blocks (1-7) are proven and verified.
7. Commits 9-10 (narrow helpers, rename) are mechanical signature
   narrowing, safe after commit 8 removed the old callers.
8. Commit 11 (remove compat wrappers) is mechanical cleanup.
9. Commit 12 (vertical processing) is the final behavioral change,
   switching from horizontal to vertical orchestration.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring on any branch (including 4.18, the
hardest target) without touching the controller at all. Once that is
proven, the controller changes build on a solid foundation.

[ffromani]

/hold

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ffromani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the NUMAResourcesOperator controller from all-trees-at-once reconciliation to per-tree iteration. It separates tree-agnostic object state (RBAC, metrics) from per-tree state derivation, adds Step state predicates and message updates, and replaces multi-phase reconciliation with a single orchestration that applies tree-agnostic resources, then reconciles each tree and aggregates results.

Changes

Per-tree reconciliation refactor with tree-agnostic object state

Layer / File(s)	Summary
Tree naming utility api/v1/helper/nodegroup/nodegroup.go, api/v1/helper/nodegroup/nodegroup_test.go	Tree.Name() returns the associated MCP pool name with safe fallbacks (<nil>, <unknown>) for nil values.
Step state and message infrastructure internal/reconcile/step.go, internal/reconcile/step_test.go	Ongoing() checks if requeue is pending, Failed() checks for errors, and UpdateMessage() prepends messages to conditions while preserving result/error state.
Condition message updates pkg/status/conditioninfo/conditioninfo.go, pkg/status/conditioninfo/conditioninfo_test.go	ConditionInfo.UpdateMessage() overwrites/augments existing messages; differs from WithMessage which only sets when empty.
Object state tree-agnostic and per-tree separation pkg/objectstate/rte/rte.go	FromClientTreeAgnostic() replaces FromClient(), building core RBAC/metrics without trees. TreeAgnostic() replaces State(). New PerTree() and PerTreeState() methods derive per-tree instances on demand.
Machine config pool handling per-tree pkg/objectstate/rte/machineconfigpool.go, pkg/objectstate/rte/machineconfigpool_test.go	MachineConfigsState() now takes explicit tree parameter instead of iterating em.trees internally. Tests refactored to call per-tree instead of batch.
Controller per-tree reconciliation orchestration internal/controller/numaresourcesoperator_controller.go	Replaces multi-phase flow with per-tree orchestration: API reconciliation (early-stop), tree-agnostic setup, per-tree machine-config/daemonset sync, and aggregation of paused MCPs and readiness status into single Step.
Controller reconciliation test updates internal/controller/numaresourcesoperator_controller_test.go	Two MachineConfigPool entries during wait phase (instead of one), paused-condition checks replacing Available assertions, and new async convergence test verifying staged DaemonSet creation as trees become ready.

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.38% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'CNF-23239: controller: nrop: per-tree processing and progress' clearly and specifically summarizes the main change: implementing per-tree processing with independent progress tracking in the NROP controller.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description is directly related to the changeset, clearly explaining the shift from 'horizontal split' to 'vertical split' reconciliation and detailing the changes across multiple files and commits.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch per-tree-processing

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Tal-or]

I know it's still WIP but already adding my comments so they won't slip.

[Tal-or]

if we have pausedMCPNames, we should not set this StepSuccess() but StepOngoing() so controller wont continue to update DS for this specific tree

[ffromani]

StepOngoing() wants to requeue the request though. I'm not 100% sure this is the right approach.

[Tal-or]

StepSuccess continues to update the DS, and doing that while the MCP paused, might causes to misalignment with security context/ SELinux policy.

Requeue the request for later is the lesser evil option IMO.
Another option is not continue and not requeue, a new request will come in anyway once the MCP moved to paused: false because we're watching it.
Not sure if we have a step that does that though.

[ffromani]

we can probably use StepOngoing with 0 value; this should report the right condition without causing a requeue

[openshift-ci-robot]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

WIP: implement "vertical split" vs current "horizontal split"

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]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits in a backport-friendly way.
Compatibility layers are added and then removed in the context of the same PR
to implify the backport process and to enable gradual testing, not because
these compatibility layers are necessary or useful besides the backporting process.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	~1 hour	Essentially clean cherry-picks
4.21	~2 hours	Minor conflicts
4.20	~3 hours	Minor conflicts
4.19	~5 hours	Moderate conflicts, same architecture
4.18	~2.5-3 days	Significant adaptation (see below)
Total	~3.5-4.5 days

4.18 effort breakdown

Commits	Effort	Nature
1-3 (utilities)	~1-2 hours	Import path adjustments
4 (reordering)	~2 hours	Different baseline
5 (rte + compat)	~1 day	Key refactoring, testable in isolation
6 (dead code)	~half day	Adaptation for missing features
7 (wiring)	~1 day	Controller rewrite, building blocks proven
8-9 (cleanup)	~2-3 hours	Mechanical

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-4 are trivial cherry-picks. Done and verified quickly.
2. Commit 5 is the rte refactoring validated independently -- existing
   controller tests prove the compat wrappers work. If something breaks,
   the bug is isolated to the rte changes.
3. Commit 6 adds dead code. If it compiles, it is correct.
4. Commit 7 is the only commit requiring real adaptation, but by this
   point all building blocks (1-6) can be proven and be verified.
5. Commits 8-9 are mechanical.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring in 4.18 (the hardest piece) without
touching the controller at all. Once that is proven, the controller
changes build on a solid foundation.

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.

[ffromani]

/hold cancel

[Tal-or]

Usually it's helpful to break the PR into smaller commits, but here (especially in the few last commits) some of the newer commits are overriding/updating the logic of their previous ones.

I know it was made to convey the steps clearly but it's bit confusing because now I see that I commented on lines of code that were deleted/moved in later commits.

Anyway lets address what we have right now and I'll have another round later on this week.

[Tal-or]

Since this area is going through a major refactoring anyway, it would be nice to make the names of variables and functions more accurate - it's the MachineConfigPool* that we're checking/updating and not MachineConfig*
as we all aware those are related though different objects.

[ffromani]

I agree about improving, thing is this function want to compute the existing state of MachineConfig objects, and compute the desired state of MachineConfig, which are indeed related to MachineConfigPools, but the pool is the scoping mechanism.

IOW, we want to reconcile MachineConfigs, and return ObjectState about these.

[Tal-or]

If that's PerTree how come more than a single DamonSet and/or MachineConfig?

[ffromani]

Mostly, because I'm reusing ExistingManifests which predates the split and had 1:N relationship because the horizontal partitioning.

We can probably simplify further as followup of this PR

[Tal-or]

slice of DSs and MCPs implies that this is for all trees so should be treesReconcileResult in plural or returning a slice of treeReconcileResult one per tree

[ffromani]

this is meant to be a reconcile result of a single tree (therefore perTreeReconcileResult is probably better), but the problem here is we can't yet enforce the 1:1 mapping between trees and MCPs (https://github.com/openshift-kni/numaresources-operator/blob/main/internal/api/annotations/annotations.go#L25).
Well, there's an argument about doing that in 5.0 - we had 3 EUS version past 4.18 already, but then should be done as prerequisite of this PR, and would make backporting significantly more complex.
This is why I'm still carrying this forward and postponing to 5.2.

[Tal-or]

forsaken code comment?

[ffromani]

no, this is is a gap on my side. Let me have another cleanup pass. Restoring WIP for the time being.

[ffromani]

/retitle WIP: CNF-23239: controller: nrop: per-tree processing and progress

thanks for the review. I'll need to do more cleanup

[openshift-ci-robot]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits in a backport-friendly way.
Compatibility layers are added and then removed in the context of the same PR
to implify the backport process and to enable gradual testing, not because
these compatibility layers are necessary or useful besides the backporting process.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	very low	Minor conflicts
4.20	very low	Minor conflicts
4.19	low/mid	Moderate conflicts, same architecture
4.18	high	Significant adaptation (see below)

4.18 effort breakdown

Commits	Effort	Nature
1-3 (utilities)	minimal/very low	Import path adjustments
4 (reordering)	very low	Different baseline
5 (rte + compat)	hard/very gard	Key refactoring, testable in isolation
6 (dead code)	mid/hard	Adaptation for missing features
7 (wiring)	hard	Controller rewrite, building blocks proven
8-9 (cleanup)	very low	Mechanical

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-4 are trivial cherry-picks. Done and verified quickly.
2. Commit 5 is the rte refactoring validated independently -- existing
   controller tests prove the compat wrappers work. If something breaks,
   the bug is isolated to the rte changes.
3. Commit 6 adds dead code. If it compiles, it is correct.
4. Commit 7 is the only commit requiring real adaptation, but by this
   point all building blocks (1-6) can be proven and be verified.
5. Commits 8-9 are mechanical.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring in 4.18 (the hardest piece) without
touching the controller at all. Once that is proven, the controller
changes build on a solid foundation.

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]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits in a backport-friendly way.
Compatibility layers are added and then removed in the context of the same PR
to implify the backport process and to enable gradual testing, not because
these compatibility layers are necessary or useful besides the backporting process.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	very low	Minor conflicts
4.20	very low	Minor conflicts
4.19	low/mid	Moderate conflicts, same architecture
4.18	high	Significant adaptation (see below)

4.18 effort breakdown

Commits	Effort	Nature
1-3 (utilities)	minimal/very low	Import path adjustments
4 (reordering)	very low	Different baseline
5 (rte + compat)	hard/very hard	Key refactoring, testable in isolation
6 (dead code)	mid/hard	Adaptation for missing features
7 (wiring)	hard	Controller rewrite, building blocks proven
8-9 (cleanup)	very low	Mechanical

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-4 are trivial cherry-picks. Done and verified quickly.
2. Commit 5 is the rte refactoring validated independently -- existing
   controller tests prove the compat wrappers work. If something breaks,
   the bug is isolated to the rte changes.
3. Commit 6 adds dead code. If it compiles, it is correct.
4. Commit 7 is the only commit requiring real adaptation, but by this
   point all building blocks (1-6) can be proven and be verified.
5. Commits 8-9 are mechanical.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring in 4.18 (the hardest piece) without
touching the controller at all. Once that is proven, the controller
changes build on a solid foundation.

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]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits in a backport-friendly way.
Compatibility layers are added and then removed in the context of the same PR explicitly and only in order
to simplify the backport process and to enable gradual testing, not because their own merit.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	very low	Minor conflicts
4.20	very low	Minor conflicts
4.19	low/mid	Moderate conflicts, same architecture
4.18	high	Significant adaptation (see below)

4.18 effort breakdown

Commits	Effort	Nature
1-3 (utilities)	minimal/very low	Import path adjustments
4 (reordering)	very low	Different baseline
5 (rte + compat)	hard/very hard	Key refactoring, testable in isolation
6 (dead code)	mid/hard	Adaptation for missing features
7 (wiring)	hard	Controller rewrite, building blocks proven
8-9 (cleanup)	very low	Mechanical

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-4 are trivial cherry-picks. Done and verified quickly.
2. Commit 5 is the rte refactoring validated independently -- existing
   controller tests prove the compat wrappers work. If something breaks,
   the bug is isolated to the rte changes.
3. Commit 6 adds dead code. If it compiles, it is correct.
4. Commit 7 is the only commit requiring real adaptation, but by this
   point all building blocks (1-6) can be proven and be verified.
5. Commits 8-9 are mechanical.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring in 4.18 (the hardest piece) without
touching the controller at all. Once that is proven, the controller
changes build on a solid foundation.

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]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits in a backport-friendly way.
Compatibility layers are added and then removed in the context of the same PR explicitly and only in order
to simplify the backport process and to enable gradual testing, not because their own merit.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	minimal	Essentially clean cherry-picks (after PR #4025 lands)
4.20	very low	Minor controller conflicts (after PR #3801 backport lands)
4.19	low	Minor conflicts; rte.go diverges more (after PR #3801 backport lands)
4.18	high	Path renames, no MachineConfigsState, machineconfigpool.go heavily diverged (see below)

4.18 effort breakdown

Commits	Effort	Nature
1 (Name() helper)	very low	Path adjustment: api/v1/helper/ → api/numaresourcesoperator/v1/helper/
2-3 (conditioninfo, step helpers)	minimal	step.go identical; conditioninfo.go nearly identical
4 (move helper around)	low	Path adjustment: internal/controller/ → controllers/
5 (rte + compat wrappers)	hard/very hard	Key refactoring; machineconfigpool.go is 134 lines (vs 244 on main), no MachineConfigObjectState type, no MachineConfigsState method; rte.go is 436 lines (vs 379) with 288 diff lines
6 (signature refactoring)	low	Path adjustment only
7 (add per-tree functions)	mid/hard	New code, but depends on compat wrappers from commit 5 being adapted first
8 (wire per-tree functions)	hard	Controller rewrite; building blocks from 5-7 must be proven first
9-10 (narrow helpers, rename)	very low	Mechanical signature changes
11 (remove compat wrappers)	mid	Adaptation for different machineconfigpool.go baseline
12 (vertical processing)	mid	Controller change; same pattern as main but different baseline

4.21/4.20/4.19 prerequisite

These branches require PR #3801 ("OCPBUGS-84226: per-pool MachineConfig state
with paused MCP awareness") to be backported first. PR #4025 covers 4.21;
4.20 and 4.19 need equivalent backports. Once that prerequisite lands,
these branches have the same MachineConfigObjectState type and
MachineConfigsState signature as main, making the cherry-picks
straightforward.

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-3 (Name helper, UpdateMessage, step helpers) are trivial
   cherry-picks on all branches. Done and verified quickly.
2. Commit 4 (move helper) is a pure reorder, low risk.
3. Commit 5 (rte compat wrappers) is the key risk point — it refactors
   the objectstate layer but adds compat wrappers so the controller continues
   working unchanged. Existing controller tests validate the compat wrappers
   without any controller changes.
4. Commit 6 (signature refactoring) is mechanical prep work.
5. Commit 7 (add per-tree functions) adds dead code. If it compiles,
   it is correct.
6. Commit 8 (wire per-tree functions) is the core behavioral change.
   By this point all building blocks (1-7) are proven and verified.
7. Commits 9-10 (narrow helpers, rename) are mechanical signature
   narrowing, safe after commit 8 removed the old callers.
8. Commit 11 (remove compat wrappers) is mechanical cleanup.
9. Commit 12 (vertical processing) is the final behavioral change,
   switching from horizontal to vertical orchestration.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring on any branch (including 4.18, the
hardest target) without touching the controller at all. Once that is
proven, the controller changes build on a solid foundation.

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]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits which are intentionally shaped to make the backport down to the distant 4.18 as low risk as possible.
Compatibility layers are added and then removed in the context of the same PR explicitly and only in order
to enable gradual testing, not because their own merit.

review notice

Adding and removing code in the same PR makes the review harder, even though the final tree state is identical.
It may be useful to leverage the intended property the final state is identical as a cleaner, main-only series and review the final tree state in addition to individual commits.
The most independent commits are intentionally pushed towards the beginning of the series to enable this review mode as much as possible.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	minimal	Essentially clean cherry-picks (after PR #4025 lands)
4.20	very low	Minor controller conflicts (after PR #3801 backport lands)
4.19	low	Minor conflicts; rte.go diverges more (after PR #3801 backport lands)
4.18	high	Path renames, no MachineConfigsState, machineconfigpool.go heavily diverged (see below)

4.18 effort breakdown

Commits	Effort	Nature
1 (Name() helper)	very low	Path adjustment: api/v1/helper/ → api/numaresourcesoperator/v1/helper/
2-3 (conditioninfo, step helpers)	minimal	step.go identical; conditioninfo.go nearly identical
4 (move helper around)	low	Path adjustment: internal/controller/ → controllers/
5 (rte + compat wrappers)	hard/very hard	Key refactoring; machineconfigpool.go is 134 lines (vs 244 on main), no MachineConfigObjectState type, no MachineConfigsState method; rte.go is 436 lines (vs 379) with 288 diff lines
6 (signature refactoring)	low	Path adjustment only
7 (add per-tree functions)	mid/hard	New code, but depends on compat wrappers from commit 5 being adapted first
8 (wire per-tree functions)	hard	Controller rewrite; building blocks from 5-7 must be proven first
9-10 (narrow helpers, rename)	very low	Mechanical signature changes
11 (remove compat wrappers)	mid	Adaptation for different machineconfigpool.go baseline
12 (vertical processing)	mid	Controller change; same pattern as main but different baseline

4.21/4.20/4.19 prerequisite

These branches require PR #3801 ("OCPBUGS-84226: per-pool MachineConfig state
with paused MCP awareness") to be backported first. PR #4025 covers 4.21;
4.20 and 4.19 need equivalent backports. Once that prerequisite lands,
these branches have the same MachineConfigObjectState type and
MachineConfigsState signature as main, making the cherry-picks
straightforward.

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-3 (Name helper, UpdateMessage, step helpers) are trivial
   cherry-picks on all branches. Done and verified quickly.
2. Commit 4 (move helper) is a pure reorder, low risk.
3. Commit 5 (rte compat wrappers) is the key risk point — it refactors
   the objectstate layer but adds compat wrappers so the controller continues
   working unchanged. Existing controller tests validate the compat wrappers
   without any controller changes.
4. Commit 6 (signature refactoring) is mechanical prep work.
5. Commit 7 (add per-tree functions) adds dead code. If it compiles,
   it is correct.
6. Commit 8 (wire per-tree functions) is the core behavioral change.
   By this point all building blocks (1-7) are proven and verified.
7. Commits 9-10 (narrow helpers, rename) are mechanical signature
   narrowing, safe after commit 8 removed the old callers.
8. Commit 11 (remove compat wrappers) is mechanical cleanup.
9. Commit 12 (vertical processing) is the final behavioral change,
   switching from horizontal to vertical orchestration.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring on any branch (including 4.18, the
hardest target) without touching the controller at all. Once that is
proven, the controller changes build on a solid foundation.

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.

[ffromani]

reviewable again

[openshift-ci-robot]

@ffromani: This pull request references CNF-23239 which is a valid jira issue.

Details

In response to this:

implement "vertical split" vs current "horizontal split"
the split is about how the reconciliation makes progress. In the model we had till now, retroactively called "horizontal split", each NodeGroup must make progress on each reconciliation stage before any of these can progress to the next stage. IOW, the slowest NodeGroup blocks all the other NodeGroups at the stage it is stuck at.

With the approach proposed here, "vertical split", each NodeGroup advances independently on the reconciliation step, and only the global reported status is set pulled to the slowest state; IOW, with N nodegroups, N-1 can be Available and settled and 1 still Progressing; the global state will still be Progressing, but changes to the other NodeGroups can happen independently.

To simplify the implementation, a single goroutine handles all the NodeGroup reconciliation, so factually a NodeGroup can still influence the others, but this (bar more serious bugs we should fix anyway) can only create reconciliation delays, a far better stance than the issues caused by the horizontal split.

backportability

This PR is broken down in commits which are intentionally shaped to make the backport down to the distant 4.18 as low risk as possible.
Compatibility layers are added and then removed in the context of the same PR explicitly and only in order
to enable gradual testing, not because their own merit.

review notice

Adding and removing code in the same PR makes the review harder, even though the final tree state is identical.
It may be useful to review the final tree state (e.g. git diff ..HEAD) in addition to individual commits, since the end result is identical to a cleaner, main-only series.
The most independent commits are intentionally pushed towards the beginning of the series to enable this review mode as much as possible.

LLM-assisted backport effort summary

Branch	Effort	Notes
4.22	minimal	Essentially clean cherry-picks
4.21	minimal	Essentially clean cherry-picks (after PR #4025 lands)
4.20	very low	Minor controller conflicts (after PR #3801 backport lands)
4.19	low	Minor conflicts; rte.go diverges more (after PR #3801 backport lands)
4.18	high	Path renames, no MachineConfigsState, machineconfigpool.go heavily diverged (see below)

4.18 effort breakdown

Commits	Effort	Nature
1 (Name() helper)	very low	Path adjustment: api/v1/helper/ → api/numaresourcesoperator/v1/helper/
2-3 (conditioninfo, step helpers)	minimal	step.go identical; conditioninfo.go nearly identical
4 (move helper around)	low	Path adjustment: internal/controller/ → controllers/
5 (rte + compat wrappers)	hard/very hard	Key refactoring; machineconfigpool.go is 134 lines (vs 244 on main), no MachineConfigObjectState type, no MachineConfigsState method; rte.go is 436 lines (vs 379) with 288 diff lines
6 (signature refactoring)	low	Path adjustment only
7 (add per-tree functions)	mid/hard	New code, but depends on compat wrappers from commit 5 being adapted first
8 (wire per-tree functions)	hard	Controller rewrite; building blocks from 5-7 must be proven first
9-10 (narrow helpers, rename)	very low	Mechanical signature changes
11 (remove compat wrappers)	mid	Adaptation for different machineconfigpool.go baseline
12 (vertical processing)	mid	Controller change; same pattern as main but different baseline

4.21/4.20/4.19 prerequisite

These branches require PR #3801 ("OCPBUGS-84226: per-pool MachineConfig state
with paused MCP awareness") to be backported first. PR #4025 covers 4.21;
4.20 and 4.19 need equivalent backports. Once that prerequisite lands,
these branches have the same MachineConfigObjectState type and
MachineConfigsState signature as main, making the cherry-picks
straightforward.

LLM-assisted risk assessment

The proposed split should significantly reduce backport risk compared to
a simpler, monolithic approach:

1. Commits 1-3 (Name helper, UpdateMessage, step helpers) are trivial
   cherry-picks on all branches. Done and verified quickly.
2. Commit 4 (move helper) is a pure reorder, low risk.
3. Commit 5 (rte compat wrappers) is the key risk point — it refactors
   the objectstate layer but adds compat wrappers so the controller continues
   working unchanged. Existing controller tests validate the compat wrappers
   without any controller changes.
4. Commit 6 (signature refactoring) is mechanical prep work.
5. Commit 7 (add per-tree functions) adds dead code. If it compiles,
   it is correct.
6. Commit 8 (wire per-tree functions) is the core behavioral change.
   By this point all building blocks (1-7) are proven and verified.
7. Commits 9-10 (narrow helpers, rename) are mechanical signature
   narrowing, safe after commit 8 removed the old callers.
8. Commit 11 (remove compat wrappers) is mechanical cleanup.
9. Commit 12 (vertical processing) is the final behavioral change,
   switching from horizontal to vertical orchestration.

The compat wrappers in commit 5 are the key risk reduction: they allow
validating the rte.go refactoring on any branch (including 4.18, the
hardest target) without touching the controller at all. Once that is
proven, the controller changes build on a solid foundation.

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]

Actionable comments posted: 3

🧹 Nitpick comments (1)

internal/reconcile/step_test.go (1)

45-61: ⚡ Quick win

Add coverage for StepOngoing(0) semantics

Please add a regression test asserting that StepOngoing(0) is still considered ongoing (and not failed). This is a critical path for paused MCP handling in the controller.

🤖 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 `@internal/reconcile/step_test.go` around lines 45 - 61, Add a regression test
that calls StepOngoing(0) and asserts it is ongoing and not failed: create a new
test (e.g., TestStepOngoingZeroIsOngoing) that constructs st := StepOngoing(0)
and uses assert.True(t, st.Ongoing()) and assert.False(t, st.Failed()) to verify
the semantics; place it alongside the existing
TestStepOngoingIsOngoing/TestStepFailedIsFailed/TestStepSuccessIsNeitherOngoingNorFailed
tests.

🤖 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 `@internal/controller/numaresourcesoperator_controller_test.go`:
- Around line 2285-2297: Add assertions that the pending tree's DaemonSet is
still absent and the global Available condition is false: attempt to Get the
DaemonSet for mcp1 (using objectnames.GetComponentName(nro.Name, mcp1.Name) and
testNamespace) and expect a NotFound error, then fetch nro and verify
getConditionByType(nro.Status.Conditions, status.ConditionAvailable) is not nil
and its Status equals metav1.ConditionFalse. This complements the existing
checks that pool2 DS exists and Progressing is true.

In `@internal/controller/numaresourcesoperator_controller.go`:
- Around line 822-824: The code currently only logs objState.Error and
continues; change this to fail fast: when objState.Error != nil, log the error
with context and immediately stop processing by returning or propagating the
error from the enclosing reconcile/processing function (do not continue to apply
manifests). Ensure you wrap or annotate objState.Error with a clear message
(e.g., "failed building manifest/state for <object>") before returning so
callers can handle requeueing or surface the failure.

In `@internal/reconcile/step.go`:
- Around line 46-49: The Ongoing() method currently returns true only when
Result.RequeueAfter > 0 which incorrectly treats StepOngoing(0) as not ongoing;
update Step.Ongoing() to treat zero as an ongoing/ progressing value (e.g.,
return rs.Result.RequeueAfter >= 0) or otherwise compare against the defined
sentinel for "no requeue" so that StepOngoing(0) is classified as ongoing;
change the condition in the Ongoing() implementation that references
Result.RequeueAfter so zero is considered progressing (and ensure this aligns
with the StepOngoing(0) semantics).

---

Nitpick comments:
In `@internal/reconcile/step_test.go`:
- Around line 45-61: Add a regression test that calls StepOngoing(0) and asserts
it is ongoing and not failed: create a new test (e.g.,
TestStepOngoingZeroIsOngoing) that constructs st := StepOngoing(0) and uses
assert.True(t, st.Ongoing()) and assert.False(t, st.Failed()) to verify the
semantics; place it alongside the existing
TestStepOngoingIsOngoing/TestStepFailedIsFailed/TestStepSuccessIsNeitherOngoingNorFailed
tests.

🪄 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: Enterprise

Run ID: cd1941fa-fd9e-4e86-911b-8ba816943886

📥 Commits

Reviewing files that changed from the base of the PR and between a81ec4d and ef94726.

📒 Files selected for processing (11)

• api/v1/helper/nodegroup/nodegroup.go
• api/v1/helper/nodegroup/nodegroup_test.go
• internal/controller/numaresourcesoperator_controller.go
• internal/controller/numaresourcesoperator_controller_test.go
• internal/reconcile/step.go
• internal/reconcile/step_test.go
• pkg/objectstate/rte/machineconfigpool.go
• pkg/objectstate/rte/machineconfigpool_test.go
• pkg/objectstate/rte/rte.go
• pkg/status/conditioninfo/conditioninfo.go
• pkg/status/conditioninfo/conditioninfo_test.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Assert the pending tree is still blocked in the first reconcile

This test currently proves only the positive path (ready tree DS exists, Progressing=true). It does not assert that the pending tree DS is still absent and Available is false at that point, so a regression to eager DS creation could still pass.

Suggested test hardening

 				By("Verify pool2 DaemonSet exists even though pool1 is still pending")
 				ds := &appsv1.DaemonSet{}
 				mcp2DSKey := client.ObjectKey{
 					Name:      objectnames.GetComponentName(nro.Name, mcp2.Name),
 					Namespace: testNamespace,
 				}
 				Expect(reconciler.Client.Get(ctx, mcp2DSKey, ds)).To(Succeed())
+
+				By("Verify pool1 DaemonSet is not created yet")
+				mcp1DSKeyPending := client.ObjectKey{
+					Name:      objectnames.GetComponentName(nro.Name, mcp1.Name),
+					Namespace: testNamespace,
+				}
+				err = reconciler.Client.Get(ctx, mcp1DSKeyPending, &appsv1.DaemonSet{})
+				Expect(apierrors.IsNotFound(err)).To(BeTrue(), "pool1 daemonset should still be pending")
 
 				By("Verify global status is Progressing, not Available")
 				Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(nro), nro)).To(Succeed())
 				progressingCond := getConditionByType(nro.Status.Conditions, status.ConditionProgressing)
 				Expect(progressingCond).ToNot(BeNil())
 				Expect(progressingCond.Status).To(Equal(metav1.ConditionTrue))
+				availableCond := getConditionByType(nro.Status.Conditions, status.ConditionAvailable)
+				Expect(availableCond).ToNot(BeNil())
+				Expect(availableCond.Status).To(Equal(metav1.ConditionFalse))

📝 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.

Suggested change

	By("Verify pool2 DaemonSet exists even though pool1 is still pending")
	ds := &appsv1.DaemonSet{}
	mcp1DSKey := client.ObjectKey{
	Name: objectnames.GetComponentName(nro.Name, mcp1.Name),
	mcp2DSKey := client.ObjectKey{
	Name: objectnames.GetComponentName(nro.Name, mcp2.Name),
	Namespace: testNamespace,
	}
	Expect(reconciler.Client.Get(ctx, mcp1DSKey, ds)).To(Succeed())
	Expect(reconciler.Client.Get(ctx, mcp2DSKey, ds)).To(Succeed())
	mcp2DSKey := client.ObjectKey{
	Name: objectnames.GetComponentName(nro.Name, mcp2.Name),
	By("Verify global status is Progressing, not Available")
	Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(nro), nro)).To(Succeed())
	progressingCond := getConditionByType(nro.Status.Conditions, status.ConditionProgressing)
	Expect(progressingCond).ToNot(BeNil())
	Expect(progressingCond.Status).To(Equal(metav1.ConditionTrue))
	By("Verify pool2 DaemonSet exists even though pool1 is still pending")
	ds := &appsv1.DaemonSet{}
	mcp2DSKey := client.ObjectKey{
	Name: objectnames.GetComponentName(nro.Name, mcp2.Name),
	Namespace: testNamespace,
	}
	Expect(reconciler.Client.Get(ctx, mcp2DSKey, ds)).To(Succeed())
	By("Verify pool1 DaemonSet is not created yet")
	mcp1DSKeyPending := client.ObjectKey{
	Name: objectnames.GetComponentName(nro.Name, mcp1.Name),
	Namespace: testNamespace,
	}
	err = reconciler.Client.Get(ctx, mcp1DSKeyPending, &appsv1.DaemonSet{})
	Expect(apierrors.IsNotFound(err)).To(BeTrue(), "pool1 daemonset should still be pending")
	By("Verify global status is Progressing, not Available")
	Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(nro), nro)).To(Succeed())
	progressingCond := getConditionByType(nro.Status.Conditions, status.ConditionProgressing)
	Expect(progressingCond).ToNot(BeNil())
	Expect(progressingCond.Status).To(Equal(metav1.ConditionTrue))
	availableCond := getConditionByType(nro.Status.Conditions, status.ConditionAvailable)
	Expect(availableCond).ToNot(BeNil())
	Expect(availableCond.Status).To(Equal(metav1.ConditionFalse))

🤖 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 `@internal/controller/numaresourcesoperator_controller_test.go` around lines
2285 - 2297, Add assertions that the pending tree's DaemonSet is still absent
and the global Available condition is false: attempt to Get the DaemonSet for
mcp1 (using objectnames.GetComponentName(nro.Name, mcp1.Name) and testNamespace)
and expect a NotFound error, then fetch nro and verify
getConditionByType(nro.Status.Conditions, status.ConditionAvailable) is not nil
and its Status equals metav1.ConditionFalse. This complements the existing
checks that pool2 DS exists and Progressing is true.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not continue when objState.Error is set

At Line 822-824, logging and continuing can mask manifest/state construction failures and may cascade into invalid apply attempts. Fail fast here to avoid partial/inconsistent reconciliation.

Suggested fix

 		if objState.Error != nil {
-			klog.Warningf("error loading object: %v", objState.Error)
+			return fmt.Errorf("failed to load object state: %w", objState.Error)
 		}

📝 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.

Suggested change

	if objState.Error != nil {
	klog.Warningf("error loading object: %v", objState.Error)
	}
	if objState.Error != nil {
	return fmt.Errorf("failed to load object state: %w", objState.Error)
	}

🤖 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 `@internal/controller/numaresourcesoperator_controller.go` around lines 822 -
824, The code currently only logs objState.Error and continues; change this to
fail fast: when objState.Error != nil, log the error with context and
immediately stop processing by returning or propagating the error from the
enclosing reconcile/processing function (do not continue to apply manifests).
Ensure you wrap or annotate objState.Error with a clear message (e.g., "failed
building manifest/state for <object>") before returning so callers can handle
requeueing or surface the failure.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Ongoing() misclassifies valid progressing steps

At Line 48, RequeueAfter > 0 treats StepOngoing(0) as not ongoing, even though it is explicitly a progressing step. This breaks per-tree aggregation semantics (progress counters/selection) for paused MCP flows.

Suggested fix

 func (rs Step) Ongoing() bool {
-	return rs.Result.RequeueAfter > 0
+	return rs.ConditionInfo.Type == status.ConditionProgressing && rs.Error == nil
 }

📝 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.

Suggested change

	// Ongoing returns true if the reconciliation is still ongoing/progressing
	func (rs Step) Ongoing() bool {
	return rs.Result.RequeueAfter > 0
	}
	// Ongoing returns true if the reconciliation is still ongoing/progressing
	func (rs Step) Ongoing() bool {
	return rs.ConditionInfo.Type == status.ConditionProgressing && rs.Error == nil
	}

🤖 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 `@internal/reconcile/step.go` around lines 46 - 49, The Ongoing() method
currently returns true only when Result.RequeueAfter > 0 which incorrectly
treats StepOngoing(0) as not ongoing; update Step.Ongoing() to treat zero as an
ongoing/ progressing value (e.g., return rs.Result.RequeueAfter >= 0) or
otherwise compare against the defined sentinel for "no requeue" so that
StepOngoing(0) is classified as ongoing; change the condition in the Ongoing()
implementation that references Result.RequeueAfter so zero is considered
progressing (and ensure this aligns with the StepOngoing(0) semantics).

[ffromani]

I acknowledge the review difficulties. Not sure how to help further unless we want to completely pivot direction and make a clean-as-possible main-only, minimal PR. That would make for a trivial backport down to 4.21 or so. Past that, it will both become hard and touch branches deeper and deeper in stable state. Which alternatives do we have?

[ffromani]

I agree about improving, thing is this function want to compute the existing state of MachineConfig objects, and compute the desired state of MachineConfig, which are indeed related to MachineConfigPools, but the pool is the scoping mechanism.

IOW, we want to reconcile MachineConfigs, and return ObjectState about these.

[ffromani]

Mostly, because I'm reusing ExistingManifests which predates the split and had 1:N relationship because the horizontal partitioning.

We can probably simplify further as followup of this PR

[ffromani]

this is meant to be a reconcile result of a single tree (therefore perTreeReconcileResult is probably better), but the problem here is we can't yet enforce the 1:1 mapping between trees and MCPs (https://github.com/openshift-kni/numaresources-operator/blob/main/internal/api/annotations/annotations.go#L25).
Well, there's an argument about doing that in 5.0 - we had 3 EUS version past 4.18 already, but then should be done as prerequisite of this PR, and would make backporting significantly more complex.
This is why I'm still carrying this forward and postponing to 5.2.

[ffromani]

/test ci-e2e-install-hypershift