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). 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,