ptreconcile,server: rework ptreconciler for multi-tenant by adityamaru · Pull Request #75688 · cockroachdb/cockroach

adityamaru · 2022-01-29T03:01:20Z

The ptreconciler is responisble for periodically scanning the
system.pts_records table, and checking if there are any stale
records to be released, based on callbacks registerd on server
startup. Previously, the ptreconciler used the meta1leaseholder
to ensure that there was only one instance of it running in the
cluster. Additionally, it was reliant on the ptcache to iterate
over the records when checking whether they were stale.

In the multi-tenant version of the protected timestamp subsystem,
the SQL pod running the reconciler cannot use the meta1leaseholder
to determine whether or not it should run the reconciliation loop.
To get around this, we move the Start of the ptreconciler to the
Resume hook of the auto span config job. We are guaranteed via the
spanconfig manager, that there will always be atmost one instance of
this job in a cluster. Further, this is a forever running job, and so
we can tie the execution of the ptreconciler to the lifetime of the
spanconfig job resumer. Additionally, since we will be doing away
with the ptcache, we switchover to doing a full table scan every time
the reconciliation loop is run. While not ideal, this is not alarming
since we have a conservative limit on the total size of all records
that can be stored in the table, and reconciliation only runs once
every 5mins by default. Additionally, we do not expect many
concurrent BACKUP/CDC jobs to exist in the cluster at a given point in
time.

This change also refactors some of the server and tenant code to plumb
a ptreconciler to the ExecutorConfig, for use by the auto span config
job. We move the relevant job+schedule tests into a ccl pacakge to allow
testing from within a secondary tenant.

Informs: #73727

Release note: None

cockroach-teamcity · 2022-01-29T03:01:26Z

This change is

shermanCRL · 2022-01-29T17:05:44Z

(Suggestion, can the explanation in this commit message be a comment closer to the code?)

pkg/ccl/jobsccl/jobs_protected_ts_test.go

pkg/sql/exec_util.go

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @arulajmani)

pkg/kv/kvserver/protectedts/ptreconcile/reconciler.go, line 34 at r1 (raw file):

// When set to zero - disables the report generation.
var ReconcileInterval = settings.RegisterDurationSetting(
	settings.TenantWritable,

why would this be tenant writable? Are we okay w/ tenant disabling this? Or setting it to 1ns?

adityamaru · 2022-01-31T02:06:58Z

why would this be tenant writable? Are we okay w/ tenant disabling this? Or setting it to 1ns?

Hmm, good point. Every reconciliation will query the jobs/schedules table given a particular record, so setting this to 1ns might not be ideal? OTOH the transaction is very short lived in that it's a point query, and then a release of the record if at all. My hunch is that we don't really see this being a problem. Jobs are, or at least we hope they are more robust than before, and they clean up properly after themselves.

I was actually ignorant of these classes until you brought it up, so going by:

//  - Control settings relevant to tenant-specific internal implementation
//    should be TenantReadOnly.
//
//  - When in doubt, the first choice to consider should be TenantReadOnly.

my vote is we flip it to TenantReadOnly.

ajwerner

Reviewed 1 of 20 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @miretskiy)

pkg/kv/kvserver/protectedts/ptreconcile/reconciler.go, line 34 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

why would this be tenant writable? Are we okay w/ tenant disabling this? Or setting it to 1ns?

I agree with tenant read-only

pkg/sql/exec_util.go, line 1203 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

This is carry forward from PTS v1, so I'm unsure of what the reasoning was when that was written. @ajwerner might remember, but my guess would be some sort of dep cycle.

Just didn't feel moved to abstract it into an interface because it was just a thing getting kicked off by the server that had no methods anybody used. Now that there are methods. Honestly, what are your thoughts on constructing this in the job rather than plumbing it down here? I think that upholds the same principle. I feel like for the spanconfig stuff, the plumbing is only justified by the weight of the dependency injection it'd take to construct such a thing in the job.

adityamaru · 2022-01-31T02:24:28Z

(Suggestion, can the explanation in this commit message be a comment closer to the code?)

Is there any part in particular you suggest? I thought the interesting part was the fact that we use the auto span config job as our singleton for which I already have a short blurb here.

https://github.com/cockroachdb/cockroach/pull/75688/files#diff-4d440c27aeac8809854348841a6c37672ff7beb80308498c4ef35b668022e8e7R49

adityamaru · 2022-01-31T03:29:05Z

Honestly, what are your thoughts on constructing this in the job

Good point, I think this is doable since execCtx has everything we need. The only piece I'm unsure about is how to add the corresponding reconciler metric struct, if we start init'ing the reconciler in the span config job Resume hook. We could plumb the metrics registry down, but is that any better than plumbing the one-time init'ed reconciler?

ajwerner

That's a sufficiently legit reason to keep constructing it in the server. Consider making an interface for the thing in protectedts?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @miretskiy)

ajwerner · 2022-01-31T04:53:32Z

pkg/kv/kvserver/protectedts/protectedts.go

+// Metrics encapsulates the metrics exported by the reconciler.
+type Metrics struct {
+	ReconcilationRuns    *metric.Counter
+	RecordsProcessed     *metric.Counter
+	RecordsRemoved       *metric.Counter
+	ReconciliationErrors *metric.Counter
+}
+
+var _ metric.Struct = (*Metrics)(nil)
+
+// MetricStruct makes Metrics a metric.Struct.
+func (m *Metrics) MetricStruct() {}


This part doesn't need to move, the server still holds on to the concrete struct.

Yup, moved it back and exported the constructor instead.

ajwerner · 2022-01-31T04:53:35Z

pkg/kv/kvserver/protectedts/protectedts.go

+	// configured StatusFunc.
+	StartReconciler(ctx context.Context, stopper *stop.Stopper) error
+	// Metrics returns the metrics exported by the reconciler.
+	Metrics() *Metrics


I don't think you need to export this.

Since we want to subsume the Reconciler interface in the Provider interface, the initialization of the reconciler needs to happen https://github.com/cockroachdb/cockroach/pull/75688/files#diff-47ad9f8fa32a4e686099e2e89dad4beca96772073e042d746b771bc58ffb9f01R55. So we don't have a handle to the concrete type of the reconciler in server/tenant.go to grab its metrics struct. I removed this method and exported the constructor of the Metrics struct instead. This is then plumbed into ptprovider.New().

pkg/kv/kvserver/protectedts/protectedts.go

pkg/kv/kvserver/protectedts/ptreconcile/reconciler.go

ajwerner

One more round and I'll be happy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, and @miretskiy)

pkg/kv/kvserver/protectedts/protectedts.go, line 165 at r2 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Since we want to subsume the Reconciler interface in the Provider interface, the initialization of the reconciler needs to happen https://github.com/cockroachdb/cockroach/pull/75688/files#diff-47ad9f8fa32a4e686099e2e89dad4beca96772073e042d746b771bc58ffb9f01R55. So we don't have a handle to the concrete type of the reconciler in server/tenant.go to grab its metrics struct. I removed this method and exported the constructor of the Metrics struct instead. This is then plumbed into ptprovider.New().

This is getting nit-picky, but I'd rather you just gave the Provider a Metrics() method which returned metric.MetricStruct as opposed to plumbing the metrics into the provider. That gives you a seamless way to add more metrics later in the provider without touching server code by potentially nesting the reconciler metric struct in some other metric struct later.

adityamaru · 2022-01-31T19:48:09Z

as opposed to plumbing the metrics into the provider

I like it, thanks for the suggestion, done.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @arulajmani, and @miretskiy)

pkg/kv/kvserver/protectedts/ptprovider/provider.go, line 47 at r4 (raw file):

	protectedts.Cache
	protectedts.Reconciler
	metric.Struct

Heh one more, instead of this, can we have a method Metrics() that returns the metric.Struct and delegates to ptreconcile.Metrics()

pkg/kv/kvserver/protectedts/ptreconcile/metrics.go, line 27 at r4 (raw file):

func makeMetrics() *Metrics {
	return &Metrics{

I think you can now revert this

pkg/kv/kvserver/protectedts/ptreconcile/reconciler.go, line 86 at r4 (raw file):

// Metrics returns the reconciler's metrics.
func (r *Reconciler) Metrics() *Metrics {
	return r.metrics

just take the address like it had before

adityamaru · 2022-01-31T20:00:13Z

can we have a method Metrics() that returns the metric.Struct and delegates to ptreconcile.Metrics()

Hmm do you mean something like:

func (p *provider) Metrics() metric.Struct {
	return p.Reconciler.Metrics()
}

That would need us to expose Metrics in the Reconciler interface right?

ajwerner · 2022-01-31T20:08:28Z

That would need us to expose Metrics in the Reconciler interface right?

In my head the provider was embedding the implementations as opposed to the interfaces. I'm indifferent when it comes to copying the metric struct out or not. Throwing Metrics on the Reconciler interface is fine. Storing it, like you have, is also fine.

--stress

The ptreconciler is responisble for periodically scanning the `system.pts_records` table, and checking if there are any stale records to be released, based on callbacks registerd on server startup. Previously, the ptreconciler used the meta1leaseholder to ensure that there was only one instance of it running in the cluster. Additionally, it was reliant on the ptcache to iterate over the records when checking whether they were stale. In the multi-tenant version of the protected timestamp subsystem, the SQL pod running the reconciler cannot use the meta1leaseholder to determine whether or not it should run the reconciliation loop. To get around this, we move the `Start` of the ptreconciler to the Resume hook of the auto span config job. We are guaranteed via the spanconfig manager, that there will always be atmost one instance of this job in a cluster. Further, this is a forever running job, and so we can tie the execution of the ptreconciler to the lifetime of the spanconfig job resumer. Additionally, since we will be doing away with the ptcache, we switchover to doing a full table scan every time the reconciliation loop is run. While not ideal, this is not alarming since we have a conservative limit on the total size of all records that can be stored in the table, and reconciliation only runs once every 5mins by default. Additionally, we do not expect many concurrent BACKUP/CDC jobs to exist in the cluster at a given point in time. This change also refactors some of the server and tenant code to plumb a ptreconciler to the ExecutorConfig, for use by the auto span config job. We move the relevant job+schedule tests into a ccl pacakge to allow testing from within a secondary tenant. Informs: cockroachdb#73727 Release note: None

adityamaru · 2022-02-01T00:24:46Z

I've seen this bazel timeout on other PRs, pretty sure it is unrelated. Started an internal thread to discuss, but going ahead with a merge here.

TFTR!
bors r=ajwerner,miretskiy

craig · 2022-02-01T02:02:54Z

Build failed (retrying...):

GitHub CI (Cockroach)

adityamaru · 2022-02-01T02:57:41Z

bors r+

craig · 2022-02-01T02:57:45Z

Already running a review

adityamaru · 2022-02-01T03:55:41Z

bors r+

craig · 2022-02-01T03:55:42Z

Already running a review

craig · 2022-02-01T06:08:36Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-01T09:06:08Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested review from ajwerner and arulajmani January 29, 2022 03:01

adityamaru requested review from a team as code owners January 29, 2022 03:01

adityamaru mentioned this pull request Jan 29, 2022

protectedts: rework protected timestamp storage for multi-tenant model #73727

Closed

23 tasks

adityamaru force-pushed the rework-reconciler branch from 7918494 to 80e6af1 Compare January 29, 2022 19:30

miretskiy reviewed Jan 29, 2022

View reviewed changes

pkg/ccl/jobsccl/jobs_protected_ts_test.go Outdated Show resolved Hide resolved

pkg/sql/exec_util.go Outdated Show resolved Hide resolved

miretskiy reviewed Jan 29, 2022

View reviewed changes

ajwerner reviewed Jan 31, 2022

View reviewed changes

adityamaru force-pushed the rework-reconciler branch from 80e6af1 to 163be93 Compare January 31, 2022 04:36

adityamaru requested a review from ajwerner January 31, 2022 04:39

ajwerner reviewed Jan 31, 2022

View reviewed changes

adityamaru force-pushed the rework-reconciler branch from 163be93 to 8549d99 Compare January 31, 2022 15:28

adityamaru requested a review from ajwerner January 31, 2022 15:32

ajwerner reviewed Jan 31, 2022

View reviewed changes

adityamaru force-pushed the rework-reconciler branch from 8549d99 to 158f05c Compare January 31, 2022 19:47

adityamaru requested a review from ajwerner January 31, 2022 19:48

ajwerner reviewed Jan 31, 2022

View reviewed changes

adityamaru force-pushed the rework-reconciler branch from 158f05c to a4bf9c0 Compare January 31, 2022 20:51

miretskiy reviewed Jan 31, 2022

View reviewed changes

--stress Outdated Show resolved Hide resolved

adityamaru force-pushed the rework-reconciler branch from a4bf9c0 to 500d190 Compare January 31, 2022 22:25

craig bot merged commit 65db9cf into cockroachdb:master Feb 1, 2022

adityamaru deleted the rework-reconciler branch February 1, 2022 13:42

Conversation

adityamaru commented Jan 29, 2022

Uh oh!

cockroach-teamcity commented Jan 29, 2022

Uh oh!

shermanCRL commented Jan 29, 2022

Uh oh!

Uh oh!

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jan 31, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jan 31, 2022

Uh oh!

adityamaru commented Jan 31, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jan 31, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajwerner commented Jan 31, 2022 • edited by petermattis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adityamaru commented Feb 1, 2022

Uh oh!

craig bot commented Feb 1, 2022

Uh oh!

adityamaru commented Feb 1, 2022

Uh oh!

craig bot commented Feb 1, 2022

Uh oh!

adityamaru commented Feb 1, 2022

Uh oh!

craig bot commented Feb 1, 2022

Uh oh!

craig bot commented Feb 1, 2022

Uh oh!

craig bot commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adityamaru commented Jan 31, 2022 •

edited

Loading

ajwerner commented Jan 31, 2022 •

edited by petermattis

Loading