feat: persist IntentFailed for node pool version validation errors

[ahitacat]

ARO-26304

What

Persist node pool version validation errors as IntentFailed conditions on the controller document instead of returning them as transient errors, and teach the operation controller to read those conditions to determine operation outcomes.

Similar work was done for control plane upgrade controller #5042

Why

Previously, version validation errors (downgrades, Cincinnati VersionNotFound, missing upgrade paths, exceeding control plane version, skipping minor versions) were returned as errors from SyncOnce, causing the controller to retry them indefinitely. These are user errors that won't resolve on retry. By persisting them as IntentFailed conditions, the operation controller can detect them and fail the operation with a clear error message, while still retrying transient Cincinnati errors (e.g. 503).

Testing

• Added new TestNodePoolVersionSyncer_SyncOnce_IntentFailed table with 6 test cases covering: successful resolution (IntentFailed=False), downgrade, Cincinnati VersionNotFound, Cincinnati upstream error (transient), exceeds control plane, and skipping minor versions
• Added operation controller tests covering: exact version match succeeds, IntentFailed marks operation failed, stale IntentFailed from previous version is ignored, 29s timeout for version resolution, and version mismatch without IntentFailed stays Accepted
• Updated TestNodePoolVersionSyncer_SyncOnce_DesiredVersionUnchangedOnFailure_ChangedOnSuccess to reflect that VersionNotFound now returns nil (persists IntentFailed) instead of returning an error

Special notes for your reviewer

• service_provider_cluster_conditions.go was renamed to service_provider_conditions.go since conditions now apply to both clusters and node pools
• The operation controller uses a [version X.Y.Z] prefix in the IntentFailed condition message to scope conditions to a specific customer version request, preventing stale errors from blocking new requests. There could be another approach to consider a dedicated field for cleaner version scoping. feat: persist IntentFailed for node pool version validation errors #5378 (comment)
• The operation controller waits twice (59s timout) the informers relist period used by the nodepool version controller. This would cover most of the calculation edge cases in which the new desiredVersion has not yet calculated.
• subscriptionLister was added to the version controller for feature flag resolution (experimental release features)

PR Checklist

• PR is scoped to a single task (no mixed concerns)
• Title follows Conventional Commits format
• Summary explains the "Why" behind the change
• Linked to relevant ticket/issue
• Screenshots included (if graph/UI/metrics changes)
• Self-reviewed the diff
• CI/CD checks are passing (ignore Tide)
• Draft PR used for WIP (if applicable)
• Commit history is clean (rebased/squashed)
• Tricky code blocks are commented
• Specific reviewers tagged
• All comment threads resolved before merge

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ahitacat
Once this PR has been reviewed and has the lgtm label, please assign mbarnes for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• backend/OWNERS
• internal/OWNERS

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

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[ahitacat]

Instead of adding this prefix and calculate we could have add it in the controllerStatus. However, as this controllerStatus is shared among cluster and nodepool I didn't want to add conditions that weren't shared.

To the reviewers: Is this approach correct? or should I opt for adding a new field?

[JameelB]

should I opt for adding a new field?

do you mean adding a LastObservedCustomerDesiredVersion to Controller.Status that you mention in the comment above? Is the Controller.Status shared with anything else apart from cluster/node pool?

My personal preference would be using LastObservedCustomerDesiredVersion since it could be useful for the cluster too? If we can't change the api though, i'm happy with what's proposed here since the prefix format is predictable.

[ahitacat]

do you mean adding a LastObservedCustomerDesiredVersion to Controller.Status that you mention in the comment above?

Yes, I mean that. The controller.Status is not shared yet, but there are no many controllers and I thought it can be potentially shared.
I opted here to no add that into an object I don't know how it will be used. Although I didn't like to parse a message to get this information.

@miguelsorianod do you have any opinion about this?

[ahitacat]

After reviewing this we opted to not based this in the message prefix wait at least 2 runs of the informers used by the controller in case of the condition is nil or intentFailed. This should be enough for the case I was trying to solve.
I'm testing this manually.

[miguelsorianod]

As discussed yesterday, let's do the following approach:

When IntentFailed is set to false, we ignore it until at least there's been double the nodepool/serviceprovidernodepool informer relist period time.

This would cover most of the cases. If it's not enough we could consider increasing the time.
The time increase should only impact when there's an upgrade, and not during all update operations, as well as the specific case where the intent failed within the upgrade.
The increased experience time occurs when there's a failure so experiencing an increased time there is less impactful.

[ahitacat]

I manual tested this. Created a node pool with version 4.20.8, upgrade it to 4.20.9 that it doesn't exists in Cincinnati. The controller failed, the intentFailed condition was set and the operation was marked as failed.

{
    "id": "/providers/Microsoft.RedHat
OpenShift/locations/westus3/hcpOperationStatuses/dff9145a-4559-4306-a64c-cf3727cf254b",
    "name": "dff9145a-4559-4306-a64c-cf3727cf254b",
    "status": "Failed",
    "startTime": "2026-05-27T11:03:16.203534595Z",
    "endTime": "2026-05-27T11:03:29.915339813Z",
    "error": {
        "code": "InvalidRequestContent",
        "message": "version 4.20.9 not found in Cincinnati channel stable-4.20"
    }
}

Then upgrade it to 4.20.15 and the operation succeeded.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates node pool version control flow to persist user-facing version resolution failures via a controller condition, and adjusts operation processing to interpret that condition during node pool update operations.

Changes:

• Persist/clear IntentFailed condition from NodePoolVersion syncer based on desired version resolution outcomes.
• Enhance node pool update operation controller to gate failure/timeout behavior based on resolved desired version vs customer requested version.
• Expand tests to cover IntentFailed persistence, stale condition handling, and mismatch timeout behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/api/service_provider_cluster_conditions.go	Clarifies condition/Reason docs to apply to both cluster and node pool resources.
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go	Persists IntentFailed on user errors; clears on success; uses subscription lister for feature gating.
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go	Adds/updates tests for IntentFailed persistence and adjusts validation call signature.
backend/pkg/controllers/operationcontrollers/operation_node_pool_update.go	Introduces desired-version resolution state + timeout logic and merges with CS state to pick worst outcome.
backend/pkg/controllers/operationcontrollers/operation_node_pool_update_test.go	Adds coverage for exact match, version mismatch, stale IntentFailed, and timeout paths.
backend/pkg/app/backend.go	Wires subscriptionLister into the node pool version controller.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

[miguelsorianod]

As we discussed, when there's no condition (nil) this would mark the operation as failed which shouldn't be the case.

[ahitacat]

Indeed, I forgot about this. I have added a check when it is nil to keep the operation Accepted until it is resolved.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

[openshift-ci]

PR needs rebase.

Details

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.