-
Notifications
You must be signed in to change notification settings - Fork 462
OCPBUGS-72553: CPMS boot image config should not override standard MachineSet configuration #5539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-72553: CPMS boot image config should not override standard MachineSet configuration #5539
Conversation
|
Skipping CI for Draft Pull Request. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-72553, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-72553, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
a7e5255 to
e5f2448
Compare
|
/cherry-pick release-4.21 |
|
@djoshy: once the present PR merges, I will cherry-pick it on top of 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. |
e5f2448 to
30911a2
Compare
2d37444 to
72730ee
Compare
| // Returns if platform supports updating boot images on machinesets, control-plane machinesets and if updates are enabled by default | ||
| func CheckBootImagePlatform(infra *configv1.Infrastructure, fgHandler FeatureGatesHandler) (bool, bool, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (non-blocking, not a strongly held opinion): I think the comment helps clarify the intended returns here, but simply returning three bools feels a touch ambiguous to me. What do you think of the idea of having named returns here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's valid, thanks for calling that out! I added some returns and asked claude to come up with a nicer comment above 😄
| // A default opt-in can happen in two ways: | ||
| // - Cluster is being installed on a release that opts the cluster in (ManagedBootImagesStatus is not defined) | ||
| // - Cluster is opted out and is being upgraded to a release that opts the cluster in (MachineSet manager has mode=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(praise): Thank you for adding these scenarios for context!
72730ee to
bdf935f
Compare
|
Changes LGTM! |
isabella-janssen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thank you for updating the returns & comment!
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-azure-mco-disruptive-techpreview-1of2 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-azure-mco-disruptive-techpreview-2of2 |
|
@djoshy: trigger 2 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/7af72780-f0da-11f0-9876-ab3f7c17da2d-0 |
|
/payload-abort |
|
D'oh, looks like I forgot to consider the partial machine manager mode, I'll have a patch up for that shortly. It broke some Azure runs in my skew e2e testing 😞 |
bdf935f to
d9b773d
Compare
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-azure-mco-disruptive-techpreview-1of2 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-azure-mco-disruptive-techpreview-2of2 I shifted the merge function one level up, so it includes the additional fields required by the partial mode while combining the machine managers. Hopefully that fixes it - I'm running the payloads above to confirm. |
|
@djoshy: trigger 2 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/dc191880-f0e3-11f0-865d-0a6427830cc1-0 |
|
@djoshy: trigger 2 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/e5707400-f0e3-11f0-9855-ee74ccba94e6-0 |
d9b773d to
2af6ed1
Compare
|
Fixed a minor verify issue in the last push, payloads look good 😄 |
isabella-janssen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, isabella-janssen 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 |
|
A new test case was created to check this scenario https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-87023 The test case was executed using this PR and passed in GCP (techpreview) and AWS (techpreview). /verified by @sergiordlr |
|
@sergiordlr: 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 all |
|
/override ci/prow/e2e-gcp-op-2of2 known failure being worked on in #5555 |
|
@djoshy: Overrode contexts on behalf of djoshy: ci/prow/e2e-gcp-op-2of2 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. |
|
@djoshy: 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. |
c8a6c4f
into
openshift:main
|
@djoshy: Jira Issue Verification Checks: Jira Issue OCPBUGS-72553 Jira Issue OCPBUGS-72553 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. |
|
@djoshy: new pull request created: #5559 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-01-19-085729 |
- What I did
This PR refactors how the boot image configuration status is populated so that the admin opinion for machine manager A does not override the platform default for machine manager B. It also updates the unit tests to handle these new scenarios.
- How to verify it
This PR can be verified by tweaking the boot image configuration and observing the
MachineConfigurationstatus object. Note that the default CPMS opinion is disabled, while the default for standard MachineSets are platform dependent. Note that this feature is still under techpreview, so you will need a cluster launched with TP enabled.Here are some examples:
On a platform that supports boot image updates by default(GCP, AWS), with no admin opinion:
On a platform that supports boot image updates but needs opt-in(Azure):
On a platform that supports boot image updates, but with an admin opt-out opinion for standard machinesets:
On a platform that supports boot image updates by default(GCP,AWS), but with an admin opt-in opinion for controlplanemachinesets:
On a platform that supports boot image updates and needs opt-in(Azure), but with an admin opt-in opinion for controlplanemachinesets:
On a platform that supports boot image updates for standard machinesets but not control plane machinesets(vsphere) (note that if #5540 has landed, this test case would not be possible):
That should cover most cases, but I'm sure there's a ton more combinations we could test for 😄