Ignore non-Shard keys in FindAllShardsInKeyspace List impl (#15117)

Signed-off-by: Matt Lord <[email protected]>

go/vt/topo/keyspace.go

@@ -43,6 +43,11 @@ import (
 // Default concurrency to use in order to avoid overhwelming the topo server.
 var DefaultConcurrency = 32
 
+// shardKeySuffix is the suffix of a shard key.
+// The full key looks like this:
+// /vitess/global/keyspaces/customer/shards/80-/Shard
+const shardKeySuffix = "Shard"
+
 func registerFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&DefaultConcurrency, "topo_read_concurrency", DefaultConcurrency, "Concurrency of topo reads.")
 }
@@ -214,6 +219,13 @@ func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string,
 		for _, entry := range kvpairs {
 			// The shard key looks like this: /vitess/global/keyspaces/commerce/shards/-80/Shard
 			shardKey := string(entry.Key)
+			// We don't want keys that aren't Shards. For example:
+			// /vitess/global/keyspaces/commerce/shards/0/locks/7587876423742065323
+			// This example key can happen with Shards because you can get a shard
+			// lock in the topo via TopoServer.LockShard().
+			if path.Base(shardKey) != shardKeySuffix {
+				continue
+			}
 			shardName := path.Base(path.Dir(shardKey)) // The base part of the dir is "-80"
 			// Validate the extracted shard name.
 			if _, _, err := ValidateShardName(shardName); err != nil {

go/vt/topo/keyspace_external_test.go

@@ -95,6 +95,19 @@ func TestServerGetServingShards(t *testing.T) {
 	keyspace := "ks1"
 	errNoListImpl := topo.NewError(topo.NoImplementation, "don't be doing no listing round here")
 
+	// This is needed because memorytopo doesn't implement locks using
+	// keys in the topo. So we simulate the behavior of other topo server
+	// implementations and how they implement TopoServer.LockShard().
+	createSimulatedShardLock := func(ctx context.Context, ts *topo.Server, keyspace, shard string) error {
+		conn, err := ts.ConnForCell(ctx, topo.GlobalCell)
+		if err != nil {
+			return err
+		}
+		lockKey := fmt.Sprintf("keyspaces/%s/shards/%s/locks/1234", keyspace, shard)
+		_, err = conn.Create(ctx, lockKey, []byte("lock"))
+		return err
+	}
+
 	tests := []struct {
 		shards   int    // Number of shards to create
 		err      string // Error message we expect, if any
@@ -138,10 +151,17 @@ func TestServerGetServingShards(t *testing.T) {
 			if tt.shards > 0 {
 				shardNames, err = key.GenerateShardRanges(tt.shards)
 				require.NoError(t, err)
+				require.Equal(t, tt.shards, len(shardNames))
 				for _, shardName := range shardNames {
 					err = ts.CreateShard(ctx, keyspace, shardName)
 					require.NoError(t, err)
 				}
+				// A shard lock typically becomes a key in the topo like this:
+				// /vitess/global/keyspaces/<keyspace>/shards/<shardname>/locks/XXXX
+				// We want to confirm that this key is ignored when building
+				// the results.
+				err = createSimulatedShardLock(ctx, ts, keyspace, shardNames[0])
+				require.NoError(t, err)
 			}
 
 			// Verify that we return a complete list of shards and that each