Remove tablet healthcheck cache record on error

[mattlord]

Description

If a tablet's health check runs concurrently with its deletion then you can end up with a timing issue where the goroutine getting the status from vtgate->vttablet via an RPC call has a connection related error (e.g. context cancelled or EOF) but a copy of the tablet's healthcheck cache record is made and that copy's updated state is stored in the healthcheck cache after the tablet has gone away. Thus you can end up with a zombie tablet record in the healthcheck cache.

The shorter your -health_check_interval tablet flag is (default is 30s), the more likely you will encounter this. I was able to repeat it quite easily and regularly in the docker_local container in part because the interval is set to 5s for those tablets and the DeleteTablet calls made in the test case (./307_delete_shard_0.sh) are somewhat slow given the execution context.

I was no longer able to repeat the bug using my test case with this patch.

Note: w/o this patch, the only way to remove these zombie tablet healthcheck records is to restart the vtgate

Related Issue(s)

#8465

Checklist

• Should this PR be backported? Thoughts?
• Tests were added
  • This demonstrates the test failing (as expected) on a branch that only has the test: Add e2e test to check for tablet healthcheck cache correctness #9109
• Documentation was added or is not required

[frouioui]

Hey Matt!

I have reproduced your test case with and without your patch. After following the instructions using your patch, the primary tablet of customer/0 is still hanging:

mysql> show vitess_tablets;
+-------+----------+-------+------------+-------------+------------------+--------------+----------------------+
| Cell  | Keyspace | Shard | TabletType | State       | Alias            | Hostname     | PrimaryTermStartTime |
+-------+----------+-------+------------+-------------+------------------+--------------+----------------------+
| zone1 | commerce | 0     | PRIMARY    | SERVING     | zone1-0000000100 | e4979245177d | 2021-10-28T05:59:59Z |
| zone1 | commerce | 0     | REPLICA    | SERVING     | zone1-0000000101 | e4979245177d |                      |
| zone1 | commerce | 0     | RDONLY     | SERVING     | zone1-0000000102 | e4979245177d |                      |
| zone1 | customer | -80   | PRIMARY    | SERVING     | zone1-0000000300 | e4979245177d | 2021-10-28T06:01:52Z |
| zone1 | customer | -80   | REPLICA    | SERVING     | zone1-0000000301 | e4979245177d |                      |
| zone1 | customer | -80   | RDONLY     | SERVING     | zone1-0000000302 | e4979245177d |                      |
| zone1 | customer | 0     | PRIMARY    | NOT_SERVING | zone1-0000000200 | e4979245177d | 2021-10-28T06:01:06Z |
| zone1 | customer | 80-   | PRIMARY    | SERVING     | zone1-0000000400 | e4979245177d | 2021-10-28T06:01:52Z |
| zone1 | customer | 80-   | REPLICA    | SERVING     | zone1-0000000401 | e4979245177d |                      |
| zone1 | customer | 80-   | RDONLY     | SERVING     | zone1-0000000402 | e4979245177d |                      |
+-------+----------+-------+------------+-------------+------------------+--------------+----------------------+
10 rows in set (0.00 sec)

After executing 306_down_shard_0.sh and 307_delete_shard_0.sh, that shard should not be around anymore, though it is still listed in show vitess_tablets. Am I missing something?

[mattlord]

I have reproduced your test case with and without your patch. After following the instructions using your patch, the primary tablet of customer/0 is still hanging:
...
After executing 306_down_shard_0.sh and 307_delete_shard_0.sh, that shard should not be around anymore, though it is still listed in show vitess_tablets. Am I missing something?

Hmm, are you sure the patch was applied? I can try to repeat it some more times too...

[mattlord]

I'm going to work on an e2e test for this that fails on main but passes on this branch to be sure that we've fixed it here and ensure we don't have similar issues crop up later.

[frouioui]

Hmm, are you sure the patch was applied? I can try to repeat it some more times too...

make docker_local was made on this pull request's head:

Version: 12.0.0-SNAPSHOT (Git revision 7c310fa6a0 branch 'ZombieTabletHeathCheckEntries') built on Thu Oct 28 05:57:39 UTC 2021 by vitess@buildkitsandbox using go1.17 linux/amd64

I have retried the same procedure a few times in a row and the outputs were always correct.

[sougou]

Looks pretty safe to me. Nice tests. I'll wait for @deepthi to do the final ack.

[mattlord]

Looks pretty safe to me. Nice tests. I'll wait for @deepthi to do the final ack.

Thanks! Once I get the new e2e test passing on this branch and failing here (against main): #9109 then I'll ping @deepthi for a review.

[frouioui]

this looks safe to me as well

[deepthi]

💯 on the tests. They give us confidence in this change 😃

[deepthi]

The new changes look good. Thank you for fixing WaitForStatus.