Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependent Resource Event Filtration with Previous version as Annotation #2249

Open
csviri opened this issue Feb 26, 2024 · 1 comment
Open
Labels
dependent-resources-epic needs-discussion Issue needs to be discussed more before working on it
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented Feb 26, 2024

When a KubernetesDependentResource makes an update, it adds an annotation with the actual version of the resource:

} else if (usePreviousAnnotation(context)) { // set a new one
eventSource().orElseThrow().addPreviousAnnotation(
Optional.ofNullable(actualResource).map(r -> r.getMetadata().getResourceVersion())
.orElse(null),
target);
}

This is then used to if an event comes it could be skipped (won't trigger the reconciliation) since it is a result of the controller. This halves the reconciliation execution with the same result.
The actual setup that uses matchers works well. No functional issue.

The matchers (SSA based matcher by default) are however an optimization, that other controller might or might do, also there are some special cases that are hard to handle. So in case the matcher simply returns false, and the reconciliation is triggered, while the controller of the dependent resource (not our controller) made changes, the update done by the dependent resource (with the changed resources version in the annotation) will trigger an update (therefore an update event in the informer). While this will be filtered out, it would not create an event (thus there would not be a change (also no resourceVersion change) if the annotation with resource version is not changed.

This causes a real problem just in the case that the controller of the resource is reacting on every change, a typical (and not found more yet) is the Deployment, where there is somehow an exceptional functionality if the annotations are changed the generation is changed in the metadata. What triggers a reconciliation of the Deployment, that results in a change of observedGeneration. Which again triggers the reconciliation on our side. (If the matchers are in place this not causes an infinite loop).

So there are things here:

  • in some cases, we might produce additional changes to the resource using the annotation
  • if the matchers are turned off in some very rare cases this might cause a loop in the reconciliation (in combination with the resources controller, like Deployment)

Note that there is a feature flag there to not use this annotation.

Should we do anything regarding this? Well would be nicer to have a simple algorithm without the annotation. The previous algorithm was more complex but did not use the annotation. So we might revisit this and see if we can do better.

@shawkins
Copy link
Collaborator

if the matchers are turned off in some very rare cases this might cause a loop in the reconciliation (in combination with the resources controller, like Deployment)

Would you just consider adding java docs to the ResourceUpdaterMatcher.matches and KubernetesDependentResource.match methods warning about this scenario?

The only simple alternative I can think of would be make the previous value based upon a hash rather than the resourceVersion. That will make it stable (as long as the serialized form of the resource is stable), but will be more intensive to compute / compare.

@csviri csviri added this to the 5.0 milestone Mar 19, 2024
@csviri csviri added the needs-discussion Issue needs to be discussed more before working on it label Mar 19, 2024
@csviri csviri removed this from the 5.0 milestone Mar 27, 2024
@csviri csviri added this to the 5.2 milestone Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent-resources-epic needs-discussion Issue needs to be discussed more before working on it
Projects
None yet
Development

No branches or pull requests

2 participants