Repository navigation
Upgrade Kubernetes dependencies to v0.28.3 and Golang to 1.20 - #1648
Conversation
astefanutti
changed the title
|
@kevin85421 @architkulkarni @blublinsky @z103cb could you please review? |
|
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? Thanks! |
|
@kevin85421 I've updated the description with more details that I hope cover your questions. Let me know otherwise. |
kevin85421
left a comment
kevin85421
left a comment
There was a problem hiding this comment.
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.
| # Produce CRDs that work back to Kubernetes 1.11 (no version conversion) | ||
| # 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" |
There was a problem hiding this comment.
Not familiar with these options. Why can we make this change?
There was a problem hiding this comment.
Update:
-
trivialVersions: I can find some references oftrivialVersionsin controller-tools v0.6.0 codebase, but I cannot find any reference in v0.13.0 codebase. I believe that this field is deprecated. -
preserveUnknownFields: I can still find some strings seem to be related to this field in v0.13.0.
@astefanutti would you mind explaining why do we remove preserveUnknownFields=false here? Thanks!
There was a problem hiding this comment.
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.
| Replicas: pointer.Int32Ptr(3), | ||
| MinReplicas: pointer.Int32Ptr(0), | ||
| MaxReplicas: pointer.Int32Ptr(10000), | ||
| Replicas: pointer.Int32(3), |
There was a problem hiding this comment.
Note: https://github.com/kubernetes/utils/blob/cf03d44ff3cf/pointer/pointer.go#L58-L61
Int32Ptr is a function variable referring to Int32
| }, | ||
| Spec: rayv1.RayClusterSpec{ | ||
| HeadGroupSpec: rayv1.HeadGroupSpec{ | ||
| EnableIngress: pointer.BoolPtr(true), |
There was a problem hiding this comment.
| } | ||
|
|
||
| var _ = BeforeSuite(func(done Done) { | ||
| var _ = BeforeSuite(func(ctx SpecContext) { |
There was a problem hiding this comment.
Note: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing
Done was removed by Ginkgo 2.0.
| func TestAPIs(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
|
|
||
| RunSpecsWithDefaultAndCustomReporters(t, |
There was a problem hiding this comment.
| return cache.MultiNamespacedCacheBuilder(watchNamespaces)(config, opts) | ||
| } | ||
| setupLog.Info(fmt.Sprintf("Only watch custom resources in multiple namespaces: %v", watchNamespaces)) | ||
| for _, namespace := range watchNamespaces { |
There was a problem hiding this comment.
Why do we decide to use multiple Cache.DefaultNamespaces[namespace] instead of a MultiNamespacedCacheBuilder?
There was a problem hiding this comment.
Note: MultiNamespacedCacheBuilder seems to be removed between controller-runtime v0.15.3 and v0.16.0.
| } | ||
| options.NewCache = cache.BuilderWithOptions(cache.Options{SelectorsByObject: selectorsByObject}) | ||
| } else { | ||
| options.NewCache = func(config *rest.Config, opts cache.Options) (cache.Cache, error) { |
There was a problem hiding this comment.
// 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.
|
|
||
| return cache.SelectorsByObject{ | ||
| return map[client.Object]cache.ByObject{ | ||
| &batchv1.Job{}: {Label: selector}, |
There was a problem hiding this comment.
A dumb question: Does it mean that the informer only caches batchv1.Job?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got it. This is very helpful!
| 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]) |
| IsController: true, | ||
| OwnerType: &rayv1.RayCluster{}, | ||
| }) | ||
| return b.Owns(&v1beta1.PodGroup{}) |
There was a problem hiding this comment.
|
@astefanutti would you mind rebasing this PR to resolve the conflicts? Thanks! |
@kevin85421 rebased, PTAL. |
Why are these changes needed?
This PR upgrades the following dependencies:
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/**andhelm-chart/**update generated by the upgraded controller-gen toolray-operator/pkg/client/**update generated by the upgraded code-generator toolThe 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