server: enable test cluster to take different store configurations

[BramGruneir]

2 commits:

• base/server: move store_spec from server to base
  This move will allow us to configure testclusters with store level granularity
  which would otherwise cause circular dependencies.
• server: enable testserver to take different store configurations
  testServerArgs will now take StoreSpecs instead of just a number of stores.
  This also adds StoreSpecsPerNode on testClusterArgs to enable per node store
  settings. Currently, only in-memory stores are supported.

Part of work towards #8473.

---

This change is

[andreimatei]

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

acceptance/cluster/localcluster.go, line 47 [r2] (raw file):

  "github.com/cockroachdb/cockroach/rpc"
  "github.com/cockroachdb/cockroach/security"
  "github.com/cockroachdb/cockroach/server"

I forgot localcluster existed. Do you know who uses this?

---

base/store_spec.go, line 37 [r1] (raw file):

// MinimumStoreSize is the smallest size in bytes that a store can have. This
// number was originally based on config's defaultZoneConfig's RangeMaxBytes,

nit: "originally" is confusing here, makes one think it's not based on that any more (even though in spirit it still is). I'd strip it.

---

base/test_server_args.go, line 83 [r2] (raw file):

  // StoreSpecsPerNode adds specific stores to specific nodes. The map's keys
  // match the node number passed into StartTestCluster. These work in

nit: "match a node's index within TestCluster.Servers"

---

base/test_server_args.go, line 86 [r2] (raw file):

  // conjunction with the StoreSpecs already in TestServerArgs and can be
  // used to supplement those stores that are already on all nodes.
  StoreSpecsPerNode map[int][]StoreSpec

hmmm so what if I want to overwrite the stores on a node (in particular the default store)?
I vote we keep these structures normalized, even though that's pedantic (but since nobody seems to actually even be using this field now, it won't matter). So make ServerArgs an array (with the asterisk that you can put a single item in there and it'll be copied to every Server) and then you can customize each Server as you wish.

---

server/status_test.go, line 155 [r2] (raw file):

func startServer(t *testing.T) *TestServer {
  tsI, _, kvDB := serverutils.StartServer(t, base.TestServerArgs{
      StoreSpecs: []base.StoreSpec{

if you happen to know, please add a comment here as to why this tests needs or wants multiple stores.

---

server/testserver.go, line 235 [r2] (raw file):

          ))
      } else {
          panic(fmt.Sprintf("test server does not yet support in non-memory stores: %s", storeSpec))

just curious, why not? Just the lack of cleanup code? Maybe add a comment if you know.

---

Comments from Reviewable

[BramGruneir]

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

acceptance/cluster/localcluster.go, line 47 [r2] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I forgot localcluster existed. Do you know who uses this?

This is all acceptance tests. Local docker runs that is.

---

base/store_spec.go, line 37 [r1] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: "originally" is confusing here, makes one think it's not based on that any more (even though in spirit it still is). I'd strip it.

Done.

---

base/test_server_args.go, line 83 [r2] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: "match a node's index within TestCluster.Servers"

Done.

---

base/test_server_args.go, line 86 [r2] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

hmmm so what if I want to overwrite the stores on a node (in particular the default store)?
I vote we keep these structures normalized, even though that's pedantic (but since nobody seems to actually even be using this field now, it won't matter). So make ServerArgs an array (with the asterisk that you can put a single item in there and it'll be copied to every Server) and then you can customize each Server as you wish.

I just made this an override map of serverArgs. So we can customize everything.

---

server/status_test.go, line 155 [r2] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

if you happen to know, please add a comment here as to why this tests needs or wants multiple stores.

Not 100% sure, but I think it's to ensure that metrics are recorded correctly for more than one store per node.

---

server/testserver.go, line 235 [r2] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

just curious, why not? Just the lack of cleanup code? Maybe add a comment if you know.

Done. And exactly, we need to add cleanup code.

---

Comments from Reviewable

[andreimatei]

---

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[tamird]

Reviewed 6 of 7 files at r1, 3 of 6 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

base/store_spec.go, line 40 [r3] (raw file):

// extremely stable. To avoid adding the dependency on config here, it is just
// hard coded to 640MiB.
const MinimumStoreSize = 10 * 64 << 20

It may be nicer to define this in terms of config.DefaultZoneConfig.

---

server/testserver.go, line 235 [r3] (raw file):

          ))
      } else {
          // TODO(bram): This will require both cleanup and on failure or if

there's something not right about this sentence.

---

server/testserver.go, line 237 [r3] (raw file):

          // TODO(bram): This will require both cleanup and on failure or if
          // desired, not cleanup (perhaps on failure).
          panic(fmt.Sprintf("test server does not yet support in non-memory stores: %s", storeSpec))

"in non-memory" is an odd way to say this. "persistent"?

---

testutils/testcluster/testcluster.go, line 137 [r3] (raw file):

  tc.stopper = stop.NewStopper()

  args.ServerArgs.PartOfCluster = true

shouldn't this have been removed? if not, why was it also added below?

---

Comments from Reviewable

[BramGruneir]

@tamird, I'll address your comments in a follow up PR shortly.

---

Comments from Reviewable

[BramGruneir]

Addressed all feedback in #8836

---

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

base/store_spec.go, line 40 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

It may be nicer to define this in terms of config.DefaultZoneConfig.

Nice yes, but that bring in another import to base. Which I'm trying to avoid.

---

server/testserver.go, line 235 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

there's something not right about this sentence.

Done.

---

server/testserver.go, line 237 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

"in non-memory" is an odd way to say this. "persistent"?

Done.

---

testutils/testcluster/testcluster.go, line 137 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

shouldn't this have been removed? if not, why was it also added below?

PartOfCluster is required to be true when running a cluster. When starting up a testserver without a testcluster is when the value can be false. I've added a comment to the struct to denote that.

---

Comments from Reviewable