Treat host names, not list positions, as identity: fix wrong keeper replica drops and host renames

[xavierleune]

Fixes #1545

Problem

Host identity is positional in two places where everything else in the operator (StatefulSet names, PVC names, k8s object cleanup) is name-based. When replicas are removed from the head or middle of the replicas list (instead of the tail), two independent defects fire:

1. The wrong replicas are dropped from (Zoo)Keeper, on a dead pod. K8s objects are cleaned up correctly, but the removed replicas stay registered in Keeper forever (system.replicas keeps showing them as inactive, total_replicas never goes down). Operator logs from a 5-replica shard (0-0…0-4) after removing 0-0 and 0-1 from the spec:

schemer.go:59] HostDropReplica():Drop replica: chi-idtest-production-0-4 at 0-0
connection.go:194] Exec():FAILED Exec(http://...@chi-idtest-production-0-0...:8123/) ... no such host
    for SQL: SYSTEM DROP REPLICA 'chi-idtest-production-0-3'
worker-deleter.go:427] dropReplica():FAILED to drop replica on host: 0-3 ...

The operator tried to drop the surviving replicas 0-3/0-4, executed on the removed host 0-0. The only reason live replicas were not actually dropped is that the executor host was already dead.

2. Bare-number replica names silently change identity — with data loss. With replicas explicitly named "0"…"4", removing entry "2" shifts "3" and "4" to list positions 2 and 3. normalizeHostName only recognizes a name as "auto-generated" if it matches patterns derived from the host's current index (IsAutoGeneratedHostName), so "3" at index 2 no longer normalizes to 0-3 — it is kept verbatim. The operator then creates a brand-new StatefulSet chi-…-3 with an empty PVC and purges the old chi-…-0-3 STS and its PVC. Two healthy replicas are wiped for removing one list entry.

Root cause

Both defects come from treating list position as identity while the rest of the operator uses names:

1. ActionPlan.WalkRemoved is built on messagediff.DeepDiff, which compares slices positionally. Removing N hosts from the head of the list makes the diff report the tail N hosts as "removed" (they survive by name) and the head hosts as renames. dropZKReplicas therefore walked the wrong host set, and it ran the SQL on hostToDrop.GetShard().FirstHost() — the first host of the old shard, itself one of the removed hosts.
2. normalizeHostName (CHI and CHK) decides whether an explicit host name may be rewritten by matching it against index-derived patterns, so the meaning of a bare-number name like "3" depends on where it currently sits in the list.

Fix

• ActionPlan.WalkRemoved now computes removed hosts by name: hosts of the old CR whose cluster/shard still exists in the new CR but whose name is no longer present there. Whole shard/cluster removals keep their existing behavior (covered by the shard/cluster callbacks). This also fixes the other WalkRemoved consumers: status.HostsDeleteCount and the per-host pre-delete hooks, which used to fire on surviving hosts.
• dropZKReplica now accepts an explicit hostToRunOn; dropZKReplicas resolves it from the new CR (first host of the same shard), i.e. a host that is guaranteed to stay. The previous behavior is kept as a fallback for the storage-loss re-provisioning path.
• normalizeHostName (CHI and CHK) canonicalizes an explicitly specified bare-number host name position-independently, substituting the number into the current naming scheme along the axis the hosts list varies on: "3" under shard "0" → 0-3 (hosts declared under shards), "3" under replica "0" → 3-0 (hosts declared under replicas). Names matching the auto-generated patterns at their natural position keep the historical behavior; custom non-numeric names are still kept verbatim; when hosts are declared under both shards and replicas the provenance is ambiguous and nothing changes.

Behavior change / migration note

A deployment that already went through the rename footgun (e.g. spec ["0","1","3","4"] currently running with verbatim host names 3/4 and STS chi-…-3/chi-…-4) will see those hosts renamed to 0-3/0-4 (STS/PVC recreated) on upgrade. Specs with bare-number names at their natural positions, canonical {shard}-{replica} names, or custom names are normalized exactly as before.

Detecting whether a deployment is affected — before upgrading, list the CHI's StatefulSets and compare against the canonical scheme: a healthy host is named chi-{chi}-{cluster}-{shard}-{replica} (e.g. chi-x-c1-0-3), a footgun host has a bare suffix (e.g. chi-x-c1-3). If every STS follows the {shard}-{replica} form, the upgrade is a no-op name-wise and none of the below applies.

Migration paths for affected deployments (the right one depends on whether every shard keeps at least one data-bearing, correctly named replica):

1. Sequential declarative rename before upgrading (recommended). Still on the old operator, rename the affected spec entries to their canonical form one at a time ("3" → "0-3"), waiting for the replacement host to be recreated and to catch up (SYSTEM SYNC REPLICA, check counts / system.replicas) before doing the next. Each rename is a regular delete+add: the new host starts empty and backfills through replication, so done sequentially redundancy is never lost. "0-3" is stable under both the old and the new operator. Afterwards, clean the stale Keeper entries left for the old names (SYSTEM DROP REPLICA 'chi-x-c1-3' on a surviving host — they are leftover anyway, that is Operator dropping wrong replicas when removing older replicas from cluster layout  #1545 itself). Once the spec is canonical, upgrading renames nothing.
2. Direct upgrade, acceptable when redundancy is guaranteed. If every shard keeps at least one correctly named, data-bearing replica, the renamed hosts come back empty and re-replicate automatically. Cost: full re-replication of those hosts at once and reduced redundancy meanwhile. Do not use this if any shard has all of its replicas on verbatim names (e.g. ["2","3","4"] after head removals): that shard would be recreated empty — total data loss.
3. PV adoption, for large volumes (expert path, no re-replication). Per affected host: set the PV reclaimPolicy to Retain, suspend the CHI reconcile, delete the old-name STS and PVC, clear the PV's claimRef, pre-create a PVC with the new canonical name (data-volume-chi-x-c1-0-3-0) bound to the same PV, then upgrade: the renamed STS adopts the existing disk. Caveat: on-disk table metadata keeps the old replica_name in Keeper (frozen at table creation), so the replica keeps registering under its old name until tables are recreated, and the operator's post-upgrade SYSTEM DROP REPLICA for the old name fails safely (active replica).

If maintainers prefer, the canonicalization could also be made opt-out behind a chop config flag (e.g. host.name.canonicalizeNumeric, default true) to let affected deployments migrate at their own pace — left out of this PR to avoid adding config surface, happy to add it if desired.

Also note: host statuses (WalkAdded/WalkModified) still follow the positional diff, so a head-removal still causes surviving STS to be re-created (same STS/PVC names — end state and data are correct). The "adding a replica in the middle skips schema migration" symptom mentioned in #1545 belongs to that same add-side and would be a follow-up.

Reproduction playbook

Any cluster with explicitly named replicas works; no special templates are needed.

1. Apply a CHI with named replicas (plus any Keeper/ZooKeeper you have around):

apiVersion: "clickhouse.altinity.com/v1"
kind: "ClickHouseInstallation"
metadata:
  name: repro
spec:
  configuration:
    zookeeper:
      nodes:
        - host: <your-keeper-host>
    clusters:
      - name: c1
        layout:
          shards:
            - name: "0"
              replicas:
                - name: "0"   # <- will be removed in step 3
                - name: "1"
                - name: "2"
                - name: "3"

The operator normalizes these to hosts 0-0…0-3 (STS chi-repro-c1-0-0…).

2. Create a replicated table so the replicas register in Keeper:

CREATE TABLE t ON CLUSTER 'c1' (n UInt64)
ENGINE = ReplicatedMergeTree ORDER BY n;
INSERT INTO t SELECT number FROM numbers(1000);

3. Remove the first entry (- name: "0") from the replicas list and re-apply.

4. Observe, before this fix:

   • defect 2: hosts "2"/"3" shift positions, stop matching the auto-name patterns and are re-created as brand-new STS chi-repro-c1-2/chi-repro-c1-3 with empty PVCs, while chi-repro-c1-0-2/0-3 (with the data) are purged;
   • defect 1: operator logs show SYSTEM DROP REPLICA for the wrong replica names, executed at chi-repro-c1-0-0 (the deleted pod), failing with no such host; on a surviving replica, SELECT total_replicas, replica_is_active FROM system.replicas WHERE table = 't' keeps listing the removed replica and total_replicas never decreases.

   After this fix: hosts 0-1…0-3 keep their names, STS and PVCs; the operator runs SYSTEM DROP REPLICA 'chi-repro-c1-0-0' on chi-repro-c1-0-1 and system.replicas shows total_replicas = 3 with no stale entry.

Tests

• New action_plan_test.go: head/middle/tail removal, in-place rename, whole-shard removal, add-only and no-op cases.
• New normalizer-host_test.go: bare-number name at shifted vs natural position, canonical and custom names, replicas-defined layout, ambiguous dual layout, leading-zero names.
• go test ./pkg/...: 609 passed; the only failures are two pre-existing TestEnforceVerifiedLegacyTLS cases that also fail on a clean tree (environment-dependent TLS error message).

🤖 Generated with Claude Code