Skip to content
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

add minimumKubeletVersion #2059

Merged
merged 4 commits into from
Nov 1, 2024
+1,249 −3
Merged

add minimumKubeletVersion #2059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83d619d
kubeapiserver: add minimum kubelet version
haircommander Oct 9, 2024
ffa1b52
config/types_node: add minimumKubeletVersion
haircommander Oct 11, 2024
0a8ec78
feature gates: Add MinimumKubeletVersion gate
haircommander Oct 22, 2024
808d881
update api validation on minimumKubeletVersion
Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions config/v1/tests/nodes.config.openshift.io/MinimumKubeletVersion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "Node"
crdName: nodes.config.openshift.io
featureGate: MinimumKubeletVersion
tests:
onCreate:
haircommander marked this conversation as resolved.
Show resolved Hide resolved
- name: Should be able to create a minimal Node
initial: |
apiVersion: config.openshift.io/v1
kind: Node
spec: {} # No spec is required for a Node
expected: |
apiVersion: config.openshift.io/v1
kind: Node
spec: {}
- name: Should be able to create an empty minimumKubeletVersion
initial: |
apiVersion: config.openshift.io/v1
kind: Node
spec:
minimumKubeletVersion: ""
expected: |
apiVersion: config.openshift.io/v1
kind: Node
spec:
minimumKubeletVersion: ""
- name: Should be able to create a minimumKubeletVersion
initial: |
apiVersion: config.openshift.io/v1
kind: Node
spec:
minimumKubeletVersion: 1.30.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test that shows the empty string passes validation

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

expected: |
apiVersion: config.openshift.io/v1
kind: Node
spec:
minimumKubeletVersion: 1.30.0
- name: Should fail to create a bogus version
initial: |
apiVersion: config.openshift.io/v1
kind: Node
spec:
minimumKubeletVersion: bogus
expectedError: "Invalid value: \"string\": minmumKubeletVersion must be in a semver compatible format of x.y.z, or empty"
19 changes: 19 additions & 0 deletions config/v1/types_node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,25 @@ type NodeSpec struct {
// the status and corresponding reaction of the cluster
// +optional
WorkerLatencyProfile WorkerLatencyProfileType `json:"workerLatencyProfile,omitempty"`

// 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.
Comment on lines +51 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to your suggestion

// 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.
// Its max length is 8, so maximum version allowed is either "9.999.99" or "99.99.99".
haircommander marked this conversation as resolved.
Show resolved Hide resolved
// Since the kubelet reports the version of the kubernetes release, not Openshift, this field references
// the underlying kubernetes version this version of Openshift is based off of.
// In other words: if an admin wishes to ensure no nodes run an older version than Openshift 4.17, then
// they should set the minimumKubeletVersion to 1.30.0.
// When comparing versions, the kubelet's version is stripped of any contents outside of major.minor.patch version.
// Thus, a kubelet with version "1.0.0-ec.0" will be compatible with minimumKubeletVersion "1.0.0" or earlier.
// +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
Copy link
Member

@enxebre enxebre Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@enxebre enxebre Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I can mention it in the library-go PR

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Why can't we block on admission?

// +optional
MinimumKubeletVersion string `json:"minimumKubeletVersion"`
haircommander marked this conversation as resolved.
Show resolved Hide resolved
}

type NodeStatus struct {
Expand Down
158 changes: 158 additions & 0 deletions ...g/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-CustomNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/1107
api.openshift.io/merged-by-featuregates: "true"
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
release.openshift.io/bootstrap-required: "true"
release.openshift.io/feature-set: CustomNoUpgrade
name: nodes.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Node
listKind: NodeList
plural: nodes
singular: node
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: |-
Node holds cluster-wide information about node specific features.

Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
properties:
cgroupMode:
description: CgroupMode determines the cgroups version on the node
enum:
- v1
- v2
- ""
type: string
minimumKubeletVersion:
description: |-
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 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.
Its max length is 8, so maximum version allowed is either "9.999.99" or "99.99.99".
Since the kubelet reports the version of the kubernetes release, not Openshift, this field references
the underlying kubernetes version this version of Openshift is based off of.
In other words: if an admin wishes to ensure no nodes run an older version than Openshift 4.17, then
they should set the minimumKubeletVersion to 1.30.0.
When comparing versions, the kubelet's version is stripped of any contents outside of major.minor.patch version.
Thus, a kubelet with version "1.0.0-ec.0" will be compatible with minimumKubeletVersion "1.0.0" or earlier.
maxLength: 8
type: string
x-kubernetes-validations:
- message: minmumKubeletVersion must be in a semver compatible format
of x.y.z, or empty
rule: self == "" || self.matches('^[0-9]*.[0-9]*.[0-9]*$')
workerLatencyProfile:
description: |-
WorkerLatencyProfile determins the how fast the kubelet is updating
the status and corresponding reaction of the cluster
enum:
- Default
- MediumUpdateAverageReaction
- LowUpdateSlowReaction
type: string
type: object
status:
description: status holds observed values.
properties:
conditions:
description: conditions contain the details and the current state
of the nodes.config object
items:
description: Condition contains details for one aspect of the current
state of this API Resource.
properties:
lastTransitionTime:
description: |-
lastTransitionTime is the last time the condition transitioned from one status to another.
This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable.
format: date-time
type: string
message:
description: |-
message is a human readable message indicating details about the transition.
This may be an empty string.
maxLength: 32768
type: string
observedGeneration:
description: |-
observedGeneration represents the .metadata.generation that the condition was set based upon.
For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date
with respect to the current state of the instance.
format: int64
minimum: 0
type: integer
reason:
description: |-
reason contains a programmatic identifier indicating the reason for the condition's last transition.
Producers of specific condition types may define expected values and meanings for this field,
and whether the values are considered a guaranteed API.
The value should be a CamelCase string.
This field may not be empty.
maxLength: 1024
minLength: 1
pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
type: string
status:
description: status of the condition, one of True, False, Unknown.
enum:
- "True"
- "False"
- Unknown
type: string
type:
description: type of condition in CamelCase or in foo.example.com/CamelCase.
maxLength: 316
pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
type: array
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
type: object
required:
- spec
type: object
served: true
storage: true
subresources:
status: {}
1 change: 1 addition & 0 deletions ...0000_10_config-operator_01_nodes.crd.yaml → ...config-operator_01_nodes-Default.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ metadata:
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
release.openshift.io/bootstrap-required: "true"
release.openshift.io/feature-set: Default
name: nodes.config.openshift.io
spec:
group: config.openshift.io
Expand Down
158 changes: 158 additions & 0 deletions .../zz_generated.crd-manifests/0000_10_config-operator_01_nodes-DevPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/1107
api.openshift.io/merged-by-featuregates: "true"
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
release.openshift.io/bootstrap-required: "true"
release.openshift.io/feature-set: DevPreviewNoUpgrade
name: nodes.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Node
listKind: NodeList
plural: nodes
singular: node
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: |-
Node holds cluster-wide information about node specific features.

Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
properties:
cgroupMode:
description: CgroupMode determines the cgroups version on the node
enum:
- v1
- v2
- ""
type: string
minimumKubeletVersion:
description: |-
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 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.
Its max length is 8, so maximum version allowed is either "9.999.99" or "99.99.99".
Since the kubelet reports the version of the kubernetes release, not Openshift, this field references
the underlying kubernetes version this version of Openshift is based off of.
In other words: if an admin wishes to ensure no nodes run an older version than Openshift 4.17, then
they should set the minimumKubeletVersion to 1.30.0.
When comparing versions, the kubelet's version is stripped of any contents outside of major.minor.patch version.
Thus, a kubelet with version "1.0.0-ec.0" will be compatible with minimumKubeletVersion "1.0.0" or earlier.
maxLength: 8
type: string
x-kubernetes-validations:
- message: minmumKubeletVersion must be in a semver compatible format
of x.y.z, or empty
rule: self == "" || self.matches('^[0-9]*.[0-9]*.[0-9]*$')
workerLatencyProfile:
description: |-
WorkerLatencyProfile determins the how fast the kubelet is updating
the status and corresponding reaction of the cluster
enum:
- Default
- MediumUpdateAverageReaction
- LowUpdateSlowReaction
type: string
type: object
status:
description: status holds observed values.
properties:
conditions:
description: conditions contain the details and the current state
of the nodes.config object
items:
description: Condition contains details for one aspect of the current
state of this API Resource.
properties:
lastTransitionTime:
description: |-
lastTransitionTime is the last time the condition transitioned from one status to another.
This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable.
format: date-time
type: string
message:
description: |-
message is a human readable message indicating details about the transition.
This may be an empty string.
maxLength: 32768
type: string
observedGeneration:
description: |-
observedGeneration represents the .metadata.generation that the condition was set based upon.
For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date
with respect to the current state of the instance.
format: int64
minimum: 0
type: integer
reason:
description: |-
reason contains a programmatic identifier indicating the reason for the condition's last transition.
Producers of specific condition types may define expected values and meanings for this field,
and whether the values are considered a guaranteed API.
The value should be a CamelCase string.
This field may not be empty.
maxLength: 1024
minLength: 1
pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
type: string
status:
description: status of the condition, one of True, False, Unknown.
enum:
- "True"
- "False"
- Unknown
type: string
type:
description: type of condition in CamelCase or in foo.example.com/CamelCase.
maxLength: 316
pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
type: array
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
type: object
required:
- spec
type: object
served: true
storage: true
subresources:
status: {}
Loading