feat: node pool upgrades frontend handler by ahitacat · Pull Request #4224 · Azure/ARO-HCP

ahitacat · 2026-02-26T08:46:15Z

What

Implement frontend handling for node pool upgrades. Initial draft, to start discussion about the design and implementation.

Why

As a part of the node pool upgrades, we need to implement the handling from the customer petitions.

Special notes for your reviewer

For easy review I am adding commits. Once it is ready to merge I will squash all of them.

openshift-ci · 2026-02-26T08:46:26Z

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

deads2k · 2026-03-10T12:14:18Z

+	// Include all standard node pool admission checks
+	errs = append(errs, AdmitNodePool(newNodePool, cluster)...)


updates should always be separate from create. Create only looks at the new object. Often a previously valid nodepool can become invalid without updating the nodepool. When this happens, we need to ensure all fields not being modified are allowed to remain invalid. A good example is version, where the control plane version will be changed without touching the API. The forced y-stream upgrade possibility remains, and thus it can happen here.

Agree, I thought this AdmitNodePool were checks for both creation and update as this was called in the updateNodePool
https://github.com/Azure/ARO-HCP/pull/4224/changes#diff-30bc17cd7777df0f7bfd82d36a2ccaa4d5e9af7a620b82a115f95ed6ee5913aaL549

Should I create smaller validations one for channelGroup and another one for subnets and a new AdmitNodePoolCreate that will also call them?

Somehow the cluster.version.channel is changed (doesn't matter how or why). Someone updates a nodepool AutoScaling section and nothing else. The update should be allowed because the autoscaling section change is allowed. This call causes a failure, correct? How would you resolve this problem in the code so that doesn't happen?

This has been addressed

deads2k · 2026-03-10T12:16:29Z

+	}
+
+	// No major version change
+	if newVersion.Major != oldVersion.Major {


followup to make this possible via the experimental features. v5 is now available and ready to install.

This should be also needed for clusters, and probably some modifications in CS as the imagerelease has changed for v5. I will create a jira issue so we can work on this after upgrades are functional.

This should be also needed for clusters, and probably some modifications in CS as the imagerelease has changed for v5. I will create a jira issue so we can work on this after upgrades are functional.

I'm fine with also getting a jira card for v5, but for this exact one, if you get an IOU in this PR, I don't want to wait for someone to work on a jira card, I want this line gated.

https://redhat.atlassian.net/browse/ARO-25229 the Jira card and the followup PR #4479

raelga · 2026-03-11T11:56:25Z

/retest

ahitacat · 2026-03-11T14:43:45Z

@deads2k to use well the service_providers I needed to refactor them and move it to the internal module.

deads2k · 2026-03-13T14:42:50Z

+	// TODO: We may relax this constraint in the future
+	clusterActiveVersions := spCluster.Status.ControlPlaneVersion.ActiveVersions
+	if len(clusterActiveVersions) > 0 {
+		lowestControlPlane := clusterActiveVersions[len(clusterActiveVersions)-1].Version


We allow z-streams of clusters to move backwards.

fast followup (as in Monday next week), correct this to read all clusterActiveVersions and find the actual lowest.

I bet hypershift handles the situation poorly.

separate from this PR, write test that

installs control plane 4.y.z-5

upgrades control plane 4.y.z-3

installs nodepool 4.y.z-3

backlevels control plane to 4.y.z-5 (this is legal and explicitly agreed with Scott Dodson to enable fearless automated upgrade

scales nodepool up

changes nodepool to 4.y.z-5

if it fails, HCP needs to update

once HCP updates to acknowledge nodepools can be on later z-streams than control planes, challenge the restriction to be LE in the common case.

follow up work #4481

deads2k · 2026-03-13T14:44:19Z

+		highestActive := nodePoolActiveVersions[0].Version
+		lowestActive := nodePoolActiveVersions[len(nodePoolActiveVersions)-1].Version


scan the list and get this accurate even if we later allow backleveling (we will).

I'll accept a fast-followup on Monday.

deads2k · 2026-03-13T14:48:14Z

+		}
+		// No major version change
+		// TODO: Add support for major version upgrades (e.g., 4.20 → 5.0) when needed
+		if newVersion.Major != lowestActive.Major {


start gating these block with the experimental AFEC. We want to be testing this in April to coordinate with OCP.

#4479

Added, I also added a comment that it might require more additional work. We can continue the major change in that PR.

deads2k · 2026-03-13T14:49:17Z

+	// Check if the newVersion <= control plane versions
+	// TODO: We may relax this constraint in the future
+	clusterActiveVersions := spCluster.Status.ControlPlaneVersion.ActiveVersions
+	if len(clusterActiveVersions) > 0 {


missing a 4.y-2 check on all control plane versions.

This check failing means the node pool is already under that skew (as we always allow upgrade to greater versions). I think this change should be part of the node pool creation and the control plane upgrade. I will create a followup PR to add it in those situations.

deads2k · 2026-03-13T14:49:56Z

+	// Check if the newVersion <= control plane versions
+	// TODO: We may relax this constraint in the future
+	clusterActiveVersions := spCluster.Status.ControlPlaneVersion.ActiveVersions
+	if len(clusterActiveVersions) > 0 {


Also check 4.y-2 on the desired version of the control plane.

deads2k · 2026-03-13T14:54:02Z

This should be also needed for clusters, and probably some modifications in CS as the imagerelease has changed for v5. I will create a jira issue so we can work on this after upgrades are functional.

I'm fine with also getting a jira card for v5, but for this exact one, if you get an IOU in this PR, I don't want to wait for someone to work on a jira card, I want this line gated.

If you can agree to the followups and timeframe (early next week for capability ones), I'm happy to approve now. If you can't, I'll hold this PR to get them updated prior to merge.

ahitacat · 2026-03-16T14:27:56Z

/test e2e-parallel

deads2k · 2026-03-16T14:42:04Z

/lgtm
/approve

ahitacat · 2026-03-16T16:35:27Z

/test e2e-parallel

machi1990 · 2026-03-16T18:10:53Z

/test e2e-parallel

ahitacat · 2026-03-17T08:30:22Z

/test e2e-parallel

This commit also adds node pool upgrade frontend tests and unit tests. Signed-off-by: Alba Hita Catala <ahitacat@redhat.com>

machi1990 · 2026-03-17T09:52:13Z

/lgtm

The new changes are rebasing following the merge of another PR #4433

openshift-ci · 2026-03-17T09:52:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahitacat, deads2k, machi1990

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~~ [deads2k]

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

stale

ahitacat · 2026-03-18T08:03:38Z

/test e2e-parallel

openshift-ci Bot added the do-not-merge/work-in-progress label Feb 26, 2026

ahitacat force-pushed the ahitacat/frontend-np-upgrade branch from dcfe2e6 to 8d6c54b Compare March 10, 2026 10:35

ahitacat changed the title ~~feat: node pool upgrades frontend handle~~ feat: node pool upgrades frontend handler Mar 10, 2026

ahitacat commented Mar 10, 2026

View reviewed changes

Comment thread frontend/pkg/frontend/node_pool.go

ahitacat force-pushed the ahitacat/frontend-np-upgrade branch 2 times, most recently from e3701e1 to 5767690 Compare March 10, 2026 12:07

deads2k reviewed Mar 10, 2026

View reviewed changes