Host deletion failures cannot propagate: operator removes the CHI finalizer unconditionally, orphaning PVCs

[somanchi004-code]

The finalizer.clickhouseinstallation.altinity.com finalizer exists to hold the CR
in the API server until the operator has cleaned up what Kubernetes cannot clean up
itself. On the delete path it is removed unconditionally — no failure below it can
prevent that.

I hit this while looking at deleteHost() and then reproduced it on a single-node
k3s cluster. Details and logs below.

The chain

Every level either discards the error below it or returns nil regardless
(pkg/controller/chi/worker-deleter.go, at 1a3fc2733 / 0.27.2):

where	what happens
:540	if host.Runtime.CurStatefulSet, err = w.c.kube.STS().Get(ctx, host); err != nil — any error, not just NotFound, takes the "already deleted" branch: emits EventReasonDeleteCompleted, returns nil
:579-580	consequently deleteTables (ZooKeeper cleanup) and w.c.deleteHost (PVC deletion) are both skipped
:634	_ = w.deleteHost(ctx, chi, h) — error discarded; shard emits DeleteCompleted regardless
:676	deleteShard can only return nil; cluster emits DeleteCompleted regardless
:747	_ = w.deleteCRProtocol(ctx, new) — error discarded
:755	uninstallFinalizer runs regardless; deleteCHI returns true

Why PVCs specifically

pkg/model/common/creator/stateful-set.go:40 sets OwnerReferences on the
StatefulSet, as do pdb.go:38 and secret.go:34. pvc.go is the exception — owner
references are commented out in two places, both annotated:

// Incompatible with PV retain policy
// Fails PV retain policy test (19)
// OwnerReferences: c.or.CreateOwnerReferences(c.cr),

That is a deliberate choice for retain-policy support, and it means Kubernetes GC can
never reclaim these PVCs. The call at :580 is the only reclaim path that exists.
Once the finalizer is removed and the CR is gone, there is no object left to
re-reconcile, so the operator cannot retry either.

For the ZooKeeper half, the comment at :574-576 states the requirement: "We need to
delete tables on the host in order to clean Zookeeper data. If just delete tables,
Zookeeper will still keep track of non-existent tables."

Reproduction

Single-node k3s v1.36.2+k3s1, operator 0.27.2 from
deploy/operator/clickhouse-operator-install-bundle.yaml, PVCs on Ceph RBD
(rook-ceph-block). One CHI, 1 shard × 1 replica, one 1Gi volume.

I induced the failure by removing get (only get) on statefulsets from the
operator's ClusterRole, so the call at :540 returns Forbidden. That is a
deterministic stand-in for the transient errors that reach the same branch — and a
plausible trigger by itself, e.g. partially-applied RBAC.

Control — unmodified RBAC, kubectl delete chi repro: CHI, StatefulSet, pod, PVC
and PV all removed within ~36s. The operator's PVC deletion works normally.

With get on statefulsets removed:

$ kubectl auth can-i get statefulsets \
    --as=system:serviceaccount:kube-system:clickhouse-operator -n chi-test
no

$ kubectl -n chi-test delete chi repro
clickhouseinstallation.clickhouse.altinity.com "repro" deleted

$ kubectl -n chi-test get chi,sts,pods
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   3m9s

$ kubectl get pv | grep chi-test
pvc-2b49bdbf-...   1Gi   RWO   Delete   Bound   chi-test/data-volume-chi-repro-c1-0-0-0   rook-ceph-block

The CR is gone, the StatefulSet is gone (Kubernetes GC via its owner reference), and
the PVC and PV remain bound with nothing left to reclaim them.

Operator log for that delete:

21:39:40.107470  worker-deleter.go:624  Delete shard: chi-test/0 - started
21:39:40.114280  worker-deleter.go:643  Delete shard: chi-test/0 - completed
21:39:40.114597  worker-deleter.go:538  Delete host: c1/0-0 - started
21:39:40.117662  worker-deleter.go:683  Delete cluster: chi-test/c1 - completed
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:
                 User "system:serviceaccount:kube-system:clickhouse-operator" cannot get
                 resource "statefulsets" in API group "apps" in the namespace "chi-test"
21:39:40.889626  worker-deleter.go:335  Delete CHI completed

Four DeleteCompleted events were emitted (host, shard, cluster, CHI), all Info,
none Warning.

Second, smaller thing visible in the same log

deleteShard (:629-636) starts host deletions as goroutines and emits its
"completed" event immediately after WalkHosts returns; wg.Wait() is only at
:321. In the trace above shard-completed (.114280) and cluster-completed
(.117662) both precede the host finishing (.121144), and shard-completed even
precedes host-started (.114597).

Suggested direction

The narrow part — not claiming success on an unknown error — I have sent as a
separate PR, since apiErrors.IsNotFound is already used that way 8 times in this
package (e.g. worker-pdb.go:41).

That alone is not sufficient, and I confirmed as much by running it. With the patched
operator the host event becomes DeleteFailed — but the PVC is still orphaned and the
CHI is still deleted, because :634 and :747 discard the returned error and the
finalizer comes off regardless:

Error  DeleteFailed     Delete host: c1/0-0 - unable to get StatefulSet, host deletion not performed
Info   DeleteCompleted  Delete CHI completed

So error classification is necessary but the leak is really caused by the missing
propagation. Making
the finalizer meaningful means letting failure reach deleteCHI so removal is skipped
and the CR requeued — which is a design decision about retry policy and about how long
a CHI should be allowed to resist deletion. I did not want to assume an answer, hence
this issue rather than a second patch.

One wrinkle worth flagging for whoever picks this up: PVC deletion partly depends on
the StatefulSet that could not be fetched. PVCDeleter.HostCanDeletePVC
(pkg/model/common/volume/deleter.go:44) walks volume mounts via
api.CurStatefulSet, and defaults to PVCReclaimPolicyDelete when it finds nothing.
So simply proceeding after a failed Get would delete PVCs whose reclaim policy could
not be confirmed — which seems worse than retrying. That is the main reason I did not
put it in the PR.

Happy to do the work on whichever shape you prefer.

What I verified and what I did not

Ran and observed: everything under Reproduction — the control delete, the
403-injected delete, the surviving PVC/PV, the log lines and timestamps quoted above,
and the four DeleteCompleted events. Also the patched-operator run quoted under
Suggested direction, where the host event becomes DeleteFailed and the PVC is still
orphaned. Cluster as described.

Traced in the source, not observed: that an ordinary transient error (timeout,
connection reset, 429) reaches the same branch as the Forbidden I injected. That
follows from the err != nil test at :540, but I did not induce a network fault to
confirm it. I also did not verify the ZooKeeper side — the reproduction had no Keeper
and a single non-replicated host, so nothing was left in ZooKeeper to observe. The ZK
claim rests on the code path and the project's own comment at :574-576, not on a run.

Related but, as far as I can tell, distinct: #1927 (ZK entries left on shard
removal — root-caused there to the no-op shard callback in dropZKReplicas and
hostToRunOn resolution) and #1943 (SYSTEM DROP REPLICA not retried). Both sit in
this file and share the symptom space; neither covers the error classification at
:540 or the unconditional finalizer removal. If you consider this a facet of #1927 I
am glad to move it there instead.