[VC]: Update Controller Runtime to 2.9

[christopherhein]

This fixes #714 and makes sure that new generated files are always created the same. Now when you run make generate instead of using go generate to generate the files this will use controller-gen to create the deepcopy funcs and when you call make manifests it will create the files in config/crds/.

Signed-off-by: Chris Hein me@chrishein.com

[christopherhein]

/assign @Fei-Guo

[christopherhein]

One caveat that people might run into using the newer version of controller-gen when you generate the files it will use the code comments to document the Fields, this while being great when you embed StatefulSet schemas into other CRDs it copies all of that data. This means if your cluster has CRDs from the original version when you apply this version kubectl sometimes fails because the last-applied-configurations annotation is larger that the 128kb limit. To resolve this you can use replaceorcreateinstead ofapply`.

I’d like to look into better ways of handling this in later versions of the ClusterVersion CRD, for example using referenced manifests through URLS or the local file system.

[christopherhein]

@charleszheng44 this should be what we need so that we can use some of the better customizations from controller-gen.

[k8s-ci-robot]

@christopherhein: GitHub didn't allow me to assign the following users: charleszheng44.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @charleszheng44

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/test-infra repository.

[Fei-Guo]

I was trying go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.3.0 but hit the following errors.

go: found sigs.k8s.io/controller-tools/cmd/controller-gen in sigs.k8s.io/controller-tools v0.3.0
#sigs.k8s.io/controller-tools/pkg/crd/markers
/Users/f.guo/go/pkg/mod/sigs.k8s.io/controller-tools@v0.3.0/pkg/crd/markers/topology.go:137:8: schema.XMapType undefined (type *"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1".JSONSchemaProps has no field or method XMapType)
/Users/f.guo/go/pkg/mod/sigs.k8s.io/controller-tools@v0.3.0/pkg/crd/markers/topology.go:152:8: schema.XMapType undefined (type *"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1".JSONSchemaProps has no field or method XMapType)

Also, I didn't expect the size of the crd file changes from 16k to 1.5MB. Is there a way to shrink it? We seldom look at crd yaml for all the field explanations.

[christopherhein]

Ah, let me change up the Makefile then, we typically with newer version of controller-gen will use the makefile to create a tmpdir and install into that instead of doing it from within the project.

About the size, I can see if I can remove the field definitions. It’s mostly preparing for the OpenAPI v3 scheme client side validation and for allowing newer clusters to use kubectl explain clusterversions and kubectl explain virtualclusters.

Chatting with other people in the past they have reimplemented the go types that they needed instead of importing full types like we have in https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/virtualcluster/pkg/apis/tenancy/v1alpha1/clusterversion_types.go#L43-L46

It might make sense to change the external type here to [runtime.RawExtension](https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime?tab=doc#RawExtension) or [runtime.Object](https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime?tab=doc#Object) for both of these types and then handle the conversions internally. At least not to break the current API. It might have made more sense to have these be more like StatefulSetTemplate and ServiceTemplate like you have with v1.PodTemplate, but that would have required understanding all the fields you wanted to expose upfront.

[christopherhein]

Oops, I also noticed I used 0.3.0 which is for the bleeding edge version of kubebuilder, the one that is currently stable and shipping with the latest kubebuilder is 2.5, I’m changed to that one and modified the Makefile setup. @Fei-Guo can you check if this works for you?

[christopherhein]

Also WRT to the descriptions I’ve opened an issue with controller-tools and am researching the feasibility and interest so we can trim down the CRDs - kubernetes-sigs/controller-tools#441

[Fei-Guo]

I try to create the crd using the new crd yaml and hit the following,

k create -f tenancy.x-k8s.io_virtualclusters.yaml
The CustomResourceDefinition "virtualclusters.tenancy.x-k8s.io" is invalid:
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[apiServer].properties[service].properties[spec].properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[apiServer].properties[statefulset].properties[spec].properties[template].properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[apiServer].properties[statefulset].properties[spec].properties[template].properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[controllerManager].properties[statefulset].properties[spec].properties[template].properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[controllerManager].properties[statefulset].properties[spec].properties[template].properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[controllerManager].properties[service].properties[spec].properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[etcd].properties[service].properties[spec].properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[etcd].properties[statefulset].properties[spec].properties[template].properties[spec].properties[containers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[etcd].properties[statefulset].properties[spec].properties[template].properties[spec].properties[initContainers].items.properties[ports].items.properties[protocol].default: Required value: this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property

my minikube version

Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.3", GitCommit:"b3cbbae08ec52a7fc73d334838e18d17e8512749", GitTreeState:"clean", BuildDate:"2019-11-14T04:24:34Z", GoVersion:"go1.12.13", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.0", GitCommit:"9e991415386e4cf155a24b1da15becaa390438d8", GitTreeState:"clean", BuildDate:"2020-03-25T14:50:46Z", GoVersion:"go1.13.8", Compiler:"gc", Platform:"linux/amd64"}

[christopherhein]

I probably need to turn on the XEmbedded flag again, it appeared like I didn’t need but I must have been wrong.

[christopherhein]

Also you might want to update your kubectl client… you technically out of the +1 support version. https://kubernetes.io/docs/setup/release/version-skew-policy/#kubectl

[christopherhein]

Correction, your latest issues it’s actually because of the 1.18 version incompatibility with controller-tools - kubernetes-sigs/controller-tools#440

[Fei-Guo]

This is problematic. It is even hard to workaround in 1.18. Adding default manually does not resolve the problem.

k create -f tenancy.x-k8s.io_virtualclusters.yaml
The CustomResourceDefinition "virtualclusters.tenancy.x-k8s.io" is invalid:
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[apiServer].properties[service].properties[spec].properties[ports].items.properties[protocol].default: Forbidden: must not be set

[christopherhein]

This is problematic. It is even hard to workaround in 1.18. Adding default manually does not resolve the problem.

k create -f tenancy.x-k8s.io_virtualclusters.yaml
The CustomResourceDefinition "virtualclusters.tenancy.x-k8s.io" is invalid:
* spec.validation.openAPIV3Schema.properties[status].properties[versionHistory].items.properties[clusterVersion].properties[spec].properties[apiServer].properties[service].properties[spec].properties[ports].items.properties[protocol].default: Forbidden: must not be set

Given it seems like controller-gen is broken with 1.18 we can hold this until kubernetes-sigs/controller-tools#440 is fixed and I have a patch in to allow you to skip the descriptions - kubernetes-sigs/controller-tools#441. I didn’t realize that before I pushed up this code but it’s pretty crazy since we have ClusterVersion embedded in the Virtualcluster resource twice and each one is 1.6m it makes the virtualcluster 2.3m.

We should explore a different way for a those to be referenced. For example:

1. We could have a finalizer on the ClusterVersion and then we could start to treat these (and implement and admissioncontroller to validate) each ClusterVersion as an immutable resource and reject changes. Then in the virtualcluster resource we could use an field as a reference by name only and remove the embedded types.

2. We could change all Objects in other specs as runtime.RawExtension on the exported types then use apimachinery conversion to set the runtime.Object, we could have validations using an admission controller to make sure that the embedded services, statefulpods and the clusterversions are the proper types. https://godoc.org/k8s.io/apimachinery/pkg/runtime#RawExtension

We could also leave this for now and discuss what a v1alpha2 could look like since you said you have production users.

wdyt?

[christopherhein]

/hold

[Fei-Guo]

1. We could have a finalizer on the ClusterVersion and then we could start to treat these (and implement and admissioncontroller to validate) each ClusterVersion as an immutable resource and reject changes. Then in the virtualcluster resource we could use an field as a reference by name only and remove the embedded types.

I think we should revisit the VirtualclusterStatus struct. We already have ClusterVersionName in the spec and I don't think it should be mutable. Our controller currently does not support changing the clusterversion. It seems that vc crd size problem is resolvable.

[zhuangqh]

could you should me how to generate these two file incubator/virtualcluster/config/crds/tenancy.x-k8s.io_clusterversions.yaml ?

[christopherhein]

could you should me how to generate these two file incubator/virtualcluster/config/crds/tenancy.x-k8s.io_clusterversions.yaml ?

@zhuangqh with the Makefile make manifests.

https://github.com/kubernetes-sigs/multi-tenancy/pull/738/files#diff-d20693c5c6e86e34be5c15bb9ba38925R79

[Fei-Guo]

The clusterversionhistory field has been removed from VirtualClusterStatus with #741 . The VC crd size is reduced to 4.7KB with controller-gen 2.5.

[christopherhein]

Wow, that’s fantastic. Let me update this and maybe we can push it through?

[Fei-Guo]

Wow, that’s fantastic. Let me update this and maybe we can push it through?

I still have concern about clusterversion crd size though. A 1.6MB yaml basically makes the crd not readable at all. The current crd yaml looks much more straightforward.

[christopherhein]

/unhold

[christopherhein]

@Fei-Guo I was able to remove all the descriptions on the specs reducing the size to 500kb for the entire spec for ClusterVersion, much more manageable and now kubectl apply -f works again.

This still has the issue with 1.18 specs, that is being tracked across kubernetes-sigs/controller-tools#440 given that all controller-runtime 2.x projects are broken with this do you want to continue to wait on this?

[Fei-Guo]

I am ok with current controller runtime 2.x and I tend to agree that we can remove statefulset object is clusterversion to further reduce the crd size.

[Fei-Guo]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, Fei-Guo

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:

• incubator/virtualcluster/OWNERS [Fei-Guo]

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