TRT-2506: Add OS version info to ClusterData#30827
TRT-2506: Add OS version info to ClusterData#30827petr-muller wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@petr-muller: This pull request references TRT-2506 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller 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
WalkthroughAdded an exported OS type with MajorVersion, extended ClusterData to include OS, and updated BuildClusterData to query MachineConfigPools (worker → cluster default → fallback "9") to populate OS.MajorVersion, collecting non-fatal warnings. Changes
Sequence DiagramsequenceDiagram
participant BuildClusterData as BuildClusterData
participant MachineConfigClient as MachineConfigClient
participant MachineConfigPool as MachineConfigPool
participant ClusterData as ClusterData
BuildClusterData->>MachineConfigClient: Request worker MCP
MachineConfigClient->>MachineConfigPool: Retrieve worker MCP data
MachineConfigPool-->>MachineConfigClient: Return OSImageURL/OSImageStream
MachineConfigClient-->>BuildClusterData: Return MCP info
alt worker OSImageStream starts with "rhel-"
BuildClusterData->>BuildClusterData: Parse major version and set OS.MajorVersion
else worker OSImageStream missing/empty
BuildClusterData->>MachineConfigClient: Query cluster default OSImageStream
MachineConfigClient-->>BuildClusterData: Return default stream (or empty)
alt default starts with "rhel-"
BuildClusterData->>BuildClusterData: Parse major version and set OS.MajorVersion
else
BuildClusterData->>BuildClusterData: Set OS.MajorVersion = "9" and append warning
end
end
BuildClusterData->>ClusterData: Attach OS.MajorVersion (and warnings)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@petr-muller: This pull request references TRT-2506 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/monitortestlibrary/platformidentification/types.go`:
- Around line 145-151: The current branch using mcp.Spec.OSImageStream.Name and
strings.CutPrefix("rhel-") leaves clusterData.OS.MajorVersion empty for
non-empty names that don't start with "rhel-"; update the branch so that when
osStreamName != "" and CutPrefix returns ok==false you either (a) append a parse
error (e.g., to the existing error/validation collection used for this
component) referencing the raw osStreamName and that it didn't match the
expected "rhel-*" pattern, and set clusterData.OS.MajorVersion to a clear
fallback value (e.g., "unknown" or your chosen default), or (b) return an
explicit error up the call chain; modify the code around
mcp.Spec.OSImageStream.Name, strings.CutPrefix and clusterData.OS.MajorVersion
to implement one of these behaviors so unrecognized OSImageStream names are
surfaced instead of silently leaving MajorVersion empty.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/monitortestlibrary/platformidentification/types.go
|
/test images |
1 similar comment
|
/test images |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn-rhcos10-techpreview-serial |
|
@petr-muller: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a6117d40-17cb-11f1-9a08-1bb1ee783c51-0 |
|
/pipeline required |
|
Scheduling required tests: |
petr-muller
force-pushed
the
trt-2506-osversion-platformidentification
branch
from
a97cccd to
88e8f82
Compare
|
@petr-muller: This pull request references TRT-2506 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn-rhcos10-techpreview-serial |
|
@petr-muller: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/55329010-17e6-11f1-9f67-eca718b8a1a5-0 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/monitortestlibrary/platformidentification/types.go (1)
194-216:⚠️ Potential issue | 🟠 MajorDon’t coerce unrecognized configured streams to
9.At Line [194] and Line [207], non-empty but unrecognized stream names are warned about, but execution still falls through to Line [216] and returns
"9". That can mislabel custom/non-RHEL streams as RHEL 9 and skew downstream correlation.🔧 Proposed fix
if err == nil { osStreamName := mcp.Spec.OSImageStream.Name if majorVersion, ok := strings.CutPrefix(osStreamName, "rhel-"); ok { return majorVersion, nil } if osStreamName != "" { warnings = append(warnings, fmt.Errorf("unrecognized worker MCP OSImageStream name %q, does not match expected rhel-* pattern", osStreamName)) + return "", warnings } } @@ if err == nil { defaultStream := osImageStream.Status.DefaultStream if majorVersion, ok := strings.CutPrefix(defaultStream, "rhel-"); ok { return majorVersion, warnings } if defaultStream != "" { warnings = append(warnings, fmt.Errorf("unrecognized OSImageStream default stream %q, does not match expected rhel-* pattern", defaultStream)) + return "", warnings } } else if !kapierrs.IsNotFound(err) { warnings = append(warnings, fmt.Errorf("error getting OSImageStream singleton: %w", err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/monitortestlibrary/platformidentification/types.go` around lines 194 - 216, The code currently appends a warning for non-empty, unrecognized stream names (osStreamName and defaultStream) but then falls through to return "9"; change this so that when a non-empty stream value fails the rhel-* check you return an empty string (or a sentinel indicating unknown) together with the accumulated warnings immediately instead of defaulting to "9". Specifically, in the branches that check osStreamName and defaultStream (the blocks referencing osStreamName and defaultStream variables and the strings.CutPrefix checks), after appending the warning for an unrecognized non-empty stream, return "", warnings so the function does not coerce custom/non-RHEL streams to "9".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/monitortestlibrary/platformidentification/types.go`:
- Around line 188-197: The GET of the worker MCP
(mcClient.MachineconfigurationV1().MachineConfigPools().Get) currently swallows
errors; instead, when err != nil capture and surface it by appending a
descriptive error to the existing warnings slice (e.g. fmt.Errorf("failed to get
worker MCP: %w", err)) so callers see API/RBAC/transport issues; keep the
existing mcp handling when err == nil (using mcp.Spec.OSImageStream.Name and
strings.CutPrefix) and only append the new warning when the Get fails.
---
Duplicate comments:
In `@pkg/monitortestlibrary/platformidentification/types.go`:
- Around line 194-216: The code currently appends a warning for non-empty,
unrecognized stream names (osStreamName and defaultStream) but then falls
through to return "9"; change this so that when a non-empty stream value fails
the rhel-* check you return an empty string (or a sentinel indicating unknown)
together with the accumulated warnings immediately instead of defaulting to "9".
Specifically, in the branches that check osStreamName and defaultStream (the
blocks referencing osStreamName and defaultStream variables and the
strings.CutPrefix checks), after appending the warning for an unrecognized
non-empty stream, return "", warnings so the function does not coerce
custom/non-RHEL streams to "9".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2d33c18-1aad-46fa-a14d-8d9bc794624f
📒 Files selected for processing (1)
pkg/monitortestlibrary/platformidentification/types.go
Populate OS major version from the worker MachineConfigPool's OSImageStream, falling back to the cluster-wide default stream from the OSImageStream v1alpha1 singleton when the MCP does not specify one. Defaults to "9" with a warning when no stream information is available (e.g. on MicroShift or when the OSStreams feature gate is not enabled). This enables downstream consumers (Sippy, risk analysis) to correlate test results with the node OS version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
petr-muller
force-pushed
the
trt-2506-osversion-platformidentification
branch
from
88e8f82 to
938e28e
Compare
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn-rhcos10-techpreview-serial |
|
@petr-muller: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/354ce780-17e8-11f1-8bef-f8eb82678be9-0 |
|
@petr-muller: This pull request references TRT-2506 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/monitortestlibrary/platformidentification/types.go`:
- Around line 191-192: The code currently returns the entire suffix from
strings.CutPrefix(osStreamName, "rhel-") (e.g., "9.4" or "9-custom") for
MajorVersion; instead extract and validate only the numeric major component
before returning: split the suffix at the first non-digit/at the '.' (or use
strconv.Atoi on the leading token), ensure it parses as an integer, and return
that numeric string (or an error) instead of the full suffix. Apply the same
change to the other occurrence that also uses strings.CutPrefix for "rhel-" so
both return a validated numeric major version.
- Around line 217-218: The code currently appends a warning when falling back to
the default OS major version "9" which turns an expected fallback into an error;
remove the warnings = append(...) call for the default path so defaulting to "9"
does not produce a warning. Instead only append to warnings when an
OSImageStream or MCP value was present but could not be parsed (i.e., preserve
warnings on actual parse errors), updating the logic in the function that
computes/returns the OS major version (the block that currently does warnings =
append(warnings, fmt.Errorf(...)) and return "9", warnings) to simply return
"9", warnings without emitting a warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa93e0f1-d0cd-4b2a-ae8b-1726a3e30d3b
📒 Files selected for processing (1)
pkg/monitortestlibrary/platformidentification/types.go
| if majorVersion, ok := strings.CutPrefix(osStreamName, "rhel-"); ok { | ||
| return majorVersion, nil |
There was a problem hiding this comment.
Normalize to an actual major version before returning.
Lines [191]-[192] and [206]-[207] return the full suffix after rhel-, which can include non-major content (for example, 9.4 or 9-custom). Since this field is MajorVersion, parse and validate just the numeric major component.
🔧 Proposed fix
import (
"context"
"errors"
"fmt"
+ "strconv"
"strings"
@@
func getOSMajorVersion(ctx context.Context, mcClient machineconfigclient.Interface) (string, []error) {
@@
if err == nil {
osStreamName := mcp.Spec.OSImageStream.Name
- if majorVersion, ok := strings.CutPrefix(osStreamName, "rhel-"); ok {
- return majorVersion, nil
+ if majorVersion, ok := parseRHELMajorVersion(osStreamName); ok {
+ return majorVersion, nil
}
if osStreamName != "" {
warnings = append(warnings, fmt.Errorf("unrecognized worker MCP OSImageStream name %q, does not match expected rhel-* pattern", osStreamName))
}
@@
if err == nil {
defaultStream := osImageStream.Status.DefaultStream
- if majorVersion, ok := strings.CutPrefix(defaultStream, "rhel-"); ok {
+ if majorVersion, ok := parseRHELMajorVersion(defaultStream); ok {
return majorVersion, warnings
}
if defaultStream != "" {
warnings = append(warnings, fmt.Errorf("unrecognized OSImageStream default stream %q, does not match expected rhel-* pattern", defaultStream))
}
@@
}
+
+func parseRHELMajorVersion(stream string) (string, bool) {
+ suffix, ok := strings.CutPrefix(stream, "rhel-")
+ if !ok || suffix == "" {
+ return "", false
+ }
+ major := strings.SplitN(suffix, ".", 2)[0]
+ if _, err := strconv.Atoi(major); err != nil {
+ return "", false
+ }
+ return major, true
+}Also applies to: 206-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/monitortestlibrary/platformidentification/types.go` around lines 191 -
192, The code currently returns the entire suffix from
strings.CutPrefix(osStreamName, "rhel-") (e.g., "9.4" or "9-custom") for
MajorVersion; instead extract and validate only the numeric major component
before returning: split the suffix at the first non-digit/at the '.' (or use
strconv.Atoi on the leading token), ensure it parses as an integer, and return
that numeric string (or an error) instead of the full suffix. Apply the same
change to the other occurrence that also uses strings.CutPrefix for "rhel-" so
both return a validated numeric major version.
| warnings = append(warnings, fmt.Errorf("could not determine OS major version from MCP or OSImageStream, defaulting to 9")) | ||
| return "9", warnings |
There was a problem hiding this comment.
Do not emit a warning for the expected default-to-9 path.
Line [217] turns a normal fallback (unset stream) into an error signal. That makes BuildClusterData return non-nil errors for healthy clusters where defaulting to "9" is intended behavior.
🔧 Proposed fix
- // No stream information available from MCP or cluster default, assuming RHEL 9
- warnings = append(warnings, fmt.Errorf("could not determine OS major version from MCP or OSImageStream, defaulting to 9"))
- return "9", warnings
+ // No stream information available from MCP or cluster default, assuming RHEL 9
+ // This is expected when no explicit stream is configured.
+ return "9", warnings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/monitortestlibrary/platformidentification/types.go` around lines 217 -
218, The code currently appends a warning when falling back to the default OS
major version "9" which turns an expected fallback into an error; remove the
warnings = append(...) call for the default path so defaulting to "9" does not
produce a warning. Instead only append to warnings when an OSImageStream or MCP
value was present but could not be parsed (i.e., preserve warnings on actual
parse errors), updating the logic in the function that computes/returns the OS
major version (the block that currently does warnings = append(warnings,
fmt.Errorf(...)) and return "9", warnings) to simply return "9", warnings
without emitting a warning.
|
Scheduling required tests: |
|
@petr-muller: The following test 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. |
2 participants
Populate OS major version from the worker MachineConfigPool's OSImageStream, defaulting to "9" when unset. This enables downstream consumers (Sippy, risk analysis) to correlate test results with the node OS version.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Chores