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

Add predicates to allow filtering watcher streams #911

Merged
merged 20 commits into from Apr 3, 2023
Merged

Add predicates to allow filtering watcher streams #911

merged 20 commits into from Apr 3, 2023

Conversation

clux
Copy link
Member

@clux clux commented May 13, 2022

A re-implementation of the go predicates from controller-runtime that allows filtering watcher streams from a new (unstable) WatchStreamExt::predicate_filter:

let changed_pods = watcher(pods, watcher::Config::default())
    .applied_objects()
    .predicate_filter(predicates::generation);

while let Some(pod) = changed_pods.try_next().await? {
   info!("saw Pod '{} with hitherto unseen generation", pod.name_any());
}

This allows filtering out events that only changes (say) the status object. It contains a set of common extractor functions (under runtime::predicates) and operates through PredicateFilter. This stream utility contains a small HashMap with hashes of the last seen value for each object, and simply compares that value (from the extractor) with the one in the new watch event.

notes and origin and discussion links

based on user predicates that allow short-circuiting `reconciler` runs if nothing important has changed:

see this commit which was used in the reconciler as an early exit gate via:

    if !generation.cmp_update(&mut *ctx.predicate_cache.write().await, &doc) {
        info!("ignoring generationally equivalent reconcile for {}", doc.name());
        return Ok(Action::requeue(Duration::from_secs(30 * 60)));
    }

relying on passing an thread-safe Evaluation cache via the reconcile context. This method has huge problems when used in controllers with more than one type, because this setup only filters based on the merged QueueStream. Crucially, if an owned object changes, it won't be captured in the root type, so you might miss important changes to child objects. Doing the check inside the reconciler is too late.

So for controller usage we have to do predicate style filters on the watcher streams instead, and have users configure predicates PER stream, then pass them to the controller using stream sharing for this to work cohesively.

Signed-off-by: clux <sszynrae@gmail.com>
@codecov-commenter

This comment was marked as off-topic.

@clux
Copy link
Member Author

clux commented May 13, 2022

Previous potential problems noted about using predicates for controllers that came up before and wanted to note them.
This is ultimately not directly related to the interface in this PR because this is only the watcher interface, which is still useful for people running straight watcher loops.

That said, I no longer think they are objections worth blocking this for. Here are the problems that came up:

1. It inhibits requeues / retriggers / only accounts for the last objects in the controller's queue stream

The system is meant to requeue based on time, and allow users to re-trigger in case something bad happened, so it is essential that this is not filtered out at the controller level. The solution is to not do this filtering inside the controller; let users filter outside (via stream sharing), and always propagate requeues/retriggers.

2. It potentially encourages non-idempotent modes of operation of a reconciler

I.e. less redundant work scheduled, leads to people being lazier. Yes, true, but isn't that a good thing?
Ultimately people want to write less code. This allows them to do that. It is an advanced feature, but i have several controllers where we i effectively have to filter out generation changes out of band, and awkwardly, so this would be very useful.

It's also not like we are doing filtering across all streams with the same brush. This is intended to be setup in the way you want for each stream.

EDIT: edited after more clarity, since it was the most matching comment.

clux added 3 commits May 14, 2022 23:25
PoC on node_reflector to only show changed labels

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Add predicates to filter reconciler runs Add predicates to allow filter watcher streams May 15, 2022
@clux clux changed the title Add predicates to allow filter watcher streams Add predicates to allow filtering watcher streams May 15, 2022
clux added 2 commits May 15, 2022 13:19
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux

This comment was marked as outdated.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added the changelog-add changelog added category for prs label May 15, 2022
Signed-off-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux marked this pull request as ready for review August 8, 2022 18:53
Signed-off-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Eirik A <sszynrae@gmail.com>
@clux clux added the unstable feature that unstable feature gating label Mar 29, 2023
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Mar 29, 2023

Wrapped this in an unstable feature and added better docs. Hoping to get this in so we can iterate if we find corner cases. I still have not found good ways to solve this, and other people keep asking about it. I think this can actually be useful with something like #1163 as an early place to experiment with stream sharing with stuff like #1163. cc @Dav1dde

Deny will fail because of a dev-dep duplicate (it's fixed in #1171)

@clux clux requested a review from a team March 29, 2023 17:00
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added this to the 0.81.0 milestone Mar 29, 2023
@clux clux changed the title Add predicates to allow filtering watcher streams Add predicates to allow filtering watcher streams Mar 29, 2023
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Apr 3, 2023

Going to send this through under the unstable feature to let it stew for a while. This is a simplistic solution and may benefit from some TTL style decay (or moving it before the flatteners to get access to raw watcher::Events) in the future. But, in the mean time, would like to run this in some of my controllers for a while to see how well it works, and if that type of over-engineering is necessary.

@clux clux merged commit dbd51df into main Apr 3, 2023
18 checks passed
@clux clux deleted the predicates branch April 3, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs unstable feature that unstable feature gating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow filtering watcher events when !observedGeneration changed
4 participants