Fix wrong MergePatchOrCreate usage #4057
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gardener-robot
added
kind/bug
|
/invite @ialidzhikov |
timebertt
added a commit
to timebertt/gardener
that referenced
this pull request
May 31, 2021
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.
timebertt
added a commit
to timebertt/gardener
that referenced
this pull request
May 31, 2021
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.
rfranzke
pushed a commit
that referenced
this pull request
Jun 1, 2021
* 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 #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 #4122. * Remove *PatchOrCreate funcs As described in #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.
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Apr 21, 2022
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Apr 21, 2022
* 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.
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Jul 5, 2022
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Jul 5, 2022
* 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/kind bug
When using
client.MergeFrom(e.g. inMergePatchOrCreate), the object should already be filled with the last-known/last applied state, so thatclient.MergeFromwill compute the correct patch including removal of optional fields, that shall be unset with the patch.For this, I added a helper func
GetAndCreateOrMergePatch, which can be used analogously toCreateOrUpdate, but is safe to use with a cached client (won't run into conflicts), as long as operating on fields that are exclusively owned by the controller.What this PR does / why we need it
In one of my recent refactorings I switched to
MergePatchOrCreatefor syncing the cluster resource to the Seed here.However I missed an import detail wrt using
client.MergeFrom:When computing a patch with
client.MergeFromfrom an empty object (like in the usage onmasterlinked above), the patch won't remove optional fields/sections (marked withomitempty) that are now unset in comparison to the current state on the cluster, because the patch does not marshal empty optional fields.For example:
When a Shoot was previously hibernated and the
Clusterobject looked like this:and the
hibernationsection is removed in theShootobject to wake it up, the patch will just look similar to this:{"spec":{"shoot":{"apiVersion":"core.gardener.cloud/v1beta1","kind":"Shoot","metadata":{"creationTimestamp":null},"spec":{"cloudProfileName":"","kubernetes":{"version":""},"networking":{"type":""},"provider":{"type":"","workers":null},"region":"","secretBindingName":""},"status":{"gardener":{"id":"","name":"","version":""},"hibernated":false,"technicalID":"","uid":""}}}}Note, that there is no
hibernationsection in the patch, which means that thehibernationsection in theClusterresource will not be changed/removed.In an ideal world, we would just use SSA (server-side-apply) and don't care about anything of this at all (just send the desired state of the fields we care about and let the API server take care of applying the changes) but unfortunately, we still have to support older k8s versions for Seeds, that don't support SSA.