kv: in v20.2, bootstrapping multiple stores can result in duplicate store IDs

[nvanbenschoten]

See https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1614352496110700 for context.

We've found that in v20.2.5, it is possible for a race between two bootstrapping nodes to result in store ID re-use. This is because the following code, which mistakenly assumes that the store ID allocated during the join RPC handshake and the additional store IDs allocated by the call to allocateStoreIDs are continuous.

cockroach/pkg/server/node.go

Lines 584 to 587 in c82e011

	startID, err := allocateStoreIDs(ctx, n.Descriptor.NodeID, storeIDAlloc, n.storeCfg.DB)
	if firstStoreID == 0 {
	firstStoreID = startID
	}

cockroach/pkg/server/node.go

Line 613 in c82e011

sIdent.StoreID++

This assumption is broken when multiple nodes are bootstrapped at the same time. In such cases, it becomes possible for the join RPC store ID allocations to interleave with each other.

There are a few action items here.

• Fix the issue on release-20.2

First and foremost, we need to fix the bug and backport that fix to release-20.2 as soon as possible, ideally for the v20.2.6. A strawman fix is:

diff --git a/pkg/server/node.go b/pkg/server/node.go
index 6add213574..c4d165a794 100644
--- a/pkg/server/node.go
+++ b/pkg/server/node.go
@@ -610,7 +610,11 @@ func (n *Node) bootstrapStores(
                                log.Warningf(ctx, "error doing initial gossiping: %s", err)
                        }
-                       sIdent.StoreID++
+                       if sIdent.StoreID == firstStoreID {
+                               sIdent.StoreID = startID
+                       } else {
+                               sIdent.StoreID++
+                       }
                }
        }

We probably should also not be calling allocateStoreIDs at all if storeIDAlloc is 0, or at least make it a no-op.

• Unit test this scenario

We should be able to create this situation in a unit test, which will then allow us to confirm that we have properly fixed the problem.

• Confirm that this bug is not present before or after v20.2

It doesn't look like it is, but we should confirm.

• Improve multi-store testing

In the past week, customers have hit this issue and #60545. We clearly aren't testing multi-store deployments enough, or we would have found them before our customers did. We now have the ability to run multi-store roachprod clusters, so we have the tools to improve our testing story here.

[nvanbenschoten]

Am I reading #56272 and #56271 correctly in that we knew about this issue back in November?

[irfansharif]

Yes, looks like the reason we didn't backport was because I mistakenly assumed we had (inadvertant) protection against it: #56271 (comment). I think what I was talking about there was another double store ID allocation bug, around the same code-paths.

[nvanbenschoten]

I see, makes sense if we thought things were ok on 20.2. Do you have thoughts on how we'll want to patch this? Should we do the least invasive thing for release-20.2 since things are so much cleaner on master already?

[irfansharif]

Yup, let's carry over in #61262. I'm planning to add same test there to master.

[irfansharif]

@nvanbenschoten, I'm going to close this issue out. Agreed that more multi-store testing seems prudent, both in our roachtests + unit tests. We'd have caught this earlier if we did so.