From 4b472407fd75d97a382e6b2bc584fdf295284062 Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Thu, 23 Jul 2026 14:10:15 +0200 Subject: [PATCH 1/2] Drop keeper replicas by name, not by list position, when hosts are removed ActionPlan.WalkRemoved relied on messagediff's positional slice diff: removing hosts from the head or the middle of the replicas list reported the TAIL hosts as removed (they survive by name), while the actually removed hosts appeared as renames. dropZKReplicas then issued SYSTEM DROP REPLICA for replicas that stay in the cluster and never for the removed ones, leaving stale replicas in (Zoo)Keeper. K8s objects are managed by name, so compute removed hosts by name as well. Additionally, run the drop on a host that survives in the new CR: the previous executor - first host of the OLD shard - is itself one of the removed hosts when the head of the list is removed, so the SQL was sent to an already deleted pod and always failed. --- .../clickhouse.altinity.com/v1/action_plan.go | 43 ++++- .../v1/action_plan_test.go | 161 ++++++++++++++++++ pkg/controller/chi/worker-deleter.go | 19 ++- pkg/controller/chi/worker-migrator.go | 2 +- 4 files changed, 216 insertions(+), 9 deletions(-) create mode 100644 pkg/apis/clickhouse.altinity.com/v1/action_plan_test.go diff --git a/pkg/apis/clickhouse.altinity.com/v1/action_plan.go b/pkg/apis/clickhouse.altinity.com/v1/action_plan.go index 90eae1a99..2cf624698 100644 --- a/pkg/apis/clickhouse.altinity.com/v1/action_plan.go +++ b/pkg/apis/clickhouse.altinity.com/v1/action_plan.go @@ -340,10 +340,49 @@ func (ap *ActionPlan) WalkRemoved( shard := ap.specDiff.Removed[path].(IShard) shardFunc(shard) case *Host: - host := ap.specDiff.Removed[path].(*Host) - hostFunc(host) + // Deliberately not walked here. messagediff compares slices positionally + // (index-by-index), so removing a host from the head or the middle of the + // hosts list reports the TAIL host(s) as removed, while they survive by name + // and the actually removed host(s) are reported as modified (renamed). + // K8s objects are managed by name, so removed hosts are computed by name below. } } + ap.walkRemovedHostsByName(hostFunc) +} + +// walkRemovedHostsByName walks hosts removed by name: hosts of the old CR whose cluster/shard +// still exists in the new CR but whose name is no longer present there. Hosts that go away +// together with their whole shard or cluster are not walked - they are covered by the +// shard/cluster callbacks of WalkRemoved, keeping parity with the positional diff behavior. +func (ap *ActionPlan) walkRemovedHostsByName(hostFunc func(host *Host)) { + if (ap.old == nil) || (ap.new == nil) { + return + } + + // Set-based lookups over WalkHosts are used instead of Find*() chains, because + // old/new may carry typed-nil concrete CRs, while WalkHosts is nil-receiver-safe. + newShards := map[string]bool{} + newHosts := map[string]bool{} + ap.new.WalkHosts(func(host *Host) error { + address := host.GetRuntime().GetAddress() + shardKey := address.GetClusterName() + "/" + address.GetShardName() + newShards[shardKey] = true + newHosts[shardKey+"/"+address.GetHostName()] = true + return nil + }) + + ap.old.WalkHosts(func(host *Host) error { + address := host.GetRuntime().GetAddress() + shardKey := address.GetClusterName() + "/" + address.GetShardName() + if !newShards[shardKey] { + // The whole shard (or cluster) is gone - not a per-host removal + return nil + } + if !newHosts[shardKey+"/"+address.GetHostName()] { + hostFunc(host) + } + return nil + }) } // WalkAdded walk added cluster items diff --git a/pkg/apis/clickhouse.altinity.com/v1/action_plan_test.go b/pkg/apis/clickhouse.altinity.com/v1/action_plan_test.go new file mode 100644 index 000000000..1b0ca08a1 --- /dev/null +++ b/pkg/apis/clickhouse.altinity.com/v1/action_plan_test.go @@ -0,0 +1,161 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v1 + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +type apShardFixture struct { + name string + hosts []string +} + +// makeHostForAP builds a host with the Runtime.Address fields WalkRemoved relies on. +// Address fields are normally populated by the normalizer. +func makeHostForAP(clusterName, shardName, hostName string) *Host { + host := &Host{Name: hostName} + host.Runtime.Address.ClusterName = clusterName + host.Runtime.Address.ShardName = shardName + host.Runtime.Address.HostName = hostName + return host +} + +func makeCRForAP(clusterName string, shards ...apShardFixture) *ClickHouseInstallation { + var chiShards []*ChiShard + for _, s := range shards { + var hosts []*Host + for _, h := range s.hosts { + hosts = append(hosts, makeHostForAP(clusterName, s.name, h)) + } + chiShards = append(chiShards, &ChiShard{Name: s.name, Hosts: hosts}) + } + return &ClickHouseInstallation{ + Spec: ChiSpec{ + Configuration: &Configuration{ + Clusters: []*Cluster{{ + Name: clusterName, + Layout: &ChiClusterLayout{Shards: chiShards}, + }}, + }, + }, + } +} + +// walkRemovedNames collects the callback invocations of WalkRemoved. +func walkRemovedNames(ap IActionPlan) (clusters, shards, hosts []string) { + ap.WalkRemoved( + func(cluster ICluster) { + clusters = append(clusters, cluster.GetName()) + }, + func(shard IShard) { + shards = append(shards, shard.GetName()) + }, + func(host *Host) { + hosts = append(hosts, host.GetName()) + }, + ) + return clusters, shards, hosts +} + +// TestActionPlanWalkRemovedHostsByName verifies that removed hosts are computed by NAME, +// not by position in the hosts list. messagediff compares slices index-by-index, so removing +// hosts from the head of the list used to report the tail hosts as removed - the operator then +// issued SYSTEM DROP REPLICA for replicas that survive and left the actually removed replicas +// in Keeper. +func TestActionPlanWalkRemovedHostsByName(t *testing.T) { + + t.Run("hosts removed from the head of the list", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1", "0-2", "0-3", "0-4"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-2", "0-3", "0-4"}}) + ap := MakeActionPlan(old, new) + + clusters, shards, hosts := walkRemovedNames(ap) + require.Empty(t, clusters) + require.Empty(t, shards) + require.Equal(t, []string{"0-0", "0-1"}, hosts) + require.Equal(t, 2, ap.GetRemovedHostsNum()) + }) + + t.Run("hosts removed from the tail of the list", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1", "0-2", "0-3", "0-4"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1", "0-2"}}) + ap := MakeActionPlan(old, new) + + clusters, shards, hosts := walkRemovedNames(ap) + require.Empty(t, clusters) + require.Empty(t, shards) + require.Equal(t, []string{"0-3", "0-4"}, hosts) + require.Equal(t, 2, ap.GetRemovedHostsNum()) + }) + + t.Run("host removed from the middle of the list", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1", "0-2"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-2"}}) + ap := MakeActionPlan(old, new) + + _, _, hosts := walkRemovedNames(ap) + require.Equal(t, []string{"0-1"}, hosts) + }) + + t.Run("host renamed in place", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "leader"}}) + ap := MakeActionPlan(old, new) + + // The old name leaves the cluster - its replica has to be dropped + _, _, hosts := walkRemovedNames(ap) + require.Equal(t, []string{"0-1"}, hosts) + }) + + t.Run("no changes", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1"}}) + ap := MakeActionPlan(old, new) + + clusters, shards, hosts := walkRemovedNames(ap) + require.Empty(t, clusters) + require.Empty(t, shards) + require.Empty(t, hosts) + }) + + t.Run("whole shard removed - hosts are covered by the shard callback only", func(t *testing.T) { + old := makeCRForAP("production", + apShardFixture{"0", []string{"0-0", "0-1"}}, + apShardFixture{"1", []string{"1-0", "1-1"}}, + ) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1"}}) + ap := MakeActionPlan(old, new) + + clusters, shards, hosts := walkRemovedNames(ap) + require.Empty(t, clusters) + require.Equal(t, []string{"1"}, shards) + require.Empty(t, hosts) + require.Equal(t, 2, ap.GetRemovedHostsNum()) + }) + + t.Run("hosts added only", func(t *testing.T) { + old := makeCRForAP("production", apShardFixture{"0", []string{"0-0"}}) + new := makeCRForAP("production", apShardFixture{"0", []string{"0-0", "0-1"}}) + ap := MakeActionPlan(old, new) + + clusters, shards, hosts := walkRemovedNames(ap) + require.Empty(t, clusters) + require.Empty(t, shards) + require.Empty(t, hosts) + }) +} diff --git a/pkg/controller/chi/worker-deleter.go b/pkg/controller/chi/worker-deleter.go index 9b1e38179..bd2dde97c 100644 --- a/pkg/controller/chi/worker-deleter.go +++ b/pkg/controller/chi/worker-deleter.go @@ -75,7 +75,11 @@ func (w *worker) dropZKReplicas(ctx context.Context, cr *api.ClickHouseInstallat func(shard api.IShard) { }, func(host *api.Host) { - _ = w.dropZKReplica(ctx, host, NewDropReplicaOptions().SetRegularDrop()) + // host belongs to the old CR. Run the drop on a host that survives in the new CR - + // when hosts are removed from the head of the hosts list, the old shard's first + // host is one of the removed hosts and its pod/service is already gone. + hostToRunOn := cr.FindShard(host.Runtime.Address.ClusterName, host.Runtime.Address.ShardName).FirstHost() + _ = w.dropZKReplica(ctx, hostToRunOn, host, NewDropReplicaOptions().SetRegularDrop()) cnt++ }, ) @@ -450,17 +454,20 @@ func (a dropReplicaOptionsArr) First() *dropReplicaOptions { return nil } -// dropZKReplica drops replica's info from Zookeeper -func (w *worker) dropZKReplica(ctx context.Context, hostToDrop *api.Host, opts *dropReplicaOptions) error { +// dropZKReplica drops replica's info from Zookeeper. +// hostToRunOn is the host to run SQL statements on - it must be a host that stays in the +// cluster. When nil, it falls back to the first host of hostToDrop's shard. +func (w *worker) dropZKReplica(ctx context.Context, hostToRunOn, hostToDrop *api.Host, opts *dropReplicaOptions) error { if hostToDrop == nil { w.a.V(1).F().Error("FAILED to drop replica. Need to have host to drop. hostToDrop: %s", hostToDrop.GetName()) return nil } // Sometimes host to drop is already unavailable, so let's run SQL statement of the first replica in the shard - var hostToRunOn *api.Host - if shard := hostToDrop.GetShard(); shard != nil { - hostToRunOn = shard.FirstHost() + if hostToRunOn == nil { + if shard := hostToDrop.GetShard(); shard != nil { + hostToRunOn = shard.FirstHost() + } } if hostToRunOn == nil { diff --git a/pkg/controller/chi/worker-migrator.go b/pkg/controller/chi/worker-migrator.go index 1e1a8d8f4..a5d10eb6e 100644 --- a/pkg/controller/chi/worker-migrator.go +++ b/pkg/controller/chi/worker-migrator.go @@ -87,7 +87,7 @@ func (w *worker) migrateTables(ctx context.Context, host *api.Host, opts *migrat Info( "Need to drop replica on host %d to shard %d in cluster %s", host.Runtime.Address.ReplicaIndex, host.Runtime.Address.ShardIndex, host.Runtime.Address.ClusterName) - w.dropZKReplica(ctx, host, NewDropReplicaOptions().SetForceDropUponStorageLoss()) + w.dropZKReplica(ctx, nil, host, NewDropReplicaOptions().SetForceDropUponStorageLoss()) } w.a.V(1). From 7930113c3f3155824c896aa53715c378164fc130 Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Thu, 23 Jul 2026 14:35:08 +0200 Subject: [PATCH 2/2] Keep bare-number host names position-independent during normalization normalizeHostName kept an explicitly specified host name verbatim unless it looked auto-generated FOR ITS CURRENT POSITION: IsAutoGeneratedHostName compares the name against index-derived patterns. With replicas named "0".."4" under shard "0", removing entry "2" shifts hosts "3" and "4" to positions 2 and 3: their names no longer match the patterns, they are kept verbatim instead of normalizing to "0-3"/"0-4", and the operator creates brand-new StatefulSets ("chi-...-3") with empty PVCs while purging the old ones - silent data loss. A bare-number name is an identity, not a position: canonicalize it into the current naming scheme along the axis the hosts list varies on ("3" under shard "0" -> "0-3", "3" under replica "0" -> "3-0"), so removing entries before it no longer changes the host's identity. Applies to both CHI and CHK normalizers. Names matching auto-generated patterns at their natural position keep the historical behavior, custom non-numeric names are still kept verbatim. --- pkg/model/chi/normalizer/normalizer-host.go | 22 +++- .../chi/normalizer/normalizer-host_test.go | 109 ++++++++++++++++++ pkg/model/chk/normalizer/normalizer-host.go | 22 +++- pkg/model/common/namer/auxiliary.go | 42 +++++++ 4 files changed, 191 insertions(+), 4 deletions(-) create mode 100644 pkg/model/chi/normalizer/normalizer-host_test.go diff --git a/pkg/model/chi/normalizer/normalizer-host.go b/pkg/model/chi/normalizer/normalizer-host.go index 7e4761e78..0ec72a5ad 100644 --- a/pkg/model/chi/normalizer/normalizer-host.go +++ b/pkg/model/chi/normalizer/normalizer-host.go @@ -208,7 +208,7 @@ func (n *Normalizer) normalizeHostStage1( shardIndex int, replicaIndex int, ) { - n.normalizeHostName(host, shard, shardIndex, replica, replicaIndex) + n.normalizeHostName(host, cluster, shard, shardIndex, replica, replicaIndex) } // normalizeHostStage2 normalizes a host @@ -239,6 +239,7 @@ func (n *Normalizer) normalizeHostEnvVars() { // normalizeHostName normalizes host's name func (n *Normalizer) normalizeHostName( host *chi.Host, + cluster chi.ICluster, shard chi.IShard, shardIndex int, replica chi.IReplica, @@ -247,10 +248,27 @@ func (n *Normalizer) normalizeHostName( hasHostName := len(host.GetName()) > 0 explicitlySpecifiedHostName := !namer.IsAutoGeneratedHostName(host.GetName(), host, shard, shardIndex, replica, replicaIndex) if hasHostName && explicitlySpecifiedHostName { - // Has explicitly specified name already, normalization is not required + // Has explicitly specified name already. + // A bare-number name ("3") is an identity, not a position in the hosts list: + // canonicalize it position-independently, otherwise removing a preceding list entry + // would shift the host onto a new name - new StatefulSet, empty PVC - and lose data. + hostsUnderShards, hostsUnderReplicas := clusterHostsProvenance(cluster) + if name, ok := namer.CanonicalizeNumericHostName(host.GetName(), shard, replica, hostsUnderShards, hostsUnderReplicas); ok { + host.Name = name + } return } // Create host name host.Name = n.namer.Name(interfaces.NameHost, host, shard, shardIndex, replica, replicaIndex) } + +// clusterHostsProvenance tells along which axis the cluster's hosts lists are declared: +// under explicitly specified shards (hosts vary along the replica axis) and/or under +// explicitly specified replicas (hosts vary along the shard axis). +func clusterHostsProvenance(cluster chi.ICluster) (underShards, underReplicas bool) { + if c, ok := cluster.(*chi.Cluster); ok && (c.Layout != nil) { + return c.Layout.ShardsExplicitlySpecified, c.Layout.ReplicasExplicitlySpecified + } + return false, false +} diff --git a/pkg/model/chi/normalizer/normalizer-host_test.go b/pkg/model/chi/normalizer/normalizer-host_test.go new file mode 100644 index 000000000..6898efc82 --- /dev/null +++ b/pkg/model/chi/normalizer/normalizer-host_test.go @@ -0,0 +1,109 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package normalizer + +import ( + "testing" + + "github.com/stretchr/testify/require" + + chi "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" +) + +// TestNormalizeHostNamePositionIndependence verifies that an explicitly specified +// bare-number host name keeps the same canonical form regardless of the host's +// position in the hosts list. Before this behavior, a host named "3" was normalized +// to "0-3" only while it sat at replica index 3; once a preceding list entry was +// removed and the host shifted to index 2, the name no longer matched the +// index-derived auto-generated patterns and was kept verbatim - producing a brand +// new StatefulSet ("chi-...-3") with an empty PVC while the old one was purged. +func TestNormalizeHostNamePositionIndependence(t *testing.T) { + n := New(nil) + + shardsLayout := &chi.Cluster{Layout: &chi.ChiClusterLayout{ShardsExplicitlySpecified: true}} + replicasLayout := &chi.Cluster{Layout: &chi.ChiClusterLayout{ReplicasExplicitlySpecified: true}} + bothLayout := &chi.Cluster{Layout: &chi.ChiClusterLayout{ShardsExplicitlySpecified: true, ReplicasExplicitlySpecified: true}} + + type tc struct { + name string + cluster *chi.Cluster + shardName string + shardIndex int + replicaName string + replicaIndex int + hostName string + expect string + } + + cases := []tc{ + { + // The dangerous case: hosts "0","1","2","3","4" under shard "0", entry "2" removed. + // Host "3" now sits at replica index 2 and must still normalize to "0-3". + name: "bare number at shifted position keeps its identity", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "3", expect: "0-3", + }, + { + // Same host at its natural position - the historical auto-generated-name path. + name: "bare number at natural position", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "2", expect: "0-2", + }, + { + name: "canonical name at shifted position is kept", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "0-3", expect: "0-3", + }, + { + name: "custom name is kept verbatim", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "leader", expect: "leader", + }, + { + name: "empty name is auto-generated", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "", expect: "0-2", + }, + { + // Hosts declared under a replica vary along the shard axis: the number is the shard part. + name: "bare number under replicas-defined layout", + cluster: replicasLayout, shardName: "1", shardIndex: 1, replicaName: "0", replicaIndex: 0, + hostName: "3", expect: "3-0", + }, + { + // With hosts declared under both shards and replicas, provenance is unknown - + // keep the historical verbatim behavior. + name: "bare number with both layouts declared is kept verbatim", + cluster: bothLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "3", expect: "3", + }, + { + // Not a canonical integer - not treated as a positional identity. + name: "leading-zero name is kept verbatim", + cluster: shardsLayout, shardName: "0", shardIndex: 0, replicaName: "2", replicaIndex: 2, + hostName: "03", expect: "03", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + shard := &chi.ChiShard{Name: c.shardName} + replica := &chi.ChiReplica{Name: c.replicaName} + host := &chi.Host{Name: c.hostName} + n.normalizeHostName(host, c.cluster, shard, c.shardIndex, replica, c.replicaIndex) + require.Equal(t, c.expect, host.Name) + }) + } +} diff --git a/pkg/model/chk/normalizer/normalizer-host.go b/pkg/model/chk/normalizer/normalizer-host.go index b26f2ee29..a18e37b6e 100644 --- a/pkg/model/chk/normalizer/normalizer-host.go +++ b/pkg/model/chk/normalizer/normalizer-host.go @@ -207,7 +207,7 @@ func (n *Normalizer) normalizeHostStage1( shardIndex int, replicaIndex int, ) { - n.normalizeHostName(host, shard, shardIndex, replica, replicaIndex) + n.normalizeHostName(host, cluster, shard, shardIndex, replica, replicaIndex) } // normalizeHostStage2 normalizes a host @@ -245,6 +245,7 @@ func (n *Normalizer) normalizeHostEnvVars() { // normalizeHostName normalizes host's name func (n *Normalizer) normalizeHostName( host *chi.Host, + cluster chi.ICluster, shard chi.IShard, shardIndex int, replica chi.IReplica, @@ -253,10 +254,27 @@ func (n *Normalizer) normalizeHostName( hasHostName := len(host.GetName()) > 0 explicitlySpecifiedHostName := !namer.IsAutoGeneratedHostName(host.GetName(), host, shard, shardIndex, replica, replicaIndex) if hasHostName && explicitlySpecifiedHostName { - // Has explicitly specified name already, normalization is not required + // Has explicitly specified name already. + // A bare-number name ("3") is an identity, not a position in the hosts list: + // canonicalize it position-independently, otherwise removing a preceding list entry + // would shift the host onto a new name - new StatefulSet, empty PVC - and lose data. + hostsUnderShards, hostsUnderReplicas := clusterHostsProvenance(cluster) + if name, ok := namer.CanonicalizeNumericHostName(host.GetName(), shard, replica, hostsUnderShards, hostsUnderReplicas); ok { + host.Name = name + } return } // Create host name host.Name = n.namer.Name(interfaces.NameHost, host, shard, shardIndex, replica, replicaIndex) } + +// clusterHostsProvenance tells along which axis the cluster's hosts lists are declared: +// under explicitly specified shards (hosts vary along the replica axis) and/or under +// explicitly specified replicas (hosts vary along the shard axis). +func clusterHostsProvenance(cluster chi.ICluster) (underShards, underReplicas bool) { + if c, ok := cluster.(*chk.Cluster); ok && (c.Layout != nil) { + return c.Layout.ShardsExplicitlySpecified, c.Layout.ReplicasExplicitlySpecified + } + return false, false +} diff --git a/pkg/model/common/namer/auxiliary.go b/pkg/model/common/namer/auxiliary.go index 1016bbcf7..87404fc4d 100644 --- a/pkg/model/common/namer/auxiliary.go +++ b/pkg/model/common/namer/auxiliary.go @@ -16,6 +16,7 @@ package namer import ( "fmt" + "strconv" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" ) @@ -30,6 +31,47 @@ func IsAutoGeneratedReplicaName(name string, replica api.IReplica, index int) bo return name == createReplicaName(replica, index) } +// isBareNumberName checks whether name is a bare non-negative integer in canonical form +// ("3" is, "03" and "-3" are not) +func isBareNumberName(name string) bool { + i, err := strconv.Atoi(name) + return (err == nil) && (i >= 0) && (strconv.Itoa(i) == name) +} + +// CanonicalizeNumericHostName builds the position-independent canonical name for a host with +// an explicitly specified bare-number name and reports whether canonicalization applies. +// +// A bare-number host name is an identity, not a position. IsAutoGeneratedHostName matches such +// names against patterns derived from the host's CURRENT indexes, so the meaning of "3" would +// change when a preceding entry is removed from the hosts list: the host silently gets another +// name and, with it, another StatefulSet and an empty PVC. To keep the identity stable, the +// number is substituted into the current naming scheme (see createHostName) along the axis the +// hosts list varies on: +// - hosts declared under a shard vary along the replica axis: "3" -> "{shard}-3" +// - hosts declared under a replica vary along the shard axis: "3" -> "3-{replica}" +// +// When hosts are declared both ways, the provenance of a host is unknown and no +// canonicalization is performed. +func CanonicalizeNumericHostName( + name string, + shard api.IShard, + replica api.IReplica, + hostsUnderShards bool, + hostsUnderReplicas bool, +) (string, bool) { + if !isBareNumberName(name) { + return "", false + } + switch { + case hostsUnderShards && !hostsUnderReplicas: + return fmt.Sprintf("%s-%s", shard.GetName(), name), true + case hostsUnderReplicas && !hostsUnderShards: + return fmt.Sprintf("%s-%s", name, replica.GetName()), true + default: + return "", false + } +} + // IsAutoGeneratedHostName checks whether name is auto-generated func IsAutoGeneratedHostName( name string,