Predicate filters will typically lead you to miss deletions

[jackkleeman]

Current and expected behavior

It appears to me that the objects that come with kube watch deletion events don't have a deletion timestamp set, or any other change that might allow you to conclude that its been deleted. When you use .touched_objects(), you totally lose the context that this object is going through a deletion, its like any other reconciliation - there are good reasons for this. However, if you are using a predicate (which leads events to be dropped when the object is identical) you will typically drop deletions, as they have no change in the objects except for resourceVersion, which I typically wouldn't look at in a predicate.

I don't know for sure if this is a bug or just surprising behaviour, but its definitely very easy to have predicates that miss deletions. What it means in practice is that my operators won't recreate deleted resources, at least until some periodic timer fires. I wonder should the predicate have access to some context that this is a deletion, in which case perhaps the predicate shouldn't apply, or can be allowed to return a different hash value?

Possible solution

Here's my workaround:

let pdb_watcher = metadata_watcher(pdb_api, cfg.clone())
        .map(|event| ensure_deletion_change(event))
        .touched_objects()
        .predicate_filter(changed_predicate);

fn ensure_deletion_change<K: Resource, E>(
    mut event: Result<kube::runtime::watcher::Event<K>, E>,
) -> Result<kube::runtime::watcher::Event<K>, E> {
    if let Ok(kube::runtime::watcher::Event::Delete(ref mut object)) = event {
        let meta = object.meta_mut();
        meta.generation = match meta.generation {
            Some(val) => Some(val + 1),
            None => Some(0),
        }
    }
    event
}

Essentially I increment generation for deletion events so that I know there has been an observed change which my predicate can check for. An alternative would be to update the deletion timestamp to be now (which will generally not be exactly identical to an existing deletion timestamp on the object, if any)

Additional context

No response

Environment

.

Configuration and features

No response

Affected crates

kube-runtime

Would you like to work on fixing this bug?

maybe

[clux]

Huh. That is interesting. It surprising if WatchEvent Deletes comes out without a deletion_timestamp set, but I have also never actually needed to rely on predicates in a way that also needed delete context. Thanks for the report and clean snippet.

I'd say this would be more a Kubernetes quirk/bug than a bug on our end; as we do deliberately truncate event context for controllers to force more idempotent patterns; so here's the standard disclaimer first; we generally try to advocate for finalisers/owner refs for GC, or relying on idempotent checks in a reconciler (is my related pdb still there), since delete events are never guaranteed to be received (deletes can happen while the controller is down or watch desynced).

But that said, your check and workaround should work fine as a 95-99% solution (depending on how happy you are in your HA/kubernetes watch api/your eventual consistency). I also think mutating the deletion_timestamp would be better for a downstream controller in terms of user expectations on the api. This could be something to include in our predicates module as some kind of pre-predicate safety-map thingy (would need a good name/placement and a way to disambiguate these things from regular predicates though).

[jackkleeman]

Totally agree re not relying on deletion events - its just that if the controller is up and synced, i want deletion of owned resources to trigger a reconciliation, so I can recreate the resource (for example). If the controller is down, or desynced, it should trigger a reconcile of everything when it comes back. One hopes...

i agree its probably a kubernetes quirk (seems to be specifically resources that get deleted in a non-graceful way, so not pods, but for example poddistruptionbudgets, probably crds too).

Yes perhaps we just need a well known function that you can pipe your stream before touched_objects such that deletionTimestamp is always set, then predicates can use this field to detect change. It may still be unintuitive if generation doesn't change, though.