MCO-1927: Add osImageStream to MCPs #2555
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-1927 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. In 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. |
|
Hello @pablintino! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/api-review |
pablintino
force-pushed
the
mco-osimagestreams
branch
2 times, most recently
from
556d5fe to
cbe1738
Compare
|
/api-review |
pablintino
force-pushed
the
mco-osimagestreams
branch
2 times, most recently
from
7009b39 to
8cc97f2
Compare
There was a problem hiding this comment.
Hmm, I don't think we've hooked up an /api-review command via github automation. Here's claude's suggestion:
Critical Issues Found
File: machineconfiguration/v1alpha1/types_osimagestream.go
Issue 1: Missing godoc comment for embedded ObjectMeta
Line 25: Missing godoc comment for metav1.ObjectMeta field
Current (problematic) code:
type OSImageStream struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
// spec contains the desired OSImageStream config configuration.
Suggested change:
type OSImageStream struct {
metav1.TypeMeta `json:",inline"`
+
+ // metadata is the standard object's metadata.
+ // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
metav1.ObjectMeta `json:"metadata,omitempty"`
Explanation: All struct fields, including embedded types like metav1.ObjectMeta, must have godoc comments explaining their purpose.
---
Issue 2: Invalid pointer usage for Spec field
Line 29: Field Spec should be a pointer since it has a valid zero value but incomplete validation
Current (problematic) code:
// spec contains the desired OSImageStream config configuration.
// +required
Spec OSImageStreamSpec `json:"spec"`
Suggested change:
// spec contains the desired OSImageStream config configuration.
+ // When not present, the OSImageStream has no desired configuration.
// +required
- Spec OSImageStreamSpec `json:"spec"`
+ Spec *OSImageStreamSpec `json:"spec"`
Explanation: The kubeapilinter requires that fields with valid zero values ({}) either have complete validation (e.g., min properties, required fields) or use a
pointer type. Since OSImageStreamSpec is currently empty, it should be a pointer to distinguish between "not set" and "empty struct". Alternatively, if the spec
should never be empty, add validation constraints or required fields to OSImageStreamSpec.
---
Issue 3: Invalid pointer usage for Status field
Line 35: Field Status should be a pointer since it has a valid zero value but incomplete validation
Current (problematic) code:
// status describes the last observed state of this OSImageStream.
// Populated by the MachineConfigOperator after reading release metadata.
// When not present, the controller has not yet reconciled this resource.
// +optional
Status OSImageStreamStatus `json:"status"`
Suggested change:
// status describes the last observed state of this OSImageStream.
// Populated by the MachineConfigOperator after reading release metadata.
// When not present, the controller has not yet reconciled this resource.
// +optional
- Status OSImageStreamStatus `json:"status"`
+ Status *OSImageStreamStatus `json:"status"`
Explanation: Status fields with +optional marker and non-complete validation should be pointers to properly distinguish between "status not yet populated" and
"status exists but is empty". This is particularly important since the comment already states "When not present, the controller has not yet reconciled this
resource."
---
Issue 4: Missing MaxItems validation for Conditions
Line 47: Field Conditions must have a maximum items validation
Current (problematic) code:
// conditions represents the latest available observations of current state.
// When empty, no conditions have been reported yet.
// +listType=map
// +listMapKey=type
// +optional
Conditions []metav1.Condition `json:"conditions,omitempty"`
Suggested change:
// conditions represents the latest available observations of current state.
// When empty, no conditions have been reported yet.
+ // A maximum of 100 conditions may be specified.
// +listType=map
// +listMapKey=type
// +optional
+ // +kubebuilder:validation:MaxItems=100
Conditions []metav1.Condition `json:"conditions,omitempty"`
Explanation: All slice/array fields must have a MaxItems constraint to prevent unbounded resource usage. The documentation must also mention this constraint. 100
is a common limit for conditions.
---
Issue 5: Missing omitempty tag for required field Name
Line 87: Field Name is marked as +required but lacks the omitempty tag
Current (problematic) code:
// name is the identifier of the stream.
//
// Must not be empty and must not exceed 70 characters in length.
// Must only contain alphanumeric characters, hyphens ('-'), or dots ('.').
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=70
// +kubebuilder:validation:XValidation:rule=`self.matches('^[\\w\\.\\-]+$')`,message="The name must consist only of alphanumeric characters, hyphens ('-') and dots
('.')."
Name string `json:"name"`
Suggested change:
// name is the identifier of the stream.
//
// Must not be empty and must not exceed 70 characters in length.
// Must only contain alphanumeric characters, hyphens ('-'), or dots ('.').
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=70
// +kubebuilder:validation:XValidation:rule=`self.matches('^[\\w\\.\\-]+$')`,message="The name must consist only of alphanumeric characters, hyphens ('-') and
dots ('.')."
- Name string `json:"name"`
+ Name string `json:"name,omitempty"`
Explanation: Even required fields should have the omitempty tag in OpenShift APIs. The validation markers will enforce that the field is present and non-empty.
Without omitempty, an empty string would still be marshaled in the JSON output.
---
Issue 6: Missing omitempty tag for required field OSImageUrl
Line 98: Field OSImageUrl is marked as +required but lacks the omitempty tag
Current (problematic) code:
// osImageUrl is an OS Image referenced by digest.
//
// The format of the URL ref is:
// host[:port][/namespace]/name@sha256:<digest>
// +requiredhttps://github.com/openshift/api/blob/master/operator/v1/types_olm.go#L22
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=447
// +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference
must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long"
// +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')
`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme"
OSImageUrl string `json:"osImageUrl"`
Suggested change:
// osImageUrl is an OS Image referenced by digest.
//
// The format of the URL ref is:
// host[:port][/namespace]/name@sha256:<digest>
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=447
// +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference
must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long"
// +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'
)`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme"
- OSImageUrl string `json:"osImageUrl"`
+ OSImageUrl string `json:"osImageUrl,omitempty"`
Explanation: Consistent with OpenShift API conventions, required fields should have the omitempty tag. The validation markers enforce presence and correctness.
---
Issue 7: Missing omitempty tag for required field OSExtensionsImageUrl
Line 109: Field OSExtensionsImageUrl is marked as +required but lacks the omitempty tag
Current (problematic) code:
// osExtensionsImageUrl is an OS Extensions Image referenced by digest.
//
// The format of the URL ref is:
// host[:port][/namespace]/name@sha256:<digest>
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=447
// +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference
must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long"
// +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')
`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme"
OSExtensionsImageUrl string `json:"osExtensionsImageUrl"`
Suggested change:
// osExtensionsImageUrl is an OS Extensions Image referenced by digest.
//
// The format of the URL ref is:
// host[:port][/namespace]/name@sha256:<digest>
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=447
// +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference
must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long"
// +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'
)`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme"
- OSExtensionsImageUrl string `json:"osExtensionsImageUrl"`
+ OSExtensionsImageUrl string `json:"osExtensionsImageUrl,omitempty"`
Explanation: Consistent with OpenShift API conventions, required fields should have the omitempty tag. The validation markers enforce presence and correctness.
---
Additional Observations (Documentation Quality)
File: machineconfiguration/v1/types.go
The osImageStream field in MachineConfigPoolSpec (machineconfiguration/v1/types.go:469) has excellent documentation that properly documents all validation markers:
✅ Optional behavior is clearly explained ("When omitted or empty, the pool uses the cluster-wide default OS images")
✅ MinLength and MaxLength constraints are documented ("maximum length of 70 characters")
✅ Pattern validation is documented ("must start with a letter and contain only alphanumeric characters, hyphens ('-'), and dots ('.')")
✅ XValidation rule matches the documented constraint
This serves as a good example of proper API documentation.
---
Validation Marker Documentation Check
File: machineconfiguration/v1alpha1/types_osimagestream.go
Most validation markers are properly documented, but there are some concerns:
OSImageStreamStatus (Line 39-40)
✅ GOOD: XValidation rules are documented in the field comments:
- Line 64: "Must reference the name of one of the streams in availableStreams"
- Line 64: "Must be set when availableStreams is not empty"
availableStreams (Line 55-59)
✅ GOOD: All validation markers are documented:
- MinItems documented: "A maximum of 100 streams may be specified" (note: this should say "minimum of 1, maximum of 100")
- MaxItems documented: "A maximum of 100 streams may be specified"
Minor documentation issue: The comment says "A maximum of 100 streams may be specified" but doesn't mention the MinItems=1 constraint.
Suggested improvement for line 52-53:
// availableStreams is a list of the available OS Image Streams
// available and their associated URLs for both OS and Extensions
// images.
//
- // A maximum of 100 streams may be specified.
+ // When not empty, at least 1 stream must be specified, with a maximum of 100 streams allowed.
// +optional
defaultStream (Line 68-70)
✅ GOOD: All validation markers are properly documented in the comment
Name, OSImageUrl, OSExtensionsImageUrl (Lines 84-109)
✅ GOOD: All validation constraints are documented in the field comments
---
Recommendation
Status: ❌ REVIEW FAILED - REQUIRES CHANGES
The PR must address all 7 linting issues before it can be approved:
1. Add godoc comment for metav1.ObjectMeta field
2. Change Spec OSImageStreamSpec to Spec *OSImageStreamSpec (or add complete validation)
3. Change Status OSImageStreamStatus to Status *OSImageStreamStatus
4. Add +kubebuilder:validation:MaxItems=100 to Conditions field
5. Add omitempty tag to Name field
6. Add omitempty tag to OSImageUrl field
7. Add omitempty tag to OSExtensionsImageUrl field
After fixing these issues, run make update-codegen-crds to regenerate the CRD manifests, then run make lint again to verify all issues are resolved.
machineconfiguration/v1/types.go
Outdated
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=70 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-zA-Z][a-zA-Z0-9.-]*$')`,message="The osImageStream must start with a letter and contain only alphanumeric characters, hyphens ('-'), and dots ('.')." | ||
| OSImageStream string `json:"osImageStream,omitempty"` |
There was a problem hiding this comment.
(non blocking thought): Thinking through our different architectures, the one place this wouldn't apply would be HCP/Hypershift, since they do not use machineconfigpool objects, nor do they have cluster scoped objects like osimagestream support generally.
We'd probably have to add a similar field in the NodePool API spec, and potentially have a separate, namespaceable object. Let's add that to the future considerations.
Also, perhaps we should add a status field for this as well to indicate whether the user-supplied OSImageStream has actually taken effect for the pool or not?
There was a problem hiding this comment.
@yuqi-zhang For the first point I've opened a spike to not loose track of the Hypershift needs.
About the second point. I agree, and I haven't thought about it till you mentioned it. About the idea of adding it to the pool: I find that piece of information in the pool status a bit misleading. What happens during a switch of the stream? Till the entire pool updates no value in that field will be valid, some nodes will have the image of X stream and others will still have the Y stream image. Shouldn't we place that information in the MCN?
There was a problem hiding this comment.
issue: Since this is going to refer to an OSImageStream object, let's be explicit that this is reference and do something like this:
type OSImageStreamReference struct {
Name string
}type MachineConfigPool struct {
// Fields on MachineConfigPool omitted for brevity
OSImageStreamRef *OSImageStreamReference
}This was something that I learned when we went through the MachineOSConfig / MachineOSBuild API reviews since we have numerous fields that refer to other objects.
There was a problem hiding this comment.
What happens during a switch of the stream? Till the entire pool updates no value in that field will be valid, some nodes will have the image of X stream and others will still have the Y stream image. Shouldn't we place that information in the MCN?
I was thinking of the spec.configuration vs status.configuration we have in the MCP already. The status will reflect the last updated for the entire pool, and the spec reflects the latest, and the conditions reflect if it's updating/degraded/etc., and we could just have the OSImageStreamRef match that. Then implicitly the conditions fields will also reflect an update in progress if spec and status don't match.
The per-MCN field is worth considering as well, but maybe we can do that as a follow up to evaluate for GA.
There was a problem hiding this comment.
Understood. I've added the field to the status section and switched it to a reference-like struct. BTW, based on recent API changes (and the linter errors I got while making this change), it seems that for optional fields like the reference, the preferred way of declaring it is no longer a pointer but using omitzero.
|
|
||
| // spec contains the desired OSImageStream config configuration. | ||
| // +required | ||
| Spec OSImageStreamSpec `json:"spec"` |
There was a problem hiding this comment.
Hmm, I don't think we need a spec if it's (planned to be) empty?
There was a problem hiding this comment.
I'm not 100% removing the spec is safe. I've searched for examples without spec and I wasn't able to locate one.
There was a problem hiding this comment.
There are objects without a spec, but those don't seem to follow the spec-status convention. The upstream docs https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status don't seem to explicitly call out what to do with objects that just reflect a state. Maybe something we should double check.
| // +kubebuilder:validation:MaxItems=100 | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| AvailableStreams []OSImageStreamURLSet `json:"availableStreams,omitempty"` |
There was a problem hiding this comment.
Perhaps we should enforce that this should never be empty? I don't think we'd ever have a situation where no OS comes with the cluster payload.
There was a problem hiding this comment.
Claude suggested me to make this one optional, as the status can be transiently nil, and thus, its fields. I wasn't too sure making it optional but it's true that all the CRs I found have all optional fields inside the status.
There was a problem hiding this comment.
Is the intent behind this to allow a single OSImageStream object to refer to more than one set of OS / extensions images? Would it be more ergonomic to have more than one OSImageStream objects that each refer to a single set of OS / extensions images.
In other words:
With single OSImageStream:
---
apiVersion: machineconfiguration.openshift.io/v1
kind: OSImageStream
metadata:
name: "os-image-stream"
status:
availableStreams:
- name: "rhcos-9.6"
osImageURL: "<rhcos 9.6 pullspec>"
osExtensionsImageURL: "<rhcos 9.6 extensions pullspec>"
- name: "rhcos-10.1"
osImageURL: "<rhcos 10.1 pullspec>"
osExtensionsImageURL: "<rhcos 10.1 extensions pullspec>"
defaultStream: "rhcos-10.1"With multiple:
---
apiVersion: machineconfiguration.openshift.io/v1
kind: OSImageStream
metadata:
name: "rhcos-9.6"
status:
availableStreams:
- name: "rhcos-9.6"
osImageURL: "<rhcos 9.6 pullspec>"
osExtensionsImageURL: "<rhcos 9.6 extensions pullspec>"
---
apiVersion: machineconfiguration.openshift.io/v1
kind: OSImageStream
metadata:
name: "rhcos-10.1"
status:
availableStreams:
- name: "rhcos-10.1"
osImageURL: "<rhcos 10.1 pullspec>"
osExtensionsImageURL: "<rhcos 10.1 extensions pullspec>"Oh wait: I just saw in the test suite that we intend for this to be a single object. I'll leave this comment here anyway just for consideration.
pablintino
force-pushed
the
mco-osimagestreams
branch
3 times, most recently
from
84a59ee to
80b1b58
Compare
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| OSImageUrl string `json:"osImageUrl,omitempty"` |
There was a problem hiding this comment.
issue: A Golang convention is to capitalize "URL" whenever it is in a variable name. So this should become OSImageURL. Granted, we don't explicitly follow a particular style-guide, but it may be worth looking at https://google.github.io/styleguide/go/decisions#initialisms
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| OSExtensionsImageUrl string `json:"osExtensionsImageUrl,omitempty"` |
There was a problem hiding this comment.
thought (non-blocking): Would it make sense for us to put the coreos-bootimages ConfigMap content in here as well? We could include it the same way that we include Ignition content in MachineConfigs by making the field type a runtime.RawExtension.
pablintino
force-pushed
the
mco-osimagestreams
branch
from
80b1b58 to
1d142f0
Compare
Introduces OSImageStream as a catalog resource that enables discovery of available OS image streams for MachineConfigPools. The resource is populated by the MachineConfigOperator from release metadata and provides OS and OS Extensions image URLs referenced by digest. The change adds the the osImageStream field to MachineConfigPoolSpec, allowing individual pools to override cluster-wide OS images with a specific OS stream. The field is optional and gated behind the OSStreams feature gate. When omitted, pools use the cluster-wide default OS images.
pablintino
force-pushed
the
mco-osimagestreams
branch
from
1d142f0 to
af05ca1
Compare
|
@pablintino: The following test failed, say
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. |
|
|
||
| // spec contains the desired OSImageStream config configuration. | ||
| // +required | ||
| Spec *OSImageStreamSpec `json:"spec,omitempty"` |
There was a problem hiding this comment.
Was trying to understand if this is ok still. https://github.com/openshift/api/blob/master/.golangci.yaml#L37-L40 seems to imply that while the combo is valid, this should only be the case if the OSImageStreamSpec has any user input to fill in. Since the type is an empty struct, I believe it should still be removed, but will let Joel or Bryce take a look when they get to reviewing this.
| // alphanumeric characters, hyphens and periods, and should start and end with an alphanumeric character. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength:=1 |
There was a problem hiding this comment.
I think this line and the following line has an extra :
(i.e. the syntax is +kubebuilder:validation:MinLength=1)
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
5 participants
Adds the osImageStream field to MachineConfigPoolSpec, allowing individual pools to override cluster-wide OS images with a specific OS stream.
The field is optional and gated behind the OSStreams feature gate. When omitted, pools use the cluster-wide default OS images.