Reapply "MCO-2120: Rework OS Image Stream label classification"#5767
Reapply "MCO-2120: Rework OS Image Stream label classification"#5767cheesesashimi wants to merge 1 commit intoopenshift:mainfrom
Conversation
This reverts commit 59b22f2.
openshift-ci-robot
added
the
jira/valid-reference
|
@cheesesashimi: This pull request references MCO-2120 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. |
WalkthroughRefactors the OS image stream classification logic from a single hard-coded discriminator to a generalized matcher-based approach using a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.11.3)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
openshift-ci
bot
added
the
approved
![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/osimagestream/image_data.go`:
- Around line 93-99: The switch currently checks matchesAny(labels,
e.osImageMatchers) before matchesAny(labels, e.extensionsImageMatchers), which
misclassifies images that have both OS and extensions labels; update the
ordering to check matchesAny(labels, e.extensionsImageMatchers) first so images
with extension markers are classified as ImageTypeExtensions, and ensure
ImageTypeOS remains fallback. Modify the logic in the same block referencing
e.osImageMatchers, e.extensionsImageMatchers, ImageTypeOS, and
ImageTypeExtensions to reflect this ordering change, and add a regression test
exercising GroupOSContainerImageMetadataToStream() with a single image that
carries both the extensions label and an OS label to assert it becomes an
extensions image (and does not produce duplicate OS images).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4307924-3beb-4eb8-8eb2-cec120de98a5
📒 Files selected for processing (5)
pkg/osimagestream/image_data.gopkg/osimagestream/image_data_test.gopkg/osimagestream/imagestream_source_test.gopkg/osimagestream/osimagestream_test.gopkg/osimagestream/urls_source_test.go
| switch { | ||
| case matchesAny(labels, e.osImageMatchers): | ||
| imageData.Type = ImageTypeOS | ||
| } else { | ||
| case matchesAny(labels, e.extensionsImageMatchers): | ||
| imageData.Type = ImageTypeExtensions | ||
| default: | ||
| return nil |
There was a problem hiding this comment.
Check extensions before OS matchers.
If an extensions image also carries containers.bootc, Line 94 classifies it as OS before Line 96 sees the extensions marker. GroupOSContainerImageMetadataToStream() then ends up with two OS images and drops the stream as incomplete. Please also add a regression case with both labels present.
Possible fix
switch {
- case matchesAny(labels, e.osImageMatchers):
- imageData.Type = ImageTypeOS
case matchesAny(labels, e.extensionsImageMatchers):
imageData.Type = ImageTypeExtensions
+ case matchesAny(labels, e.osImageMatchers):
+ imageData.Type = ImageTypeOS
default:
return nil
}📝 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.
| switch { | |
| case matchesAny(labels, e.osImageMatchers): | |
| imageData.Type = ImageTypeOS | |
| } else { | |
| case matchesAny(labels, e.extensionsImageMatchers): | |
| imageData.Type = ImageTypeExtensions | |
| default: | |
| return nil | |
| switch { | |
| case matchesAny(labels, e.extensionsImageMatchers): | |
| imageData.Type = ImageTypeExtensions | |
| case matchesAny(labels, e.osImageMatchers): | |
| imageData.Type = ImageTypeOS | |
| default: | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/osimagestream/image_data.go` around lines 93 - 99, The switch currently
checks matchesAny(labels, e.osImageMatchers) before matchesAny(labels,
e.extensionsImageMatchers), which misclassifies images that have both OS and
extensions labels; update the ordering to check matchesAny(labels,
e.extensionsImageMatchers) first so images with extension markers are classified
as ImageTypeExtensions, and ensure ImageTypeOS remains fallback. Modify the
logic in the same block referencing e.osImageMatchers,
e.extensionsImageMatchers, ImageTypeOS, and ImageTypeExtensions to reflect this
ordering change, and add a regression test exercising
GroupOSContainerImageMetadataToStream() with a single image that carries both
the extensions label and an OS label to assert it becomes an extensions image
(and does not produce duplicate OS images).
There was a problem hiding this comment.
then ends up with two OS images and drops the stream as incomplete
And that's exactly what we want for a malformed image set. This is fine.
There was a problem hiding this comment.
@pablintino, understood — an image carrying both containers.bootc and io.openshift.os.extensions is considered malformed, and letting it be classified as OS (resulting in two OS images and the stream being silently dropped by GroupOSContainerImageMetadataToStream) is the intentional handling. I'll retract my concern.
(^･o･^)ﾉ"
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
/retest-required |
1 similar comment
|
/retest-required |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, pablintino 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 |
|
Pre-merge tested: Environment Variable: Steps:
$ oc get osimagestream cluster -o json | jq '.status.availableStreams'
[
{
"name": "rhel-9",
"osExtensionsImage": "registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:35b261673caa1bc92861d3f85ab549fe5e175246646aaca9baddbccd6281676d",
"osImage": "registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:fe9b41b68fa15547b45123785f14ca88828c8442a9882334b34da13f0b14c360"
},
{
"name": "rhel-10",
"osExtensionsImage": "registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:8aadae660efea658d5b14c5a1d381983ab882ee5126227291f6835e4c089f753",
"osImage": "registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:4740daf76d0919be012ec8c26a496230bc5e455fa2a63bbd692c670b8b1e4745"
}
]
$ oc image info --filter-by-os linux/amd64 -o json \
"registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:fe9b41b68fa15547b45123785f14ca88828c8442a9882334b34da13f0b14c360" \
2>/dev/null | jq '.config.config.Labels | {
"containers.bootc": .["containers.bootc"],
"ostree.linux": .["ostree.linux"],
"io.openshift.os.streamclass": .["io.openshift.os.streamclass"],
"io.openshift.os.extensions": .["io.openshift.os.extensions"]
}'
{
"containers.bootc": "1",
"ostree.linux": "true",
"io.openshift.os.streamclass": "rhel-9",
"io.openshift.os.extensions": null
}
$ oc image info --filter-by-os linux/amd64 -o json \
"registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:35b261673caa1bc92861d3f85ab549fe5e175246646aaca9baddbccd6281676d" \
2>/dev/null | jq '.config.config.Labels | {
"containers.bootc": .["containers.bootc"],
"ostree.linux": .["ostree.linux"],
"io.openshift.os.streamclass": .["io.openshift.os.streamclass"],
"io.openshift.os.extensions": .["io.openshift.os.extensions"]
}'
{
"containers.bootc": null,
"ostree.linux": null,
"io.openshift.os.streamclass": "rhel-9",
"io.openshift.os.extensions": "true"
}
$ oc image info --filter-by-os linux/amd64 -o json \
"registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:4740daf76d0919be012ec8c26a496230bc5e455fa2a63bbd692c670b8b1e4745" \
2>/dev/null | jq '.config.config.Labels | {
"containers.bootc": .["containers.bootc"],
"ostree.linux": .["ostree.linux"],
"io.openshift.os.streamclass": .["io.openshift.os.streamclass"],
"io.openshift.os.extensions": .["io.openshift.os.extensions"]
}'
{
"containers.bootc": "1",
"ostree.linux": "true",
"io.openshift.os.streamclass": "rhel-10",
"io.openshift.os.extensions": null
}
$ oc image info --filter-by-os linux/amd64 -o json \
"registry.build11.ci.openshift.org/ci-ln-5ytmki2/stable@sha256:8aadae660efea658d5b14c5a1d381983ab882ee5126227291f6835e4c089f753" \
2>/dev/null | jq '.config.config.Labels | {
"containers.bootc": .["containers.bootc"],
"ostree.linux": .["ostree.linux"],
"io.openshift.os.streamclass": .["io.openshift.os.streamclass"],
"io.openshift.os.extensions": .["io.openshift.os.extensions"]
}'
{
"containers.bootc": null,
"ostree.linux": null,
"io.openshift.os.streamclass": "rhel-10",
"io.openshift.os.extensions": "true"
} |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@cheesesashimi: This pull request references MCO-2120 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. |
openshift-ci-robot
added
the
verified
|
@ptalgulk01: 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. |
|
/test e2e-aws-ovn-upgrade |
|
@cheesesashimi: 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. |
|
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. |
openshift-ci
bot
added
the
needs-rebase
4 participants
This reverts commit 59b22f2 and re-adds the reworked image label detection for OSImageStreams.