Skip to content

Do not report host deletion as completed on an unknown API error - #2057

Open
somanchi004-code wants to merge 1 commit into
Altinity:masterfrom
somanchi004-code:fix/delete-host-error-classification
Open

Do not report host deletion as completed on an unknown API error#2057
somanchi004-code wants to merge 1 commit into
Altinity:masterfrom
somanchi004-code:fix/delete-host-error-classification

Conversation

@somanchi004-code

Copy link
Copy Markdown

deleteHost() fetches the host's StatefulSet and treats any error from that Get as
"already deleted":

if host.Runtime.CurStatefulSet, err = w.c.kube.STS().Get(ctx, host); err != nil {
    ... EventReasonDeleteCompleted ...
        Info("Delete host: %s/%s - completed StatefulSet not found - already deleted? err: %v", ...)
    return nil
}

Only NotFound means the host is actually gone. A Forbidden, a timeout, a 429 or a
reset connection all take the same branch, and the early return skips the two cleanup
steps below it:

  • deleteTables() — which the comment just above it notes is needed so ZooKeeper stops
    tracking the host's tables
  • Controller.deleteHost() — which deletes the host's PVCs

The PVCs matter most because they have no owner reference. creator/pvc.go comments
it out in two places to stay compatible with the PV retain policy, so Kubernetes GC
will not reclaim them and this call is the only thing that ever does.

This PR only changes the classification: NotFound keeps today's behaviour, anything
else emits DeleteFailed and returns the error instead of reporting success.
apiErrors.IsNotFound is already used this way 8 times in this package — worker-pdb.go:41
is the same get / classify / act shape.

Verified on a cluster

Single-node k3s v1.36.2+k3s1, operator 0.27.2, PVCs on Ceph RBD. To make the failure
deterministic I removed get on statefulsets from the operator's ClusterRole, so the
call returns Forbidden.

Before this change, deleting the CHI removed the CR and the StatefulSet but left
the PVC and PV bound, with no CR remaining to reconcile them away:

21:39:40.121144  worker-deleter.go:545  Delete host: c1/0-0 - completed StatefulSet not found -
                 already deleted? err: statefulsets.apps "chi-repro-c1-0-0" is forbidden: ...
21:39:40.889626  worker-deleter.go:335  Delete CHI completed
$ kubectl -n chi-test get chi,sts
No resources found in chi-test namespace.
$ kubectl -n chi-test get pvc
data-volume-chi-repro-c1-0-0-0   Bound   pvc-2b49bdbf-...   1Gi   RWO   rook-ceph-block

A control run with unmodified RBAC removed CHI, StatefulSet, pod, PVC and PV cleanly,
so the normal path works — the leak is specific to the misclassified error.

After this change, the same delete reports the failure instead of claiming success:

Error  DeleteFailed  Delete host: c1/0-0 - unable to get StatefulSet, host deletion
                     not performed. err: statefulsets.apps "chi-repro-c1-0-0" is
                     forbidden: ...

I also re-created the CHI under the patched build first and it reconciled to
Completed normally, so the NotFound path is unaffected.

Scope

This is deliberately narrow, and on its own it does not stop the leak. In the run
above the PVC was still orphaned and the CHI was still deleted, because the error this
patch now returns is discarded by the callers at worker-deleter.go:634 and :747 and
the finalizer is removed regardless. Fixing that means deciding a retry policy, which is
not mine to decide, so I have raised it separately in #2056 rather than widening this
patch. What this change buys by itself is that the operator stops reporting a deletion
it did not perform.

I also considered simply proceeding with cleanup after a failed Get, but
PVCDeleter.HostCanDeletePVC walks volume mounts via api.CurStatefulSet and defaults
to PVCReclaimPolicyDelete when it finds none — so that path would delete PVCs whose
reclaim policy could not be confirmed. Returning the error seemed clearly safer.

Notes

No behaviour change when the StatefulSet is genuinely absent, which is the common case
this branch was written for.

AI disclosure: I used Claude Code while investigating this. The reproduction above was
run by me on my own cluster, and I have described separately in #2056 which parts I
observed and which I only traced in the source.

deleteHost() fetches the host StatefulSet and treats any error from that
Get as "StatefulSet not found - already deleted", emitting a
DeleteCompleted event and returning nil. Only NotFound actually means the
host is gone; a Forbidden, a timeout or any other transient API error
takes the same branch.

That early return skips both deleteTables() - which the surrounding
comment notes is required so ZooKeeper stops tracking the host's tables -
and Controller.deleteHost(), which deletes the host's PVCs. Those PVCs
carry no owner reference (see model/common/creator/pvc.go, where it is
commented out to stay compatible with the PV retain policy), so the
operator's own call is the only thing that ever reclaims them.

Classify the error instead: keep the existing behaviour for NotFound, and
on any other error emit DeleteFailed and return it rather than claiming
the host was deleted. apiErrors.IsNotFound is already used this way
elsewhere in the package, for example in worker-pdb.go.

Signed-off-by: Somanchi Poorna Sobhita <somanchi004@gmail.com>
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.

1 participant