helm: fix dependent resource watches that miss changes - #7138
Open
AbhishekPathania wants to merge 4 commits into
Open
AbhishekPathania wants to merge 4 commits into
AbhishekPathania wants to merge 4 commits into
Conversation
The release hook decides per dependent whether to watch it through owner references or through annotations. It reads the stored release manifest, where a template that omits metadata.namespace has an empty namespace, so SupportsOwnerReference sees a namespace mismatch and picks the annotation handler. Helm installs such resources into the release namespace, where ownerRefInjectingClient.Build gives them owner references, so no handler matched them and changes waited for the next periodic reconcile. Use the release namespace when the resource has none. Resources with an explicit namespace keep it, because SupportsOwnerReference uses a non-empty depNamespace in place of the resource's own namespace. Signed-off-by: Abhishek Pathania <theabhishekpathania@gmail.com>
Watches were keyed by GVK, so the first resource of a kind decided the handler for every resource of that kind. A kind rendered both in the release namespace (owner references) and elsewhere (annotations) got only one handler, and because releaseutil.SplitManifests returns a map, which one it got could change on every operator restart. Key watches by GVK and handler type. Both watches share one informer. Signed-off-by: Abhishek Pathania <theabhishekpathania@gmail.com>
ownerRefInjectingClient.Build annotates resources that carry helm.sh/resource-policy: keep instead of giving them owner references, so that deleting the custom resource does not garbage-collect them. The watch decision did not apply the same rule and watched them through owner references. helm-operator-plugins fixed the same mismatch in operator-framework/helm-operator-plugins#83. Export ContainsResourcePolicyKeep so both paths use the same check. Signed-off-by: Abhishek Pathania <theabhishekpathania@gmail.com>
SetOwnerAnnotations writes the owner's GroupKind to operator-sdk/primary-resource-type, and EnqueueRequestForAnnotation only enqueues when that annotation equals its Type. Type was set to the dependent's GroupKind, so the handler never enqueued anything and changes to cluster-scoped, cross-namespace and resource-policy keep dependents were never reconciled. This is the behavior reported in operator-framework#2727, which operator-framework#2987 set out to fix. Use the owner's GroupKind, as helm-operator-plugins does. Signed-off-by: Abhishek Pathania <theabhishekpathania@gmail.com>
This branch has not been deployed
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
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.
Description of the change:
With
watchDependentResources: true, editing or deleting a resource from the chart should reconcile the owning custom resource. For many resources it does not. The event reaches the controller, but no handler enqueues the owner, so the change stays until the next periodic reconcile.watchDependentResourcespicks a handler per dependent:EnqueueRequestForOwner, orEnqueueRequestForAnnotationfor resources thatownerRefInjectingClient.Buildannotated instead of owning. That choice has to match whatBuilddid to the resource, and it did not in four cases. Each commit fixes one of them and adds a test for it.metadata.namespacehas an empty namespace in the stored manifest, so it was compared against""and got the annotation handler. Helm installs it into the release namespace, whereBuildgives it an owner reference. The release namespace is now used when the resource has none.releaseutil.SplitManifestsreturns a map, which one could change on every restart. Watches are now keyed by GVK and handler type.helm.sh/resource-policy: keepare annotated instead of owned since fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation #4389, but the watch decision did not check forkeep. It now uses the same helper asBuild. helm-operator-plugins made the same change in objects with resource policy "keep" should use the annotation-based watch helm-operator-plugins#83.EnqueueRequestForAnnotationonly enqueues whenoperator-sdk/primary-resource-typeequals itsType.SetOwnerAnnotationswrites the owner's GroupKind there, butTypewas the dependent's GroupKind, so the handler never enqueued anything. This covers every cluster-scoped and cross-namespace dependent, which is the behavior reported in Helm Operator missing ownerReferences for ClusterRole and ClusterRoleBinding #2727.Typeis now the owner's GroupKind, as in helm-operator-plugins.The tests read the handler from controller-runtime's
source.Kindthrough reflection, becausesource.Kinddoes not expose it.Motivation for the change:
We run a Helm-based operator with a longer reconcile period and rely on dependent watches to undo manual changes to chart resources. On v1.42.1, changes to cluster-scoped resources, resources in other namespaces and
keepresources were not undone until the next periodic reconcile, and neither were some kinds in the release namespace. With these commits they trigger a reconcile within about a second.Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs