OCPNODE-3029: features: add requiredMinimumComponentVersion #2224

haircommander · 2025-03-10T18:41:57Z

this is an optional field that allows a feature author to declare the minimum version a kubelet on an openshift node can be to use said feature.

This PR also sets UserNamespace related features to set their requiredMinimumKubeletVersion to 1.30.0.

openshift-ci · 2025-03-10T18:42:04Z

Hello @haircommander! 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.

JoelSpeed · 2025-03-11T15:32:01Z

config/v1/types_feature.go

+	// RequiredMinimumKubeletVersion is the lowest version the MinimumKubeletVersion field in the
+	// nodes.config object may be set to to enable this feature.
+	// +openshift:enable:FeatureGate=MinimumKubeletVersion
+	RequiredMinimumKubeletVersion string `json:"requiredMinimumKubeletVersion,omitempty"`


Would prefer we make this future extensible, by making this a list type with a map key

requiredMinimumComponentVersion: - component: Kubelet minimumVersion: 1.30.0

This would mean in the future we can add other minimum required components e.g. boot images, KAS etc if we find a need for it in the future

This would prevent people proxying the version through an association with kubelet and allow specific operators to act upon their minimum for their component

Field needs to be marked as optional, with map key of component, component should be an enum type with a single valid value for now, maximum length of the list is 1

JoelSpeed

Please run the API linter over this and fix the reported issues make lint

JoelSpeed · 2025-03-11T17:06:05Z

config/v1/types_feature.go

+	// RequiredMinimumKubeletVersion is the lowest version the MinimumKubeletVersion field in the
+	// nodes.config object may be set to to enable this feature.


Godoc has not been updated per the new version of this field

JoelSpeed · 2025-03-11T17:07:11Z

config/v1/types_feature.go

 	// possible (probable?) future additions include
 	// 1. support level (Stable, ServiceDeliveryOnly, TechPreview, DevPreview)
 	// 2. description
 }

+type RequiredMinimumComponentVersion struct {
+	Component RequiredMinimumComponent `json:"component"`
+	// +kubebuilder:validation:XValidation:rule="self == \"\" || self.matches('^[0-9]*.[0-9]*.[0-9]*$')",message="minmumKubeletVersion must be in a semver compatible format of x.y.z, or empty"


What is the use case for empty string?

JoelSpeed · 2025-03-11T17:07:45Z

config/v1/types_feature.go

+	Component RequiredMinimumComponent `json:"component"`
+	// +kubebuilder:validation:XValidation:rule="self == \"\" || self.matches('^[0-9]*.[0-9]*.[0-9]*$')",message="minmumKubeletVersion must be in a semver compatible format of x.y.z, or empty"
+	// +kubebuilder:validation:MaxLength:=8
+	// +openshift:enable:FeatureGate=MinimumKubeletVersion


Only need this at the parent level

JoelSpeed · 2025-03-11T17:08:05Z

config/v1/types_feature.go

+	Version string `json:"version"`
+}
+
+// +kubebuilder:validation:Enum:=kubelet


Enums should be PascalCase

Suggested change

// +kubebuilder:validation:Enum:=kubelet

// +kubebuilder:validation:Enum:=Kubelet

haircommander · 2025-03-11T17:40:42Z

Please run the API linter over this and fix the reported issues make lint

there's a number of errors not related to the PR, should I fix these too?

JoelSpeed · 2025-03-11T17:42:24Z

there's a number of errors not related to the PR, should I fix these too?

Make sure your local master branch is up to date, and it should only show changes since the master branch

haircommander · 2025-03-11T19:08:10Z

/retest

openshift-ci-robot · 2025-03-11T19:09:37Z

@haircommander: This pull request references OCPNODE-3029 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.19.0" version, but no target version was set.

In response to this:

this is an optional field that allows a feature author to declare the minimum version a kubelet on an openshift node can be to use said feature.

This PR also sets UserNamespace related features to set their requiredMinimumKubeletVersion to 1.30.0.

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.

config/v1/types_feature.go

features/util.go

JoelSpeed · 2025-03-12T15:20:04Z

config/v1/types_feature.go

+	// Currently, the only supported component is Kubelet, and setting a required minimum kubelet component will set the
+	// minimumKubeletVersion field in the nodes.config.openshift.io CRD.


and setting a required minimum kubelet component will set the
// minimumKubeletVersion field in the nodes.config.openshift.io CRD

I'm struggling with what this actually means. Who/what is modifying nodes.config? Does the presence of a gate having a minimum version force that nodes.config to a specific value?

cluster admin sets it. it functionally forces though not practically. if the admin wants access to that feature then they set the correct minimum kubelet version, otherwise they don't get it

How would an admin know that they need to do this? When the admin has no opinion, I would normally expect the feature to get enabled automatically assuming that the cluster can handle it.

Would it make sense (you may have discussed this already) for the feature gate to be considered enabled when the nodes.config minimum is not set, and leave it up to MCO (I assume MCO knows what version of Kubelet a pool is running) to filter out the gates that aren't applicable to this pool? Or does that make a weird situation where gates are enabled in some pools but not others?

What happens for a new cluster, does the minimumKubeletVersion get set on the nodes.config automatically? Or would we have these features just turned off unless someone happens to set the config?

How would an admin know that they need to do this?

I intend on this being a documentation thing

When the admin has no opinion, I would normally expect the feature to get enabled automatically assuming that the cluster can handle it.

i prefer the opt in because min kubelet verison is mostly a promise that the admin won't try to add nodes that are a lower version (in addition to the promise that they have none currently

Would it make sense (you may have discussed this already) for the feature gate to be considered enabled when the nodes.config minimum is not set, and leave it up to MCO (I assume MCO knows what version of Kubelet a pool is running) to filter out the gates that aren't applicable to this pool? Or does that make a weird situation where gates are enabled in some pools but not others?

yeah since featuregate object is cluster wide I think we should have featuregates be consistent

What happens for a new cluster, does the minimumKubeletVersion get set on the nodes.config automatically?

nope, fully opt in IMO

yeah since featuregate object is cluster wide I think we should have featuregates be consistent

What do you mean here, can you expand? I'm not following?

nope, fully opt in IMO

Ok, so, lets say over time there are many features that are now gated based on a minimum version. And the cluster admin has never set a minimum version in the nodes.config, what will be the state of all these features?

I expect over time that the gates are actually removed upstream, so eventually the features will come on, but later (3 releases?) than they otherwise would have done? Does this mean then, that the feature here allows an admin to opt-into these features earlier, but in the worst case, if they do nothing, the features still get turned on based on the upstream cadence? Or is it then behind the upstream cadence?

What node skew do we support within OpenShift, is it N-3? So lets say we had a feature gate gated by 1.30, would it be safe to enable it in 1.33 without any nodes.config minimum kubelet version?

What do you mean here, can you expand? I'm not following?

Since the openshift api for configuring feature gates is cluster level (not configured per machine pool) it'd feel odd to me to have differing feature based on node pool

Right exactly. A feature author can remove the required min version for a feature when the skew is such that we will support it on all supported node skews, at which point the feature will be on by default for everyone. Until then, it should be gated and would only be on by default if min version was set

Going back to the original question on the thread, this sentence doesn't say to me that the nodes config needs to be set, it says it will be set, implying that an action here makes it so there. We should update the wording here

This thread is still outstanding

config/v1/types_feature.go

JoelSpeed · 2025-03-18T09:29:57Z

config/v1/types_feature.go

@@ -112,6 +112,16 @@ type FeatureGateStatus struct {
 	// +listType=map
 	// +listMapKey=version
 	FeatureGates []FeatureGateDetails `json:"featureGates"`
+


We wanted to include a generation field in here that would be monotonically increasing every time the featureGates/renderedMinimumComponentVersions list changes

I don't currently see this as necessary actually . How would the handling be different than a user manually changing the featureset in a custom no upgrade case?

Yeah, it would be about the same. The intention of the generation was that we thought we'd be able to use it to show the MCP had rolled out the latest observed generation of the feature gates. What signal will we use to check the MCP has rolled out the latest version of these changes?

my current approach is using the MCP updated condition similar to how the MCS serves the config, because the MCS serving in the config will be how a lower versioned node gets its config, so after the new config is rendered it should be safe for a lower version node to be created

Is there a race condition that you need to be careful of here? The feature gates get updated, the MCP condition says it's up to date, you continue, the MCP controller then realises the FG change and marks the updated condition false?

How do we ensure that the MCP controller has seen the change before we observe the updated condition?

yeah I have to get some advice from MCO team on that, but there is an ObservedGeneration field in the mcp status I intend on checking

JoelSpeed · 2025-03-18T09:32:26Z

config/v1/types_feature.go

+	// Currently, the only supported component is Kubelet, and setting a required minimum kubelet component will set the
+	// minimumKubeletVersion field in the nodes.config.openshift.io CRD.


Going back to the original question on the thread, this sentence doesn't say to me that the nodes config needs to be set, it says it will be set, implying that an action here makes it so there. We should update the wording here

openshift-ci · 2025-03-20T17:49:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haircommander
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:

OWNERS

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

JoelSpeed · 2025-03-25T13:06:52Z

config/v1/types_feature.go

+	// Currently, the only supported component is Kubelet, and setting a required minimum kubelet component will set the
+	// minimumKubeletVersion field in the nodes.config.openshift.io CRD.


This thread is still outstanding

config/v1/types_feature.go

JoelSpeed · 2025-03-25T13:09:26Z

config/v1/tests/featuregates.config.openshift.io/MinimumKubeletVersion.yaml

Can we add some tests specifically for the new fields?

config/v1/types_feature.go

config/v1/types_node.go

features/util.go

JoelSpeed · 2025-03-26T09:15:28Z

features/util.go

@@ -166,6 +192,20 @@ func (b *featureGateBuilder) register() (configv1.FeatureGateName, error) {
 			} else {
 				allFeatureGates[clusterProfile][featureSet].Disabled = append(allFeatureGates[clusterProfile][featureSet].Disabled, description)
 			}
+			if b.minimumKubeletVersion != "" && featureSet == configv1.Default {


Does minimumKubeletVersion only work with the default feature set? Won't work in tech preview?

correct I think it should only apply to the default. I think we'd go Custom->TechPreview->minimumKubeletVersion (optional)->GA

Huh, I had imagined they would be orthogonal 🤔

Does that mean moving to a minimumKubeletVersion counts as promotion of the feature to the stable feature set? Or can the addition of the minimum kubelet be done without the feature being in the stable feature set?

Does that mean moving to a minimumKubeletVersion counts as promotion of the feature to the stable feature set?

I am thinking of it this way, for user namespaces at least.

JoelSpeed · 2025-03-26T09:18:42Z

payload-command/render/render.go

I wonder if this is the wrong place to do this. We render these files based on static information, and there should be no external config.

I wonder if for these kinds of variable configurations, we want the config operator to re-arrange the enable/disable list within the cluster.

Without that, the matrix of rendered configs is likely to explode right?

this is actually mostly for hypershift, who uses this binary to render the featuregates in the ignitionconfig provider. I can update the commit message to reflect that

Signed-off-by: Peter Hunt <[email protected]>

…rsion to 1.30.0 which is the lowest version the kubelet will deny a pod if a userns can't be created Signed-off-by: Peter Hunt <[email protected]>

Signed-off-by: Peter Hunt <[email protected]>

This is largely done for hypershift, who will use this to render the active featuregates for hosted clusters Signed-off-by: Peter Hunt <[email protected]>

JoelSpeed

Even though the feature gate helper is called "enableInDefault...", I don't see logic that separates the gates out in feature sets when it's rendering them based on the min kubelet version. Is the name stale?

JoelSpeed · 2025-03-27T09:09:07Z

features/features.go

@@ -677,6 +677,7 @@ var (
 						productScope(kubernetes).
 						enhancementPR("https://github.com/kubernetes/enhancements/issues/127").
 						enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
+						enableInDefaultWhenRequiredMinimumComponentVersion(configv1.MinimumComponentKubelet, "1.30.0").


Nit, would name this

Suggested change

enableInDefaultWhenRequiredMinimumComponentVersion(configv1.MinimumComponentKubelet, "1.30.0").

enableInDefaultWithRequiredMinimumComponentVersion(configv1.MinimumComponentKubelet, "1.30.0").

JoelSpeed · 2025-03-27T09:13:06Z

features/util.go

@@ -110,6 +112,11 @@ func (b *featureGateBuilder) enableForClusterProfile(clusterProfile ClusterProfi
 	return b
 }

+func (b *featureGateBuilder) enableInDefaultWhenRequiredMinimumComponentVersion(component configv1.MinimumComponent, version string) *featureGateBuilder {


What does tying this to the default feature set buy for us?

Assuming we had a feature we had just introduced, and we wanted to start testing it in a new release, in tech preview, why would we not just populate the nodes config in tech preview clusters to match the release image version?

I know we talked about a controller setting a status nodes config version, where did that land? Future extension?

And to be 100% clear, the enabledIn must contain configv1.Default for this to work right? This doesn't act as a promotion for the feature?

I know we talked about a controller setting a status nodes config version, where did that land? Future extension?

it should land here openshift/machine-config-operator#4929

And to be 100% clear, the enabledIn must contain configv1.Default for this to work right? This doesn't act as a promotion for the feature?

yeah I need to update this it is currently adding if it's in the disabled list which isn't right

JoelSpeed · 2025-03-27T09:15:55Z

payload-command/render/render.go

+			return fmt.Errorf("error converting NodeConfig: %w", err)
+		}
+		// TODO FIXME: how do we handle multiple?
+		if nodeConfig.Spec.MinimumKubeletVersion != "" {


We had intended to add a status field, do we have that yet? We have to pick the lower of spec and status when rendering the current gates

good point we have added the status field but have not used it yet

JoelSpeed · 2025-03-27T09:41:40Z

payload-command/render/render.go

+					// Programming error, as these are built into the binary in features/feature.go
+					panic(err)
+				}
+				if targetVersion.GT(gateVersion) {


I'm struggling to reason if this is the right way around, can you add a comment explaining why this is the right way around

TargetVersion here is the minimum version that comes from the nodes config right?

And the GateVersion comes from the version itself?

So it target version is greater than the minimum gate version, I expected it to be enabled?

openshift-ci · 2025-04-12T02:29:05Z

@haircommander: 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/e2e-azure	`0e36791`	link	false	`/test e2e-azure`
ci/prow/e2e-gcp	`0e36791`	link	false	`/test e2e-gcp`
ci/prow/okd-scos-e2e-aws-ovn	`0e36791`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-serial-techpreview	`0e36791`	link	true	`/test e2e-aws-serial-techpreview`
ci/prow/e2e-upgrade-out-of-change	`0e36791`	link	true	`/test e2e-upgrade-out-of-change`
ci/prow/e2e-aws-ovn-techpreview	`0e36791`	link	true	`/test e2e-aws-ovn-techpreview`
ci/prow/minor-e2e-upgrade-minor	`0e36791`	link	true	`/test minor-e2e-upgrade-minor`
ci/prow/e2e-aws-ovn-techpreview	`0e36791`	link	true	`/test e2e-aws-ovn-techpreview`
ci/prow/e2e-aws-ovn	`0e36791`	link	true	`/test e2e-aws-ovn`
ci/prow/e2e-aws-serial-techpreview	`0e36791`	link	true	`/test e2e-aws-serial-techpreview`
ci/prow/verify-crd-schema	`0e36791`	link	true	`/test verify-crd-schema`
ci/prow/verify-crd-schema	`0e36791`	link	true	`/test verify-crd-schema`
ci/prow/e2e-aws-serial	`0e36791`	link	true	`/test e2e-aws-serial`
ci/prow/e2e-aws-serial	`0e36791`	link	true	`/test e2e-aws-serial`
ci/prow/e2e-aws-ovn-hypershift	`0e36791`	link	true	`/test e2e-aws-ovn-hypershift`
ci/prow/minor-e2e-upgrade-minor	`0e36791`	link	true	`/test minor-e2e-upgrade-minor`
ci/prow/e2e-upgrade	`0e36791`	link	true	`/test e2e-upgrade`
ci/prow/e2e-aws-ovn-hypershift	`0e36791`	link	true	`/test e2e-aws-ovn-hypershift`
ci/prow/verify-client-go	`0e36791`	link	true	`/test verify-client-go`

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.

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 10, 2025

openshift-ci bot requested review from bparees and JoelSpeed March 10, 2025 18:42

JoelSpeed reviewed Mar 11, 2025

View reviewed changes

haircommander force-pushed the required-min-kubelet-version branch from c06e6c6 to 49b8e6d Compare March 11, 2025 16:56

JoelSpeed reviewed Mar 11, 2025

View reviewed changes

haircommander changed the title ~~features: add requiredMinimumKubeletVersion~~ features: add requiredMinimumComponentVersion Mar 11, 2025

haircommander force-pushed the required-min-kubelet-version branch from 49b8e6d to cab7937 Compare March 11, 2025 17:40

haircommander changed the title ~~features: add requiredMinimumComponentVersion~~ OCPNODE-3029: features: add requiredMinimumComponentVersion Mar 11, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 11, 2025

JoelSpeed reviewed Mar 12, 2025

View reviewed changes

config/v1/types_feature.go Outdated Show resolved Hide resolved

config/v1/types_feature.go Show resolved Hide resolved

config/v1/types_feature.go Outdated Show resolved Hide resolved

features/util.go Outdated Show resolved Hide resolved

haircommander force-pushed the required-min-kubelet-version branch 2 times, most recently from 55e32df to 1cedbe3 Compare March 12, 2025 14:48

JoelSpeed reviewed Mar 12, 2025

View reviewed changes

haircommander force-pushed the required-min-kubelet-version branch from 1cedbe3 to 59c2758 Compare March 17, 2025 16:28

JoelSpeed reviewed Mar 18, 2025

View reviewed changes

haircommander force-pushed the required-min-kubelet-version branch from 59c2758 to 2b1af83 Compare March 20, 2025 17:45

JoelSpeed reviewed Mar 25, 2025

View reviewed changes

haircommander force-pushed the required-min-kubelet-version branch from 2b1af83 to 619b24b Compare March 25, 2025 20:09

JoelSpeed reviewed Mar 26, 2025

View reviewed changes

haircommander force-pushed the required-min-kubelet-version branch from 619b24b to 556d9c2 Compare March 26, 2025 14:46

featuregates: add requiredMinimumKubeletVersion field

df25159

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the required-min-kubelet-version branch 2 times, most recently from 4e692eb to d3df047 Compare March 26, 2025 16:12

haircommander force-pushed the required-min-kubelet-version branch from d3df047 to 6893a63 Compare March 26, 2025 18:23

haircommander added 5 commits March 26, 2025 15:31

API: add renderedMinimumComponentVersion to featuregate.Status

91baceb

Signed-off-by: Peter Hunt <[email protected]>

nodes config: add minimumKubeletVersion to status

c69d52d

Signed-off-by: Peter Hunt <[email protected]>

features: set userns related features to have required min kubelet ve…

3d1c35f

…rsion to 1.30.0 which is the lowest version the kubelet will deny a pod if a userns can't be created Signed-off-by: Peter Hunt <[email protected]>

vendor: add semver package

ab1b261

Signed-off-by: Peter Hunt <[email protected]>

payload-command: handle minimum kubelet version

0e36791

This is largely done for hypershift, who will use this to render the active featuregates for hosted clusters Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the required-min-kubelet-version branch from 6893a63 to 0e36791 Compare March 26, 2025 19:34

JoelSpeed reviewed Mar 27, 2025

View reviewed changes

		// RequiredMinimumKubeletVersion is the lowest version the MinimumKubeletVersion field in the
		// nodes.config object may be set to to enable this feature.

	// +kubebuilder:validation:Enum:=kubelet
	// +kubebuilder:validation:Enum:=Kubelet

		// Currently, the only supported component is Kubelet, and setting a required minimum kubelet component will set the
		// minimumKubeletVersion field in the nodes.config.openshift.io CRD.

	enableInDefaultWhenRequiredMinimumComponentVersion(configv1.MinimumComponentKubelet, "1.30.0").
	enableInDefaultWithRequiredMinimumComponentVersion(configv1.MinimumComponentKubelet, "1.30.0").

OCPNODE-3029: features: add requiredMinimumComponentVersion #2224

Are you sure you want to change the base?

OCPNODE-3029: features: add requiredMinimumComponentVersion #2224

Conversation

haircommander commented Mar 10, 2025

openshift-ci bot commented Mar 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Mar 11, 2025

JoelSpeed commented Mar 11, 2025

haircommander commented Mar 11, 2025

openshift-ci-robot commented Mar 11, 2025 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 12, 2025

openshift-ci-robot commented Mar 11, 2025 •

edited by openshift-ci bot

Loading