Prefer latest if no gateway from minor for active versions and all candidates - #6122
Conversation
| for _, active := range activeVersions { | ||
| isGateway, err := isGatewayToNextMinor(ctx, active, cincinnatiClient, channelGroup, nextMinor) | ||
| if err != nil { | ||
| return nil, utils.TrackError(err) | ||
| } | ||
| if isGateway { | ||
| return nil, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
seeing it written in code, would this mean that if we somehow end up at a 4.y.z with no path to 4.y+1, then we can keep upgrading to the very latest 4.y.z that still has path to 4.y+1 because it's so new? That would be unfortunate.
There was a problem hiding this comment.
would this mean that if we somehow end up at a 4.y.z with no path to 4.y+1
For z-stream upgrade, If we reach this branch, it is because neither of the upgrade candidates in 4.y have an upgrade path to 4.y+1.
then we can keep upgrading to the very latest 4.y.z that still has path to 4.y+1 because it's so new?
Here we know that 4.y+1 is still new and/or all 4.y active versions in the cluster plus 4.y.z+1 upgrade candidates do not have an upgrade to 4.y+1; in this case yes we continue honoring z-stream upgrade within 4.y; maintaining our priority to security & bug fixes within 4.y. We can do this as we don't have the upgrade path to 4.y+1 that we'd want to preserve
That would be unfortunate.
Yes, I don't have the data how often this happened when a new minor is released but while I was at #6110 this came up as a gap. If this at all happens without this improvement ; the following will occur.
- For existing clusters, we'll stop z-stream upgrades until the new 4.y+1 is reachable from 4.y and possible candidates. Maybe in O(week) as edges are added?
- For initial install, all clusters will be install at a 4.y.0 final resolved version: https://github.com/machi1990/ARO-HCP/blob/b528d137b5efe7fbfdad50f533dfe8cf468752a1/backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go#L279. For this case, I wonder why we chose to set preferLatestOverGateway=false, https://github.com/machi1990/ARO-HCP/blob/b528d137b5efe7fbfdad50f533dfe8cf468752a1/backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go#L266 thinking about it preferLatestOverGateway=true will be most appropriate for initial
@deads2k thoughts?
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated Cincinnati “graph channel existence” client and updates control plane desired-version selection so z-stream upgrades prefer the latest candidate when there is no next-minor gateway path to preserve (across both active versions and candidates).
Changes:
- Add
internal/cincinnati.GraphClient(with unit tests + gomock) to check whether a next-minor channel/stream is published. - Wire
GraphClientinto the control plane desired-version controller and update the selection algorithm to return latest candidate when no next-minor gateway exists for either active versions or candidates. - Update controller unit tests to mock channel existence and cover the new z-stream behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cincinnati/graph_client.go | New HTTP-based client for checking channel/stream existence in public Cincinnati APIs. |
| internal/cincinnati/mock_graph_client.go | Generated GoMock for GraphClient. |
| internal/cincinnati/graph_client_test.go | Unit tests for GraphClient.ChannelExists across stable/candidate/fast and nightly behaviors. |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller.go | Inject GraphClient and adjust version-selection behavior for z-stream upgrades when no gateway exists. |
| backend/pkg/controllers/upgradecontrollers/control_plane_desired_version_controller_test.go | Update tests to mock GraphClient.ChannelExists and validate new selection outcomes. |
Files not reviewed (1)
- internal/cincinnati/mock_graph_client.go: Generated file
|
|
||
| func (c graphClient) channelExistsFromGraphURL(ctx context.Context, channelGroup, minor string) (bool, error) { | ||
| channel := fmt.Sprintf("%s-%s", channelGroup, minor) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("%s/api/upgrades_info/v1/graph?channel=%s", c.graphAPIBase, url.QueryEscape(channel)), nil) |
| // NewGraphClient returns a GraphClient that queries the public Cincinnati APIs. | ||
| func NewGraphClient() GraphClient { | ||
| return graphClient{ | ||
| graphAPIBase: "https://api.openshift.com", | ||
| nightlyAPIBase: "https://multi.ocp.releases.ci.openshift.org", | ||
| } |
When the next-minor channel exists but neither candidates nor active versions are gateways, return the latest candidate instead of nil — there is no existing next-minor path to preserve. Preferring the latest candidate prioritizes security and bug fixes over withholding an upgrade that cannot unlock the next minor anyway. Follows up Azure#6110 (comment)
b528d13 to
627961c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: machi1990 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Superseded by #6192 |
What
fix: prefer latest z-stream candidate when no next-minor path exists
Why
When the next-minor channel exists but neither candidates nor active
versions are gateways, return the latest candidate instead of nil —
there is no existing next-minor path to preserve. Preferring the latest
candidate prioritizes security and bug fixes over withholding an upgrade
that cannot unlock the next minor anyway.
Testing
Testing is required for feature completion and tests should be part of the pull
request along with the feature changes.
Describe the testing provided. If you did not add tests, provide a clear
justification.
Special notes for your reviewer
Review the second commit only; the first commit comes from #6110
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.