[WIP] MCO-1669: add BootImageSkewEnforcement API #2357
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
| name: "MachineConfiguration" | ||
| crdName: machineconfigurations.operator.openshift.io | ||
| featureGates: | ||
| - BootImageSkewEnforcement | ||
| tests: | ||
| onCreate: | ||
| - name: Should be able to create a minimal MachineConfiguration | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: {} # No spec is required for a MachineConfiguration | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| - name: Should be able to create an manual BootImageSkewEnforcement configuration knob | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Manual | ||
| clusterBootImage: | ||
| ocpVersion: "4.18.2" | ||
| rhcosVersion: "9.6.20250523-1" | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| bootImageSkewEnforcement: | ||
| mode: Manual | ||
| clusterBootImage: | ||
| ocpVersion: "4.18.2" | ||
| rhcosVersion: "9.6.20250523-1" | ||
| - name: Should be able to create an automatic BootImageSkewEnforcement configuration knob | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Automatic | ||
| clusterBootImage: | ||
| ocpVersion: "4.18.2" | ||
| rhcosVersion: "9.6.20250523-1" | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| bootImageSkewEnforcement: | ||
| mode: Automatic | ||
| clusterBootImage: | ||
| ocpVersion: "4.18.2" | ||
| rhcosVersion: "9.6.20250523-1" | ||
| - name: Should be able to create a disabled BootImageSkewEnforcement configuration knob | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Disabled | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| bootImageSkewEnforcement: | ||
| mode: Disabled | ||
| - name: Should not be able to add ClusterBootImage field if bootImageSkewEnforcement.mode is set to Disabled | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Disabled | ||
| clusterBootImage: | ||
| ocpVersion: "4.18.2" | ||
| rhcosVersion: "9.6.20250523-1" | ||
| expectedError: "clusterBootImage is required when type is Automatic or Manual, and forbidden otherwise" | ||
| - name: ClusterBootImage field should be set if bootImageSkewEnforcement.mode is set to Automatic | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Automatic | ||
| expectedError: "clusterBootImage is required when type is Automatic or Manual, and forbidden otherwise" | ||
| - name: ClusterBootImage field should be set if bootImageSkewEnforcement.mode is set to Manual | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: MachineConfiguration | ||
| spec: | ||
| bootImageSkewEnforcement: | ||
| mode: Manual | ||
| expectedError: "clusterBootImage is required when type is Automatic or Manual, and forbidden otherwise" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -56,8 +56,66 @@ type MachineConfigurationSpec struct { | |||||||||||||||||||||||||||||
| // +openshift:enable:FeatureGate=NodeDisruptionPolicy | ||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||
| NodeDisruptionPolicy NodeDisruptionPolicyConfig `json:"nodeDisruptionPolicy"` | ||||||||||||||||||||||||||||||
| // bootImageSkewEnforcement allows an admin to set the behavior of the boot image skew enforcement mechanism. | ||||||||||||||||||||||||||||||
| // +openshift:enable:FeatureGate=BootImageSkewEnforcement | ||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||
| BootImageSkewEnforcement SkewEnforcementSelector `json:"bootImageSkewEnforcement"` | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="has(self.mode) && (self.mode == 'Automatic' || self.mode =='Manual') ? has(self.clusterBootImage) : !has(self.clusterBootImage)",message="clusterBootImage is required when type is Automatic or Manual, and forbidden otherwise" | ||||||||||||||||||||||||||||||
| // +union | ||||||||||||||||||||||||||||||
| type SkewEnforcementSelector struct { | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Nit: Being more explicit in the type name helps eliminate potential for future conflicts in naming of similar features within the same package. Also makes it a bit more clear from the dev perspective that this type is explicitly used for boot image skew enforcement.
Suggested change
Suggested change
|
||||||||||||||||||||||||||||||
| // mode determines the underlying behavior of skew enforcement mechanism. | ||||||||||||||||||||||||||||||
| // Valid values are Automatic, Manual and Disabled. | ||||||||||||||||||||||||||||||
| // Automatic means that the MCO will store the OCP version associated with the last boot image update in the | ||||||||||||||||||||||||||||||
| // clusterBootImage field. | ||||||||||||||||||||||||||||||
| // Manual means that the cluster admin is expected to perform manual boot image updates and store OCP version | ||||||||||||||||||||||||||||||
| // associated with the last boot image update in the clusterBootImage field. | ||||||||||||||||||||||||||||||
| // In Automatic and Manual mode, the MCO will prevent upgrades when the boot image skew exceeds the | ||||||||||||||||||||||||||||||
| // skew limit described by the release image. | ||||||||||||||||||||||||||||||
| // Disabled means that the MCO will permit upgrades when the boot image exceeds the skew limit | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Nit: In general we try to avoid using the term |
||||||||||||||||||||||||||||||
| // described by the release image. This may affect the cluster's ability to scale. | ||||||||||||||||||||||||||||||
| // +unionDiscriminator | ||||||||||||||||||||||||||||||
| // +required | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Explicitly mention in the GoDoc that this field is required |
||||||||||||||||||||||||||||||
| Mode SkewEnforcementSelectorMode `json:"mode"` | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // clusterBootImage describes the current boot image of the cluster. This will be used to enforce the skew limit. | ||||||||||||||||||||||||||||||
| // Only permitted when mode is set to "Automatic" or "Manual". | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Match the CEL validation error message:
Suggested change
|
||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||
| ClusterBootImage ClusterBootImage `json:"clusterBootImage,omitempty"` | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. I believe this falls into the criteria where this should be a pointer:
Suggested change
I suspect you don't want something like: clusterBootImage:
ocpVersion: ""to be valid |
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // ClusterBootImage describes the boot image of a cluster. It stores the RHCOS version of the boot image and | ||||||||||||||||||||||||||||||
| // the OCP release version which shipped with that RHCOS boot image. | ||||||||||||||||||||||||||||||
| type ClusterBootImage struct { | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. What is the relationship between |
||||||||||||||||||||||||||||||
| // ocpVersion provides a string which represents the OCP version of the boot image | ||||||||||||||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[0-9]+\\\\.[0-9]+\\\\.[0-9]+$')",message="bootImageOCPVersion must match the OCP semver compatible format of x.y.z" | ||||||||||||||||||||||||||||||
| // +kubebuilder:validation:MaxLength:=8 | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Curious, why was 8 chosen? |
||||||||||||||||||||||||||||||
| // +required | ||||||||||||||||||||||||||||||
|
Comment on lines
+92
to
+94
There was a problem hiding this comment. Explicitly mention these constraints, in plain english sentences, in the GoDoc. Users don't get to see the markers in the generated documentation so spelling them out helps make it so they are aware of any constraints. |
||||||||||||||||||||||||||||||
| OCPVersion string `json:"ocpVersion"` | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Are you wanting to allow the empty string If you do, what does this being an empty string mean? |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // rhcosVersion provides a string which represents the RHCOS version of the boot image | ||||||||||||||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[0-9]+\\\\.[0-9]+\\\\.[0-9]{8}-[0-9]+$')",message="rhcosVersion must match format [major].[minor].[datestamp(YYYYMMDD)]-[buildnumber]" | ||||||||||||||||||||||||||||||
| // +kubebuilder:validation:MaxLength:=15 | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Why 15? is this long enough? Looking at https://access.redhat.com/solutions/3787021 it looks like something like |
||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||
|
Comment on lines
+98
to
+100
There was a problem hiding this comment. Explicitly mention these constraints in the godoc. |
||||||||||||||||||||||||||||||
| RHCOSVersion string `json:"rhcosVersion,omitempty"` | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // SkewEnforcementSelectorMode is a string enum used to indicate the cluster's boot image skew enforcement mode. | ||||||||||||||||||||||||||||||
| // +kubebuilder:validation:Enum:="Automatic";"Manual";"Disabled" | ||||||||||||||||||||||||||||||
| type SkewEnforcementSelectorMode string | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||
| // Automatic represents a configuration mode that allows automatic skew enforcement. | ||||||||||||||||||||||||||||||
| Automatic SkewEnforcementSelectorMode = "Automatic" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Manual represents a configuration mode that allows manual skew enforcement. | ||||||||||||||||||||||||||||||
| Manual SkewEnforcementSelectorMode = "Manual" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Disabled represents a configuration mode that disables boot image skew enforcement. | ||||||||||||||||||||||||||||||
| Disabled SkewEnforcementSelectorMode = "Disabled" | ||||||||||||||||||||||||||||||
|
Comment on lines
+110
to
+116
There was a problem hiding this comment. Prevent naming conflicts and maintain consistency with how we define constants within OpenShift by following the format
Suggested change
|
||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type MachineConfigurationStatus struct { | ||||||||||||||||||||||||||||||
| // observedGeneration is the last generation change you've dealt with | ||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why does an admin care about configuring this? What does configuring this allow them to achieve?