gossip: remove frequent gossiping of gossip client connections #89613

a-robinson · 2022-10-08T18:51:03Z

These gossip-clients keys make up two thirds or more of the gossip traffic in various large clusters I've inspected, consuming almost an entire CPU core in the worst case I've seen. They don't provide enough value to justify that sort of ongoing cost, so this commit removes them entirely as well as the periodic logging of the gossip network and the crdb_internal.gossip_network table, both of which relied on them.

These gossip-clients keys make up two thirds or more of the gossip
traffic in various large clusters I've inspected, consuming almost an
entire CPU core in the worst case I've seen. They don't provide enough
value to justify that sort of ongoing cost, so this commit removes them
entirely as well as the periodic logging of the gossip network and the
crdb_internal.gossip_network table, both of which relied on them.

Release note (backward-incompatible change): We've stopped
supporting/populating the crdb_internal.gossip_network table. It was an
internal table with no API guarantees (so perhaps no meriting a release
note?).

Release note (performance improvement): Significantly reduced CPU usage
of the underlying gossip network in large clusters.

Informs #51838 (largely fixes it for practical purposes, although there's likely still more that could be done)

This is clearly going to break the gossip roachtest, but between @irfansharif kindly volunteering to fix that up separately and his existing TODO in that file I've left that out of this change.

I don't know if completely removing the gossip_network table is really the best idea or if it should just be left in and only populated with the clients from the local node. For example, when run in a mixed version cluster does debug zip run all of its sql commands against the local node or does it run some against remote nodes? If an old node ever tries to query the gossip_network table on a different node it could have a bad time.

@irfansharif @kvoli

blathers-crl · 2022-10-08T18:51:08Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

@knz (commented on gossip: abnormal CPU usage growth with increasing node count #51838)
@irfansharif (commented on gossip: abnormal CPU usage growth with increasing node count #51838)
@ajwerner (commented on gossip: abnormal CPU usage growth with increasing node count #51838)
@kvoli (assigned to gossip: abnormal CPU usage growth with increasing node count #51838)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2022-10-08T18:51:14Z

This change is

a-robinson · 2022-10-08T19:04:18Z

(apologies in advance if some tests fail in CI -- my laptop is able to build cockroach but not really set up to run the full test suite, so I'm relying on CI to point out any issues)

blathers-crl · 2022-10-09T19:18:05Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

kvoli

Thanks for the PR @a-robinson.

For example, when run in a mixed version cluster does debug zip run all of its sql commands against the local node or does it run some against remote nodes?

I'm not certain on this one. I think local node but I'm not familiar here. cc @irfansharif.

The failing test TestGossipPropagation seems due to no component actively removing expired infos. Previously the clientsTimer would tick in this test every 2 seconds so it would eventually succeed.

I think in a real cluster, given the store gossip @ 10s so there is still something removing it, should be fine - just specific to this test. Triggering gossip w/ a key inside the retry loop should fix

mustAdd(remote, "remote-trigger", nil, 2 * time.Minute)

kvoli · 2022-10-11T15:57:04Z

pkg/sql/crdb_internal.go

-			if _, err := g.GetInfo(gossip.MakeGossipClientsKey(d.NodeID)); err == nil {
-				alive[d.NodeID] = true
+			var gossipLiveness livenesspb.Liveness
+			if err := g.GetInfoProto(gossip.MakeNodeLivenessKey(d.NodeID), &gossipLiveness); err == nil {


Was this change needed to pass some tests?

I haven't actually tried running any tests without this. It just seemed like it would be pretty bad if crdb_internal.gossip_nodes suddenly switched to always saying that all nodes are not live (due to the gossip clients keys being missing). Especially since IIRC it's the table that's used by CLI commands like cockroach node status.

And basing it on node liveness seems like a reasonable alternative, although I'm open to other ideas.

a-robinson · 2022-10-11T17:01:16Z

I think in a real cluster, given the store gossip @ 10s so there is still something removing it, should be fine - just specific to this test. Triggering gossip w/ a key inside the retry loop should fix

Thanks, I suspect so too. I just haven't had a good chance to fix it up yet, but will do soon.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @a-robinson, @irfansharif, and @kvoli)

pkg/sql/crdb_internal.go line 4234 at r1 (raw file):

// crdbInternalGossipNetwork exposes the local view of the gossip network (i.e
// the gossip client connections from source_id node to target_id node).
var crdbInternalGossipNetworkTable = virtualSchemaTable{

drive-by comment: i'd be slightly more inclined to make this an empty table (or do the idea to only show local clients) rather than removing the table outright

we could also have it do p.BufferClientNotice to warn that this table is no longer reliable

irfansharif · 2022-10-14T13:57:22Z

I'll just take over this PR + fallout, through next week.

a-robinson · 2022-10-16T02:13:27Z

I'll just take over this PR + fallout, through next week.

Thanks!

a-robinson · 2022-12-11T02:25:22Z

Congrats on getting 22.2 out! I'd hope this is something that will make it into v23.1?

Let me know if it'd help for me to clean something up here, although it's unlikely that I'm the best person to make the call on what to do with the crdb_internal.gossip_network` table (or any other compatibility concerns) at this point.

irfansharif

I need a reviewer here. @kvoli and @tbg, up for it? Alex did most of the work so you know it's good. Do review it with backportability to 22.2 (at the very least) in mind. Was thinking we'd let this bake for a two-ish weeks pre-backport. If it doesn't feel eligible for a backport now that 22.2.0 was published publicly, I'll be a bit regretful of dropping it earlier but would understand.

I don't know if completely removing the gossip_network table is really the best idea or if it should just be left in and only populated with the clients from the local node.

I've re-added the table but left it empty. It just makes it slightly more palatable for a backport.

we could also have it do p.BufferClientNotice to warn that this table is no longer reliable

Done.

For example, when run in a mixed version cluster does debug zip run all of its sql commands against the local node or does it run some against remote nodes? If an old node ever tries to query the gossip_network table on a different node it could have a bad time.

Runs it against the local node. But we do want to support older version CRDB binaries in a point release querying newer point release CRDB servers, so keeping the table around seems ok. For mixed-version clusters while the cluster version hasn't yet been finalized, we want to be able to use the older binary generally. There too keeping the table around seems helpful. In a future release we can get rid of it entirely.

The failing test TestGossipPropagation seems due to no component actively removing expired infos.

Fixed.

This is clearly going to break the gossip roachtest, but between @irfansharif kindly volunteering to fix that up separately and his existing TODO in that file I've left that out of this change.

Fixed.

It just seemed like it would be pretty bad if crdb_internal.gossip_nodes suddenly switched to always saying that all nodes are not live (due to the gossip clients keys being missing). Especially since IIRC it's the table that's used by CLI commands like cockroach node status. And basing it on node liveness seems like a reasonable alternative, although I'm open to other ideas.

Keeping gossip_nodes backed by the gossiped view of node liveness seems better than using the KV node liveness. It's generally good that these tables are highly available and don't need leaseholder participation. We could get that with liveness too by looking at the in-memory state on the node, but I'll keep it as is.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @a-robinson and @kvoli)

These gossip-clients keys make up two thirds or more of the gossip traffic in various large clusters I've inspected, consuming almost an entire CPU core in the worst case I've seen. They don't provide enough value to justify that sort of ongoing cost, so this commit removes them entirely as well as the periodic logging of the gossip network and the crdb_internal.gossip_network table, both of which relied on them. Release note (backward-incompatible change): We've stopped supporting/populating the crdb_internal.gossip_network table. It was an internal table with no API guarantees (so perhaps no meriting a release note?). Release note (performance improvement): Significantly reduced CPU usage of the underlying gossip network in large clusters.

tbg

thanks for pushing this over the finish line!

Reviewed 5 of 22 files at r1, 8 of 17 files at r2, 1 of 2 files at r3, 3 of 11 files at r4, all commit messages.
Dismissed @kvoli from a discussion.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @a-robinson and @irfansharif)

-- commits line 9 at r5:
Still reading the PR, but is there a chance in the ./cockroach node status data sources (see cli roachtest change)? If so, worth pointing out.

pkg/cmd/roachtest/tests/cli.go line 112 at r5 (raw file):

	waitUntil([]string{
		"is_available is_live",
		"false false",

I am confused by this change, what's going on here?

pkg/sql/crdb_internal.go line 4108 at r5 (raw file):

			if err := g.GetInfoProto(gossip.MakeNodeLivenessKey(d.NodeID), &gossipLiveness); err == nil {
				if now.Before(gossipLiveness.Expiration.ToTimestamp().GoTime()) {
					alive[d.NodeID] = true

This works, but I wonder if this is the canonical way to do this sort of thing in unified architecture. I would've thought the tenant would have a liveness view of KV around somewhere (possibly powered by Gossip under the hood). Not to block the merge - after all this works - but maybe @knz can take a look and see if this is palatable long-term.

kvoli

Do review it with backportability to 22.2 (at the very least) in mind. Was thinking we'd let this bake for a two-ish weeks pre-backport. If it doesn't feel eligible for a backport now that 22.2.0 was published publicly, I'll be a bit regretful of dropping it earlier but would understand.

I don't see an issue backporting.

Reviewed 2 of 22 files at r1, 1 of 17 files at r2, 8 of 11 files at r4, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @a-robinson and @irfansharif)

irfansharif

TFTR!

bors r+

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @a-robinson and @tbg)

-- commits line 9 at r5:

Previously, tbg (Tobias Grieger) wrote…

Still reading the PR, but is there a chance in the ./cockroach node status data sources (see cli roachtest change)? If so, worth pointing out.

There's no change.

pkg/cmd/roachtest/tests/cli.go line 112 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I am confused by this change, what's going on here?

It's because of the changes under pkg/sql/crdb_internal.go. Before the is_live column was computed off of the existence of a gossip clients key, which for n1 in test test, would've existed (n1 isn't taken down). But now that we no longer have these keys, is_live is backed by gossiped back liveness. And n1 can't heartbeat its liveness because we've killed 2/3 nodes.

pkg/sql/crdb_internal.go line 4108 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This works, but I wonder if this is the canonical way to do this sort of thing in unified architecture. I would've thought the tenant would have a liveness view of KV around somewhere (possibly powered by Gossip under the hood). Not to block the merge - after all this works - but maybe @knz can take a look and see if this is palatable long-term.

I'll defer to Rafa to sort out what a tenant-with-capabilities view of this would look like. I assume it's just going to be a table populated by data returned through the Connector interface, which in turn would do a very similar thing to what we're doing here.

a-robinson

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)

pkg/cmd/roachtest/tests/cli.go line 112 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

It's because of the changes under pkg/sql/crdb_internal.go. Before the is_live column was computed off of the existence of a gossip clients key, which for n1 in test test, would've existed (n1 isn't taken down). But now that we no longer have these keys, is_live is backed by gossiped back liveness. And n1 can't heartbeat its liveness because we've killed 2/3 nodes.

FWIW it may be possible to use gossiped store descriptors instead of gossiped liveness since my understanding is that those get refreshed every couple minutes, but I don't know whether that would really be better overall. Using liveness seemed better to me at the time, but I didn't consider what the output should look like during cluster instability.

craig · 2022-12-21T16:39:16Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2022-12-21T16:39:32Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

Backport to branch 22.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

irfansharif · 2022-12-21T16:44:13Z

blathers backport 22.2

blathers-crl · 2022-12-21T16:44:19Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

Backport to branch 22.2 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

erikgrinaker · 2023-05-23T13:46:25Z

I think the 22.2 backport here broke backwards compatibility with 22.1. In a mixed 22.2/22.1 cluster, running cockroach node status on a 22.1 node will report is_live = false for 22.2 nodes. I'll have a closer look at exactly why.

erikgrinaker · 2023-05-23T13:58:47Z

Ah, right, 22.1 uses GossipClientsKey to determine liveness, but 22.2 no longer gossips that key and relies on NodeLivenessKey instead. This should've been behind a version gate.

I don't know if there's much we can do about this now. 22.1 is out of support with no further releases planned. We can consider backporting a retroactive 22.2 version gate, but it's not clear that it's worth the risk and effort.

erikgrinaker · 2023-05-23T14:16:35Z

I guess we'll have the same problem with mixed 22.2 patch releases before/after this backport, and with mixed 22.2/23.1 clusters before this backport. The right approach here would've been to add a 23.1 version gate and not backport to 22.2.

I think the best option here would be to backport a 23.1 version gate, and to semi-revert the 22.2 backport such that it still gossips the GossipClientsKey but doesn't actually use it for anything. Alternatively, live with a broken is_live in cockroach node status with certain combinations of 22.2 mixed-version clusters.

erikgrinaker · 2023-05-23T14:26:26Z

For reference, this was initially released in v22.2.3.

erikgrinaker · 2023-05-23T15:18:55Z

Opened #103788 with a partial 22.2 revert. I'm not entirely convinced we want to do that, considering the cost of gossiping these, but breaking backwards compatibility isn't great either.

irfansharif · 2023-05-23T16:18:41Z

This should've been behind a version gate.

This was intentional BTW, since we did want to backport it to 22.2 and backports can't carry version gates. The is_live breakage in mixed version settings is slightly unfortunate but it's less meaningful IMO than the CPU% the backport reduces.

I'm not entirely convinced we want to do that, considering the cost of gossiping these

I'm with you. I feel like a release note pointing to newer patch releases is sufficient: #103788 (review).

erikgrinaker · 2023-05-23T16:22:06Z

We can't add version gates, but we can piggyback on the existing V22_2 version gate.

Of course, that still breaks patch release compat, but at least we stop the bleeding in mixed-major clusters.

a-robinson requested review from a team as code owners October 8, 2022 18:51

a-robinson requested a review from a team October 8, 2022 18:51

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Oct 8, 2022

blathers-crl bot requested review from ajwerner, irfansharif, knz and kvoli October 8, 2022 18:51

a-robinson force-pushed the rm-gossip-clients branch from c1ae3a5 to 6cdf785 Compare October 9, 2022 19:18

irfansharif removed request for a team, knz and ajwerner October 10, 2022 13:41

kvoli reviewed Oct 11, 2022

View reviewed changes

rafiss reviewed Oct 11, 2022

View reviewed changes

irfansharif self-assigned this Oct 14, 2022

irfansharif force-pushed the rm-gossip-clients branch from 6cdf785 to e9d3ca0 Compare December 19, 2022 16:49

irfansharif requested a review from a team as a code owner December 19, 2022 16:49

irfansharif requested review from herkolategan and smg260 and removed request for a team December 19, 2022 16:49

irfansharif reviewed Dec 19, 2022

View reviewed changes

irfansharif requested a review from tbg December 19, 2022 22:13

irfansharif added the backport-22.2.x label Dec 19, 2022

irfansharif changed the title ~~gossip: Remove frequent gossiping of gossip client connections~~ gossip: remove frequent gossiping of gossip client connections Dec 19, 2022

irfansharif force-pushed the rm-gossip-clients branch from 42d6cb8 to faec9a5 Compare December 19, 2022 22:43

irfansharif force-pushed the rm-gossip-clients branch from faec9a5 to 2114678 Compare December 19, 2022 22:45

tbg approved these changes Dec 20, 2022

View reviewed changes

kvoli approved these changes Dec 20, 2022

View reviewed changes

irfansharif reviewed Dec 21, 2022

View reviewed changes

a-robinson commented Dec 21, 2022

View reviewed changes

craig bot merged commit 8ae6026 into cockroachdb:master Dec 21, 2022

blathers-crl bot mentioned this pull request Dec 21, 2022

release-22.2: gossip: remove frequent gossiping of gossip client connections #94071

Closed

blathers-crl bot mentioned this pull request Dec 21, 2022

release-22.2: gossip: remove frequent gossiping of gossip client connections #94072

Closed

irfansharif mentioned this pull request Dec 21, 2022

release-22.2: gossip: remove frequent gossiping of gossip client connections #94074

Merged

This was referenced Dec 22, 2022

PR #89613 - gossip: remove frequent gossiping of gossip client connections (1 of 2) cockroachdb/docs#15881

Open

PR #89613 - gossip: remove frequent gossiping of gossip client connections (2 of 2) cockroachdb/docs#15882

Open

koorosh mentioned this pull request Jan 18, 2023

server: add NetworkConnectivity endpoint to get network connection statuses between nodes #95429

Merged

erikgrinaker mentioned this pull request May 23, 2023

release-22.2: gossip: reintroduce GossipClientsKey gossip #103788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gossip: remove frequent gossiping of gossip client connections #89613

gossip: remove frequent gossiping of gossip client connections #89613

a-robinson commented Oct 8, 2022 •

edited by irfansharif

Loading

blathers-crl bot commented Oct 8, 2022

cockroach-teamcity commented Oct 8, 2022

a-robinson commented Oct 8, 2022

blathers-crl bot commented Oct 9, 2022

kvoli left a comment

kvoli Oct 11, 2022

a-robinson Oct 11, 2022

a-robinson commented Oct 11, 2022

rafiss left a comment

irfansharif commented Oct 14, 2022

a-robinson commented Oct 16, 2022

a-robinson commented Dec 11, 2022

irfansharif left a comment

tbg left a comment •

edited

Loading

kvoli left a comment

irfansharif left a comment

a-robinson left a comment

craig bot commented Dec 21, 2022

blathers-crl bot commented Dec 21, 2022

irfansharif commented Dec 21, 2022

blathers-crl bot commented Dec 21, 2022

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023 •

edited

Loading

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023 •

edited

Loading

irfansharif commented May 23, 2023

erikgrinaker commented May 23, 2023 •

edited

Loading

gossip: remove frequent gossiping of gossip client connections #89613

gossip: remove frequent gossiping of gossip client connections #89613

Conversation

a-robinson commented Oct 8, 2022 • edited by irfansharif Loading

blathers-crl bot commented Oct 8, 2022

cockroach-teamcity commented Oct 8, 2022

a-robinson commented Oct 8, 2022

blathers-crl bot commented Oct 9, 2022

kvoli left a comment

Choose a reason for hiding this comment

kvoli Oct 11, 2022

Choose a reason for hiding this comment

a-robinson Oct 11, 2022

Choose a reason for hiding this comment

a-robinson commented Oct 11, 2022

rafiss left a comment

Choose a reason for hiding this comment

irfansharif commented Oct 14, 2022

a-robinson commented Oct 16, 2022

a-robinson commented Dec 11, 2022

irfansharif left a comment

Choose a reason for hiding this comment

tbg left a comment • edited Loading

Choose a reason for hiding this comment

kvoli left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

a-robinson left a comment

Choose a reason for hiding this comment

craig bot commented Dec 21, 2022

blathers-crl bot commented Dec 21, 2022

irfansharif commented Dec 21, 2022

blathers-crl bot commented Dec 21, 2022

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023 • edited Loading

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023

erikgrinaker commented May 23, 2023 • edited Loading

irfansharif commented May 23, 2023

erikgrinaker commented May 23, 2023 • edited Loading

a-robinson commented Oct 8, 2022 •

edited by irfansharif

Loading

tbg left a comment •

edited

Loading

erikgrinaker commented May 23, 2023 •

edited

Loading

erikgrinaker commented May 23, 2023 •

edited

Loading

erikgrinaker commented May 23, 2023 •

edited

Loading