Remove Cincinnati upgrade-graph validation for nodepool upgrades by adalrsjr1 · Pull Request #5153 · Azure/ARO-HCP

Adalberto Sampaio (adalrsjr1) · 2026-05-06T23:11:24Z

(updated)

What

Replace Cincinnati upgrade-edge validation with a version existence check in the nodepool version controller. The backend now verifies the desired version exists in
Cincinnati but no longer requires an upgrade edge from the current version. Static Kubernetes version skew constraints remain enforced (nodepool ≤ CP version, no downgrade,
within minor+1).

Per David Eads (@deads2k) suggestion, this work is split into two PRs:

(this PR) Remove edge requirement, keep version existence check
Remove downgrade restriction for nodepool versions (Allow nodepool version downgrades for HCP clusters #5184)

Why

HCP nodepools use the Replace upgrade strategy — nodes are destroyed and recreated at the target version, not upgraded in-place. Cincinnati upgrade edges are irrelevant for
this strategy.

The controller was querying Cincinnati for every nodepool upgrade and requiring an edge from the current to the desired version. When no edge existed (e.g. 4.20.8 →
4.21.1), it entered a permanent error-retry loop. ARM reported Succeeded while the upgrade never progressed, leaving customers with no visibility into the failure.

Testing

Unit tests: Replaced edge-check mocks with existence-check mocks. Added SucceedsWithoutCincinnatiEdge (version exists but no edge → succeeds) and
VersionNotInCincinnatiFails (version doesn't exist → fails). Updated DesiredVersionUnchangedOnFailure to test VersionNotFound instead of missing edges.
E2E: Existing y-stream upgrade test (4.20.z → 4.21.zLatest) validates the fix end-to-end. Verified locally against a dev cluster — nodepool upgraded from 4.20.20 to
4.21.14 successfully.

Special notes for your reviewer

Depends on CS MR !322 — CS also validates upgrade edges via AvailableUpgrades
in ValidateNodePoolUpgradeVersion(). Without the CS change, the backend accepts the version but CS blocks the upgrade policy. Safe to deploy in any order — neither change
alone creates a regression.

A rationale comment in validateDesiredNodePoolVersion notes that if an InPlace upgrade strategy is introduced in the future, strategy-specific upgrade validation should
be added.

Copilot

Pull request overview

Removes Cincinnati upgrade-graph validation from the backend nodepool version controller so nodepool upgrades are validated only via static Kubernetes/OpenShift version skew constraints (no downgrades, nodepool ≤ control plane, no minor skipping), aligning with the HCP nodepool Replace upgrade strategy.

Changes:

Removed Cincinnati client initialization/caching and upgrade-edge checks from nodepool_version_controller.
Updated unit tests to drop Cincinnati mocks and assert static validation behavior instead.
Adjusted the nodepool version upgrade E2E test wording/comments to reflect that backend validation no longer depends on Cincinnati edges (while still using Cincinnati for test version discovery).

Reviewed changes

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

File	Description
test/e2e/nodepool_version_upgrade.go	Updates test comments/log messages around version selection for nodepool upgrades.
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go	Removes Cincinnati upgrade-edge validation and related client/cache code; keeps static version-skew validation.
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go	Removes Cincinnati mocking and updates/rewrites tests to validate new static-only behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

 	syncer := &nodePoolVersionSyncer{
-		cooldownChecker:                       &alwaysSyncCooldownChecker{},
-		clusterManagementClusterContentLister: newValidHostedClusterContentLister(t),
-		cosmosClient:                          mockDB,
-		clusterServiceClient:                  mockCS,
-		clusterToCincinnatiClient:             lru.New(100),
+		cooldownChecker:      &alwaysSyncCooldownChecker{},
+		cosmosClient:         mockDB,
+		clusterServiceClient: mockCS,
 	}


+	// --- Phase 2: Change version to a downgrade, should fail, desired should NOT change ---

-	// Update the HCPNodePool with a new desired version
+	// Update the HCPNodePool with a version that is a downgrade from the active version (4.19.10)
 	nodePool, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroupName).
 		NodePools(testClusterName).Get(ctx, testNodePoolName)


Simon Wiencki (swiencki)

Three comments inline. Substance-only review across rubber-duck + GPT-5.5 + Opus 4.7 1M; the headline finding is the test-coverage gap vs ARO-26792 AC #2.

Simon Wiencki (swiencki) · 2026-05-06T23:36:32Z


 	// Validate the customer's desired version before setting it
-	if err := c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, existingServiceProviderNodePool, existingServiceProviderCluster, subscription, nodePool.Properties.Version.ChannelGroup, clusterKey, clusterUUID); err != nil {
+	if err := c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, existingServiceProviderNodePool, existingServiceProviderCluster, subscription); err != nil {


Cincinnati was also implicitly checking the version exists in the channel. After this PR a non-existent version like 4.20.999 passes the static gate. Is CS the authoritative availability check now? Worth being explicit so we don't regress into the same "ARM Succeeded, never progresses" shape for a different reason.

CS checks that the version exists but we so also check it in here. Otherwise we will have the same issue, as this validation will pass for a well formed but not existed version (4.20.999) but fail "silently" to the user in CS.

So, we need to make a call here to Cincinnati to check that the version exists.

Not sure what we do for deployment/install but why not just use the same validation as we do there? Is that different from this flow? I would expect that this would prevent both of those situations

We don't have this for install. The creation of the nodepool is not yet done in the backend, and adding the cincinnati call to the frontend will increase our time response. Let's add it here, and once we have the node pool creation call in the backend we can add the same validation there.

Manyanda Chitimbo (machi1990)

I personally still on the fence about this change and I'd have kept the validation and give customer two options:

Either they recreate their node pool if they don't care about "validated" upgradeability
Or they do hops from v1, v2...vDesired

I understand that these might not always be better UX but I personally feel like they are better options that doesn't bypass upgrades gates.
If this use case gets an approval from BU & David Eads (@deads2k) I won't stand on the way of it. Please let's get that approval from them. Also I am looping in Alba Hita (@ahitacat) for review as well.

CS component is also involved in the upgrade and has the upgradeability check; can you double check if there is anything that needs to be done on the CS component as well?

Alba Hita (ahitacat)

In addition to Manyanda Chitimbo (@machi1990) review #5153 (review)

In CS when creating the upgrade policy we also check that the version has an upgradepath. That should be change before this, so otherwise we could have the same issue, as the controller that creates these policies (trigger_nodepool_upgrade_controller) will fail.

Alba Hita (ahitacat) · 2026-05-07T08:48:19Z


 	// Validate the customer's desired version before setting it
-	if err := c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, existingServiceProviderNodePool, existingServiceProviderCluster, subscription, nodePool.Properties.Version.ChannelGroup, clusterKey, clusterUUID); err != nil {
+	if err := c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, existingServiceProviderNodePool, existingServiceProviderCluster, subscription); err != nil {


CS checks that the version exists but we so also check it in here. Otherwise we will have the same issue, as this validation will pass for a well formed but not existed version (4.20.999) but fail "silently" to the user in CS.

So, we need to make a call here to Cincinnati to check that the version exists.

Adalberto Sampaio (adalrsjr1) · 2026-05-07T20:40:37Z

per David Eads (@deads2k) suggestion this change was split into two parts (part 2 in #5184)

pr/1 (this one) remove the requirement for an edge, but keep the requirement for "version exists in cincinnati"
pr/2 (Allow nodepool version downgrades for HCP clusters #5184) remove the restriction about moving nodepool versions backwards and update testing to include one example of doing this (no need to check this for every version)

The previous comments were addressed in this PR. Both PRs are pending on the update on CS (MR 322) FYI Manyanda Chitimbo (@machi1990) Alba Hita (@ahitacat)

Simon Wiencki (swiencki)

Reviewed, code looks good, no blockers from my side. Concerns in #5153 (review) should be addressed. Reviewed the CS MR too (https://gitlab.cee.redhat.com/service/aro-hcp-clusters-service/-/merge_requests/322) with one non-blocking note.

David Eads (deads2k) · 2026-05-07T23:16:03Z

/approve

I'll leave lgtm to another reviewer.

Alba Hita (ahitacat)

After this is merged we can remove the check of the upgrade path. I.e. so we don't skip this.
https://github.com/Azure/ARO-HCP/blob/main/test/e2e/nodepool_version_upgrade.go#L184

Also, I think is worth it to test this with an e2e. Right now from 4.20.8 -> 4.20.9 there is no upgrade path. We could have an e2e that checks in cincinnati a case like this and upgrades succesfully. Can you add it (it should be skipped before this gets merge but we can test it)

Adalberto Sampaio (adalrsjr1) · 2026-05-08T18:29:39Z

After this is merged we can remove the check of the upgrade path. I.e. so we don't skip this. https://github.com/Azure/ARO-HCP/blob/main/test/e2e/nodepool_version_upgrade.go#L184

Also, I think is worth it to test this with an e2e. Right now from 4.20.8 -> 4.20.9 there is no upgrade path. We could have an e2e that checks in cincinnati a case like this and upgrades succesfully. Can you add it (it should be skipped before this gets merge but we can test it)

Alba Hita (@ahitacat)

added the e2e test and marked to skip for now; to be allowed in the upcoming PR Allow nodepool version downgrades for HCP clusters #5184
https://github.com/Azure/ARO-HCP/blob/main/test/e2e/nodepool_version_upgrade.go#L184; to be removed in the upcoming PR Allow nodepool version downgrades for HCP clusters #5184

I also addressed your comments on CS 322; pending on your approval.

Adalberto Sampaio (adalrsjr1) · 2026-05-20T02:03:44Z

/retest

53 minutes waiting for a free identity container

Adalberto Sampaio (adalrsjr1) · 2026-05-20T02:15:41Z

/retest

Gerd Oberlechner (geoberle) · 2026-05-20T06:18:17Z

/lgtm

openshift-merge-bot · 2026-05-20T07:04:08Z

/retest-required

Remaining retests: 0 against base HEAD 4cf9d9c and 2 for PR HEAD 6224f84 in total

Adalberto Sampaio (adalrsjr1) · 2026-05-20T12:41:23Z

/retest

Adalberto Sampaio (adalrsjr1) · 2026-05-20T16:03:23Z

/retest due to container identity exhaustion (tests passed, clean ups failed)

Adalberto Sampaio (adalrsjr1) · 2026-05-20T16:05:13Z

/retest

Adalberto Sampaio (adalrsjr1) · 2026-05-20T20:00:09Z

/retest

Adalberto Sampaio (adalrsjr1) · 2026-05-20T22:19:23Z

/retest

openshift-merge-bot · 2026-05-21T07:59:41Z

/retest-required

Remaining retests: 0 against base HEAD 2d20aac and 1 for PR HEAD 6224f84 in total

@deads2k

… check for nodepool upgrades HCP nodepools use the Replace upgrade strategy — nodes are destroyed and recreated at the target version, not upgraded in-place. Cincinnati upgrade edges are irrelevant; only version existence and Kubernetes version skew constraints apply. Per @deads2k suggestion, this work is split into two PRs: 1/2: Remove edge requirement, keep version existence check (this PR) 2/2: Remove downgrade restriction for nodepool versions Replaces validateUpgradePathAvailable (which required a Cincinnati edge from current to desired version) with validateVersionExistsInCincinnati (which only verifies the desired version exists in the Cincinnati graph). Static validation remains: no downgrade, ≤ CP version, within minor+1. Depends on CS MR !322: https://gitlab.cee.redhat.com/service/aro-hcp-clusters-service/-/merge_requests/322 Fixes: ARO-26792 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Add GetVersionPairWithoutUpgradeEdge helper to find version pairs without Cincinnati upgrade edges for E2E testing - Add pending E2E test for forward upgrade without Cincinnati edge, exercising the actual bug scenario (e.g. 4.20.8 -> 4.20.9) - Rename UpgradePathExistsSucceeds to ValidUpgradeSucceeds to reflect that the test validates static version checks, not edge existence Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Add HyperShift Replace strategy docs link - Rename ErrNoNoEdgePairFound to ErrNoEdgePairFound (duplicated "No") - Build Cincinnati channel from parsed major.minor to handle X.Y.Z input - Return clearer error when fewer than 2 parseable versions in minor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

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

+	channel := fmt.Sprintf("%s-%d.%d", channelGroup, requestedMinor.Major, requestedMinor.Minor)
+	graphURL := fmt.Sprintf("https://api.openshift.com/api/upgrades_info/v1/graph?channel=%s", url.QueryEscape(channel))
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, graphURL, nil)
+	if err != nil {
+		return semver.Version{}, semver.Version{}, fmt.Errorf("create graph request for %s: %w", channel, err)
+	}
+
+	client := &http.Client{Timeout: graphAPIRequestTimeout}


+	DescribeTable("should upgrade a nodepool to a version without Cincinnati upgrade edge",
+		func(ctx context.Context, minor string) {
+			Skip("skipped until PR #5184 is merged and deployed")
+


Gerd Oberlechner (geoberle) · 2026-05-21T13:04:01Z

/lgtm

openshift-ci · 2026-05-21T13:04:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adalrsjr1, deads2k, geoberle

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:

~~backend/OWNERS~~ [deads2k,geoberle]
~~test/OWNERS~~ [deads2k,geoberle]

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

…st coverage Absorb main (PRs #5153, #5239), then apply downgrade-specific changes: - Remove downgrade guard from admission webhook - Rename ValidateNodePoolUpgrade → ValidateNodePoolVersionChange - Add downgrade E2E tests (z-stream, y-stream at N-2 boundary) - Add multi-element activeVersions unit tests - Add cross-major downgrade path tests (4.21, 4.23, 5.1, 5.2) - Align doc comments across validators, admission, and controller Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings May 6, 2026 23:11

openshift-ci Bot requested review from Matthew Barnes (mbarnes) and Miquel Simon (miquelsi) May 6, 2026 23:11

Adalberto Sampaio (adalrsjr1) requested a review from Gerd Oberlechner (geoberle) May 6, 2026 23:11

Copilot started reviewing on behalf of Adalberto Sampaio (adalrsjr1) May 6, 2026 23:12 View session

Adalberto Sampaio (adalrsjr1) self-assigned this May 6, 2026

Copilot AI reviewed May 6, 2026

View reviewed changes

Simon Wiencki (swiencki) reviewed May 6, 2026

View reviewed changes

Manyanda Chitimbo (machi1990) previously requested changes May 7, 2026

View reviewed changes

Alba Hita (ahitacat) reviewed May 7, 2026

View reviewed changes

David Eads (deads2k) reviewed May 7, 2026

View reviewed changes

Comment thread backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go Outdated

openshift-ci Bot added the needs-rebase label May 7, 2026

Adalberto Sampaio (adalrsjr1) force-pushed the asampaio/ARO-26792 branch from 64d2e24 to 53170ec Compare May 7, 2026 19:02

openshift-ci Bot removed the needs-rebase label May 7, 2026

Adalberto Sampaio (adalrsjr1) mentioned this pull request May 7, 2026

Allow nodepool version downgrades for HCP clusters #5184

Merged

Adalberto Sampaio (adalrsjr1) requested review from Alba Hita (ahitacat), David Eads (deads2k), Manyanda Chitimbo (machi1990) and Simon Wiencki (swiencki) May 7, 2026 20:37

Simon Wiencki (swiencki) reviewed May 7, 2026

View reviewed changes

openshift-ci Bot added approved needs-rebase labels May 7, 2026

Alba Hita (ahitacat) reviewed May 8, 2026

View reviewed changes

Adalberto Sampaio (adalrsjr1) force-pushed the asampaio/ARO-26792 branch from 53170ec to b5d396a Compare May 8, 2026 18:25

Copilot AI review requested due to automatic review settings May 8, 2026 18:25

openshift-ci Bot removed the needs-rebase label May 8, 2026

Copilot started reviewing on behalf of Adalberto Sampaio (adalrsjr1) May 8, 2026 18:27 View session

openshift-ci Bot removed the lgtm label May 19, 2026

openshift-ci Bot added the lgtm label May 20, 2026

Adalberto Sampaio (adalrsjr1) and others added 6 commits May 21, 2026 08:51

ARO-26792: Update test suite fixtures for no-edge upgrade E2E test

4397901

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ARO-26792: fix lint errors

cb2f82a

ARO-26792: Fix log messages after rebase

e91841c

Copilot AI review requested due to automatic review settings May 21, 2026 12:57

Adalberto Sampaio (adalrsjr1) force-pushed the asampaio/ARO-26792 branch from 6224f84 to e91841c Compare May 21, 2026 12:57

openshift-ci Bot removed the lgtm label May 21, 2026

Copilot started reviewing on behalf of Adalberto Sampaio (adalrsjr1) May 21, 2026 12:57 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

openshift-ci Bot added the lgtm label May 21, 2026

openshift-merge-bot Bot merged commit 7ff36d2 into main May 21, 2026
16 checks passed

openshift-merge-bot Bot deleted the asampaio/ARO-26792 branch May 21, 2026 20:51

Adalberto Sampaio (adalrsjr1) mentioned this pull request May 27, 2026

ARO-27344: Filter pre-release versions from E2E test version selection #5409

Merged

12 tasks

Uh oh!

Conversation

Adalberto Sampaio (adalrsjr1) commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Testing

Special notes for your reviewer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Simon Wiencki (swiencki) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Simon Wiencki (swiencki) May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Alba Hita (ahitacat) May 7, 2026

Choose a reason for hiding this comment

Uh oh!

jboutaud May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Alba Hita (ahitacat) May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Manyanda Chitimbo (machi1990) left a comment

Choose a reason for hiding this comment

Uh oh!

Alba Hita (ahitacat) left a comment

Choose a reason for hiding this comment

Uh oh!

Alba Hita (ahitacat) May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simon Wiencki (swiencki) left a comment

Choose a reason for hiding this comment

Uh oh!

David Eads (deads2k) commented May 7, 2026

Uh oh!

Alba Hita (ahitacat) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 8, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

Gerd Oberlechner (geoberle) commented May 20, 2026

Uh oh!

openshift-merge-bot Bot commented May 20, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

Adalberto Sampaio (adalrsjr1) commented May 20, 2026

Uh oh!

openshift-merge-bot Bot commented May 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Adalberto Sampaio (adalrsjr1) commented May 6, 2026 •

edited

Loading

Adalberto Sampaio (adalrsjr1) commented May 7, 2026 •

edited

Loading

Alba Hita (ahitacat) left a comment •

edited

Loading

Adalberto Sampaio (adalrsjr1) commented May 20, 2026 •

edited

Loading