ListAllTablets: improve efficiency within vitess and for caller

[mattlord]

Description

The ListAllTablets vtctl command is one of the most widely used methods to observe the state of a Vitess cluster. Previously this code path would query the topo server for every tablet and if you had many things using this command simultaneously, and you had many hundreds or even thousands of tablets, you could begin to overwhelm vtctld and the topo server with client server requests. This call also produced many rows of free form text that can be expensive to filter on the caller side and cause issues within the monitoring/alerting and automation related systems leveraging this command.

Within Vitess

This work makes a single call to the topo server — for the topo implementations that support scans using a key prefix (etcd, consul, and k8s) — from vtctld to get the information on all tablets in a cell. So we go from 1 topo request per tablet to one topo request per cell.

You can see this demonstrated here (->> click to expand):

The test:

make docker_local && ./docker/local/run.sh
for i in {5..9}; do ./201_newkeyspace_tablets.sh myks${i} ${i}; done
echo -e "\n\n-----------------------------------------------------------------------------------\n\n"
curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'
vtctlclient ListAllTablets | wc -l
curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'

The previous results:

vitess@5824418c40be:/vt/local$ curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'
712
vitess@5824418c40be:/vt/local$ vtctlclient ListAllTablets | wc -l
18
vitess@5824418c40be:/vt/local$ curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'
732

The new results on this branch:

vitess@04c85ee4f7d3:/vt/local$ curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'
712
vitess@04c85ee4f7d3:/vt/local$ vtctlclient ListAllTablets | wc -l
18
vitess@04c85ee4f7d3:/vt/local$ curl -s http://localhost:2379/metrics | grep 'etcd_server_client_requests_total{client_api_version="3.5",type="unary"}' | awk '{print $2}'
714

For the caller

This work adds support for server side filtering on the following common parameters (filtering by cell(s) already existed):

• Keyspace
• Tablet type

This can eliminate a lot of work on the caller side to filter out the full list of tablets.

You can see this demonstrated here (->> click to expand):

vitess@2a3263386d69:/vt/local$ vtctlclient ListAllTablets
zone1-0000000100 commerce 0 primary 2a3263386d69:15100 2a3263386d69:17100 [] 2022-01-25T19:50:38Z
zone1-0000000101 commerce 0 replica 2a3263386d69:15101 2a3263386d69:17101 [] <null>
zone1-0000000102 commerce 0 rdonly 2a3263386d69:15102 2a3263386d69:17102 [] <null>
zone1-0000000500 myks5 - primary 2a3263386d69:15500 2a3263386d69:17500 [] 2022-01-25T19:50:53Z
zone1-0000000501 myks5 - replica 2a3263386d69:15501 2a3263386d69:17501 [] <null>
zone1-0000000502 myks5 - rdonly 2a3263386d69:15502 2a3263386d69:17502 [] <null>
zone1-0000000600 myks6 - primary 2a3263386d69:15600 2a3263386d69:17600 [] 2022-01-25T19:51:07Z
zone1-0000000601 myks6 - replica 2a3263386d69:15601 2a3263386d69:17601 [] <null>
zone1-0000000602 myks6 - rdonly 2a3263386d69:15602 2a3263386d69:17602 [] <null>
zone1-0000000700 myks7 - primary 2a3263386d69:15700 2a3263386d69:17700 [] 2022-01-25T19:51:21Z
zone1-0000000701 myks7 - replica 2a3263386d69:15701 2a3263386d69:17701 [] <null>
zone1-0000000702 myks7 - rdonly 2a3263386d69:15702 2a3263386d69:17702 [] <null>
zone1-0000000800 myks8 - primary 2a3263386d69:15800 2a3263386d69:17800 [] 2022-01-25T19:51:35Z
zone1-0000000801 myks8 - replica 2a3263386d69:15801 2a3263386d69:17801 [] <null>
zone1-0000000802 myks8 - rdonly 2a3263386d69:15802 2a3263386d69:17802 [] <null>
zone1-0000000900 myks9 - primary 2a3263386d69:15900 2a3263386d69:17900 [] 2022-01-25T19:51:50Z
zone1-0000000901 myks9 - replica 2a3263386d69:15901 2a3263386d69:17901 [] <null>
zone1-0000000902 myks9 - rdonly 2a3263386d69:15902 2a3263386d69:17902 [] <null>

vitess@2a3263386d69:/vt/local$ vtctlclient ListAllTablets -keyspace=myks8
zone1-0000000800 myks8 - primary 2a3263386d69:15800 2a3263386d69:17800 [] 2022-01-25T19:51:35Z
zone1-0000000801 myks8 - replica 2a3263386d69:15801 2a3263386d69:17801 [] <null>
zone1-0000000802 myks8 - rdonly 2a3263386d69:15802 2a3263386d69:17802 [] <null>

vitess@2a3263386d69:/vt/local$ vtctlclient ListAllTablets -keyspace=commerce -tablet_type=primary
zone1-0000000100 commerce 0 primary 2a3263386d69:15100 2a3263386d69:17100 [] 2022-01-25T19:50:38Z

vitess@2a3263386d69:/vt/local$ vtctlclient ListAllTablets -keyspace=myks9 -tablet_type=replica
zone1-0000000901 myks9 - replica 2a3263386d69:15901 2a3263386d69:17901 [] <null>

Related Issue(s)

Fixes: #9384

Checklist

• Should this PR be backported? No
• Tests were added
• Documentation: Add new ListAllTablets flags website#953

[mattlord]

FWIW, I did a quick/simple manual test with Consul too (using this code):

✔ ~/git/vitess-consul-tests [main|✔]
05:09 $ kubectl -n vitess get pods
NAME                      READY   STATUS    RESTARTS      AGE
consul-client-0           1/1     Running   0             22m
consul-server-0           1/1     Running   0             22m
consul-server-1           1/1     Running   0             22m
consul-server-2           1/1     Running   0             22m
mysql-74fdccf994-xn67l    3/3     Running   2 (19m ago)   20m
mysql2-7bf9fdb988-8w7jh   3/3     Running   2 (19m ago)   20m
mysql3-75867d657d-hd52z   3/3     Running   2 (19m ago)   20m
vtctld-9f9fc99c8-9rghm    2/2     Running   0             22m
vtgate-7d89947f79-ng7jp   2/2     Running   1 (19m ago)   20m

✔ ~/git/vitess-consul-tests [main|✔]
05:09 $ kubectl -n vitess exec -it vtctld-9f9fc99c8-9rghm -c vtctld -- /vt/bin/vtctlclient -server=localhost:15999 ListAllTablets
us_east_1-1126369102 vitess-test 0 replica mysql:3380 mysql:3306 [] <null>
us_east_1-1126369103 vitess-test 0 primary mysql:3380 mysql:3306 [] 2022-01-27T04:50:08Z
us_east_1-1126369104 vitess-test 0 replica mysql:3380 mysql:3306 [] <null>

✔ ~/git/vitess-consul-tests [main|✔]
05:09 $ kubectl -n vitess exec -it vtctld-9f9fc99c8-9rghm -c vtctld -- /vt/bin/vtctlclient -server=localhost:15999 ListAllTablets -tablet_type=replica
us_east_1-1126369102 vitess-test 0 replica mysql:3380 mysql:3306 [] <null>
us_east_1-1126369104 vitess-test 0 replica mysql:3380 mysql:3306 [] <null>

[GuptaManan100]

Everything else looks good to me!

[mattlord]

Everything else looks good to me!

Thanks for the review, @GuptaManan100! I've addressed those things here: 648bb21

[GuptaManan100]

Thankyou very much!

[deepthi]

A few comments/questions:

• The new function probably belongs more properly in directory.go than in file.go
• The current ListDir implementations seem to have some special logic (for locks/leases etc.). Is any of that relevant to List?
• In order for this new API to be generic, I believe we should also be returning the version for each object
• What happens if one of the children is a directory? What do we return for that object? I understand that files within that directory will be returned as well.

[mattlord]

• The new function probably belongs more properly in directory.go than in file.go

Why? This is a scan of keys based on the key prefix. These are K/V stores, the key is the full path. While they present a filesystem like interface, that is not what is there.

There's no such thing as a "folder" or "directory" in a K/V store. In etcd, all of the data is in a single file called <datadir>/member/snap/db, for example: /vt/vtdataroot/etcd/member/snap/db. There's not actually files and directories.

I apologize if you already know all this, but I think it might be helpful in this discussion.

• The current ListDir implementations seem to have some special logic (for locks/leases etc.). Is any of that relevant to List?

ListDir simply returns the keys (not values) with a matching key prefix before the final / in the key. It does take a lock when generating that slice of matching keys:

	c.factory.mu.Lock()
	defer c.factory.mu.Unlock()

So it locks the entire topo when generating that list of matching keys. It makes sense there as it's needed in order to generate that using a consistent read, because it's making N calls to the underlying topo implementation to build it.

With List() there's a single call to the topo implementation and the consistent read is managed by the topo implementation itself. So I don't see the need here (and it's pretty heavy).

• In order for this new API to be generic, I believe we should also be returning the version for each object

Can you explain why? For a list, I would think MVCC (view) version info is important and not a version of an individual record. The version is important when doing a read-update-write cycle. I'm not sure that would ever be used with List...

• What happens if one of the children is a directory? What do we return for that object? I understand that files within that directory will be returned as well.

Again, there aren't directories or files. :-)

[mattlord]

@deepthi was kind enough to sit down with me one and one can chat about this, I wanted to summarize the discussion:

• The new function probably belongs more properly in directory.go than in file.go

We're OK with the current location and name of the function. It may be a little awkward (as we're mapping several concepts), but it's not significantly more or less awkward than alternatives.

• The current ListDir implementations seem to have some special logic (for locks/leases etc.). Is any of that relevant to List?

This was about what to do when the key represents a lock/lease, and in the case of List it does not matter. List returns an array of byte arrays (slice of slices) and its up to to the caller to decode that into whatever types it wants/expects and take actions from there.

• In order for this new API to be generic, I believe we should also be returning the version for each object

We both agreed on this. I will add another commit so that List returns an array of a Struct/type that has two fields:

1. value []byte
2. version Version

This then maps to what Get returns and does not make any assumptions about how the results may be used and for what.

• What happens if one of the children is a directory? What do we return for that object? I understand that files within that directory will be returned as well.

We agreed that this is not an issue.

[mattlord]

@deepthi I modified the List interface so that it returns a slice of generic KV structures with additional metadata (version so far): []KVInfo. While we don't care about the Key in this case — because the Value is a TabletInfo record that contains the tablet identifier) — it makes sense to return that from a List function in the interface: eeb4197

Thanks for the great suggestions!

[deepthi]

Very nice. The implementation is very clean, and the test cases are good.
I like the way we fall back when List is not provided by the underlying implementation.
Only one more thing to fix and then we can merge. I believe you need to run make vtadmin_web_proto_types whenever changes are made to vtctldata.proto.
https://vitess.slack.com/archives/CMDJ2KFEZ/p1643297135016100