add minimumKubeletVersion #2059

haircommander · 2024-10-09T17:05:44Z

No description provided.

openshift-ci · 2024-10-09T17:05:50Z

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.

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

haircommander · 2024-10-21T18:19:39Z

/retest

haircommander · 2024-10-22T14:30:30Z

/retest

sjenning · 2024-10-24T16:02:47Z

kubecontrolplane/v1/types.go

@@ -62,6 +62,11 @@ type KubeAPIServerConfig struct {

 	// TODO this needs to be removed.
 	APIServerArguments map[string]Arguments `json:"apiServerArguments"`
+
+	// MinimumKubeletVersion is the lowest version of a kubelet that can meaningfully join the cluster.


s/can meaningfully join/is allowed to join/

it's allowed to join but it's blocked from doing anything meaningful in the cluster.

haircommander · 2024-10-24T18:36:22Z

/retest

features/features.go

config/v1/types_node.go

kubecontrolplane/v1/types.go

enxebre · 2024-10-25T11:04:13Z

config/v1/types_node.go

+	// Specifically, the apiserver will deny (most) authorization requests of kubelets that are older
+	// than the specified version.
+	// +kubebuilder:validation:Pattern=`^[0-9]*\.[0-9]*\.[0-9]*$`
+	// +openshift:enable:FeatureGate=MinimumKubeletVersion


Since this field is not marked as immutable and it's optional, what happens if I change it to a version value higher than existing kubelets?

There's an admission plugin in the kube-apiserver that denies such changes https://github.com/openshift/kubernetes/pull/2104/files#diff-5c437405eeba0789e9c42802e3f36cf6bdafd59d6d5c5dbaa6b66a2e02948bd7R100-R125
something similar will be used in hypershift, but set a condition on the hosted cluster object, rather than block on admission

ack. I think we should probably clarify somewhere that this validation on admission is by the nature of a distributed system only a best effort. Inflight operations might still result in older kubelet bypassing it if the right lucky timing takes place.

For hypershift yes the norm is signal via conditions. However for improving consumer UX we might also want to explore introducing VAP and let the current minimal kubelet be reported via vap parameter that is inferred from e.g. the HC.status.

yeah I can mention it in the library-go PR

but set a condition on the hosted cluster object, rather than block on admission

Why can't we block on admission?

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

enxebre · 2024-10-28T09:18:01Z

@haircommander do we have a Jira/other way today to tag all related PRs for this effort in a single place? so it's easy for ourselves later in time or for others with less context to catch up?
E.g Add minimumKubeletVersion knob and validation epic involves
openshift/hypershift#4980
openshift/kubernetes#2104
#2059

config/v1/types_node.go

haircommander · 2024-10-28T17:49:34Z

@enxebre there's also openshift/library-go#1831 and openshift/cluster-kube-apiserver-operator#1754

the overarching epic is https://issues.redhat.com/browse/OCPNODE-2506. Note a number of the cards tracking these PRs are closed because they were originally scoped to drafts for the work until I got feedback and could size the time to respond

haircommander · 2024-10-28T18:13:55Z

thanks @enxebre @JoelSpeed I've updated for your comments, PTAL 🙂

haircommander · 2024-10-29T13:31:44Z

/retest

config/v1/types_node.go

JoelSpeed · 2024-10-30T10:13:24Z

config/v1/types_node.go

+	// Specifically, the apiserver will deny most authorization requests of kubelets that are older
+	// than the specified version, only allowing the kubelet to get and update its node object, and perform
+	// subjectaccessreviews.


This is interesting technical detail, but, what does this mean to an end user?

good question, I added it from @enxebre 's review, I'm not feeling too opinionated on keeping it

I think Alberto's question was fair, I'm just not sure we are adding the right context

What does it look like to an end user when the kubelet is having these various authzs fail?

Does it mean pods won't schedule there? Does it mean the node goes not ready? Is the node actually still functional?

how does

// minimumKubeletVersion is the lowest version of a kubelet that can join the cluster. // Specifically, the apiserver will deny most authorization requests of kubelets that are older // than the specified version, only allowing the kubelet to get and update its node object, and perform // subjectaccessreviews. // This means the kubelet won't be able to view API objects it's responsible for running, // and will eventually be marked as NotReady.

work for you?

What happens to a running pod if this version is changed to mean then that the kubelet no longer meets the requirements? Is that possible?

Does won't be able to view API objects it's responsible for running mean that existing pods break, or, new pods won't be able to execute?

that should be blocked at admission: https://github.com/openshift/kubernetes/pull/2104/files#diff-5c437405eeba0789e9c42802e3f36cf6bdafd59d6d5c5dbaa6b66a2e02948bd7R117-R122
So really this will only affect new kubelets that attempt to join the cluster after the min version is in place. those kubelets won't be able to run anything

So perhaps then we take what you had above with a slight tweak?

// This means, any kubelet that attempts to join the cluster will not be able to run any assigned workloads, and will eventually be marked as not ready.

When you say will eventually be marked not ready, surely it will never get ready, since the CNI won't be able to initialise the network right?

yeah unless it manages to upgrade itself which would require manual intervetion

updated to your suggestion

config/v1/types_node.go

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

config/v1/types_node.go

kubecontrolplane/v1/types.go

config/v1/types_node.go

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

haircommander · 2024-10-31T14:20:12Z

/retest

@JoelSpeed @enxebre any other thoughts?

haircommander · 2024-10-31T20:42:04Z

/retest

openshift-ci · 2024-10-31T22:40:34Z

@haircommander: all tests passed!

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

Once we have this final test case, I'm good to merge, thank you

JoelSpeed · 2024-11-01T11:56:28Z

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

+        apiVersion: config.openshift.io/v1
+        kind: Node
+        spec:
+          minimumKubeletVersion: 1.30.0


Please add a test that shows the empty string passes validation

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

JoelSpeed · 2024-11-01T15:58:58Z

/lgtm

Thanks @haircommander

openshift-ci · 2024-11-01T15:59:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, JoelSpeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

openshift-ci-robot · 2024-11-01T16:40:50Z

/retest-required

Remaining retests: 0 against base HEAD ddd439f and 2 for PR HEAD 808d881 in total

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 9, 2024

openshift-ci bot requested review from deads2k and sjenning October 9, 2024 17:06

haircommander added a commit to haircommander/kubernetes that referenced this pull request Oct 9, 2024

WIP: vendor openshift/api#2059

cf4c2bb

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

haircommander mentioned this pull request Oct 11, 2024

WIP: add support for minimumKubeletVersion openshift/library-go#1831

Open

haircommander force-pushed the min-kubelet-version branch from 3786488 to e76c711 Compare October 11, 2024 20:55

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 11, 2024

haircommander added a commit to haircommander/kubernetes that referenced this pull request Oct 17, 2024

WIP: vendor openshift/api#2059

8d3cbaf

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

haircommander added a commit to haircommander/kubernetes that referenced this pull request Oct 18, 2024

WIP: vendor openshift/api#2059

598c507

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

haircommander force-pushed the min-kubelet-version branch from e76c711 to 0908b45 Compare October 21, 2024 19:21

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2024

haircommander force-pushed the min-kubelet-version branch from eb53d96 to 68a03ec Compare October 22, 2024 19:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2024

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from 905a012 to 6471c90 Compare October 24, 2024 14:51

sjenning reviewed Oct 24, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

haircommander mentioned this pull request Oct 24, 2024

WIP: add support for minimumKubeletVersion openshift/hypershift#4980

Open

4 tasks

deads2k reviewed Oct 24, 2024

View reviewed changes

features/features.go Outdated Show resolved Hide resolved

enxebre reviewed Oct 25, 2024

View reviewed changes

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

enxebre reviewed Oct 25, 2024

View reviewed changes

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

enxebre reviewed Oct 25, 2024

View reviewed changes

kubecontrolplane/v1/types.go Outdated Show resolved Hide resolved

enxebre reviewed Oct 25, 2024

View reviewed changes

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2024

haircommander force-pushed the min-kubelet-version branch from 81ee4e9 to 2c6ee12 Compare October 25, 2024 15:30

feature gates: Add MinimumKubeletVersion gate

0a8ec78

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

haircommander force-pushed the min-kubelet-version branch from 2c6ee12 to 0a8ec78 Compare October 25, 2024 16:07

JoelSpeed reviewed Oct 28, 2024

View reviewed changes

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

JoelSpeed reviewed Oct 28, 2024

View reviewed changes

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

haircommander force-pushed the min-kubelet-version branch from f2a585d to a329290 Compare October 28, 2024 18:13

JoelSpeed reviewed Oct 30, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from 865cb02 to 85078a8 Compare October 30, 2024 17:14

haircommander added a commit to haircommander/kubernetes that referenced this pull request Oct 30, 2024

WIP: vendor openshift/api#2059

3fb3085

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

haircommander force-pushed the min-kubelet-version branch from 85078a8 to 786be86 Compare October 30, 2024 19:48

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from e8e382b to 75510c3 Compare October 31, 2024 18:09

JoelSpeed reviewed Nov 1, 2024

View reviewed changes

update api validation on minimumKubeletVersion

808d881

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

haircommander force-pushed the min-kubelet-version branch from 75510c3 to 808d881 Compare November 1, 2024 13:38

openshift-ci bot assigned JoelSpeed Nov 1, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add minimumKubeletVersion #2059

add minimumKubeletVersion #2059

haircommander commented Oct 9, 2024

openshift-ci bot commented Oct 9, 2024

haircommander commented Oct 21, 2024

haircommander commented Oct 22, 2024

sjenning Oct 24, 2024

haircommander Oct 24, 2024

haircommander commented Oct 24, 2024

enxebre Oct 25, 2024 •

edited

Loading

haircommander Oct 25, 2024

enxebre Oct 28, 2024 •

edited

Loading

haircommander Oct 28, 2024

JoelSpeed Oct 30, 2024

enxebre commented Oct 28, 2024

haircommander commented Oct 28, 2024

haircommander commented Oct 28, 2024

haircommander commented Oct 29, 2024

JoelSpeed Oct 30, 2024

haircommander Oct 30, 2024

JoelSpeed Oct 30, 2024

haircommander Oct 30, 2024

JoelSpeed Oct 30, 2024

haircommander Oct 30, 2024

JoelSpeed Oct 31, 2024

haircommander Oct 31, 2024

haircommander Oct 31, 2024

haircommander commented Oct 31, 2024

haircommander commented Oct 31, 2024

openshift-ci bot commented Oct 31, 2024

JoelSpeed left a comment

JoelSpeed Nov 1, 2024

haircommander Nov 1, 2024

JoelSpeed commented Nov 1, 2024

openshift-ci bot commented Nov 1, 2024

openshift-ci-robot commented Nov 1, 2024

add minimumKubeletVersion #2059

Are you sure you want to change the base?

add minimumKubeletVersion #2059

Conversation

haircommander commented Oct 9, 2024

openshift-ci bot commented Oct 9, 2024

haircommander commented Oct 21, 2024

haircommander commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Oct 24, 2024

enxebre Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Oct 28, 2024

haircommander commented Oct 28, 2024

haircommander commented Oct 28, 2024

haircommander commented Oct 29, 2024

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

haircommander commented Oct 31, 2024

haircommander commented Oct 31, 2024

openshift-ci bot commented Oct 31, 2024

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

JoelSpeed commented Nov 1, 2024

openshift-ci bot commented Nov 1, 2024

openshift-ci-robot commented Nov 1, 2024

enxebre Oct 25, 2024 •

edited

Loading

enxebre Oct 28, 2024 •

edited

Loading