Make extension components ready for cached client #4027

timebertt · 2021-05-11T09:52:56Z

How to categorize this PR?

/area dev-productivity scalability
/kind cleanup

What this PR does / why we need it:

Similar to previous PRs, this PR works towards getting rid of the DirectClient.
This one tackles usages in the extension components and makes them ready for cached clients.

Which issue(s) this PR fixes:
Part of #2822

Special notes for your reviewer:

c7bd017 is a prefactoring of the extension helper funcs, that is necessary for making the components ready for a cached client (see commit message for more details/reasoning).
The other commits perform the actual refactoring/adaption per component.

/squash

Release note:

rfranzke · 2021-05-11T09:57:03Z

/assign

timebertt · 2021-05-11T11:24:25Z

@ialidzhikov, this is the bigger refactoring I was talking about earlier.
Let's see how the review goes and if we have enough time left to validate it before the release is cut, we can include it, otherwise it doesn't hurt to wait a bit :)

timebertt · 2021-05-11T11:29:38Z

Strange...
The verify step failed twice with the following error:

Summarizing 1 Failure:

[Fail] extensions #WaitUntilExtensionObjectReady [It] should set passed object to latest state once ready 
/go/src/github.com/gardener/gardener/pkg/extensions/customresources_test.go:214

Ran 40 of 40 Specs in 0.165 seconds
FAIL! -- 39 Passed | 1 Failed | 0 Pending | 0 Skipped

Though locally, in can't get the test to fail:

$ stress ./pkg/extensions/extensions.test
343 runs so far, 0 failures
696 runs so far, 0 failures
1016 runs so far, 0 failures
1320 runs so far, 0 failures
1603 runs so far, 0 failures
1891 runs so far, 0 failures
2166 runs so far, 0 failures
2441 runs so far, 0 failures
2702 runs so far, 0 failures
2946 runs so far, 0 failures

rfranzke

/lgtm

Well done!

pkg/operation/botanist/botanist.go

pkg/operation/botanist/component/extensions/controlplane/controlplane.go

timebertt · 2021-05-12T14:35:31Z

Thanks @rfranzke for your review, PTAL :)

For the reasons explained in gardener#4057, MergePatchOrCreate is not safe to use without reading objects from the API server / cache first. Ideally we would switch to server-side apply to simplify such operations on controlled objects (e.g. extension resources), though we cannot leverage it for now because of several issues described in gardener#4122.

As described in gardener#4057, *PatchOrCreate is easy to use incorrectly. Remove *PatchOrCreate funcs in order to minimize the risk of misuse. There might still be good use cases for this mechanism, though they are limited. If there is such a good use case, we can use it also without a helper func.

timebertt · 2021-05-31T07:11:19Z

Some MergePatchOrCreate usages in this PR need to be changed, because of the reasons explained in #4057.
I will rebase the PR onto #4057 and adapt accordingly, once it's merged.

Rebased the PR and added two more commits to make it ready again:

one to switch from MergePatchOrCreate to GetAndCreateOrMergePatch: 3576fd6
and one to Remove the *PatchOrCreate funcs: 9cabff7

/unassign
/unhold
/remove status/author-action

@rfranzke PTAL :)

rfranzke

/lgtm

rfranzke · 2021-06-01T08:38:20Z

@timebertt What about the CreateOr{Strategic}MergePatch function (ref)? Do you think their current usage is safe or should it be adapted as well (i.e., first try Create, but if the resource already exists, then we Get it and Patch it afterwards)?

timebertt · 2021-06-01T08:52:31Z

@timebertt What about the CreateOr{Strategic}MergePatch function (ref)? Do you think their current usage is safe or should it be adapted as well (i.e., first try Create, but if the resource already exists, then we Get it and Patch it afterwards)?

Generally, the same applies to CreateOr{Strategic}MergePatch as to the *PatchOrCreate logic: if you're not trying to remove any optional fields in the patch, it's safe to use without doing a Get before. But if you do want to remove optional fields you have to fetch the object first, so that the diff contains an explicit null value.

So for example, this usage needs to be adapted, as labels that were removed from the seed template won't be removed from the seed itself:

gardener/pkg/gardenlet/controller/managedseed/managedseed_actuator.go

Line 309 in 93e4efb

    
           seed.Labels = utils.MergeStringMaps(managedSeed.Spec.SeedTemplate.Labels, map[string]string{

Probably, adapting the helper func to do a Get before Patch wouldn't hurt. But I haven't checked all the usages in detail. Though, such a helper func brings a good chance of misuse (that's why I removed the *PatchOrCreate funcs).

rfranzke · 2021-06-01T09:54:10Z

OK, thanks. For the sake of simplicity and prevention of coding bugs, I'd vote for changing the function in all cases as described above (create->get->patch) instead of (create->patch). Do you agree?

timebertt · 2021-06-01T10:16:58Z

Yes, I agree. That seems to be the more sustainable approach.

* Cleanup unused/dead code * Make EXPECTPatch easier to debug * Refactor extensions helper funcs This prefactoring performs several changes in the extension helper funcs: - the funcs directly work on the passed in objects instead of creating new objects via separately passed newObjFuncs. Passed objects are expected to be filled with the latest state the controller/component applied/observed/retrieved, but at least namespace and name. This way, controllers can keep working on a single in-memory object instead of retrieving the same object over and over again. - WaitUntilExtensionObjectReady additionally checks that the added timestamp annotation is present on the object to prevent false early exits when reading stale data (e.g. from a cached client). - makes naming more consistent and clearer This provides the following benefits: - makes the funcs ready to be used with cached clients, as they are able to properly handle stale data now - controllers/components automatically have the latest state of the objects present without retrieving them again from the API server. This will ease further refactorings and allows to safely use patches in a lot of cases instead of CreateOrUpdate (+RetryOnConflict). - the funcs are easier to use and comprehend: only the desired object needs to be passed, no need to pass newObjFuncs, namespace and name - the funcs are more similar to other helper funcs (e.g. CreateOrUpdate) and the mechanisms employed in e.g. c-r clients - no need to pass a client.Object to postReadyFuncs which needs to be casted again * Make BackupBucket controller ready for cached client * Make BackupEntry controller/component ready for cached client * Make ContainerRuntime component ready for cached client * Make ControlPlane component ready for cached client * Make Extension component ready for cached client * Make Infrastructure component ready for cached client * Make Network component ready for cached client * Make OperatingSystemConfig component ready for cached client * Make Worker component ready for cached client * Fix failing unit test * Address review feedback * Switch from MergePatchOrCreate to GetAndCreateOrMergePatch For the reasons explained in gardener#4057, MergePatchOrCreate is not safe to use without reading objects from the API server / cache first. Ideally we would switch to server-side apply to simplify such operations on controlled objects (e.g. extension resources), though we cannot leverage it for now because of several issues described in gardener#4122. * Remove *PatchOrCreate funcs As described in gardener#4057, *PatchOrCreate is easy to use incorrectly. Remove *PatchOrCreate funcs in order to minimize the risk of misuse. There might still be good use cases for this mechanism, though they are limited. If there is such a good use case, we can use it also without a helper func.

timebertt requested a review from a team as a code owner May 11, 2021 09:52

gardener-robot-ci-2 added the reviewed/ok-to-test label May 11, 2021

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

gardener-robot assigned rfranzke May 11, 2021

gardener-robot-ci-3 added the reviewed/ok-to-test label May 12, 2021

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

rfranzke previously approved these changes May 12, 2021

View reviewed changes

pkg/operation/botanist/botanist.go Show resolved Hide resolved

pkg/operation/botanist/component/extensions/controlplane/controlplane.go Outdated Show resolved Hide resolved

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

timebertt dismissed rfranzke’s stale review via 3f699ca May 12, 2021 14:33

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

timebertt requested a review from rfranzke May 12, 2021 14:35

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

gardener-robot added needs/review and removed needs/review labels May 12, 2021

timebertt added 7 commits May 31, 2021 08:24

Make Network component ready for cached client

1f145ac

Make OperatingSystemConfig component ready for cached client

14f9340

Make Worker component ready for cached client

7348cfd

Fix failing unit test

0aba5e5

Address review feedback

747b41c

timebertt force-pushed the cleanup/directclient-5 branch from df555de to 9cabff7 Compare May 31, 2021 07:10

gardener-robot-ci-1 added the reviewed/ok-to-test label May 31, 2021

gardener-robot unassigned timebertt May 31, 2021

gardener-robot removed reviewed/do-not-merge labels May 31, 2021

timebertt removed the needs/second-opinion label May 31, 2021

gardener-robot-ci-2 removed the reviewed/ok-to-test label May 31, 2021

timebertt mentioned this pull request May 31, 2021

Make DNS components ready for cached client #4141

Merged

rfranzke added the status/in-progress label Jun 1, 2021

rfranzke approved these changes Jun 1, 2021

View reviewed changes

gardener-robot added the reviewed/lgtm label Jun 1, 2021

rfranzke merged commit 2e0ba2a into gardener:master Jun 1, 2021

timebertt deleted the cleanup/directclient-5 branch June 1, 2021 08:43

rfranzke mentioned this pull request Jun 1, 2021

CreateAnd*Patch now GETs the object if it already exists #4144

Merged

This was referenced Jun 8, 2021

CPM Is failing on step "Deleting all extension CRs from the Shoot namespace" #4180

Closed

Fix bugs in CPM introduced by latest refactorings #4191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make extension components ready for cached client #4027

Make extension components ready for cached client #4027

timebertt commented May 11, 2021 •

edited

Loading

rfranzke commented May 11, 2021

timebertt commented May 11, 2021

timebertt commented May 11, 2021

rfranzke left a comment

timebertt commented May 12, 2021

timebertt commented May 31, 2021

rfranzke left a comment

rfranzke commented Jun 1, 2021 •

edited

Loading

timebertt commented Jun 1, 2021

rfranzke commented Jun 1, 2021

timebertt commented Jun 1, 2021

Make extension components ready for cached client #4027

Make extension components ready for cached client #4027

Conversation

timebertt commented May 11, 2021 • edited Loading

rfranzke commented May 11, 2021

timebertt commented May 11, 2021

timebertt commented May 11, 2021

rfranzke left a comment

Choose a reason for hiding this comment

timebertt commented May 12, 2021

timebertt commented May 31, 2021

rfranzke left a comment

Choose a reason for hiding this comment

rfranzke commented Jun 1, 2021 • edited Loading

timebertt commented Jun 1, 2021

rfranzke commented Jun 1, 2021

timebertt commented Jun 1, 2021

timebertt commented May 11, 2021 •

edited

Loading

rfranzke commented Jun 1, 2021 •

edited

Loading