OCPBUGS-98744: compare post-upgrade RHCOS version against pre-upgrade instead of release metadata#9014
OCPBUGS-98744: compare post-upgrade RHCOS version against pre-upgrade instead of release metadata#9014jparrill wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe Karpenter control-plane upgrade test now extracts RHCOS versions directly from node OS image strings. It records the pre-upgrade version and verifies that post-upgrade node versions are not older. The previous release-image registry lookup and Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98744, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/karpenter_test.go (1)
1885-1887: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a two-value lookup for the deduplication set.
The repository guidelines require explicit map-existence checks. Prefer
map[string]struct{}withif _, ok := seen[release]; ok.Proposed change
- seen := map[string]bool{} + seen := make(map[string]struct{}) ... - if !seen[regionData.Release] { - seen[regionData.Release] = true + if _, ok := seen[regionData.Release]; !ok { + seen[regionData.Release] = struct{}{}As per coding guidelines, check map existence with the two-value assignment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/karpenter_test.go` around lines 1885 - 1887, Update the deduplication check in the versions collection to use an explicit two-value lookup on the seen map, such as checking whether the release key exists, while preserving the existing behavior of marking unseen releases and appending them to versions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/karpenter_test.go`:
- Around line 1862-1892: Add focused unit tests for machineOSVersions covering
nil and default streams, multiple OSStreams, missing AWS or region data, empty
releases, and duplicate release values. Construct minimal releaseinfo and stream
fixtures for each case, assert the returned versions, and keep the tests scoped
to this helper’s behavior.
---
Nitpick comments:
In `@test/e2e/karpenter_test.go`:
- Around line 1885-1887: Update the deduplication check in the versions
collection to use an explicit two-value lookup on the seen map, such as checking
whether the release key exists, while preserving the existing behavior of
marking unseen releases and appending them to versions.
🪄 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: Enterprise
Run ID: 0d580540-a03f-40e1-a9ed-f16569b53e32
📒 Files selected for processing (2)
test/e2e/karpenter_control_plane_upgrade_test.gotest/e2e/karpenter_test.go
| func machineOSVersions(releaseImage *releaseinfo.ReleaseImage, region string) []string { | ||
| seen := map[string]bool{} | ||
| var versions []string | ||
| for _, tag := range releaseImage.ImageStream.Spec.Tags { | ||
| buildVersions, ok := tag.Annotations["io.openshift.build.versions"] | ||
| if !ok { | ||
|
|
||
| streams := []*stream.Stream{releaseImage.StreamMetadata} | ||
| for _, s := range releaseImage.OSStreams { | ||
| if s != nil { | ||
| streams = append(streams, s) | ||
| } | ||
| } | ||
|
|
||
| for _, s := range streams { | ||
| if s == nil { | ||
| continue | ||
| } | ||
| for _, pair := range strings.Split(buildVersions, ",") { | ||
| parts := strings.SplitN(strings.TrimSpace(pair), "=", 2) | ||
| if len(parts) == 2 && parts[0] == "machine-os" { | ||
| versions = append(versions, parts[1]) | ||
| for _, arch := range s.Architectures { | ||
| if arch.Images.Aws == nil { | ||
| continue | ||
| } | ||
| regionData, ok := arch.Images.Aws.Regions[region] | ||
| if !ok || regionData.Release == "" { | ||
| continue | ||
| } | ||
| if !seen[regionData.Release] { | ||
| seen[regionData.Release] = true | ||
| versions = append(versions, regionData.Release) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return versions |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add focused tests for machineOSVersions.
Cover nil/default streams, multiple OSStreams, missing AWS or region entries, empty releases, and deduplication. As per coding guidelines, all Go code changes and additions must be unit tested.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/karpenter_test.go` around lines 1862 - 1892, Add focused unit tests
for machineOSVersions covering nil and default streams, multiple OSStreams,
missing AWS or region data, empty releases, and duplicate release values.
Construct minimal releaseinfo and stream fixtures for each case, assert the
returned versions, and keep the tests scoped to this helper’s behavior.
Source: Coding guidelines
|
/area testing |
|
/acknowledge-critical-fixes-only |
|
/jira refresh |
|
@jparrill: This pull request references Jira Issue OCPBUGS-98744, which is invalid:
Comment 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. |
openshift-ci
Bot
added
the
area/testing
openshift-ci
Bot
added
approved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9014 +/- ##
=========================================
Coverage 44.11% 44.11%
=========================================
Files 772 779 +7
Lines 96226 98077 +1851
=========================================
+ Hits 42448 43269 +821
- Misses 50832 51773 +941
- Partials 2946 3035 +89 see 27 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
csrwng
added
the
acknowledge-critical-fixes-only
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Clean, well-targeted fix. Aligning the expected RHCOS version with the stream metadata AMI source (rather than ImageStream annotations) eliminates the flake from AMI publication lag. Both fields (StreamMetadata, OSStreams) are properly used, the single caller is updated, and the dedup logic is correct.
/lgtm
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, jparrill 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 |
Test Resultse2e-aws
Failed TestsTotal failed tests: 4
e2e-aks
|
|
New changes are detected. LGTM label has been removed. |
jparrill
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
openshift-ci-robot
removed
the
jira/invalid-bug
|
@jparrill: No Jira issue is referenced in the title of this pull request. 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. |
jparrill
changed the title
|
Thanks for the review @csrwng — the approach changed since your approval, so I want to explain why. Why the stream metadata approach didn't work either: The initial fix (reading
The first CI run after the initial fix confirmed this: nodes reported Why the before/after comparison works: Instead of depending on any release metadata source, the test now captures the pre-upgrade |
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98744, 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. |
|
@jparrill: 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-98744, which is valid. 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
| } | ||
|
|
||
| return false, fmt.Sprintf("expected node OS image %q to contain one of %v", fullOSImageString, expectedRHCOSVersions), nil | ||
| if postVersion <= preUpgradeRHCOSVersion { |
There was a problem hiding this comment.
Can we confirm that the rhcos version will always change on a newer OCP release? If not, then at most all we could check is that it's not older after upgrade.
There was a problem hiding this comment.
Good point — z-stream upgrades may ship the same RHCOS. Changed from strict greater-than to not-older (>= instead of >). The check now only fails if the post-upgrade RHCOS is strictly older than the pre-upgrade version.
…grade instead of release metadata TestKarpenterUpgradeControlPlane was failing because the expected RHCOS version from the release payload did not match what nodes actually booted with. Neither the ImageStream machine-os annotation (reflects build-time RHCOS, can be newer than the AMI) nor the stream metadata Release field (can be stale when the AMI ID is updated without refreshing the version string) reliably predicts the on-disk RHCOS version. Replace the version-list check with a before/after comparison: capture the pre-upgrade node OSImage, then after the Replace upgrade assert that the new node's RHCOS version is not older. The RHCOS version string (e.g. 9.8.20260713-1) embeds a YYYYMMDD date, so lexicographic comparison gives correct ordering. The check uses >= (not >) because z-stream upgrades may ship the same RHCOS version. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/karpenter_control_plane_upgrade_test.go`:
- Around line 99-101: Validate that preUpgradeRHCOSVersion returned by
extractRHCOSVersion is non-empty before the later version comparison. Add a
fail-fast assertion or test error immediately after extraction, preserving the
existing logging and comparison flow only when a valid version is present.
- Around line 163-172: Update rhcosVersionRe used by extractRHCOSVersion to
accept the 12-digit build timestamp by removing the fixed eight-digit
requirement and matching the full numeric build segment with
(\d+\.\d+\.\d+-\d+). Preserve the existing extraction and empty-result behavior.
🪄 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: Enterprise
Run ID: 22b2c1bd-ccf6-4b14-b617-f7906abb55e5
📒 Files selected for processing (2)
test/e2e/karpenter_control_plane_upgrade_test.gotest/e2e/karpenter_test.go
💤 Files with no reviewable changes (1)
- test/e2e/karpenter_test.go
| preUpgradeRHCOSVersion := extractRHCOSVersion(preUpgradeOSImage) | ||
| t.Logf("Pre-upgrade RHCOS version: %s", preUpgradeRHCOSVersion) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate that preUpgradeRHCOSVersion is non-empty before using it for comparison.
If extractRHCOSVersion returns "" for the pre-upgrade OSImage (e.g., unexpected format), the comparison at line 116 (postVersion < preUpgradeRHCOSVersion) becomes postVersion < "", which is always false for any non-empty postVersion. The test would silently pass without actually verifying the version ordering. Add a guard after extraction.
🛡️ Proposed fix: fail fast on empty pre-upgrade version
preUpgradeRHCOSVersion := extractRHCOSVersion(preUpgradeOSImage)
+ if preUpgradeRHCOSVersion == "" {
+ t.Fatalf("could not extract pre-upgrade RHCOS version from OS image %q", preUpgradeOSImage)
+ }
t.Logf("Pre-upgrade RHCOS version: %s", preUpgradeRHCOSVersion)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| preUpgradeRHCOSVersion := extractRHCOSVersion(preUpgradeOSImage) | |
| t.Logf("Pre-upgrade RHCOS version: %s", preUpgradeRHCOSVersion) | |
| preUpgradeRHCOSVersion := extractRHCOSVersion(preUpgradeOSImage) | |
| if preUpgradeRHCOSVersion == "" { | |
| t.Fatalf("could not extract pre-upgrade RHCOS version from OS image %q", preUpgradeOSImage) | |
| } | |
| t.Logf("Pre-upgrade RHCOS version: %s", preUpgradeRHCOSVersion) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 101-101: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(replicas)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/karpenter_control_plane_upgrade_test.go` around lines 99 - 101,
Validate that preUpgradeRHCOSVersion returned by extractRHCOSVersion is
non-empty before the later version comparison. Add a fail-fast assertion or test
error immediately after extraction, preserving the existing logging and
comparison flow only when a valid version is present.
|
|
||
| var rhcosVersionRe = regexp.MustCompile(`Red Hat Enterprise Linux CoreOS (\d+\.\d+\.\d{8}-\d+)`) | ||
|
|
||
| func extractRHCOSVersion(osImage string) string { | ||
| matches := rhcosVersionRe.FindStringSubmatch(osImage) | ||
| if len(matches) < 2 { | ||
| return "" | ||
| } | ||
| return matches[1] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the regex against known OSImage formats from upstream test vectors
cat <<'EOF' | go run -
package main
import (
"fmt"
"regexp"
)
func main() {
re := regexp.MustCompile(`Red Hat Enterprise Linux CoreOS (\d+\.\d+\.\d{8}-\d+)`)
reFixed := regexp.MustCompile(`Red Hat Enterprise Linux CoreOS (\d+\.\d+\.\d+-\d+)`)
cases := []string{
"Red Hat Enterprise Linux CoreOS 419.97.202505081234-0 (Plow)",
"Red Hat Enterprise Linux CoreOS 510.97.202506011200-0 (Plow)",
"Red Hat Enterprise Linux CoreOS 418.94.202501011200-0 (Plow)",
}
for _, c := range cases {
m := re.FindStringSubmatch(c)
mf := reFixed.FindStringSubmatch(c)
fmt.Printf("input: %s\n \\d{8} match: %q\n \\d+ match: %q\n", c, m, mf)
}
}
EOFRepository: openshift/hypershift
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file excerpt ==\n'
sed -n '1,240p' test/e2e/karpenter_control_plane_upgrade_test.go | cat -n
printf '\n== upstream OSImage examples ==\n'
rg -n "Red Hat Enterprise Linux CoreOS" -S . | sed -n '1,120p'Repository: openshift/hypershift
Length of output: 13631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
re_bad = re.compile(r'Red Hat Enterprise Linux CoreOS (\d+\.\d+\.\d{8}-\d+)')
re_fixed = re.compile(r'Red Hat Enterprise Linux CoreOS (\d+\.\d+\.\d+-\d+)')
cases = [
"Red Hat Enterprise Linux CoreOS 419.97.202505081234-0 (Plow)",
"Red Hat Enterprise Linux CoreOS 510.97.202506011200-0 (Plow)",
"Red Hat Enterprise Linux CoreOS 418.94.202501011200-0 (Plow)",
]
for c in cases:
print(c)
print(" bad ->", re_bad.search(c).groups() if re_bad.search(c) else None)
print(" fixed->", re_fixed.search(c).groups() if re_fixed.search(c) else None)
print("\ncall sites:")
PY
rg -n "extractRHCOSVersion|preUpgradeRHCOSVersion|postUpgradeRHCOSVersion|postVersion <|postVersion == \"\"" test/e2e/karpenter_control_plane_upgrade_test.goRepository: openshift/hypershift
Length of output: 1121
Fix the RHCOS version regex The current pattern requires an 8-digit build timestamp, but these OSImage strings use a 12-digit timestamp (202505081234), so extractRHCOSVersion returns "" and the upgrade wait never matches. Use (\d+\.\d+\.\d+-\d+) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/karpenter_control_plane_upgrade_test.go` around lines 163 - 172,
Update rhcosVersionRe used by extractRHCOSVersion to accept the 12-digit build
timestamp by removing the fixed eight-digit requirement and matching the full
numeric build segment with (\d+\.\d+\.\d+-\d+). Preserve the existing extraction
and empty-result behavior.
4 participants
Summary
TestKarpenterUpgradeControlPlanewas failing ~27% of e2e-aws presubmit runs because the expected RHCOS version from the release payload did not match what nodes actually booted withmachine-osannotation nor the stream metadataReleasefield reliably predicts the on-disk RHCOS version:9.8.20260713-1vs AMI with9.8.20260708-0)Releasefield can be stale when the AMI ID is updated without refreshing the version string (e.g.9.8.20260428-0while the AMI actually boots9.8.20260713-1)OSImage, then assert the post-upgrade RHCOS version is strictly newer9.8.20260713-1) embeds aYYYYMMDDdate, so lexicographic comparison gives correct orderingmachineOSVersions()helperRoot Cause
The CI release payload's metadata has two version sources that can both diverge from the actual node RHCOS:
io.openshift.build.versionsmachine-os=X— reflects RHCOS sources at build time, can be ahead of the published AMIArchitectures[arch].Images.Aws.Regions[region].Release— can be stale (AMI ID updated butReleasefield not refreshed)Neither source reflects what the AMI actually contains. A before/after comparison against the live node is the only reliable assertion.
Fixes
Test plan
e2e-awspresubmit passes (TestKarpenterUpgradeControlPlane)Summary by CodeRabbit