OCPBUGS-83389: fix(supportedversion): normalize OCP 5.x versions for skew and release validation#8225
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved pointer-based clamping and the 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
bot
added
the
approved
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
support/supportedversion/version_test.go (1)
424-517: Add caller-level coverage forIsValidReleaseVersion.These additions exercise the helper mapping and skew path, but the other changed normalization consumer still has no 5.x boundary cases. A few
TestIsValidReleaseVersionscenarios for5.0 ↔ 4.23, clampedmaxSupportedVersion, and an unsupported future major would make this change much safer.Also applies to: 519-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/supportedversion/version_test.go` around lines 424 - 517, Add unit tests for IsValidReleaseVersion that cover the 5.x ↔ 4.23 normalization mapping and boundary/skew behavior: create cases where input is "5.0.0" (expect valid via normalizeToV4 → "4.23.0"), a 5.x with patch/prerelease to ensure preservation, a case where maxSupportedVersion is clamped (set a lower max and assert IsValidReleaseVersion respects it), and a case with an unsupported future major (e.g., major > supported range) to assert it returns false; reference the IsValidReleaseVersion function and the helper functions normalizeToV4 and denormalizeFromV4 when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@support/supportedversion/version.go`:
- Around line 130-132: The error messages still include the original
maxSupportedVersion even after you clamp it, causing a mismatch between
validation and message; update the error paths to reference the clamped variable
normalizedMax (which is adjusted against normalizedLatest) instead of
maxSupportedVersion wherever the error string is constructed (the occurrences
around the normalizedMax.GT(normalizedLatest) block and the similar block at
lines ~158-160) so the returned error reflects the actual capped maximum.
- Around line 355-370: normalizeToV4 currently silently returns unknown majors
unchanged which can misclassify inputs like 6.0; change normalizeToV4 to return
(semver.Version, error) and validate majors: if Major==5 map to 4.(23+minor) as
before, if Major==4 return the version, otherwise return a zero value and a
descriptive error (e.g., "unsupported major version") so callers (e.g., places
that compare against minSupportedVersion) must handle the error instead of
assuming a 4.x outcome; update all call sites to propagate or handle this error
and remove the implicit major assumption in downstream comparisons.
---
Nitpick comments:
In `@support/supportedversion/version_test.go`:
- Around line 424-517: Add unit tests for IsValidReleaseVersion that cover the
5.x ↔ 4.23 normalization mapping and boundary/skew behavior: create cases where
input is "5.0.0" (expect valid via normalizeToV4 → "4.23.0"), a 5.x with
patch/prerelease to ensure preservation, a case where maxSupportedVersion is
clamped (set a lower max and assert IsValidReleaseVersion respects it), and a
case with an unsupported future major (e.g., major > supported range) to assert
it returns false; reference the IsValidReleaseVersion function and the helper
functions normalizeToV4 and denormalizeFromV4 when adding these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 06c22e12-1f6f-47c1-ba4d-caec94a75fea
📒 Files selected for processing (2)
support/supportedversion/version.gosupport/supportedversion/version_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/supportedversion/version_test.go (1)
477-477: Minor: Inconsistent Gomega initialization style.The new tests use
NewWithT(t)while the rest of the file usesNewGomegaWithT(t). Both are functionally equivalent aliases, but consistency would improve readability.♻️ Optional: Align with existing style
- g := NewWithT(t) + g := NewGomegaWithT(t)Apply to lines 477, 527, and 687.
Also applies to: 527-527, 687-687
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/supportedversion/version_test.go` at line 477, Replace the inconsistent Gomega initialization calls: locate uses of NewWithT(t) in the test file (they appear near other Gomega setup code) and change them to NewGomegaWithT(t) so they match the existing style; update each occurrence of NewWithT(t) to NewGomegaWithT(t) (e.g., in the test functions that call NewWithT) to keep the file consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@support/supportedversion/version_test.go`:
- Line 477: Replace the inconsistent Gomega initialization calls: locate uses of
NewWithT(t) in the test file (they appear near other Gomega setup code) and
change them to NewGomegaWithT(t) so they match the existing style; update each
occurrence of NewWithT(t) to NewGomegaWithT(t) (e.g., in the test functions that
call NewWithT) to keep the file consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8930884b-9b68-4f93-a101-cc4bb5c5e1c5
📒 Files selected for processing (2)
support/supportedversion/version.gosupport/supportedversion/version_test.go
bryan-cox
force-pushed
the
fix-version-skew-5x
branch
from
a351710 to
d76616e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
support/supportedversion/version.go (1)
249-258: Normalize the lower bound here as well.The upper bound now lives in the normalized 4.x space, but the lower bound still formats
minSupportedVersiondirectly. That works for today’s4.xminimums, yet the next time the minimum supported release moves into5.x, this query will silently fall back to>=4.0unless the lower side is normalized too. A small special-case for the existing0.0.0skip-validation sentinel keeps the current behavior intact.♻️ Suggested direction
minSupportedVersion := GetMinSupportedVersion(hc) + normalizedMin := minSupportedVersion + if minSupportedVersion.Major != 0 { + var err error + normalizedMin, err = normalizeToV4(minSupportedVersion) + if err != nil { + return "", fmt.Errorf("failed to normalize minimum supported version: %w", err) + } + } // Normalize LatestSupportedVersion to 4.x so the filter range is correct // even when LatestSupportedVersion is 5.x (e.g. 5.0 -> 4.23). normalizedLatest, err := normalizeToV4(LatestSupportedVersion) if err != nil { return "", fmt.Errorf("failed to normalize latest supported version: %w", err) } prefix := "https://multi.ocp.releases.ci.openshift.org/api/v1/releasestream/4-stable-multi/latest" filter := fmt.Sprintf("in=>4.%d.%d+<+4.%d.0-a", - minSupportedVersion.Minor, minSupportedVersion.Patch, normalizedLatest.Minor+1) + normalizedMin.Minor, normalizedMin.Patch, normalizedLatest.Minor+1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/supportedversion/version.go` around lines 249 - 258, The lower bound should be normalized to 4.x like the upper bound: call normalizeToV4 on minSupportedVersion (e.g., produce normalizedMin via normalizeToV4(minSupportedVersion)) and use normalizedMin.Minor and normalizedMin.Patch when building filter; preserve the existing sentinel behavior by bypassing normalization when minSupportedVersion equals the skip-validation sentinel (0.0.0) so the old >=4.0 behavior remains. Update the filter construction to reference normalizedMin instead of minSupportedVersion and reuse normalizedLatest as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@support/supportedversion/version.go`:
- Around line 378-400: normalizeToV4 adds 5.x -> 4.(23+x) mapping but
GetKubeVersionForSupportedVersion still does an exact lookup against
ocpVersionToKubeVersion, so LatestSupportedVersion (5.0.0 → 4.23.0) is reported
as unknown; update ocpVersionToKubeVersion to include the 4.23.0 boundary
mapping to the correct Kubernetes version (add the 4.23.0 entry) so
GetKubeVersionForSupportedVersion can resolve normalized 5.0/4.23 targets;
verify behavior using LatestSupportedVersion and the
GetKubeVersionForSupportedVersion function after the table update.
---
Nitpick comments:
In `@support/supportedversion/version.go`:
- Around line 249-258: The lower bound should be normalized to 4.x like the
upper bound: call normalizeToV4 on minSupportedVersion (e.g., produce
normalizedMin via normalizeToV4(minSupportedVersion)) and use
normalizedMin.Minor and normalizedMin.Patch when building filter; preserve the
existing sentinel behavior by bypassing normalization when minSupportedVersion
equals the skip-validation sentinel (0.0.0) so the old >=4.0 behavior remains.
Update the filter construction to reference normalizedMin instead of
minSupportedVersion and reuse normalizedLatest as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fa16514e-c2ed-4a84-aae6-4f02ff345490
📒 Files selected for processing (3)
hypershift-operator/controllers/nodepool/nodepool_controller.gosupport/supportedversion/version.gosupport/supportedversion/version_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/supportedversion/version_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8225 +/- ##
==========================================
+ Coverage 34.63% 34.65% +0.01%
==========================================
Files 767 767
Lines 93200 93256 +56
==========================================
+ Hits 32282 32318 +36
- Misses 58243 58259 +16
- Partials 2675 2679 +4
🚀 New features to boost your workflow:
|
|
/test e2e-aws |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 9
... and 4 more failed tests |
|
/pipeline required |
|
Scheduling tests matching the |
|
/verified by e2e tests |
AI Test Failure AnalysisJob: Failed tests
Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
…dation Update TestValidMinorVersionCompatibility to expect the new error format from ValidateVersionSkew when a 5.x NodePool is created against a 4.x HostedCluster. Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…ease validation
OCP uses dual versioning where 5.0 == 4.23 (both succeed 4.22).
ValidateVersionSkew previously rejected cross-major-version skew
(e.g. HC=5.0, NP=4.22) with a major-version-mismatch error,
breaking CI tests like TestNodePoolPrevReleaseN1.
- Add normalizeToV4/denormalizeFromV4 helpers that map 5.x to
4.(23+x) for consistent comparison across the 4.x/5.x boundary
- Rewrite ValidateVersionSkew and IsValidReleaseVersion to normalize
before comparing, preserving original version numbers in errors
- Fix NodePool caller to pass nil instead of &semver.Version{} when
no current version exists, matching hostedcluster and karpenter
- Normalize input in GetKubeVersionForSupportedVersion so 5.0
resolves via the 4.23 lookup table; add 4.23.0 -> 1.36.0 mapping
- Fix LookupLatestSupportedRelease filter for 5.x by normalizing
LatestSupportedVersion before building URL bounds
- Fix uint64 underflow in subtractMinor, remove dead maxInt64 helper
- Hoist repeated semver.MustParse constants to package-level vars
- Add comprehensive tests covering cross-major-version scenarios,
normalization edge cases, and IsValidReleaseVersion 5.x boundary
Signed-off-by: Bryan Cox <brcox@redhat.com>
Commit-Message-Assisted-by: Claude (via Claude Code)
bryan-cox
force-pushed
the
fix-version-skew-5x
branch
from
057042f to
55c6fca
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by e2e |
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
bryan-cox
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-83389, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aks |
1 similar comment
|
/test e2e-aks |
|
/test e2e-aks-4-22 Hoping it was a flake because the issue that happened should not happen |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
I now have all the evidence needed. Both failures are CI infrastructure issues completely unrelated to the PR. Let me produce the final report. Test Failure Analysis CompleteJob InformationJob 1: e2e-aks-4-22
Job 2: e2e-aws-4-22
Test Failure AnalysisErrorSummaryBoth failures are CI infrastructure issues completely unrelated to the PR's code changes. Job 1 (e2e-aks-4-22) failed during release import setup — ci-operator could not populate the Root CauseJob 1 (e2e-aks-4-22) — OCP 4.18 Release Import Failure: ci-operator failed to fully import the OCP 4.18 release payload into the
The fact that ci-operator encountered "Unable to create image stream import up to conflicts" 3 times before succeeding suggests contention or an issue with the OCP 4.18 release image. The full mirror pod was never scheduled, indicating ci-operator's step graph determined it couldn't proceed after the conflicts. This is a ci-operator infrastructure issue, likely related to the OCP 4.18 CI release stream health. Job 2 (e2e-aws-4-22) — AWS API Rate Limiting:
Recommendations
Evidence
|
|
/override ci/prow/e2e-aks ci/prow/e2e-aks-4-22 ci/prow/e2e-aws-4-22 |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aks, ci/prow/e2e-aks-4-22, ci/prow/e2e-aws-4-22 DetailsIn response to this:
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. |
|
@bryan-cox: all tests passed! 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sjenning: This pull request references Jira Issue OCPBUGS-83389, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-83389 Jira Issue OCPBUGS-83389 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
5 participants
What this PR does / why we need it:
OCP 5.0 is equivalent to OCP 4.23 (dual versioning).
ValidateVersionSkewrejects cross-major-version skew (e.g., HC=5.0 with NP=4.22) because it compares major versions directly, andIsValidReleaseVersionrejects 5.x releases when bounds are expressed in 4.x.This PR adds
normalizeToV4/denormalizeFromV4helpers that map 5.x → 4.(23+x) before comparison and back for error messages. BothValidateVersionSkewandIsValidReleaseVersionnow use normalization so the n-3 skew policy and release bounds work correctly across the 4.x/5.x boundary.Which issue(s) this PR fixes:
Fixes https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_release/77738/rehearse-77738-pull-ci-openshift-hypershift-main-e2e-aws/2043772424360562688
TestNodePoolPrevReleaseN1fails with:Special notes for your reviewer:
normalizeToV4maps 5.x → 4.(23+x);denormalizeFromV4converts back for error messagesv5MinorOffsetconstant (23) encodes the 5.0 == 4.23 mappingIsValidReleaseVersionis also normalized to handle 5.x release images against 4.x boundsChecklist:
Summary by CodeRabbit
Bug Fixes
Tests