MCO-1972: Removes OSImageURLConfig from the build controller#5424
MCO-1972: Removes OSImageURLConfig from the build controller#5424cheesesashimi wants to merge 6 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test unit verify e2e-gcp-op-ocl |
| BaseOSContainerImage: m.MachineConfig.Spec.OSImageURL, | ||
| BaseOSExtensionsContainerImage: m.MachineConfig.Spec.BaseOSExtensionsContainerImage, | ||
| // This value is purposely left empty because the ConfigMap does not actually | ||
| // populate this value. However, we want the hashing to be stable. |
There was a problem hiding this comment.
Note to reviewer: This might be a moot point if someone is upgrading from one OCP release to another, the hashes will change. However, that means that old images may get rebuilt in the process, which is undesirable.
cheesesashimi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@cheesesashimi: This pull request references MCO-1972 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.21.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
bot
added
the
approved
|
/test e2e-gcp-op-ocl |
cheesesashimi
force-pushed
the
zzlotnik/osimageurl-redux
branch
from
49a39f6 to
3ffaec0
Compare
|
/test unit verify e2e-gcp-op-ocl |
|
/test e2e-gcp-op-ocl |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test unit |
cheesesashimi
force-pushed
the
zzlotnik/osimageurl-redux
branch
from
adb5a66 to
b66544e
Compare
|
/retest-required |
|
Pre-merge verified: Environment Setup: Pre-requisites
Steps
|
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test verify |
cheesesashimi
force-pushed
the
zzlotnik/osimageurl-redux
branch
from
b66544e to
352592e
Compare
|
@cheesesashimi: This pull request references MCO-1972 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. |
|
/test e2e-gcp-op-techpreview |
|
@pablintino: Overrode contexts on behalf of pablintino: ci/prow/e2e-gcp-op-part2, ci/prow/e2e-hypershift 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. |
openshift-merge-robot
added
the
needs-rebase
|
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. |
WalkthroughThe changes refactor build request and MachineOSBuild construction to source OS image configuration from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Ensures that a cluster admin may only override the OSImageURL field or set the desired OSImageStream name; but not both. This ensures that either the cluster admin or the MCO will manage the OS image and prevents the MCO from overriding this setting.
cheesesashimi
force-pushed
the
zzlotnik/osimageurl-redux
branch
from
6303082 to
4b77c26
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
bot
removed
lgtm
|
/lgtm |
|
/verified by @ptalgulk01 |
openshift-ci-robot
added
the
verified
|
@djoshy: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, djoshy, 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e-techpreview/osimagestreamrender_test.go (2)
96-97: Consider capturing the return value from WaitForRenderedConfig.The returned rendered config name is discarded. While the test may not need it, capturing it could be useful for debugging if the test fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-techpreview/osimagestreamrender_test.go` around lines 96 - 97, The call to helpers.WaitForRenderedConfig currently discards its return value; change the call in osimagestreamrender_test.go to capture the returned rendered config name (e.g., renderedName := helpers.WaitForRenderedConfig(t, cs, poolName, "00-worker")) and then use that variable in subsequent debug output or t.Logf to aid failure diagnosis; keep the helper invocation and parameters the same but store and log the return from WaitForRenderedConfig for easier debugging.
27-29: Consider increasing test timeout for multiple subtests.The 5-minute context timeout applies to the entire test including all three recovery path subtests. Each subtest involves pool creation, degradation, and recovery operations. This timeout might be tight in slower CI environments.
💡 Suggested adjustment
func TestOSImageStreamOSImageURL(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), time.Minute*5) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute*10) t.Cleanup(cancel)Alternatively, consider creating a fresh context with timeout per subtest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-techpreview/osimagestreamrender_test.go` around lines 27 - 29, The test-wide 5-minute context in TestOSImageStreamOSImageURL may be too short for all three recovery-path subtests; either increase the top-level timeout (e.g., to 10+ minutes) by adjusting the context.WithTimeout call in TestOSImageStreamOSImageURL, or create a fresh per-subtest context.WithTimeout inside each t.Run (within the subtest closures that perform pool creation/degradation/recovery) so each subtest gets its own ample timeout and calls t.Cleanup(cancel).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e-techpreview/osimagestreamrender_test.go`:
- Around line 96-97: The call to helpers.WaitForRenderedConfig currently
discards its return value; change the call in osimagestreamrender_test.go to
capture the returned rendered config name (e.g., renderedName :=
helpers.WaitForRenderedConfig(t, cs, poolName, "00-worker")) and then use that
variable in subsequent debug output or t.Logf to aid failure diagnosis; keep the
helper invocation and parameters the same but store and log the return from
WaitForRenderedConfig for easier debugging.
- Around line 27-29: The test-wide 5-minute context in
TestOSImageStreamOSImageURL may be too short for all three recovery-path
subtests; either increase the top-level timeout (e.g., to 10+ minutes) by
adjusting the context.WithTimeout call in TestOSImageStreamOSImageURL, or create
a fresh per-subtest context.WithTimeout inside each t.Run (within the subtest
closures that perform pool creation/degradation/recovery) so each subtest gets
its own ample timeout and calls t.Cleanup(cancel).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa44a399-4222-4df0-84c5-8e9784cab920
📒 Files selected for processing (16)
pkg/controller/build/buildrequest/buildrequest.gopkg/controller/build/buildrequest/buildrequest_test.gopkg/controller/build/buildrequest/buildrequestopts.gopkg/controller/build/buildrequest/buildrequestopts_test.gopkg/controller/build/buildrequest/machineosbuild.gopkg/controller/build/buildrequest/machineosbuild_test.gopkg/controller/build/fixtures/objects.gopkg/controller/build/osbuildcontroller_test.gopkg/controller/build/reconciler.gopkg/controller/common/images.gopkg/controller/render/render_controller.gopkg/controller/render/render_controller_test.gotest/e2e-ocl-1of2/onclusterlayering_test.gotest/e2e-ocl-2of2/onclusterlayering_test.gotest/e2e-ocl/onclusterlayering_test.gotest/e2e-techpreview/osimagestreamrender_test.go
💤 Files with no reviewable changes (2)
- pkg/controller/build/buildrequest/buildrequestopts_test.go
- pkg/controller/common/images.go
|
/retest |
|
@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. |
openshift-ci
bot
added
the
needs-rebase
|
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. |
6 participants
- What I did
This decouples the Build Controller from
OSImageURLConfigand makes the OSImageURL and BaseOSExtensionsImage fields on the rendered MachineConfig the source of truth for the base OS and extensions images to use for Image-Mode OpenShift. The idea is that if a different OS image is selected on a per-pool basis (e.g., one is RHEL9 and one is RHEL10 for dual-streams), then the Build Controller should use the appropriate source of truth for the appropriate pool.However, if one also sets the OSImageStream name on the MachineConfigPool and also sets OSImageURL on a MachineConfig, the MCO should degrade in this state because it would override value provided by the cluster admin. This PR also includes an E2E test which verifies that this is the case. This new E2E test will not be automatically ran until openshift/release#75329 is merged.
- How to verify it
The best way to verify this is to create a cluster and then create a MachineConfig which overrides the OSImageURL value. The Build Controller should build a new OS image based upon the new OSImageURL value.
- Description for the changelog
MachineConfigs should be the source of truth for the Build Controller