simplify patching spec and status in reconciler #39
Conversation
This file contains hidden or 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
The way that we were handling patching spec/metadata and status in the main reconciler loop was starting to get awkward and messy. We were calling `patchResource`, `patchResourceStatus` or `patchResourceMetadataAndSpec` in quite a few places, the main `resourceReconciler.Sync()` code path was getting messy and hard to read, and we were in danger of having more subtle bugs creep into the reconciler logic because of the duplicative patch calls and non-obvious nature of the calls. This patch simplifies and cleans up the logic around the core `resourceReconciler.Sync()` code paths in the following ways: * Breaks out the create and update code paths into their own separate `createResource` and `updateResource` methods. * Renames the `cleanup` method to `deleteResource` in order to match the create and update methods * Removes ALL calls to `patchResourceStatus` in favor of always ensuring Status patching is done in the `resourceReconciler.HandleReconcileError` wrapper method * Removes the `resourceReconciler.patchResource` method entirely (which patched both Metadata/Spec as well as Status) and ensures that the `createResource`, `updateResource` and `deleteResource` methods on `resourceReconciler` only ever call `patchResourceMetadataAndSpec` (since `patchResourceStatus` is now always called by the `HandleReconcileError` wrapper) These changes should make the late initialize work easier to fit into the core reconciler logic and provide a more explicit contract about what is responsible for patching status (**only** the `HandleReconcileError` wrapper) and what is responsible for patching metadata and spec (the `createResource`, `updateResource` and `deleteResource` methods).
vijtrip2
approved these changes
Jul 30, 2021
| // No changes to metadata or spec so Patch on the object shouldn't be done | ||
| kc.AssertNotCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) | ||
| statusWriter.AssertCalled(t, "Patch", ctx, latestRTObj, client.MergeFrom(desiredRTObj)) | ||
| // Only the HandleReconcilerError wrapper function ever calls patchResourceStatus |
There was a problem hiding this comment.
nit: should the test be renamed to TestSync_Update() , which is correct representation of the test.
a-hilaly
approved these changes
Jul 30, 2021
Comment on lines
+57
to
+58
| // NOTE(jaypipes): This is really only here for dependency injection | ||
| // purposes in unit testing in order to simplify test setups. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, vijtrip2 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:
Approvers can indicate their approval by writing |
RedbackThomson
pushed a commit
to RedbackThomson/ack-runtime
that referenced
this pull request
Aug 2, 2021
The way that we were handling patching spec/metadata and status in the main reconciler loop was starting to get awkward and messy. We were calling `patchResource`, `patchResourceStatus` or `patchResourceMetadataAndSpec` in quite a few places, the main `resourceReconciler.Sync()` code path was getting messy and hard to read, and we were in danger of having more subtle bugs creep into the reconciler logic because of the duplicative patch calls and non-obvious nature of the calls. This patch simplifies and cleans up the logic around the core `resourceReconciler.Sync()` code paths in the following ways: * Breaks out the create and update code paths into their own separate `createResource` and `updateResource` methods. * Renames the `cleanup` method to `deleteResource` in order to match the create and update methods * Removes ALL calls to `patchResourceStatus` in favor of always ensuring Status patching is done in the `resourceReconciler.HandleReconcileError` wrapper method * Removes the `resourceReconciler.patchResource` method entirely (which patched both Metadata/Spec as well as Status) and ensures that the `createResource`, `updateResource` and `deleteResource` methods on `resourceReconciler` only ever call `patchResourceMetadataAndSpec` (since `patchResourceStatus` is now always called by the `HandleReconcileError` wrapper) These changes should make the late initialize work easier to fit into the core reconciler logic and provide a more explicit contract about what is responsible for patching status (**only** the `HandleReconcileError` wrapper) and what is responsible for patching metadata and spec (the `createResource`, `updateResource` and `deleteResource` methods). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ack-bot
pushed a commit
that referenced
this pull request
Aug 4, 2021
Fixes: aws-controllers-k8s/community#886 The reconciler will indefinitely requeue even when there have been no changes made to the `spec` or `status`. This is happening in both the adoption and resource reconciler types. The root cause comes from `controller-runtime` requeue-ing the resource when we patch the `status` subresource - see [this issue](kubernetes-sigs/kubebuilder#618) for details. This bug was introduced for the resource reconciler as part of #39 , since we now call `patchResourceStatus` on every reconcile loop. This fix adds an event filter to each manager, with a predicate that the resource must have changed generation. The generation is not changed unless there has been a modification to the `spec`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The way that we were handling patching spec/metadata and status in the
main reconciler loop was starting to get awkward and messy. We were
calling
patchResource,patchResourceStatusorpatchResourceMetadataAndSpecin quite a few places, the mainresourceReconciler.Sync()code path was getting messy and hard toread, and we were in danger of having more subtle bugs creep into the
reconciler logic because of the duplicative patch calls and non-obvious
nature of the calls.
This patch simplifies and cleans up the logic around the core
resourceReconciler.Sync()code paths in the following ways:createResourceandupdateResourcemethods.cleanupmethod todeleteResourcein order to match thecreate and update methods
patchResourceStatusin favor of always ensuringStatus patching is done in the
resourceReconciler.HandleReconcileErrorwrapper methodresourceReconciler.patchResourcemethod entirely (whichpatched both Metadata/Spec as well as Status) and ensures that the
createResource,updateResourceanddeleteResourcemethods onresourceReconcileronly ever callpatchResourceMetadataAndSpec(since
patchResourceStatusis now always called by theHandleReconcileErrorwrapper)These changes should make the late initialize work easier to fit into
the core reconciler logic and provide a more explicit contract about
what is responsible for patching status (only the
HandleReconcileErrorwrapper) and what is responsible for patchingmetadata and spec (the
createResource,updateResourceanddeleteResourcemethods).By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.