Skip to content

Commit

Permalink
Merge #71994 #73674
Browse files Browse the repository at this point in the history
71994: spanconfig: introduce spanconfig.Reconciler r=irfansharif a=irfansharif

#### spanconfig: introduce spanconfig.Reconciler

Reconciler is responsible for reconciling a tenant's zone configs (SQL
construct) with the cluster's span configs (KV construct). It's the
central engine for the span configs infrastructure; a single Reconciler
instance is active for every tenant in the system.

```go
type Reconciler interface {
  // Reconcile starts the incremental reconciliation process from
  // the given checkpoint. If it does not find MVCC history going
  // far back enough[1], it falls back to a scan of all
  // descriptors and zone configs before being able to do more
  // incremental work. The provided callback is invoked with
  // timestamps that can be safely checkpointed. A future
  // Reconciliation attempt can make use of this timestamp to
  // reduce the amount of necessary work (provided the MVCC
  // history is still available).
  //
  // [1]: It's possible for system.{zones,descriptor} to have been
  //      GC-ed away; think suspended tenants.
  Reconcile(
    ctx context.Context,
    checkpoint hlc.Timestamp,
    callback func(checkpoint hlc.Timestamp) error,
  ) error
}
```

Let's walk through what it does. At a high-level, we maintain an
in-memory data structure that's up-to-date with the contents of the KV
(at least the subset of spans we have access to, i.e. the keyspace
carved out for our tenant ID). We watch for changes to SQL state
(descriptors, zone configs), translate the SQL updates to the flattened
span+config form, "diff" the updates against our data structure to see
if there are any changes we need to inform KV of. If so, we do, and
ensure that our data structure is kept up-to-date. We continue watching
for future updates and repeat as necessary.

There's only single instance of the Reconciler running for a given
tenant at a given point it time (mutual exclusion/leasing is provided by
the jobs subsystem). We needn't worry about contending writers, or the
KV state being changed from underneath us. What we do have to worry
about, however, is suspended tenants' not being reconciling while
suspended. It's possible for a suspended tenant's SQL state to be GC-ed
away at older MVCC timestamps; when watching for changes, we could fail
to observe tables/indexes/partitions getting deleted. Left as is, this
would result in us never issuing a corresponding deletion requests for
the dropped span configs -- we'd be leaving orphaned span configs lying
around (taking up storage space and creating pointless empty ranges). A
"full reconciliation pass" is our attempt to find all these extraneous
entries in KV and to delete them.

We can use our span config data structure here too, one that's
pre-populated with the contents of KV. We translate the entire SQL state
into constituent spans and configs, diff against our data structure to
generate KV updates that we then apply. We follow this with clearing out
all these spans in our data structure, leaving behind all extraneous
entries to be found in KV -- entries we can then simply issue deletes
for. 

\----

#### server,kvaccessor: record span configs during tenant creation/gc

For newly created tenants, we want to ensure hard splits on tenant
boundaries. The source of truth for split points in the span configs
subsystem is the contents of `system.span_configurations`. To ensure
hard splits, we insert a single key record at the tenant prefix. In a
future commit we'll introduce the spanconfig.Reconciler process, which
runs per tenant and governs all config entries under each tenant's
purview. This has implications for this initial record we're talking
about (this record might get cleaned up for e.g.); we'll explore it in
tests for the Reconciler itself.

Creating a single key record is easy enough -- we could've written
directly to `system.span_configurations`. When a tenant is GC-ed
however, we need to clear out all tenant state transactionally. To that
end we plumb in a txn-scoped KVAccessor into the planner where
`crdb_internal.destroy_tenant` is executed. This lets us easily delete
all abandoned tenant span config records.  

Note: We get rid of `spanconfig.experimental_kvaccessor.enabled`. Access
to spanconfigs infrastructure is already sufficiently gated through
COCKROACH_EXPERIMENTAL_SPAN_CONFIGS. Now that
crdb_internal.create_tenant attempts to write through the KVAccessor,
it's cumbersome to have to enable the setting manually in every
multi-tenant test (increasingly the default) enabling some part of the
span configs infrastructure.  

This commit introduces the need for a migration -- for existing clusters
with secondary tenants, when upgrading we need to install this initial
record at the tenant prefix for all extant tenants (and make sure to
continue doing so for subsequent newly created tenants). This is to
preserve the hard-split-on-tenant-boundary invariant we wish to provide.
It's possible for an upgraded multi-tenant cluster to have dormant sql
pods that have never reconciled. If KV switches over to the span configs
subsystem, splitting only on the contents of
`system.span_configurations`, we'll fail to split on all tenant
boundaries. We'll introduce this migration in a future PR (before
enabling span configs by default).  

Release note: None

---

Only the last two commits here are of interest (first one is from #73531).


73674: scplan: break out stage building into scstage package and invert rule directions r=postamar a=postamar

These 4 commits introduce a new scstage package. This was motivated by the more immediate goal of inverting dependency edge directions, to express them as precedence constraints instead of succession constraints.

Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
  • Loading branch information
3 people committed Dec 15, 2021
3 parents 0f69631 + f22eefa + 5fc93c8 commit 5e70321
Show file tree
Hide file tree
Showing 83 changed files with 4,090 additions and 1,682 deletions.
2 changes: 1 addition & 1 deletion docs/generated/settings/settings-for-tenants.txt
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,4 @@ trace.debug.enable boolean false if set, traces for recent requests can be seen
trace.jaeger.agent string the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.
trace.opentelemetry.collector string address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.
trace.zipkin.collector string the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.
version version 21.2-28 set the active cluster version in the format '<major>.<minor>'
version version 21.2-32 set the active cluster version in the format '<major>.<minor>'
2 changes: 1 addition & 1 deletion docs/generated/settings/settings.html
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,6 @@
<tr><td><code>trace.jaeger.agent</code></td><td>string</td><td><code></code></td><td>the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.</td></tr>
<tr><td><code>trace.opentelemetry.collector</code></td><td>string</td><td><code></code></td><td>address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.</td></tr>
<tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.</td></tr>
<tr><td><code>version</code></td><td>version</td><td><code>21.2-28</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
<tr><td><code>version</code></td><td>version</td><td><code>21.2-32</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
</tbody>
</table>
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ ALL_TESTS = [
"//pkg/ccl/serverccl/diagnosticsccl:diagnosticsccl_test",
"//pkg/ccl/serverccl/statusccl:statusccl_test",
"//pkg/ccl/serverccl:serverccl_test",
"//pkg/ccl/spanconfigccl/spanconfigreconcilerccl:spanconfigreconcilerccl_test",
"//pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl:spanconfigsqltranslatorccl_test",
"//pkg/ccl/sqlproxyccl/denylist:denylist_test",
"//pkg/ccl/sqlproxyccl/idle:idle_test",
Expand Down Expand Up @@ -184,6 +185,7 @@ ALL_TESTS = [
"//pkg/spanconfig/spanconfigkvaccessor:spanconfigkvaccessor_test",
"//pkg/spanconfig/spanconfigkvsubscriber:spanconfigkvsubscriber_test",
"//pkg/spanconfig/spanconfigmanager:spanconfigmanager_test",
"//pkg/spanconfig/spanconfigreconciler:spanconfigreconciler_test",
"//pkg/spanconfig/spanconfigsqltranslator:spanconfigsqltranslator_test",
"//pkg/spanconfig/spanconfigsqlwatcher:spanconfigsqlwatcher_test",
"//pkg/spanconfig/spanconfigstore:spanconfigstore_test",
Expand Down
1 change: 1 addition & 0 deletions pkg/ccl/kvccl/kvtenantccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ go_library(
"//pkg/config",
"//pkg/config/zonepb",
"//pkg/gossip",
"//pkg/kv",
"//pkg/kv/kvclient/kvcoord:with-mocks",
"//pkg/kv/kvclient/kvtenant",
"//pkg/kv/kvclient/rangecache:with-mocks",
Expand Down
6 changes: 6 additions & 0 deletions pkg/ccl/kvccl/kvtenantccl/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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/kv"
"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvtenant"
"github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangecache"
Expand Down Expand Up @@ -453,6 +454,11 @@ func (c *Connector) UpdateSpanConfigEntries(
})
}

// WithTxn implements the spanconfig.KVAccessor interface.
func (c *Connector) WithTxn(context.Context, *kv.Txn) spanconfig.KVAccessor {
panic("not applicable")
}

// withClient is a convenience wrapper that executes the given closure while
// papering over InternalClient retrieval errors.
func (c *Connector) withClient(
Expand Down
6 changes: 6 additions & 0 deletions pkg/ccl/migrationccl/migrationsccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,24 @@ go_test(
srcs = [
"main_test.go",
"records_based_registry_external_test.go",
"seed_tenant_span_configs_external_test.go",
],
deps = [
"//pkg/base",
"//pkg/ccl/baseccl",
"//pkg/ccl/kvccl/kvtenantccl",
"//pkg/clusterversion",
"//pkg/keys",
"//pkg/roachpb:with-mocks",
"//pkg/security",
"//pkg/security/securitytest",
"//pkg/server",
"//pkg/spanconfig",
"//pkg/testutils/serverutils",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
"//pkg/util/leaktest",
"//pkg/util/log",
"//pkg/util/protoutil",
"@com_github_stretchr_testify//require",
],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
// Copyright 2021 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package migrationsccl_test

import (
"context"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
_ "github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/spanconfig"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/stretchr/testify/require"
)

// TestPreSeedSpanConfigsWrittenWhenActive tests that seed span configs are
// written to for fresh tenants if the cluster version that introduced it is
// active.
func TestPreSeedSpanConfigsWrittenWhenActive(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
EnableSpanConfigs: true, // we use spanconfig.KVAccessor to check if its contents are as we'd expect
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DisableAutomaticVersionUpgrade: 1,
BinaryVersionOverride: clusterversion.ByKey(
clusterversion.PreSeedTenantSpanConfigs,
),
},
},
},
})

defer tc.Stopper().Stop(ctx)
ts := tc.Server(0)

tenantID := roachpb.MakeTenantID(10)
_, err := ts.StartTenant(ctx, base.TestTenantArgs{TenantID: tenantID})
require.NoError(t, err)

scKVAccessor := ts.SpanConfigKVAccessor().(spanconfig.KVAccessor)
tenantPrefix := keys.MakeTenantPrefix(tenantID)
tenantSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd()}
tenantSeedSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.Next()}

{
entries, err := scKVAccessor.GetSpanConfigEntriesFor(ctx, []roachpb.Span{
tenantSpan,
})
require.NoError(t, err)
require.Len(t, entries, 1)
require.Equal(t, entries[0].Span, tenantSeedSpan)
}
}

// TestSeedTenantSpanConfigs tests that the migration installs relevant seed
// span configs for existing secondary tenants.
func TestSeedTenantSpanConfigs(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
EnableSpanConfigs: true, // we use spanconfig.KVAccessor to check if its contents are as we'd expect
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DisableAutomaticVersionUpgrade: 1,
BinaryVersionOverride: clusterversion.ByKey(
clusterversion.PreSeedTenantSpanConfigs - 1,
),
},
},
},
})

defer tc.Stopper().Stop(ctx)
ts := tc.Server(0)
tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
scKVAccessor := ts.SpanConfigKVAccessor().(spanconfig.KVAccessor)

tenantID := roachpb.MakeTenantID(10)
tenantPrefix := keys.MakeTenantPrefix(tenantID)
tenantSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd()}
tenantSeedSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.Next()}
{
_, err := ts.StartTenant(ctx, base.TestTenantArgs{TenantID: tenantID})
require.NoError(t, err)
}

{ // Ensure that no span config entries are to be found
entries, err := scKVAccessor.GetSpanConfigEntriesFor(ctx, []roachpb.Span{
tenantSpan,
})
require.NoError(t, err)
require.Empty(t, entries)
}

tdb.Exec(t,
"SET CLUSTER SETTING version = $1",
clusterversion.ByKey(clusterversion.SeedTenantSpanConfigs).String(),
)

{ // Ensure that the tenant now has a span config entry.
entries, err := scKVAccessor.GetSpanConfigEntriesFor(ctx, []roachpb.Span{
tenantSpan,
})
require.NoError(t, err)
require.Len(t, entries, 1)
require.Equal(t, entries[0].Span, tenantSeedSpan)
}
}

// TestSeedTenantSpanConfigsWithExistingEntry tests that the migration ignores
// tenants with existing span config entries.
func TestSeedTenantSpanConfigsWithExistingEntry(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
EnableSpanConfigs: true, // we use spanconfig.KVAccessor to check if its contents are as we'd expect
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DisableAutomaticVersionUpgrade: 1,
BinaryVersionOverride: clusterversion.ByKey(
clusterversion.PreSeedTenantSpanConfigs,
),
},
},
},
})

defer tc.Stopper().Stop(ctx)
ts := tc.Server(0)
tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
scKVAccessor := ts.SpanConfigKVAccessor().(spanconfig.KVAccessor)

tenantID := roachpb.MakeTenantID(10)
tenantPrefix := keys.MakeTenantPrefix(tenantID)
tenantSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.PrefixEnd()}
tenantSeedSpan := roachpb.Span{Key: tenantPrefix, EndKey: tenantPrefix.Next()}
{
_, err := ts.StartTenant(ctx, base.TestTenantArgs{TenantID: tenantID})
require.NoError(t, err)
}

{ // Ensure that the tenant already has a span config entry.
entries, err := scKVAccessor.GetSpanConfigEntriesFor(ctx, []roachpb.Span{
tenantSpan,
})
require.NoError(t, err)
require.Len(t, entries, 1)
require.Equal(t, entries[0].Span, tenantSeedSpan)
}

// Ensure the cluster version bump goes through successfully.
tdb.Exec(t,
"SET CLUSTER SETTING version = $1",
clusterversion.ByKey(clusterversion.SeedTenantSpanConfigs).String(),
)

{ // Ensure that the tenant's span config entry stay as it was.
entries, err := scKVAccessor.GetSpanConfigEntriesFor(ctx, []roachpb.Span{
tenantSpan,
})
require.NoError(t, err)
require.Len(t, entries, 1)
require.Equal(t, entries[0].Span, tenantSeedSpan)
}
}
35 changes: 35 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
load("@io_bazel_rules_go//go:def.bzl", "go_test")

go_test(
name = "spanconfigreconcilerccl_test",
srcs = [
"datadriven_test.go",
"main_test.go",
],
data = glob(["testdata/**"]),
deps = [
"//pkg/base",
"//pkg/ccl/kvccl/kvtenantccl",
"//pkg/ccl/partitionccl",
"//pkg/ccl/utilccl",
"//pkg/jobs",
"//pkg/keys",
"//pkg/roachpb:with-mocks",
"//pkg/security",
"//pkg/security/securitytest",
"//pkg/server",
"//pkg/spanconfig",
"//pkg/spanconfig/spanconfigtestutils",
"//pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster",
"//pkg/testutils",
"//pkg/testutils/serverutils",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
"//pkg/util/hlc",
"//pkg/util/leaktest",
"//pkg/util/randutil",
"@com_github_cockroachdb_datadriven//:datadriven",
"@com_github_cockroachdb_errors//:errors",
"@com_github_stretchr_testify//require",
],
)
Loading

0 comments on commit 5e70321

Please sign in to comment.