basic version selection test suite - #6192
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new control-plane version selection implementation (with transitive “gateway chain” validation across minors) and migrates existing controllers/tests to use it, alongside new Cincinnati test harness utilities and a shared GetUpdates response cache.
Changes:
- Added
backend/pkg/hcpversionselectionto select a control plane z-stream that preserves upgradeability across subsequent minors. - Added
internal/cincinnatiutilities: an in-process Cincinnati test server/graph builder and a TTL-basedCachingClientforGetUpdates. - Updated upgrade controllers and E2E coverage to use the new selection + caching approach; removed older cache/gateway helper code.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/control_plane_minor_upgrade.go | Switches minor-upgrade E2E flow to use the new version selection logic and cached Cincinnati client. |
| internal/cincinnati/testserver/server.go | Adds httptest-backed Cincinnati server for deterministic graph-based tests. |
| internal/cincinnati/testserver/server_test.go | Tests testserver behavior via real CVO client calls. |
| internal/cincinnati/testserver/graph.go | Adds a small declarative graph builder for Cincinnati responses. |
| internal/cincinnati/caching_client.go | Introduces TTL caching around Client.GetUpdates. |
| internal/cincinnati/caching_client_test.go | Validates caching, expiry, and concurrent access behavior. |
| internal/cincinnati/cache.go | Removes prior per-cluster client cache implementation. |
| internal/cincinnati/mock_cache.go | Removes gomock generated cache mock tied to the deleted cache interface. |
| backend/pkg/hcpversionselection/select_version.go | New core selection algorithm with transitive gateway-chain validation. |
| backend/pkg/hcpversionselection/select_version_test.go | Adds unit test coverage for selection behavior across many graph scenarios. |
| backend/pkg/controllers/upgradecontrollers/utils.go | Removes obsolete gateway helper used by prior selection approach. |
| backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go | Switches to a shared cached Cincinnati client and removes dependency on per-cluster UUID discovery. |
| backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go | Updates tests to inject a mock Cincinnati client directly (no client-cache mock). |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go | Delegates desired version selection to hcpversionselection and switches to shared cached Cincinnati client. |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go | Reworks tests to use the new Cincinnati test server graphs instead of mocked GetUpdates/GraphClient behavior. |
| backend/pkg/app/backend.go | Updates controller wiring to pass the clock into the nodepool version controller. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
backend/pkg/hcpversionselection/select_version.go:410
- isVersionOrChannelNotFound treats any Cincinnati ResponseFailed as "channel not found". ResponseFailed is also used for transient upstream failures (see internal/cincinnati/graph_client.go notes + control_plane_desired_version_controller_test treating ResponseFailed as transient), so swallowing it here can incorrectly terminate the transitive-gateway check and select a version based on incomplete data.
// isVersionOrChannelNotFound returns true when the Cincinnati error indicates
// either the queried version is not in the channel's graph (VersionNotFound)
// or the channel itself does not exist (ResponseFailed from a non-200 status).
func isVersionOrChannelNotFound(err error) bool {
var cErr *cvocincinnati.Error
if !errors.As(err, &cErr) {
return false
}
return cErr.Reason == "VersionNotFound" || cErr.Reason == "ResponseFailed"
}
deads2k
changed the title
- Only treat VersionNotFound as "not found"; propagate ResponseFailed as a transient error instead of silently swallowing it - Propagate GetUpdates errors in hasValidUpgradePath and isNextMinorReachableFromCurrentMinor instead of returning false - Skip version selection when activeVersions is non-empty but the cached HostedCluster is unavailable to avoid falling into the install path incorrectly - Only cache successful responses and VersionNotFound in CachingClient; transient errors are not cached so callers retry immediately - Test server returns 200 with empty graph for unknown channels to match production Cincinnati behavior - Pass non-nil transport in testserver.NewClient for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:658
- This struct literal field is missing indentation, which will fail gofmt/goimports checks.
clock: clocktesting.NewFakePassiveClock(now),
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:773
- This struct literal field is missing indentation, which will fail gofmt/goimports checks.
clock: clocktesting.NewFakePassiveClock(now),
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:868
- This line is not indented like the surrounding code, which will fail gofmt/goimports checks and make the test harder to read.
now := time.Date(2026, 6, 25, 12, 0, 0, 0, time.UTC)
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:1101
- This struct literal field is missing indentation, which will fail gofmt/goimports checks.
clock: clocktesting.NewFakePassiveClock(now),
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:257
- This line is not indented like the surrounding code, which will fail gofmt/goimports checks and make the test harder to read.
This issue also appears on line 868 of the same file.
now := time.Date(2026, 6, 25, 12, 0, 0, 0, time.UTC)
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go:469
- This struct literal field is missing indentation, which will fail gofmt/goimports checks.
This issue also appears in the following locations of the same file:
- line 658
- line 773
- line 1101
clock: clocktesting.NewFakePassiveClock(now),
internal/cincinnati/caching_client.go:72
- Expired cache entries are never deleted, so the map can grow without bound over time (even though entries are treated as stale). Consider deleting expired entries on lookup to avoid unbounded memory growth for long-running processes.
c.mu.RLock()
if entry, ok := c.cache[key]; ok && c.clock.Now().Before(entry.expiresAt) {
c.mu.RUnlock()
return entry.current, entry.updates, entry.conditionalUpdates, entry.err
}
c.mu.RUnlock()
|
/retest |
| // doesNextMinorExist checks whether Cincinnati has a channel for the next | ||
| // minor version after currentMinor by probing for its .0 release. |
There was a problem hiding this comment.
for candidates channel; pre-releases like .rc or .ec will fail this probe
We probably want something like #6110
There was a problem hiding this comment.
for candidates channel; pre-releases like .rc or .ec will fail this probe
We probably want something like #6110
We'll need some example versions that fail with production candidates channel so we can make sure OCP sees the same thing in the API they share with us..
| // versions to check). | ||
| func isNextMinorReachableFromCurrentMinor(ctx context.Context, client cincinnati.Client, cincinnatiURI *url.URL, channelStability string, currentMinor semver.Version, alreadyChecked map[string]bool) (bool, error) { | ||
| channel := formatChannel(channelStability, currentMinor) | ||
| dotZero := semver.Version{Major: currentMinor.Major, Minor: currentMinor.Minor} |
There was a problem hiding this comment.
This #6192 (comment) might apply here too;
in candidate channel as this will ignore pre-releases versions.
| for _, candidate := range candidates { | ||
| valid, err := hasValidUpgradePath(ctx, client, cincinnatiURI, channelStability, currentMinor, candidate, make(map[string]bool)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if valid { | ||
| return &candidate, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Should our graph traversal logic also care about picking the candidate that gets us to the next minor in the least amount of hops as possible?
Or maybe not since the candidates are sorted and we simply care for candidate that has the CVE fix (it'll be latest candidates and hence the first in our sorted slice) that has any upgrade path to the next minor ?
There was a problem hiding this comment.
I dont' think least matters.
…xists The control plane minor upgrade test was failing because it selected an install version (e.g. 4.22.7) but did not verify that version had an actual upgrade path to the target minor (e.g. 4.23) before creating a cluster. When the install version was not in the target channel's graph, the backend's version resolution failed with VersionNotFound. Add an early probe that calls SelectControlPlaneVersion with the install version against the target minor. If no path exists (VersionNotFound, NoGatewayError, or nil result), the test now skips instead of proceeding to create a cluster that cannot upgrade. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
internal/cincinnati/caching_client.go:72
- Expired cache entries are never removed, and the cache map is unbounded. In long-running controllers (especially with many nightly versions), this can lead to steadily increasing memory use even though entries are no longer valid.
c.mu.RLock()
if entry, ok := c.cache[key]; ok && c.clock.Now().Before(entry.expiresAt) {
c.mu.RUnlock()
return entry.current, entry.updates, entry.conditionalUpdates, entry.err
}
internal/cincinnati/testserver/graph.go:62
- Graph nodes always append "-multi" to the payload tag. For nightly versions that already contain "-multi" (e.g. "4.19.0-0.nightly-multi-…"), this produces an invalid payload like "…-multi-multi", which can make the test graph less representative and could break future tests that start relying on the payload field.
idx := len(g.nodes)
g.nodes = append(g.nodes, graphNode{
Version: version,
Payload: "quay.io/openshift-release-dev/ocp-release:" + version + "-multi",
})
backend/pkg/hcpversionselection/select_version.go:227
- doesNextMinorExist (and the .0-based discovery in isNextMinorReachableFromCurrentMinor) assumes the next minor's plain x.y.0 version is present in the channel. That’s not true for nightly graphs, where versions are typically prerelease strings like "4.20.0-0.nightly-multi-…" and "4.20.0" may never exist. This will incorrectly treat the next minor as non-existent/reachable and can short-circuit the transitive gateway checks.
// doesNextMinorExist checks whether Cincinnati has a channel for the next
// minor version after currentMinor by probing for its .0 release.
func doesNextMinorExist(ctx context.Context, client cincinnati.Client, cincinnatiURI *url.URL, channelStability string, currentMinor semver.Version) (bool, error) {
nextMajor, nextMinorNum := nextMinorVersion(currentMinor)
nextChannel := formatChannel(channelStability, semver.Version{Major: nextMajor, Minor: nextMinorNum})
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go:183
- SyncOnce currently skips desired version selection whenever activeVersions is populated but the cached HostedCluster is unavailable. Since activeVersions already contains the information needed to drive version selection, this can stall desired-version updates due to temporary ReadDesire/informer lag.
// When active versions exist but the cached HostedCluster is unavailable,
// we cannot safely run version selection because SelectControlPlaneVersion
// would see nil and fall into the install path. Skip until the ReadDesire
// is populated; the informer will re-enqueue us.
if len(activeVersions) > 0 && hostedCluster == nil {
internal/cincinnati/testserver/server.go:90
- NewClient doesn't actually "point" at the test server (the Cincinnati URI is still supplied per call to GetUpdates). The comment is misleading for readers and for future refactors.
// NewClient creates a real CVO Cincinnati client pointed at this test server.
activeVersionsFromHostedCluster now includes all history entries up to and including the first Completed entry. Partial entries more recent than the first Completed are in-progress upgrades still active on the cluster and must constrain version selection. Only Partial entries older than a Completed entry are excluded as superseded. Three test cases cover the three scenarios: - Partial older than Completed → excluded - Partial more recent than Completed → included (graph designed so each active-set scenario produces a unique result) - All Partial → every entry included as active Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eFromCurrentMinor The only caller passes nil and the function never writes to the map, so the nil-map read always returns false. Remove the dead parameter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
doesNextMinorExist and isNextMinorReachableFromCurrentMinor probed Cincinnati with clean .0 versions (e.g. 4.23.0) to check channel existence and discover versions. This fails for candidate and nightly channels where versions have pre-release suffixes (.rc, .ec, .nightly-multi-...) and .0 is not in the graph. doesNextMinorExist now uses GraphClient.ChannelExists which queries the Cincinnati API directly to check if a channel has nodes, without relying on a specific version probe. isNextMinorReachableFromCurrentMinor now falls back to checking the candidates (from active version queries) for gateway edges when .0 is not found, instead of returning false. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed comments in separate commits |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
internal/cincinnati/caching_client.go:66
- Caching key uses fmt verb %s with a semver.Version argument. In fmt, %s is for string/[]byte; this will produce a "%!s(semver.Version=...)" placeholder string rather than the version string, which is error-prone and makes the cache key format misleading.
Use version.String() (or switch the verb to %v) so the key is stable and readable.
func (c *CachingClient) GetUpdates(ctx context.Context, uri *url.URL, desiredArch, currentArch, channel string, version semver.Version) (configv1.Release, []configv1.Release, []configv1.ConditionalUpdate, error) {
key := fmt.Sprintf("%s://%s%s|%s|%s|%s|%s", uri.Scheme, uri.Host, uri.Path, desiredArch, currentArch, channel, version)
internal/cincinnati/caching_client.go:51
- CachingClient stores entries in an unbounded map and never deletes expired keys. In a long-running controller, unique (channel, version) queries over time can grow this map without bound (TTL only prevents reuse; it doesn't reclaim memory).
Consider deleting expired entries (at least opportunistically on lookup) and/or using a bounded cache (LRU) to cap memory usage.
type CachingClient struct {
inner Client
clock utilsclock.PassiveClock
ttl time.Duration
mu sync.RWMutex
cache map[string]*cacheEntry
}
backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go:86
- The Cincinnati CVO client is now constructed with uuid.Nil for all requests. Cincinnati APIs can use the cluster ID to shape responses (e.g., conditional updates / telemetry), and the codebase already has helpers to parse the real HostedCluster.Spec.ClusterID.
If cluster-specific behavior matters, using uuid.Nil can change upgrade eligibility and can also make caching unsafe across clusters (responses may differ by cluster ID). Consider threading the real cluster UUID into the Cincinnati client creation (or reintroducing a per-cluster client cache) instead of hardcoding uuid.Nil.
cincinnatiClient: cincinnati.NewCachingClient(
cvocincinnati.NewClient(uuid.Nil, http.DefaultTransport.(*http.Transport).Clone(), "ARO-HCP", cincinnati.NewAlwaysConditionRegistry()),
clock, 1*time.Hour,
),
backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go:103
- The Cincinnati CVO client is now created once with uuid.Nil. If Cincinnati responses/conditional updates depend on the cluster UUID, this changes behavior and makes it impossible to safely share a single cached client across clusters.
Since SyncOnce already fetches the HostedCluster (which carries Spec.ClusterID), consider constructing the underlying CVO client with the real UUID (or using a per-cluster client/cache) rather than hardcoding uuid.Nil.
syncer := &controlPlaneDesiredVersionSyncer{
clock: clock,
readDesireLister: readDesireLister,
cincinnatiClient: cincinnati.NewCachingClient(
cvocincinnati.NewClient(uuid.Nil, http.DefaultTransport.(*http.Transport).Clone(), "ARO-HCP", cincinnati.NewAlwaysConditionRegistry()),
clock, 1*time.Hour,
),
graphClient: cincinnati.NewGraphClient(),
|
/test e2e-parallel |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 similar comment
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Is because of Is the log I see; Additionally, in this kusto query I see logs like Which could partly explain the alert firing And that alert is also firing because each time we can't compute the next level z-stream we return an error in (https://github.com/deads2k/ARO-HCP/blob/8a0c20c0ef93ce40ea0680fd3de70ba54407b9fc/backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go#L263) regardless of whether it is a pure z-stream evaluation or not e.g |
|
Interesting that a bunch of failures in https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6192/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2082188785050718208 points to the desiredversion controller not running e.g Was because the cluster was never created in CS because the desired version was never there; From this kusto log And Only shows |
| // currentMinor — AND that the reached version in the next minor has a valid | ||
| // chain through subsequent minors. |
There was a problem hiding this comment.
I've been thinking through this;
AND that the reached version in the next minor has a valid
// chain through subsequent minors.
Why do we care about reach a minor + 5 version?
Shouldn't we care about reaching only the next minor or maybe the next two next minors to avoid traversing the graph all the time which is expensive?
If we take an example from these logs for one cluster cluster-candidate-4-20-vp6krh
28/07/2026, 20:22:39.227 Resolving initial desired version
28/07/2026, 20:56:12.074 desired version resolution failed, persisting IntentFailed condition no upgrade path from active versions [] to next minor 4.21: the 4.21 channel exists but no reachable candidate in 4.20 is a gateway; the cluster must wait for a new edge to be published
28/07/2026, 20:56:12.09 cluster not found, skipping sync
We spent 33 minutes evaluating just the initial install version only for it to fail here.
With a threadiness of 20; we can only process 20 clusters max if the sync method isn't fast enough;
28/07/2026, 20:56:12.074 desired version resolution failed, persisting IntentFailed condition no upgrade path from active versions [] to next minor 4.21: the 4.21 channel exists but no reachable candidate in 4.20 is a gateway; the cluster must wait for a new edge to be published
This log confuses me still; why does it think that the 4.21 channel exists but no reachable candidate in 4.20 is a gateway? While a gateway e.g 4.20.31 exists?
There was a problem hiding this comment.
This also explains why the queue depth alert fired in https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6192/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2082188785050718208
{alert fired 1 time(s) Firing 1:
State: Resolved
Started: 2026-07-28T20:37:52Z
Ended: 2026-07-28T21:02:52Z
Severity: Sev3
Labels: alertname="BackendControllerQueueDepthHigh", cluster="ci01-j0718208-svc", component="backend", name="controlplanedesiredversion", severity="warning"
Description: Backend controller workqueue controlplanedesiredversion has had a depth > 10 for more than 15 minutes, indicating work is accumulating faster than it can be processed.
}
When the version selector returns a NoGatewayError ("the X channel exists
but no reachable candidate is a gateway"), the error message now includes
the channel-existence and gateway-probe URLs so operators can curl them
to verify the claim. Removes the unused gomock-based GraphClient mock in
favor of the testserver implementation.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds structured logging (url, duration, status/err) after each uncached HTTP call to Cincinnati: CachingClient.GetUpdates on cache miss, graphClient.channelExistsFromGraphURL, and graphClient.nightlyChannelExists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
got a unit test reproducing now and I'm working with the bot for a fix. |
The CVO client moves versions that appear in both unconditional and conditional edges into the conditional-only list, then prunes conditional updates whose risk conditions use unrecognized types (e.g. PromQL). This caused gateway versions like 4.22.3 to vanish entirely, blocking version selection for clusters at 4.22.2. ARO-HCP does not evaluate conditional risk gates on-cluster — all Cincinnati edges should be treated as available upgrade paths. This commit: 1. Replaces NewAlwaysConditionRegistry (only handled "Always" type) with NewAcceptAllConditionRegistry — a no-op registry that accepts all condition types so nothing is pruned. 2. Merges conditional updates back into the unconditional list in CachingClient.GetUpdates, since the CVO client still moves dual-listed versions to conditional-only even with an accept-all registry. 3. Adds live integration tests against real Cincinnati for SelectControlPlaneVersion and the graph/CVO clients. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/cincinnati/graph_client_live_test.go:188
TestLiveGetUpdates_Graphcurrently has the reachability/env gating disabled (skipUnlessCincinnatiReachableis commented). That makes the test always attempt real network calls, which is likely to be flaky or fail in CI environments without egress. Re-enable the gating so the live test only runs when explicitly enabled.
func TestLiveGetUpdates_Graph(t *testing.T) {
//skipUnlessCincinnatiReachable(t)
t.Parallel()
backend/pkg/hcpversionselection/select_version_live_test.go:16
- This is a live/network test but it has no opt-in gating (env var/build tag), so it will run by default and hit external Cincinnati endpoints, which can make CI flaky or fail in restricted environments. Consider making it opt-in (e.g., a
//go:build livebuild tag, or an env var gate similar tointernal/cincinnati/graph_client_live_test.go).
// limitations under the License.
package hcpversionselection
No description provided.