gossip: demonstrate problem with loopback info propagation

[petermattis]

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Glad the problem is where we hoped it was going to be. Is the "fix" as simple as making sure that the first time a node receives Gossip, it receives everything that isn't expired?

More generally, infinite TTLs are problematic in themselves because we can't delete from Gossip (and I'd rather not introduce it), but at the same time this isn't a problem with infinite TTLs, right? It would affect any TTL that was longer-lived. In the case of node liveness, we could Gossip liveness record X with a TTL that makes the gossip record expire at the liveness entry's expiration + one week, meaning that a week after decommissioning, the node would disappear from Gossip.

[petermattis]

Glad the problem is where we hoped it was going to be. Is the "fix" as simple as making sure that the first time a node receives Gossip, it receives everything that isn't expired?

Is that sufficient? What happens if a node starts up and connects to another node that is partitioned? It might only only receive the the non-expired entries from that node. Or were you suggesting that we send all non-expired entries every time a gossip connection is made, disregarding the highwater stamps?

More generally, infinite TTLs are problematic in themselves because we can't delete from Gossip (and I'd rather not introduce it), but at the same time this isn't a problem with infinite TTLs, right? It would affect any TTL that was longer-lived. In the case of node liveness, we could Gossip liveness record X with a TTL that makes the gossip record expire at the liveness entry's expiration + one week, meaning that a week after decommissioning, the node would disappear from Gossip.

The problem here is bad for any gossip record that is gossiped by more than one node and that isn't gossiped on a regular interval. I think the liveness records might be the most problematic case, but I need to do an audit of the other keys.

I'm hesitant to replace infinite TTLs with ones that expire after a week. Figuring out that a gossip record expired after a week and that is causing problems in a cluster isn't something I want to debug. Super subtle problems in gossip like we're continuing to discover make me want to replace it, not keep on applying bandaids.

[petermattis]

I think the liveness records might be the most problematic case, but I need to do an audit of the other keys.

This might also affect the table-stat-added infos and the system-config info. I don't think the table-stat-added infos is problematic, because that key simply causes the table stat cache to invalidate an entry. If a node restarts, its cache will be empty. The system-config may also not be problematic, but I think that is only because there is careful logic to re-gossip the system-config if the leaseholder for the ranges changes. See:

	if gossipedCfg := r.store.Gossip().GetSystemConfig(); gossipedCfg != nil && gossipedCfg.Equal(loadedCfg) &&
		r.store.Gossip().InfoOriginatedHere(gossip.KeySystemConfig) {
		log.VEventf(ctx, 2, "not gossiping unchanged system config")
		return nil
	}

I also just discovered that we're aware that a node's gossiped infos won't necessarily come back to it on restart. See 1b43a93:

storage: Always gossip system config after lease changes hands

Without doing this, it's possible for n1 to gossip the system config,
lose its lease, and restart with the source node of the system config
gossip still being n1. If this happens, other nodes will not provide the
system config gossip back to n1 because its watermark for itself will be
higher than the system config's timestamp.

[bdarnell]

I'm hesitant to replace infinite TTLs with ones that expire after a week.

Yeah, a week is long enough to be very hard to debug. I'd suggest that we have a limit of say an hour for any non-infinite TTL (so anything that needs to live longer than that needs to be re-gossiped periodically).

Is that sufficient? What happens if a node starts up and connects to another node that is partitioned? It might only only receive the the non-expired entries from that node. Or were you suggesting that we send all non-expired entries every time a gossip connection is made, disregarding the highwater stamps?

I think what we probably need is to start identifying gossip connections by a tuple like (node id, start time) so that a restarted node is not seen as a continuation of its previous incarnation.

[petermattis]

I think what we probably need is to start identifying gossip connections by a tuple like (node id, start time) so that a restarted node is not seen as a continuation of its previous incarnation.

Heh, I just filed a bug where I mention that as being an ideal solution. I think there are complexities in actually implementing this and handling the backwards compatibility, though. I need to think about this more.

[petermattis]

I added a 2nd commit which works around the problem with loopback propagation of infos. This is similar to what was done in #17285. I consider this a temporary bandaid that is being utilized because it is much simpler and more easily backported than a fix inside gossip itself.

PTAL

[petermattis]

@tbg or @bdarnell I'd like to get this merged and backported. Any concerns with the hackiness of this "fix"?

[tbg]

Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)

[petermattis]

bors r=tbg

[craig]

Build succeeded

• GitHub CI (Cockroach)