Skip to content

OCPBUGS-54287: MachineOSConfig name should match MachineConfigPool #2399

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-54287: MachineOSConfig name should match MachineConfigPool #2399

wants to merge 1 commit into from

Conversation

umohnani8
Copy link
Contributor

@umohnani8 umohnani8 commented Jul 10, 2025 •
edited
Loading

Add a restriction where the name of the MachineOSConfig object must be the same as the MachineConfigPool it is created for. This will ensure that we only have one MOSC per MCP.

Fixes https://issues.redhat.com/browse/OCPBUGS-54287

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 10, 2025

Hello @umohnani8! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 10, 2025
@openshift-ci openshift-ci bot requested review from cgwalters and yuqi-zhang July 10, 2025 14:21
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: umohnani8
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@umohnani8
MachineOSConfig name should match MachineConfigPool
d231abe 
Add a restriction where the name of the MachineOSConfig
object must be the same as the MachineConfigPool it is
created for. This will ensure that we only have one MOSC
per MCP.

Signed-off-by: Urvashi <[email protected]>
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 10, 2025
@umohnani8 umohnani8 changed the title [WIP] MachineOSConfig name should match MachineConfigPool OCPBUGS-54287: MachineOSConfig name should match MachineConfigPool Jul 10, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2025
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 10, 2025
@openshift-ci-robot
Copy link

@umohnani8: This pull request references Jira Issue OCPBUGS-54287, which is invalid:

  • expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Add a restriction where the name of the MachineOSConfig object must be the same as the MachineConfigPool it is created for. This will ensure that we only have one MOSC per MCP.

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.

@umohnani8
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 10, 2025
@openshift-ci-robot
Copy link

@umohnani8: This pull request references Jira Issue OCPBUGS-54287, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.20.0) matches configured target version for branch (4.20.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

In response to this:

/jira refresh

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.

@umohnani8
Copy link
Contributor Author

@cheesesashimi @yuqi-zhang @JoelSpeed PTAL

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 10, 2025

@umohnani8: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/integration d231abe link true /test integration
ci/prow/e2e-aws-serial-1of2 d231abe link true /test e2e-aws-serial-1of2
ci/prow/e2e-aws-serial-2of2 d231abe link true /test e2e-aws-serial-2of2
ci/prow/e2e-upgrade-out-of-change d231abe link true /test e2e-upgrade-out-of-change
ci/prow/okd-scos-e2e-aws-ovn d231abe link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

JoelSpeed
JoelSpeed reviewed Jul 11, 2025
View reviewed changes
machineconfiguration/v1/types_machineosconfig.go
@@ -18,6 +18,7 @@ import (
// MachineOSConfig describes the configuration for a build process managed by the MCO
// Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
// +openshift:compatibility-gen:level=1
// +kubebuilder:validation:XValidation:rule="self.metadata.name == self.spec.machineConfigPool.name", message="MachineOSConfig name must match the referenced MachineConfigPool name; can only have one MachineOSConfig per MachineConfigPool"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible in any circumstance for either of these fields to be omitted? As that would then cause an error here? Perhaps adding a test that reports that spec is missing and is required will show what the full error message becomes in this case?

To make this safe you could

Suggested change
// +kubebuilder:validation:XValidation:rule="self.metadata.name == self.spec.machineConfigPool.name", message="MachineOSConfig name must match the referenced MachineConfigPool name; can only have one MachineOSConfig per MachineConfigPool"
// +kubebuilder:validation:XValidation:rule="self.?metadata.name.orValue(\"\") == self.?spec.machineConfigPool.name.orValue(\"\")", message="MachineOSConfig name must match the referenced MachineConfigPool name; can only have one MachineOSConfig per MachineConfigPool"

And then the error messages would be cleaner (assuming this does throw an error as I expect)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like spec.machineConfigPool.name is required all the way down the chain, but metadata is an optional field.

@JoelSpeed
Copy link
Contributor

Please add a selection of integration tests based on the readme in the tests/ folder in this repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@everettraven everettraven everettraven left review comments

@cgwalters cgwalters Awaiting requested review from cgwalters

@yuqi-zhang yuqi-zhang Awaiting requested review from yuqi-zhang

Assignees
No one assigned
Labels
jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@umohnani8 @openshift-ci-robot @JoelSpeed @everettraven