add support for minimumKubeletVersion #1831

haircommander · 2024-10-11T20:43:16Z

carrying openshift/api#2059

haircommander · 2024-10-21T19:52:59Z

/retest

haircommander · 2024-10-22T13:55:51Z

/retest

haircommander · 2024-11-04T14:40:33Z

api changes merged, so this is ready

benluddy · 2024-11-04T14:48:40Z

pkg/config/node/minimum_kubelet_version.go

+	}
+
+	fieldPath := field.NewPath("spec", "minimumKubeletVersion")
+	nodes, err := nodesGetter.Nodes().List(context.TODO(), metav1.ListOptions{})


Can this be made to accept the lister interface instead of the client interface? Listing all nodes as part of validating an API field is probably more expensive than we would like.

pkg/config/node/minimum_kubelet_version.go

haircommander · 2024-11-04T17:36:53Z

I dropped the field references and also I thought about it more and being opinonated about the of the nodes (node lister vs nodesGetter) began to feel weird so I now just have it taking a slice of nodes and the various callers can decide where to get them. left in a separate commit in case you liked it the old way @benluddy

benluddy · 2024-11-04T17:41:27Z

I dropped the field references and also I thought about it more and being opinonated about the of the nodes (node lister vs nodesGetter) began to feel weird so I now just have it taking a slice of nodes and the various callers can decide where to get them. left in a separate commit in case you liked it the old way @benluddy

That's OK, but note that the items field in NodeList has type []Node, not []*Node, so the caller would have to transform one to the other anyway.

haircommander · 2024-11-04T17:52:04Z

/retest

haircommander · 2024-11-04T19:48:26Z

any other thoughts @benluddy ??

benluddy · 2024-11-18T18:39:56Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, err)
+		} else { // but raise a warning
+			klog.Warningf("nodes.config.openshift.io/cluster object could not be found")


This would be pretty weird if it lasts very long. It's probably reasonable to emit a warning with the events.Recorder so that it gets noticed. I see us doing that for similar cases in other config observers.

benluddy · 2024-11-18T18:44:17Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version_test.go

+			if !cmp.Equal(test.expectedObservedConfig, actualObservedConfig) {
+				t.Fatalf("unexpected configuration, diff = %v", cmp.Diff(test.expectedObservedConfig, actualObservedConfig))
+			}


Nit, the contract of Diff(a, b) guarantees that it returns the empty string IFF Equal(a, b), so you can do:

Suggested change

if !cmp.Equal(test.expectedObservedConfig, actualObservedConfig) {

t.Fatalf("unexpected configuration, diff = %v", cmp.Diff(test.expectedObservedConfig, actualObservedConfig))

}

if diff := cmp.Diff(test.expectedObservedConfig, actualObservedConfig); diff != "" {

t.Fatalf("unexpected configuration, diff = %v", diff)

}

benluddy · 2024-11-18T18:44:46Z

pkg/config/node/minimum_kubelet_version.go

+)
+
+// ValidateMinimumKubeletVersion takes a list of nodes and a currently set min version.
+// It parsees the min version and iterates through the nodes, comparing the version of the kubelets


Suggested change

// It parsees the min version and iterates through the nodes, comparing the version of the kubelets

// It parses the min version and iterates through the nodes, comparing the version of the kubelets

benluddy · 2024-11-18T18:57:06Z

/lgtm

haircommander · 2024-11-18T19:39:26Z

/retest

benluddy · 2024-11-18T20:28:06Z

/lgtm

haircommander · 2024-11-19T16:03:47Z

@benluddy sorry for the churn, but I've updated the Is*TooOld functions to only return an error (and a specific one if the node is too old) so callers can only check one value (most were anyway, and openshift/kubernetes#2104 (comment) lead me in that direction)

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

p0lyn0mial · 2024-11-20T07:50:02Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, err)
+		} else { // but raise a warning
+			recorder.Warningf("ObserveMinimumKubeletVersion", "nodes.%s/cluster not found", configv1.GroupName)


How often will the observer be called? If the node cluster object is not found, won't this cause event spam?

Important question, although I see this being done in the other observers as well. I think we never reach this code patch because by the time observers start running, these objects are always available.

I would remove the event recording. Fabio is right, the resource is always present. When it isn’t, we will have more issues.

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

pkg/config/node/minimum_kubelet_version.go

p0lyn0mial · 2024-11-20T08:15:25Z

pkg/config/node/minimum_kubelet_version.go

+// ParseKubeletVersion parses it into a semver.Version object, stripping
+// any information in the version that isn't "major.minor.patch".
+func ParseKubeletVersion(kubeletVersion string) (*semver.Version, error) {
+	version, err := semver.Parse(strings.TrimPrefix(kubeletVersion, "v"))


I think there is no test case that prefixes the version with v.

added that to all the tests, because kubelet will always send with a v afaict

bertinatto

Added a few comments, but overall LGTM. I'll let the final tag for Lukasz.

/approve

bertinatto · 2024-11-20T12:46:46Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+	// we got an error so without the node object we are not able to determine minimumKubeletVersion
+	if err != nil {
+		// if config/v1/node/cluster object is not found, that can be treated as a non-error case
+		if !apierrors.IsNotFound(err) {


nit:

if apierrors.IsNotFound(err) { (...) } if err != nil { (...) }

is easier to read

I reordered the cases but left the else because adding an additional return didn't look any better

bertinatto · 2024-11-20T12:48:45Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, err)
+		} else { // but raise a warning
+			recorder.Warningf("ObserveMinimumKubeletVersion", "nodes.%s/cluster not found", configv1.GroupName)


Important question, although I see this being done in the other observers as well. I think we never reach this code patch because by the time observers start running, these objects are always available.

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

pkg/operator/configobserver/node/observe_minimum_kubelet_version_test.go

pkg/config/node/minimum_kubelet_version.go

bertinatto · 2024-11-20T12:56:04Z

pkg/config/node/minimum_kubelet_version.go

+// node is older than min version.
+// When the node is too old, it returns the error ErrKubeletOutdated. If a different error occurs, an error is returned.
+// If the node is new enough and no error happens, nil is returned.
+func IsNodeTooOld(node *corev1.Node, minVersion *semver.Version) error {


Do we actually need this function? Currently it does the same thing as IsKubeletVerisionTooOld, just with a different signature.

I thouht it was cleaner than needing to make a fake node object in openshift/hypershift#4980 (where we just pass around the lowest kubelet version in the hosted cluster)

haircommander · 2024-11-20T14:55:46Z

thanks @bertinatto and @p0lyn0mial ! I have updated / addressed your comments, PTAL again :)

pkg/config/node/minimum_kubelet_version_test.go

p0lyn0mial · 2024-11-21T07:45:57Z

pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+		if !apierrors.IsNotFound(err) {
+			errs = append(errs, err)
+		} else { // but raise a warning
+			recorder.Warningf("ObserveMinimumKubeletVersion", "nodes.%s/cluster not found", configv1.GroupName)


I would remove the event recording. Fabio is right, the resource is always present. When it isn’t, we will have more issues.

pkg/config/node/minimum_kubelet_version.go

pkg/operator/configobserver/node/observe_minimum_kubelet_version_test.go

p0lyn0mial · 2024-11-21T07:58:07Z

I left a few suggestions, but overall, this PR looks great, thanks!

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

openshift-ci · 2024-11-21T18:06:58Z

@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.

p0lyn0mial

Left a few more suggestions that can be addressed in a new PR.

/lgtm

p0lyn0mial · 2024-11-22T08:55:21Z

pkg/apiserver/node/minimum_kubelet_version_test.go

+		name         string
+		version      string
+		shouldReject bool
+		tooOld       bool


unused, can be removed.

p0lyn0mial · 2024-11-22T09:01:52Z

pkg/apiserver/node/minimum_kubelet_version_test.go

+			err := ValidateMinimumKubeletVersion(testCase.nodes, testCase.version)
+			assert.Equal(t, testCase.shouldReject, err != nil, "minimum kubelet version %q %s rejected", testCase.version, shouldStr)
+
+			if testCase.shouldReject {


I would remove testCase.shouldReject as well and update the code to:

if err == nil && scenario.expectedError != nil { t.Fatal("expected to get an error") } if err != nil && scenario.expectedError == nil { t.Fatal(err) // unexpected error } if err != nil && scenario.expectedError != nil { if err.Error() != scenario.expectedError.Error() { t.Fatalf("unexpected error = %v, expected = %v", err, scenario.expectedError) } if strings.Contains(err.Error(), ErrKubeletOutdated.Error()) { assert.True(t, errors.Is(err, ErrKubeletOutdated), "error message should be ErrKubeletOutdated" } }

openshift-ci · 2024-11-22T09:03:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, bertinatto, haircommander, p0lyn0mial

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~~ [bertinatto,p0lyn0mial]

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2024

openshift-ci bot requested review from deads2k and harche October 11, 2024 20:45

haircommander force-pushed the min-kubelet-version branch 3 times, most recently from 21ccc58 to 10f3207 Compare October 18, 2024 16:32

haircommander force-pushed the min-kubelet-version branch from 10f3207 to 2b2ffda Compare October 21, 2024 19:29

haircommander mentioned this pull request Oct 28, 2024

add minimumKubeletVersion openshift/api#2059

Merged

haircommander force-pushed the min-kubelet-version branch from 2b2ffda to 271a103 Compare November 4, 2024 14:39

haircommander changed the title ~~WIP: add support for minimumKubeletVersion~~ add support for minimumKubeletVersion Nov 4, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2024

benluddy reviewed Nov 4, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from de7feb4 to d53287b Compare November 4, 2024 17:23

benluddy reviewed Nov 4, 2024

View reviewed changes

pkg/config/node/minimum_kubelet_version.go Outdated Show resolved Hide resolved

benluddy reviewed Nov 4, 2024

View reviewed changes

pkg/config/node/minimum_kubelet_version.go Outdated Show resolved Hide resolved

haircommander force-pushed the min-kubelet-version branch from 699007c to d54530a Compare November 4, 2024 18:06

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from 43e3c1e to e0b2031 Compare November 5, 2024 22:03

benluddy reviewed Nov 18, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch from e0b2031 to 31c75c3 Compare November 18, 2024 18:54

openshift-ci bot assigned benluddy Nov 18, 2024

haircommander force-pushed the min-kubelet-version branch from 6990d22 to 12c30bb Compare November 18, 2024 19:17

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

haircommander force-pushed the min-kubelet-version branch from 12c30bb to a050054 Compare November 19, 2024 16:02

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

haircommander force-pushed the min-kubelet-version branch from a050054 to 4f2e854 Compare November 19, 2024 16:42

p0lyn0mial reviewed Nov 20, 2024

View reviewed changes

bertinatto reviewed Nov 20, 2024

View reviewed changes

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

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from 3396de1 to 512a9b6 Compare November 20, 2024 14:55

p0lyn0mial reviewed Nov 21, 2024

View reviewed changes

pkg/config/node/minimum_kubelet_version.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Nov 21, 2024

View reviewed changes

pkg/operator/configobserver/node/observe_minimum_kubelet_version_test.go Show resolved Hide resolved

haircommander force-pushed the min-kubelet-version branch from 512a9b6 to e3952f3 Compare November 21, 2024 15:53

haircommander added 3 commits November 21, 2024 12:46

vendor o/api changes for minimumKubeletVersion

32cd9cf

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

add minimumKubeletVersionObserver

f394abb

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

add package for node config

6f09d18

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

haircommander force-pushed the min-kubelet-version branch from e3952f3 to 6f09d18 Compare November 21, 2024 17:46

p0lyn0mial reviewed Nov 22, 2024

View reviewed changes

openshift-ci bot assigned p0lyn0mial Nov 22, 2024

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

openshift-merge-bot bot merged commit c450187 into openshift:master Nov 22, 2024
4 checks passed

p0lyn0mial mentioned this pull request Nov 26, 2024

NO-JIRA: node/minimim_kubelet_version_test: simplify TestValidateConfigNodeForMinimumKubeletVersion #1896

Merged

	// It parsees the min version and iterates through the nodes, comparing the version of the kubelets
	// It parses the min version and iterates through the nodes, comparing the version of the kubelets

add support for minimumKubeletVersion #1831

add support for minimumKubeletVersion #1831

Conversation

haircommander commented Oct 11, 2024

haircommander commented Oct 21, 2024

haircommander commented Oct 22, 2024

haircommander commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Nov 4, 2024

benluddy commented Nov 4, 2024

haircommander commented Nov 4, 2024

haircommander commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluddy commented Nov 18, 2024

haircommander commented Nov 18, 2024

benluddy commented Nov 18, 2024

haircommander commented Nov 19, 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

bertinatto 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

haircommander commented Nov 20, 2024

Choose a reason for hiding this comment

p0lyn0mial commented Nov 21, 2024

openshift-ci bot commented Nov 21, 2024

p0lyn0mial left a 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 Nov 22, 2024