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

Dispatch delete events on .reflect_shared #1590

Open
pando85 opened this issue Oct 2, 2024 · 3 comments
Open

Dispatch delete events on .reflect_shared #1590

pando85 opened this issue Oct 2, 2024 · 3 comments
Labels
bug Something isn't working runtime controller runtime related unstable feature that unstable feature gating

Comments

@pando85
Copy link

pando85 commented Oct 2, 2024

Current and expected behavior

I have a dummy controller that creates a deployment based on a CRD. I'm watching the deployments this controller owns and triggering reconcile events on changes there.

Current behavior doesn't dispatch delete events: current code

I expect to apply changes if my CRD defines a deployment that should be running and that deployment disappears.

Possible solution

I did a workaround sending a bulk to reconcile on Event::Delete(_) to the controller through a mpsc::channel. This is an ugly workaround because of the architecture and because it forces all objects in the controller to reconcile.

Additional context

No response

Environment

kubectl version
Client Version: v1.30.3
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.30.4

Configuration and features

kube = { version = "0.95", default-features = true, features = ["client", "derive", "unstable-runtime"] }

Affected crates

kube-runtime

Would you like to work on fixing this bug?

yes

@pando85 pando85 added the bug Something isn't working label Oct 2, 2024
@clux clux added runtime controller runtime related unstable feature that unstable feature gating labels Oct 2, 2024
@clux
Copy link
Member

clux commented Oct 2, 2024

Yeah, this stuff is a bit awkward / not finalised at the moment: #1472

The shared interface, in its main loop does basically two things

  1. apply_watcher_event :: same as for regular reflectors
  2. dispatch_event :: to subscribers, only for shared reflectors

And the dispatcher there currently has the setup of not passing up the delete events. Sometimes this is right (watches/owns relation typically don't care about deletes), sometimes it is not (root stream cares about deletes because it needs to finalize).

For regular (non-shared) reflectors, the choice of whether to receive Delete events actually happens later in the filter pipeline by using one of applied_objects or touched_objects either implicitly or explicitly in the streams api.

The subscriber cannot use those flattener interfaces because the stream Item in the shared stream interfaces require an Arc<Child> (see e.g. owns_shared_stream).

I don't have a great answer for how to suitably fix this today. We were discussing this a bit with @mateiidavid a while ago, and we were considering doing reworking the EventFlatten interface (partly started in #1517). We might need to go further here; allow the two different stream sources to be "flattened/filtered" (terminology is wrong now, pr is fixing that).

@pando85
Copy link
Author

pando85 commented Oct 3, 2024

Thanks for your very explanatory answer. Now I understand better the complexity and the caveats that the current crate has.

I can offer myself if I can solve small things that have already been decided. I will keep an eye on this.

I love this project and I think that it is great to add these missing features to push forward Rust operators. Thank you, @clux for all your work.

@mateiidavid
Copy link
Contributor

Just to echo out @clux it would be great to fix this. I think at the time when we first implemented this, we made it so that a shared reflector receives a reference to an object and then retrieves it from the store. IIRC for deletes this did not work (since you can't retrieve a delete object from the store; store operation happens first).

I'd have to page back into this, but we could perhaps:

  • Figure out in the shared stream if we are dealing with a deleted object (if it's not in the store, assume it's deleted?)
  • Come up with a type that encodes whether an object was deleted? (instead of passing object_refs)

...in addition to everything suggested above. I think the pagination work changed a few of our assumptions and possibly made it easier for us to come up with a more robust API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working runtime controller runtime related unstable feature that unstable feature gating
Projects
None yet
Development

No branches or pull requests

5 participants
@clux @pando85 @mateiidavid and others