Skip to content

Don't remove finalizer on reschedule#49

Open
alex-hunt-materialize wants to merge 1 commit into
mainfrom
dont_remove_finalizer_on_reschedule
Open

Don't remove finalizer on reschedule#49
alex-hunt-materialize wants to merge 1 commit into
mainfrom
dont_remove_finalizer_on_reschedule

Conversation

@alex-hunt-materialize

Copy link
Copy Markdown
Contributor

We need to be able to reschedule cleanup tasks without throwing errors and without removing the finalizer. The kube_runtime::finalizer helpers don't allow this, so we need to manage the finalizer ourselves.

@alex-hunt-materialize alex-hunt-materialize force-pushed the dont_remove_finalizer_on_reschedule branch from 84adac3 to c96294a Compare June 3, 2026 17:19
We need to be able to reschedule cleanup tasks without throwing errors
and without removing the finalizer. The kube_runtime::finalizer helpers
don't allow this, so we need to manage the finalizer ourselves.
@alex-hunt-materialize alex-hunt-materialize force-pushed the dont_remove_finalizer_on_reschedule branch from c96294a to 7504ff3 Compare June 3, 2026 17:32
@alex-hunt-materialize alex-hunt-materialize marked this pull request as ready for review June 3, 2026 17:39

@doy-materialize doy-materialize left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm, i'm really not a fan of reimplementing everything like this. what is the actual problem we're running into here? could we fix the callers instead? alternately, could we translate an Action::requeue into an error in the Event::Cleanup codepath? it really feels like there should be a way to get the behavior we want without fully just copying and pasting the code around like this.

@alex-hunt-materialize

alex-hunt-materialize commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

We could convert it into an error, but that tends to spam logs. We don't want to have a bunch of error messages for what is actually a very normal event. Errors are also requeued immediately, which would make this spam much worse.

We'd also have to handle this everywhere we use this library, and it leaves a very dangerous footgun that isn't obvious to users. There is nothing stopping a user from thinking that they are doing something reasonable by returning Ok(Action), but that silently removes the finalizer.

Honestly, this should probably be fixed in the upstream kube-runtime. I'm going to open a PR there.
kube-rs/kube#1994

@doy-materialize

doy-materialize commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

yeah, if we can fix this upstream, that would be best. if that ends up not being possible, we could change the trait api here to prevent people from passing a requeue action (like, have cleanup just return Result<(), Error> instead), or we could translate requeue actions in cleanup into an error before we return them from finalize (we control the error reporting, so this doesn't necessarily need to lead to error log spam - we could just wrap the errors that are returned in something like enum ControllerError<E> { User(E), CleanupRequeue(Duration) } or whatever, and just not emit log lines for the CleanupRequeue variant (this would also allow us to do the right thing in the error handler by returning the requeue action there directly instead of calling error_action)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants