Prevent Race Conditions Between Tablet Deletes and Updates

[mattlord]

Description

There was a race condition between deleting a tablet's healthcheck record from the authoritative map (hc.healthByAlias) and updating the same tablet's health data record (hc.healthData).

This could cause us to effectively re-add an updated copy of the tablet healthcheck record after it's been deleted due to the open stream with the tablet encounting an error -- often a context being cancelled or a gRPC goaway error seen due to the tablet shutting down. This then leads to "zombie" tablet records in the SHOW VITESS_TABLETS output as it is based on what is in the hc.healthData map and it had records for tablets that were no longer in hc.healthByAlias (so the authoritative map was no longer authoritative):

vitess/go/vt/vtgate/executor.go

Line 1108 in 693c5db

status := e.scatterConn.GetHealthCheckCacheStatus()

...

vitess/go/vt/discovery/healthcheck.go

Lines 537 to 557 in 693c5db

	func (hc *HealthCheckImpl) cacheStatusMap() map[string]*TabletsCacheStatus {
	tcsMap := make(map[string]*TabletsCacheStatus)
	hc.mu.Lock()
	defer hc.mu.Unlock()
	for _, ths := range hc.healthData {
	for _, th := range ths {
	key := fmt.Sprintf("%v.%v.%v.%v", th.Tablet.Alias.Cell, th.Target.Keyspace, th.Target.Shard, th.Target.TabletType.String())
	var tcs *TabletsCacheStatus
	var ok bool
	if tcs, ok = tcsMap[key]; !ok {
	tcs = &TabletsCacheStatus{
	Cell: th.Tablet.Alias.Cell,
	Target: th.Target,
	}
	tcsMap[key] = tcs
	}
	tcs.TabletsStats = append(tcs.TabletsStats, th)
	}
	}
	return tcsMap
	}

Changes

• KEY: The healthcheck will ONLY delete from its own lower level cache directly -- via hc.deleteTablet() -- if there's mismatching endpoints for the tablet indicating that the healthcheck record is no longer valid (this was the case before Remove tablet healthcheck cache record on error #9106)
  • This is the only safe exception to the rule that the healthcheck cache be updated (add,replace,delete) via topo server -> topology watcher -> healthcheck here, as in this case the record is simply not valid and will get corrected in the noted path via the topo server tablet records in the next refresh
• KEY: We avoid the race condition betweenhc.deleteTablet() and hc.updateHealth() by confirming that the tablet record still exists in the authoritative hc.healthByAlias map before updating the health of the tablet in the sibling hc.healthData map because the mutex does not enforce order of operations. If it does NOT exist there then the update is a no-op (the old tablet health data record copy made here or here will go out of scope and be GC'd).
• The vtgate/tablet_healthcheck_cache/correctness end2end test was updated to verify the correct behavior (unfortunately the test originally added in Remove tablet healthcheck cache record on error #9106 was simply verifying bad behavior)
• We create the record in the sibling hc.healthData map -- if it doesn't exist -- when updating the health record for a tablet that we have confirmed exists in the authoritative hc.healthByAlias map

With these changes, I could no longer repeat the zombie tablet record using the steps shown here, and we see the expected log messages indicating that we've avoided the race condition:

/vt/vtdataroot/tmp/vtgate.INFO:I1115 23:31:18.311915    2021 healthcheck.go:424] Tablet zone1-0000000201 has been deleted, skipping health update
/vt/vtdataroot/tmp/vtgate.INFO:I1115 23:31:18.311976    2021 healthcheck.go:424] Tablet zone1-0000000200 has been deleted, skipping health update
/vt/vtdataroot/tmp/vtgate.INFO:I1115 23:31:18.311994    2021 healthcheck.go:424] Tablet zone1-0000000202 has been deleted, skipping health update

Related Issue(s)

Fixes: #9238
Properly Fixes: #8465

Checklist

• Should this PR be backported? NO
• Tests were added/updated
• Documentation is not required

[deepthi]

Found a tiny typo in comments, otherwise LGTM.
Nice work!