Altinity / Altinity/clickhouse-operator

ZooKeeper entries not cleaned up when a shard is removed

Open
#1,927 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Go
Stars
2.6k
Forks
574
Avg merge
8d 6h
Merged PRs (30d)
6

Description

Summary

When a shard is removed from a CHI (e.g. reducing shardsCount from 2 to 1), the operator does not clean up ZooKeeper entries for the removed shard's replicated tables. After the reconcile completes, paths like /clickhouse/{cluster}/tables/{removed_shard_index}/... remain in ZooKeeper.

This is confirmed by test_010014_0 "Remove shard" step, which has been XFAIL'd:

with Then(f"Shard is removed from {self.context.keeper_type}", flags=XFAIL):
    out = clickhouse.query_with_error(
        chi_name,
        f"SELECT count() FROM system.zookeeper WHERE path ='/clickhouse/{cluster}/tables/1/default'",
    )
    assert "DB::Exception: No node" in out or out == "0"

The assertion always fails: count() = 1 (or more) instead of 0 / "No node".

Root Cause

Issue 1: dropZKReplicas shard callback is a no-op

pkg/controller/chi/worker-deleter.go line 64:

func (w *worker) dropZKReplicas(ctx context.Context, cr *api.ClickHouseInstallation) {
    cr.EnsureRuntime().ActionPlan.WalkRemoved(
        func(cluster api.ICluster) {},
        func(shard api.IShard) {},       // ← no-op: shard removal is silently ignored
        func(host *api.Host) {
            _ = w.dropZKReplica(ctx, host, NewDropReplicaOptions().SetRegularDrop())
        },
    )
}

When shardsCount decreases, messagediff.DeepDiff detects the removal at the shard level and adds the *ChiShard to specDiff.Removed. Since *ChiShard implements IShard, shardFunc is called — which is a no-op. The hostFunc callback for individual hosts is never reached.

Issue 2: dropZKReplica uses hosts from the removed shard itself

Even if hosts were enumerated individually, dropZKReplica resolves the "host to run on" as:

if shard := hostToDrop.GetShard(); shard != nil {
    hostToRunOn = shard.FirstHost()   // first host of the REMOVED shard
}

When an entire shard is removed, FirstHost() of the removed shard is also being deleted. By the time dropZKReplicas runs (after clean()), those pods are already gone — the SQL connection fails silently.

Issue 3: SYSTEM DROP REPLICA is wrong tool for whole-shard removal

Even if hostToRunOn were a surviving host from another shard, SYSTEM DROP REPLICA 'host-1-x' run on shard 0 would not clean shard 1's ZK paths — shard 0's ClickHouse doesn't have shard 1's tables in system.replicas.

Correct Fix

The proper approach is to run DROP TABLE IF EXISTS ... SYNC on the removed shard's hosts before clean() deletes their pods. This is the same mechanism used during full CHI deletion (deleteTables / HostDropTables), and it correctly:

  1. Removes the replica's ZK entries
  2. If it's the last replica, cleans the entire table ZK subtree

Concretely, in worker-reconciler-chi.go, before w.clean(ctx, new):

w.dropTablesFromRemovedHosts(ctx, new)   // NEW: drop tables on removed hosts while pods still running
w.clean(ctx, new)
...
w.dropZKReplicas(ctx, new)

Where dropTablesFromRemovedHosts walks ActionPlan.WalkRemoved and calls deleteTables() on each host in removed shards (respecting the existing HostCanDeleteAllPVCs guard for reclaimPolicy: Retain).

Additionally, dropZKReplicas should implement the shardFunc callback to handle removed shards explicitly.

Impact

  • ZooKeeper accumulates stale table paths every time a shard is scaled down
  • If the same shard index is later re-added, it may see stale ZK metadata
  • SYSTEM DROP REPLICA is never logged for shard removal (confirmed by query log check in test_010014_0)

Related

  • test_010014_0 "Remove shard" step XFAILs:
    • "Shard is removed from zookeeper"
    • "system.query_log should contain SYSTEM DROP REPLICA statements"
  • Full CHI deletion (deleteCHIProtocol) does NOT have this issue — it explicitly calls deleteTables on every host before deleting K8s resources
  • Single-replica removal (scaling replicas 2→1 within a shard) also has a related issue: dropZKReplica correctly gets shard.FirstHost() as the surviving replica, but that only works when at least one replica remains in the shard

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Review worker-reconciler-chi.go around clean(), worker-deleter.go, and the ActionPlan.WalkRemoved callbacks, then inspect test_010014_0's XFAIL'd Remove shard steps. Trace the existing deleteTables and HostDropTables flow and verify that removed-shard ZooKeeper paths are gone and the expected query-log checks pass before considering the work done.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
databases, infrastructure
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.