Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 #1648

astefanutti · 2023-11-15T17:19:13Z

Why are these changes needed?

This PR upgrades the following dependencies:

Kubernetes dependencies to v0.28.3
controller-runtime to v0.16.3
code-generator v0.28.3
controller-tools /controller-gen to v0.13.0
Ginkgo to v2.11.0

This also upgrades Go to v1.20 as it's required.

The files matching the following paths have been fully regenerated, and do not contain any manual changes:

ray-operator/config/** and helm-chart/** update generated by the upgraded controller-gen tool
ray-operator/pkg/client/** update generated by the upgraded code-generator tool

The majority of the manual changes are updates required because of some deprecated / removed APIs.

Note that these changes do not affect the client / server compatibility as they do not affect the requests made to the API server. So this PR should not affect the compatibility with Kubernetes versions, which remains bound to the APIs being used.

The Go upgrade is required by the newer version of controller-runtime, and generally the Kubernetes dependencies.

The operator unit tests had to be adapted with the behavioural changes to the controller-runtime client, which now take into account the status sub-resource.

Related issue number

Fixes #1337.

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

astefanutti · 2023-11-16T18:09:41Z

@kevin85421 @architkulkarni @blublinsky @z103cb could you please review?

kevin85421 · 2023-11-16T18:37:48Z

This is great! I will review this PR next week. Would you mind explaining the following questions in the PR descriptions?

(1) Which file changes are fully generated? Which commands/components generate the changes?
(2) Why do you manually make some file changes? Are there any references to prove they are backward compatible?
(3) What is the compatibility between these upgrade dependencies with Kubernetes? Are they compatible with Kubernetes 1.19?

Thanks!

astefanutti · 2023-11-17T15:47:55Z

@kevin85421 I've updated the description with more details that I hope cover your questions. Let me know otherwise.

blublinsky

Looks great

kevin85421

This is super amazing. I've never approved such a large PR during the first round of review. Thank you for your contribution! I've left some comments, most of which are notes for myself, and I also have a few questions.

kevin85421 · 2023-11-21T22:06:10Z

ray-operator/Makefile

 # Golang forbids converting float from Json to struct. Need allowDangerousTypes=true to unblock build.
-CRD_OPTIONS ?= "crd:maxDescLen=0,trivialVersions=true,preserveUnknownFields=false,generateEmbeddedObjectMeta=true,allowDangerousTypes=true"
+CRD_OPTIONS ?= "crd:maxDescLen=0,generateEmbeddedObjectMeta=true,allowDangerousTypes=true"


Not familiar with these options. Why can we make this change?

Update:

trivialVersions: I can find some references of trivialVersions in controller-tools v0.6.0 codebase, but I cannot find any reference in v0.13.0 codebase. I believe that this field is deprecated.

v0.13.0

v0.6.0

preserveUnknownFields: I can still find some strings seem to be related to this field in v0.13.0.

v0.13.0

@astefanutti would you mind explaining why do we remove preserveUnknownFields=false here? Thanks!

The preserveUnknownFields option has been removed from the controller-gen CLI, in favor of the kubebuilder:pruning:PreserveUnknownFields marker, that can be used at the type and field level of the Golang API structs, to control pruning.

Pruning unknown fields is the default for CRDs declared in apiextensions.k8s.io/v1, which is equivalent to preserveUnknownFields=false, so there should be not changed.

kevin85421 · 2023-11-21T22:10:30Z

ray-operator/apis/ray/v1/raycluster_types_test.go

-				Replicas:    pointer.Int32Ptr(3),
-				MinReplicas: pointer.Int32Ptr(0),
-				MaxReplicas: pointer.Int32Ptr(10000),
+				Replicas:    pointer.Int32(3),


Note: https://github.com/kubernetes/utils/blob/cf03d44ff3cf/pointer/pointer.go#L58-L61

Int32Ptr is a function variable referring to Int32

kevin85421 · 2023-11-21T22:20:42Z

ray-operator/controllers/ray/common/route_test.go

@@ -22,7 +22,7 @@ var instanceWithRouteEnabled = &rayv1.RayCluster{
 	},
 	Spec: rayv1.RayClusterSpec{
 		HeadGroupSpec: rayv1.HeadGroupSpec{
-			EnableIngress: pointer.BoolPtr(true),


Note: https://github.com/kubernetes/utils/blob/cf03d44ff3cf/pointer/pointer.go#L159-L162

kevin85421 · 2023-11-21T22:39:54Z

ray-operator/controllers/ray/suite_test.go

 }

-var _ = BeforeSuite(func(done Done) {
+var _ = BeforeSuite(func(ctx SpecContext) {


Note: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing

Done was removed by Ginkgo 2.0.

kevin85421 · 2023-11-21T22:40:43Z

ray-operator/controllers/ray/suite_test.go

@@ -52,12 +51,10 @@ var (
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)

-	RunSpecsWithDefaultAndCustomReporters(t,


Note: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-custom-reporters

kevin85421 · 2023-11-21T22:46:50Z

ray-operator/main.go

 		setupLog.Info(fmt.Sprintf("Only watch custom resources in multiple namespaces: %v", watchNamespaces))
+		for _, namespace := range watchNamespaces {


Why do we decide to use multiple Cache.DefaultNamespaces[namespace] instead of a MultiNamespacedCacheBuilder?

Note: MultiNamespacedCacheBuilder seems to be removed between controller-runtime v0.15.3 and v0.16.0.

kevin85421 · 2023-11-21T23:08:46Z

ray-operator/main.go

 	} else {
-		options.NewCache = func(config *rest.Config, opts cache.Options) (cache.Cache, error) {


Note: https://sourcegraph.com/github.com/kubernetes-sigs/controller-runtime@2154ffbc22e26ffd8c3b713927f0df2fa40841f2/-/blob/pkg/manager/manager.go?L113-L120

// NewCache is the function that will create the cache to be used
// by the manager. If not set this will use the default new cache function.
//
// When using a custom NewCache, the Cache options will be passed to the
// NewCache function.
//
// NOTE: LOW LEVEL PRIMITIVE!
// Only use a custom NewCache if you know what you are doing.

kevin85421 · 2023-11-21T23:14:30Z

ray-operator/main.go

 	label, err := labels.NewRequirement(common.KubernetesCreatedByLabelKey, selection.Equals, []string{common.ComponentName})
 	if err != nil {
 		return nil, err
 	}
 	selector := labels.NewSelector().Add(*label)

-	return cache.SelectorsByObject{
+	return map[client.Object]cache.ByObject{
 		&batchv1.Job{}: {Label: selector},


A dumb question: Does it mean that the informer only caches batchv1.Job?

There is no dumb question :)

Anything that's not specified here falls through the defaults. Excerpted from the ByObject documentation:

// The defaulting follows the following precedence order: // 1. ByObject // 2. DefaultNamespaces[namespace] // 3. Default*

Some examples can be found at https://github.com/kubernetes-sigs/controller-runtime/blob/2154ffbc22e26ffd8c3b713927f0df2fa40841f2/designs/cache_options.md, as well as some details at kubernetes-sigs/controller-runtime#2421.

Got it. This is very helpful!

kevin85421 · 2023-11-21T23:20:46Z

ray-operator/controllers/ray/raycluster_controller_fake_test.go

@@ -936,7 +936,7 @@ func TestReconcile_PodEvicted_DiffLess0_OK(t *testing.T) {
 	// Simulate head pod get evicted.
 	podList.Items[0].Status.Phase = corev1.PodFailed
 	podList.Items[0].Status.Reason = "Evicted"
-	err = fakeClient.Update(ctx, &podList.Items[0])
+	err = fakeClient.Status().Update(ctx, &podList.Items[0])


Good catch!

kevin85421 · 2023-11-21T23:25:36Z

ray-operator/controllers/ray/batchscheduler/volcano/volcano_scheduler.go

-			IsController: true,
-			OwnerType:    &rayv1.RayCluster{},
-		})
+	return b.Owns(&v1beta1.PodGroup{})


Note: https://yash-kukreja-98.medium.com/develop-on-kubernetes-series-demystifying-the-for-vs-owns-vs-watches-controller-builders-in-c11ab32a046e

kevin85421 · 2023-11-21T23:29:15Z

@astefanutti would you mind rebasing this PR to resolve the conflicts? Thanks!

astefanutti · 2023-11-22T11:46:27Z

@astefanutti would you mind rebasing this PR to resolve the conflicts? Thanks!

@kevin85421 rebased, PTAL.

…ay-project#1648)

astefanutti changed the title ~~Upgrade Kubernetes APIs and controller-runtime versions~~ Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 Nov 15, 2023

astefanutti force-pushed the pr-06 branch 7 times, most recently from 1054036 to 1c26641 Compare November 16, 2023 16:37

astefanutti force-pushed the pr-06 branch from 1c26641 to 9bad1ae Compare November 17, 2023 15:20

astefanutti force-pushed the pr-06 branch from 9bad1ae to a8c82a8 Compare November 17, 2023 15:51

blublinsky approved these changes Nov 18, 2023

View reviewed changes

kevin85421 approved these changes Nov 21, 2023

View reviewed changes

astefanutti added 2 commits November 22, 2023 11:50

Upgrade Kubernetes APIs and controller-runtime versions

0d93064

Upgrade Go to version 1.20

cb4c16e

astefanutti force-pushed the pr-06 branch from a8c82a8 to cb4c16e Compare November 22, 2023 10:50

kevin85421 merged commit e3bdc83 into ray-project:master Nov 22, 2023
25 checks passed

astefanutti deleted the pr-06 branch November 22, 2023 16:19

kevin85421 mentioned this pull request Nov 28, 2023

[Feature] Should we upgrade envtest? #1697

Closed

2 tasks

astefanutti mentioned this pull request Dec 7, 2023

[test] Upgrade envtest to latest version #1720

Merged

4 tasks

ChristianZaccaria pushed a commit to ChristianZaccaria/kuberay that referenced this pull request Jan 5, 2024

PATCH: Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 (r…

89c318d

…ay-project#1648)

ChristianZaccaria pushed a commit to ChristianZaccaria/kuberay that referenced this pull request Jan 5, 2024

DROP: Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 (r…

52c263b

…ay-project#1648)

astefanutti added a commit to opendatahub-io/kuberay that referenced this pull request Jan 5, 2024

DROP: Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 (r…

e9fb86a

…ay-project#1648)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 #1648

Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 #1648

astefanutti commented Nov 15, 2023 •

edited

Loading

astefanutti commented Nov 16, 2023

kevin85421 commented Nov 16, 2023

astefanutti commented Nov 17, 2023

blublinsky left a comment

kevin85421 left a comment

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

astefanutti Nov 22, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

astefanutti Nov 22, 2023

kevin85421 Nov 22, 2023

kevin85421 Nov 21, 2023

kevin85421 Nov 21, 2023

kevin85421 commented Nov 21, 2023

astefanutti commented Nov 22, 2023

		setupLog.Info(fmt.Sprintf("Only watch custom resources in multiple namespaces: %v", watchNamespaces))
		for _, namespace := range watchNamespaces {

		} else {
		options.NewCache = func(config *rest.Config, opts cache.Options) (cache.Cache, error) {

Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 #1648

Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 #1648

Conversation

astefanutti commented Nov 15, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

astefanutti commented Nov 16, 2023

kevin85421 commented Nov 16, 2023

astefanutti commented Nov 17, 2023

blublinsky left a comment

Choose a reason for hiding this comment

kevin85421 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

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

kevin85421 commented Nov 21, 2023

astefanutti commented Nov 22, 2023

astefanutti commented Nov 15, 2023 •

edited

Loading