Skip to content

Commit

Permalink
server: use "read-only" Gossip for tenants
Browse files Browse the repository at this point in the history
We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  cockroachdb#47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    cockroachdb#49691
  - `database.Cache`, tracked:
    cockroachdb#49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: cockroachdb#48432

Release note: None
  • Loading branch information
tbg committed Jun 2, 2020
1 parent 06de45a commit 4d036c3
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 18 deletions.
6 changes: 6 additions & 0 deletions pkg/gossip/gossip.go
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,12 @@ type DeprecatedGossip struct {
w errorutil.TenantSQLDeprecatedWrapper
}

// Start calls .Start() on the underlying Gossip instance, which is assumed to
// be non-nil.
func (dg DeprecatedGossip) Start(advertAddr net.Addr, resolvers []resolver.Resolver) {
dg.w.Deprecated(0).(*Gossip).Start(advertAddr, resolvers)
}

// deprecated trades a Github issue tracking the removal of the call for the
// wrapped Gossip instance.
func (dg DeprecatedGossip) deprecated(issueNo int) *Gossip {
Expand Down
79 changes: 61 additions & 18 deletions pkg/server/testserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
"github.com/cockroachdb/cockroach/pkg/gossip"
"github.com/cockroachdb/cockroach/pkg/gossip/resolver"
"github.com/cockroachdb/cockroach/pkg/jobs"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
Expand Down Expand Up @@ -437,27 +438,22 @@ func (d dummyProtectedTSProvider) Protect(context.Context, *kv.Txn, *ptpb.Record
// this is tracked in https://github.com/cockroachdb/cockroach/issues/47892.
const fakeNodeID = roachpb.NodeID(1)

func testSQLServerArgs(ts *TestServer, tenID roachpb.TenantID) sqlServerArgs {
stopper := ts.Stopper()
clusterName := ts.Cfg.ClusterName
// If we used a dummy gossip, DistSQL and random other things won't work.
// Just use the test server's for now.
//
// TODO(tbg): drop the Gossip dependency.
g := ts.Gossip()
ts = nil // prevent usage below
func testSQLServerArgs(
stopper *stop.Stopper, kvClusterName string, tenID roachpb.TenantID,
) sqlServerArgs {

st := cluster.MakeTestingClusterSettings()

sqlCfg := makeTestSQLConfig(st, tenID)

baseCfg := makeTestBaseConfig(st)
baseCfg.AmbientCtx.AddLogTag("sql", nil)
// TODO(tbg): this is needed so that the RPC heartbeats between the testcluster
// and this tenant work.
//
// TODO(tbg): address this when we introduce the real tenant RPCs in:
// https://github.com/cockroachdb/cockroach/issues/47898
baseCfg.ClusterName = clusterName
baseCfg.ClusterName = kvClusterName

clock := hlc.NewClock(hlc.UnixNano, time.Duration(baseCfg.MaxOffset))

Expand All @@ -474,13 +470,41 @@ func testSQLServerArgs(ts *TestServer, tenID roachpb.TenantID) sqlServerArgs {
rpcTestingKnobs,
)

// TODO(tbg): expose this registry via prometheus. See:
// https://github.com/cockroachdb/cockroach/issues/47905
registry := metric.NewRegistry()

var dsKnobs kvcoord.ClientTestingKnobs
if dsKnobsP, ok := baseCfg.TestingKnobs.DistSQL.(*kvcoord.ClientTestingKnobs); ok {
dsKnobs = *dsKnobsP
}
rpcRetryOptions := base.DefaultRetryOptions()
resolver := gossip.AddressResolver(g) // TODO(tbg): break gossip dep
nodeDialer := nodedialer.New(rpcContext, resolver)

// TODO(nvb): this use of Gossip needs to go. Tracked in:
// https://github.com/cockroachdb/cockroach/issues/47909
var g *gossip.Gossip
{
var nodeID base.NodeIDContainer
nodeID.Set(context.Background(), fakeNodeID)
var clusterID base.ClusterIDContainer
dummyGossipGRPC := rpc.NewServer(rpcContext) // never Serve()s anything
g = gossip.New(
baseCfg.AmbientCtx,
&clusterID,
&nodeID,
rpcContext,
dummyGossipGRPC,
stopper,
registry,
baseCfg.Locality,
&baseCfg.DefaultZoneConfig,
)
}

nodeDialer := nodedialer.New(
rpcContext,
gossip.AddressResolver(g), // TODO(nvb): break gossip dep
)
dsCfg := kvcoord.DistSenderConfig{
AmbientCtx: baseCfg.AmbientCtx,
Settings: st,
Expand All @@ -497,9 +521,7 @@ func testSQLServerArgs(ts *TestServer, tenID roachpb.TenantID) sqlServerArgs {
if p, ok := baseCfg.TestingKnobs.KVClient.(*kvcoord.ClientTestingKnobs); ok {
clientKnobs = *p
}
// TODO(tbg): expose this registry via prometheus. See:
// https://github.com/cockroachdb/cockroach/issues/47905
registry := metric.NewRegistry()

txnMetrics := kvcoord.MakeTxnMetrics(baseCfg.HistogramWindowInterval())
registry.AddMetricStruct(txnMetrics)
tcsFactory := kvcoord.NewTxnCoordSenderFactory(
Expand Down Expand Up @@ -594,13 +616,24 @@ func (ts *TestServer) StartTenant(params base.TestTenantArgs) (pgAddr string, _
return "", err
}

args := testSQLServerArgs(ts, params.TenantID)
if params.AllowSettingClusterSettings {
return startTenant(ts.Stopper(), ts.Cfg.ClusterName, ts.RPCAddr(), params.TenantID, params.AllowSettingClusterSettings)
}

func startTenant(
stopper *stop.Stopper,
kvClusterName string,
tsRPCAddr string,
tenID roachpb.TenantID,
allowSetClusterSetting bool,
) (pgAddr string, _ error) {
args := testSQLServerArgs(stopper, kvClusterName, tenID)
// TODO(tbg): clean this up.
if allowSetClusterSetting {
args.TestingKnobs.TenantTestingKnobs = &sql.TenantTestingKnobs{
ClusterSettingsUpdater: args.Settings.MakeUpdater(),
}
}
ts = nil // proves we're not using it below
ctx := context.Background()

s, err := newSQLServer(ctx, args)
if err != nil {
Expand Down Expand Up @@ -639,6 +672,16 @@ func (ts *TestServer) StartTenant(params base.TestTenantArgs) (pgAddr string, _
)
orphanedLeasesTimeThresholdNanos := args.clock.Now().WallTime

{
rsvlr, err := resolver.NewResolver(tsRPCAddr)
if err != nil {
return "", err
}
// NB: gossip server is not bound to any address, so the advertise addr does
// not matter.
args.gossip.Start(pgL.Addr(), []resolver.Resolver{rsvlr})
}

if err := s.start(ctx,
args.stopper,
args.TestingKnobs,
Expand Down

0 comments on commit 4d036c3

Please sign in to comment.