Vendor k/*@v0.20 and controller-runtime@v0.8 #3651

rfranzke · 2021-03-03T08:12:13Z

How to categorize this PR?

/area open-source quality
/kind task
/priority normal

What this PR does / why we need it:
This PR vendors all the Kubernetes dependencies in version v0.20.2 and controller-runtime version v0.8.3.

Special notes for your reviewer:
/invite @timebertt @ialidzhikov
/squash

I got rid of our fork of k8s.io/kube-openapi ~~and our pinning of sigs.k8s.io/structured-merge-diff/v4~~ - do you see issues with this?

Release note:

⚠️ Go dependencies to `kubernetes/*` and `kubernetes-sigs/controller-runtime` were updated to `v0.20.2` and `v0.8.3` respectively.

rfranzke · 2021-03-03T08:13:54Z

pkg/apis/seedmanagement/v1alpha1/zz_generated.conversion.go

-		// TODO: Inefficient conversion - can we improve it?
-		if err := s.Convert(*in, *out, 0); err != nil {


Due to this, I had to introduce https://github.com/gardener/gardener/pull/3651/files#diff-c7b64578d2ac95623dfd44edeb4a58700b755fa9bda6b8267965ec156363a6e6R76-R83. I'm not sure why the generation does not find the functions here:

gardener/pkg/apis/core/v1beta1/zz_generated.conversion.go

Lines 4366 to 4369 in 6f6cf8a

// Convert_v1beta1_SeedTemplate_To_core_SeedTemplate is an autogenerated conversion function.

func Convert_v1beta1_SeedTemplate_To_core_SeedTemplate(in *SeedTemplate, out *core.SeedTemplate, s conversion.Scope) error {

return autoConvert_v1beta1_SeedTemplate_To_core_SeedTemplate(in, out, s)

}

I played with --extra-peer-dirs but without success. Any pointers are appreciated.

/cc @stoyanr @ialidzhikov

We also ran into a similar problem with k8s 1.20.
It looks like a regression in the code generator for k8s 1.20: kubernetes/kubernetes#98380

Thanks @schrodit for pointing this out. Then I guess the workaround I applied is fine for now until a fix was released, right?

pkg/gardenlet/apis/config/v1alpha1/zz_generated.conversion.go

ialidzhikov · 2021-03-03T08:35:31Z

I got rid of our fork of k8s.io/kube-openapi and our pinning of sigs.k8s.io/structured-merge-diff/v4 - do you see issues with this?

Yes. I guess we will be able to get rid of the replace for sigs.k8s.io/structured-merge-diff/v4 only after kubernetes/kubernetes#99038.

rfranzke · 2021-03-04T07:24:41Z

OK, thanks @ialidzhikov. Also, @timuthy mentioned that we now should be able to get rid of https://github.com/gardener/gardener/tree/master/third_party/forked/kubernetes, so I will look into that.
/status author-action

gardener-robot · 2021-03-04T07:24:48Z

@rfranzke The pull request was assigned to you under author-action. Please unassign yourself when you are done. Thank you.

gardener-robot · 2021-03-09T11:43:16Z

@rfranzke Label priority/normal does not exist.

rfranzke · 2021-03-09T11:57:42Z

OK, thanks to @ialidzhikov and @timuthy I've updated the PR now (and also use the latest, just released v0.8.3 version of the controller-runtime library).

ialidzhikov · 2021-03-10T06:57:59Z

/assign

ialidzhikov · 2021-03-10T08:12:08Z

extensions/pkg/controller/utils_test.go


-			c.EXPECT().Patch(ctx, worker, client.MergeFrom(workerWithAnnotation))
+			ctx := context.TODO()
+			test.EXPECTPatch(ctx, c, expectedWorker, workerWithAnnotation)


Hmm, this is odd. I really liked the way it was before this PR (without any helper func for expecting patch)

c.EXPECT().Patch(ctx, worker, client.MergeFrom(workerWithAnnotation))

I see that the old way fails with error like:

Unexpected call to *client.MockClient.Patch([context.TODO 0xc00025ad80 0xc000899da0]) at /go/src/github.com/gardener/gardener/extensions/pkg/controller/utils.go:193 because: expected call at /go/src/github.com/gardener/gardener/extensions/pkg/controller/utils_test.go:171 doesn't match the argument at index 2. Got: &{application/merge-patch+json 0x1e25600 0xc00025b200 {false}} Want: is equal to &{application/merge-patch+json 0x1e25600 0xc00025afc0 {false}}

Do you know why with the new version of controller-runtime it now fails with similar error ? I see kubernetes-sigs/controller-runtime#1406 but I cannot correlate it to the failure.

The reason I am asking is that currently the helper func test.EXPECTPatch is always expecting a MergeFrom PatchOption and I assume this func won't be useful if there is usage of StrategicMergeFrom or anything other PatchOption.

Do you know why with the new version of controller-runtime it now fails with similar error ? I see kubernetes-sigs/controller-runtime#1406 but I cannot correlate it to the failure.

This is explained in the commit message of 0fdab6e:

Due to kubernetes-sigs/controller-runtime#1413,
naive gomock expectations don't work anymore. The reason is that the
mergeFromPatch struct has now a function attribute (createPatch),
and as gomega is using reflect.DeepEqual under the hood, the
comparison now fails:

Func values are deeply equal if both are nil; otherwise they are not deeply equal.
(from https://golang.org/pkg/reflect/#DeepEqual)

Hence, we introduce our own test.EXPECTPatch() function.

Thanks to @timuthy for pointing this out.

I'm also not too happy with it and am very much open for better ideas.

The reason I am asking is that currently the helper func test.EXPECTPatch is always expecting a MergeFrom PatchOption and I assume this func won't be useful if there is usage of StrategicMergeFrom or anything other PatchOption.

Maybe, but we don't use StrategicMergeFrom or any PatchOption yet, so the function can be extended as soon as we need it.

Ok, thanks for explaining. I guess the proper way to fix this is to adapt the createPatch field from func to an interface in the upstream. I can have a look into this one with some lower prio. But yeah, it seems that it is out of the scope of this PR.

Yeah, probably @timebertt can help here as the initial author of this change in k-sigs/controller-runtime :)

I now opened kubernetes-sigs/controller-runtime#1439. Let me know if you have more elegant solution to the problem in mind.

ialidzhikov

/lgtm
/needs second-opinion

@timuthy

Due to kubernetes-sigs/controller-runtime#1413, naive gomock expectations don't work anymore. The reason is that the `mergeFromPatch` struct has now a function attribute (`createPatch`), and as `gomega` is using `reflect.DeepEqual` under the hood, the comparison now fails: > Func values are deeply equal if both are nil; otherwise they are not deeply equal. (from https://golang.org/pkg/reflect/#DeepEqual) Hence, we introduce our own `test.EXPECTPatch()` function. Thanks to @timuthy for pointing this out.

rfranzke · 2021-03-12T06:50:13Z

Rebased to resolve the conflicts.

ialidzhikov

/lgtm

timuthy

/lgtm
Thanks for cleaning up the forked code as well 👍

@timuthy

* make revendor generate * Get rid of third_party/forked/kubernetes * Adapt Patch() expectations of MockClient Due to kubernetes-sigs/controller-runtime#1413, naive gomock expectations don't work anymore. The reason is that the `mergeFromPatch` struct has now a function attribute (`createPatch`), and as `gomega` is using `reflect.DeepEqual` under the hood, the comparison now fails: > Func values are deeply equal if both are nil; otherwise they are not deeply equal. (from https://golang.org/pkg/reflect/#DeepEqual) Hence, we introduce our own `test.EXPECTPatch()` function. Thanks to @timuthy for pointing this out. * Adapt kubelet config

@timuthy

* make revendor generate * Get rid of third_party/forked/kubernetes * Adapt Patch() expectations of MockClient Due to kubernetes-sigs/controller-runtime#1413, naive gomock expectations don't work anymore. The reason is that the `mergeFromPatch` struct has now a function attribute (`createPatch`), and as `gomega` is using `reflect.DeepEqual` under the hood, the comparison now fails: > Func values are deeply equal if both are nil; otherwise they are not deeply equal. (from https://golang.org/pkg/reflect/#DeepEqual) Hence, we introduce our own `test.EXPECTPatch()` function. Thanks to @timuthy for pointing this out. * Adapt kubelet config

rfranzke requested a review from a team as a code owner March 3, 2021 08:12

gardener-robot requested a review from timebertt March 3, 2021 08:12

gardener-robot-ci-2 added the reviewed/ok-to-test label Mar 3, 2021

gardener-robot-ci-3 added needs/ok-to-test and removed reviewed/ok-to-test labels Mar 3, 2021

rfranzke commented Mar 3, 2021

View reviewed changes

gardener-robot requested a review from ialidzhikov March 3, 2021 08:15

gardener-robot added the status/author-action label Mar 4, 2021

gardener-robot assigned rfranzke Mar 4, 2021

gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/normal priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021

rfranzke force-pushed the enh/ctrlruntime-upgrade branch from 664f0e6 to 0ae1899 Compare March 9, 2021 11:57

gardener-robot-ci-3 added the reviewed/ok-to-test label Mar 9, 2021

rfranzke removed their assignment Mar 9, 2021

gardener-robot removed the status/author-action label Mar 9, 2021

gardener-robot-ci-1 removed the reviewed/ok-to-test label Mar 9, 2021

gardener-robot assigned ialidzhikov Mar 10, 2021

gardener deleted a comment from gardener-robot Mar 10, 2021

ialidzhikov reviewed Mar 10, 2021

View reviewed changes

rfranzke requested review from ialidzhikov and schrodit March 10, 2021 09:56

ialidzhikov previously approved these changes Mar 10, 2021

View reviewed changes

gardener-robot removed needs/review labels Mar 10, 2021

rfranzke added 4 commits March 12, 2021 07:47

make revendor generate

b3f46ad

Get rid of third_party/forked/kubernetes

6072fee

Adapt kubelet config

e98ce76

rfranzke dismissed ialidzhikov’s stale review via e98ce76 March 12, 2021 06:50

rfranzke force-pushed the enh/ctrlruntime-upgrade branch from 0ae1899 to e98ce76 Compare March 12, 2021 06:50

rfranzke requested a review from ialidzhikov March 12, 2021 06:50

gardener-robot added the needs/review label Mar 12, 2021

gardener-robot-ci-1 added the reviewed/ok-to-test label Mar 12, 2021

gardener-robot added the needs/second-opinion label Mar 12, 2021

gardener-robot-ci-2 removed the reviewed/ok-to-test label Mar 12, 2021

ialidzhikov approved these changes Mar 12, 2021

View reviewed changes

gardener-robot added reviewed/lgtm and removed needs/review labels Mar 12, 2021

timuthy approved these changes Mar 12, 2021

View reviewed changes

timuthy merged commit 06242c9 into gardener:master Mar 15, 2021

rfranzke deleted the enh/ctrlruntime-upgrade branch March 15, 2021 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor k/*@v0.20 and controller-runtime@v0.8 #3651

Vendor k/*@v0.20 and controller-runtime@v0.8 #3651

rfranzke commented Mar 3, 2021 •

edited by ialidzhikov

rfranzke Mar 3, 2021

schrodit Mar 9, 2021 •

edited

rfranzke Mar 9, 2021

ialidzhikov commented Mar 3, 2021

rfranzke commented Mar 4, 2021

gardener-robot commented Mar 4, 2021

gardener-robot commented Mar 9, 2021

rfranzke commented Mar 9, 2021

ialidzhikov commented Mar 10, 2021

ialidzhikov Mar 10, 2021

rfranzke Mar 10, 2021

ialidzhikov Mar 10, 2021

rfranzke Mar 10, 2021

ialidzhikov Mar 18, 2021

ialidzhikov left a comment

rfranzke commented Mar 12, 2021

ialidzhikov left a comment

timuthy left a comment

		// TODO: Inefficient conversion - can we improve it?
		if err := s.Convert(in, out, 0); err != nil {

	// Convert_v1beta1_SeedTemplate_To_core_SeedTemplate is an autogenerated conversion function.
	func Convert_v1beta1_SeedTemplate_To_core_SeedTemplate(in SeedTemplate, out core.SeedTemplate, s conversion.Scope) error {
	return autoConvert_v1beta1_SeedTemplate_To_core_SeedTemplate(in, out, s)
	}

Vendor k/*@v0.20 and controller-runtime@v0.8 #3651

Vendor k/*@v0.20 and controller-runtime@v0.8 #3651

Conversation

rfranzke commented Mar 3, 2021 • edited by ialidzhikov

rfranzke Mar 3, 2021

Choose a reason for hiding this comment

schrodit Mar 9, 2021 • edited

Choose a reason for hiding this comment

rfranzke Mar 9, 2021

Choose a reason for hiding this comment

ialidzhikov commented Mar 3, 2021

rfranzke commented Mar 4, 2021

gardener-robot commented Mar 4, 2021

gardener-robot commented Mar 9, 2021

rfranzke commented Mar 9, 2021

ialidzhikov commented Mar 10, 2021

ialidzhikov Mar 10, 2021

Choose a reason for hiding this comment

rfranzke Mar 10, 2021

Choose a reason for hiding this comment

ialidzhikov Mar 10, 2021

Choose a reason for hiding this comment

rfranzke Mar 10, 2021

Choose a reason for hiding this comment

ialidzhikov Mar 18, 2021

Choose a reason for hiding this comment

ialidzhikov left a comment

Choose a reason for hiding this comment

rfranzke commented Mar 12, 2021

ialidzhikov left a comment

Choose a reason for hiding this comment

timuthy left a comment

Choose a reason for hiding this comment

rfranzke commented Mar 3, 2021 •

edited by ialidzhikov

schrodit Mar 9, 2021 •

edited