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

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

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)
+	})
+}

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 {

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

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
+}