server: missing support for DISABLE_STARTING_BACKGROUND_JOBS in secondary tenants #90524
Assignees
Comments
knz
added
C-bug
exalate-issue-sync
bot
removed
the
T-multitenant
knz
added a commit
to knz/cockroach
that referenced
this issue
Oct 23, 2022
The original motivation (and ultimate goal) for this commit is to split the tenant server initialization into three phases: `New()`, `PreStart()`, `AcceptClients()`, so as to reuse a common process startup logic in a separate (later) commit. To achieve this goal, this commit re-orders the initialization steps in `server.NewTenantServer` (previously known as `server.StartTenant`), and extracts many of them into a new method `(*SQLServerWrapper).PreStart()`. The specific order of the code in `NewTenantServer()` and `(*SQLServerWrapper).PreStart()` was chosen to mirror the order of things in `NewServer()` and `(*Server).PreStart()`. Reasons for using the same order: - it makes the review of this change easier: the reviewer can pull `server.go` and `tenant.go` and read them side-by-side, to satisfy themselves that the two implementations of `NewServer`/`NewTenantServer` and `PreStart` are equivalent. - it will make it easier for future maintainers to keep them in sync. - it helps us discover the fact that both sides share a lot of code. This opens an opportunity to merge them to a common implementation at a later stage. While doing this work, care was also taken to discover which steps of `(*Server).PreStart()` were *missing* from the tenant server initialization. We found the following: - the Sentry context enhancement (to report cluster ID, etc) was missing. This commit fixes that. - several log entries that report the server configuration to the OPS channel were not emitted. This commit fixes that. - the Graphite metric reporting was never enabled, even when configured. This commit fixes that. - the Obs Service testing knobs (TestingKnobs.EventExporter) were not configured on the ObsServer instance. This commit fixes that. - the `go.scheduler_latency` metric was not being measured. This commit fixes that. Additionally, two followup issues were filed for the following missing steps: - missing support for the special file that blocks background jobs. (cockroachdb#90524) - missing support for the system.eventlog cleanup loop. (cockroachdb#90521) Release note: None
knz
added a commit
to knz/cockroach
that referenced
this issue
Oct 26, 2022
The original motivation (and ultimate goal) for this commit is to split the tenant server initialization into three phases: `New()`, `PreStart()`, `AcceptClients()`, so as to reuse a common process startup logic in a separate (later) commit. To achieve this goal, this commit re-orders the initialization steps in `server.NewTenantServer` (previously known as `server.StartTenant`), and extracts many of them into a new method `(*SQLServerWrapper).PreStart()`. The specific order of the code in `NewTenantServer()` and `(*SQLServerWrapper).PreStart()` was chosen to mirror the order of things in `NewServer()` and `(*Server).PreStart()`. Reasons for using the same order: - it makes the review of this change easier: the reviewer can pull `server.go` and `tenant.go` and read them side-by-side, to satisfy themselves that the two implementations of `NewServer`/`NewTenantServer` and `PreStart` are equivalent. - it will make it easier for future maintainers to keep them in sync. - it helps us discover the fact that both sides share a lot of code. This opens an opportunity to merge them to a common implementation at a later stage. While doing this work, care was also taken to discover which steps of `(*Server).PreStart()` were *missing* from the tenant server initialization. We found the following: - the Sentry context enhancement (to report cluster ID, etc) was missing. This commit fixes that. - several log entries that report the server configuration to the OPS channel were not emitted. This commit fixes that. - the Graphite metric reporting was never enabled, even when configured. This commit fixes that. - the Obs Service testing knobs (TestingKnobs.EventExporter) were not configured on the ObsServer instance. This commit fixes that. - the `go.scheduler_latency` metric was not being measured. This commit fixes that. Additionally, two followup issues were filed for the following missing steps: - missing support for the special file that blocks background jobs. (cockroachdb#90524) - missing support for the system.eventlog cleanup loop. (cockroachdb#90521) Release note: None
knz
added a commit
to knz/cockroach
that referenced
this issue
Oct 26, 2022
The original motivation (and ultimate goal) for this commit is to split the tenant server initialization into three phases: `New()`, `PreStart()`, `AcceptClients()`, so as to reuse a common process startup logic in a separate (later) commit. To achieve this goal, this commit re-orders the initialization steps in `server.NewTenantServer` (previously known as `server.StartTenant`), and extracts many of them into a new method `(*SQLServerWrapper).PreStart()`. The specific order of the code in `NewTenantServer()` and `(*SQLServerWrapper).PreStart()` was chosen to mirror the order of things in `NewServer()` and `(*Server).PreStart()`. Reasons for using the same order: - it makes the review of this change easier: the reviewer can pull `server.go` and `tenant.go` and read them side-by-side, to satisfy themselves that the two implementations of `NewServer`/`NewTenantServer` and `PreStart` are equivalent. - it will make it easier for future maintainers to keep them in sync. - it helps us discover the fact that both sides share a lot of code. This opens an opportunity to merge them to a common implementation at a later stage. While doing this work, care was also taken to discover which steps of `(*Server).PreStart()` were *missing* from the tenant server initialization. We found the following: - the Sentry context enhancement (to report cluster ID, etc) was missing. This commit fixes that. - several log entries that report the server configuration to the OPS channel were not emitted. This commit fixes that. - the Graphite metric reporting was never enabled, even when configured. This commit fixes that. - the Obs Service testing knobs (TestingKnobs.EventExporter) were not configured on the ObsServer instance. This commit fixes that. - the `go.scheduler_latency` metric was not being measured. This commit fixes that. Additionally, two followup issues were filed for the following missing steps: - missing support for the special file that blocks background jobs. (cockroachdb#90524) - missing support for the system.eventlog cleanup loop. (cockroachdb#90521) Release note: None
craig bot
pushed a commit
that referenced
this issue
Oct 26, 2022
90384: server: split the tenant server creation into 3 stages r=dhartunian a=knz Needed as prerequisite to #90176, towards fixing #89974. First two commits from #90523. The original motivation (and ultimate goal) for this PR is to split the tenant server initialization into three phases: `New()`, `PreStart()`, `AcceptClients()`, so as to reuse a common process startup logic in a separate PR (#90176). To achieve this goal, this PR re-orders the initialization steps in `server.NewTenantServer` (previously known as `server.StartTenant`), and extracts many of them into a new method `(*SQLServerWrapper).PreStart()`. The specific order of the code in `NewTenantServer()` and `(*SQLServerWrapper).PreStart()` was chosen to mirror the order of things in `NewServer()` and `(*Server).PreStart()`. Reason for using the same order: - it makes the review of this change easier: the reviewer can pull `server.go` and `tenant.go` and read them side-by-side, to satisfy themselves that the two implementations of `NewServer`/`NewTenantServer` and `PreStart` are equivalent. - it will make it easier for future maintainers to keep them in sync. - it helps us discover the fact that both sides share a lot of code. This opens an opportunity to merge them to a common implementation at a later stage. While doing this work, care was also taken to discover which steps of `(*Server).PreStart()` were *missing* from the tenant server initialization. We found the following: - the Sentry context enhancement (to report cluster ID, etc) was missing. This commit fixes that. - several log entries that report the server configuration to the OPS channel were not emitted. This commit fixes that. - the Graphite metric reporting was never enabled, even when configured. This commit fixes that. - the Obs Service testing knobs (TestingKnobs.EventExporter) were not configured on the ObsServer instance. This commit fixes that. - the `go.scheduler_latency` metric was not being measured. This commit fixes that. Additionally, two followup issues were filed for the following missing steps: - missing support for the special file that blocks background jobs. (#90524) - missing support for the system.eventlog cleanup loop. (#90521) Epic: CRDB-14537 Co-authored-by: Raphael 'kena' Poss <[email protected]>
craig bot
pushed a commit
that referenced
this issue
Oct 31, 2022
90176: cli/start: unify code between `cockroach start` and `cockroach mt start-sql` r=andreimatei a=knz Fixes #89974. Fixes #90831. Fixes #90524. This PR merges the server initialization code between `cockroach start` and `cockroach mt start-sql`. In doing so, it brings `cockroach mt start-sql` closer to what we expect from proper CockroachDB server processes: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from #4754) - it adds a tracing span for the startup code. (from #8712!!) - it properly supports `--listening-url-file`. (from #15468) - it properly does sanitization of `--external-io-dir`. (from #19725) - it sets the proper log severity level for gRPC. (from #20308) - it reports the command-line and env config to logs. (from #21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from #42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from #44043) - it recovers support for `DISABLE_STARTING_BACKGROUND_JOBS`. (from #44786) - it sets `GOMAXPROCS` from current cgroup limits. (from #57390) - it stops the server early if the storage disk is full. (from #66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from #73876) See the individual commit for details. 90660: sql: add contention_events to cluster_execution_insights r=j82w a=j82w The original contention column will remain to make query operations faster. The events are being put into a json column because it's possible there could be multiple blocking events for a single statement. The json column avoids the complexity of adding another table and keeping it in sync with the insights table. The table can be joined with index_columns and tables to get the database name, table name, and index name that contention occurred on. This does not contain the blocking statement information, and the blocking fingerprint id. closes: #88561 Release note (sql change): Adds contention_events to cluster_execution_insights. This is used to see which transaction is blocking the specific statement. 90719: opgen: added a bool field in struct opgen.transition r=Xiang-Gu a=Xiang-Gu This PR adds a bool field in struct opgen.transition that indicates whether it results from a `equiv(xx)` transition spec in the opgen file. It will be useful for a test where we need to find the inital status on a adding/dropping path. Without such a change, it can be problematic if we have a `equiv(xx)` spec as the first transition. E.g. ``` ToAbsent( PUBLIC, equiv(VALIDATED), to(WRITE_ONLY), to(ABSENT), ) ``` Without this change, the inital status will confusingly be `VALIDATED`, and the next status will be `PUBLIC`. With this change, the initial status will be `PUBLIC`, and the next status will be `WRITE_ONLY`. We also added some comments when we make transitions from the specs. Epic: None Release note: None 90865: sql: use bare name string of new pk to compare with pk name when altering primary key r=chengxiong-ruan a=chengxiong-ruan Fixes #90836 Release note (sql change): previously, the `DROP CONSTRAINT, ADD CONSTRAINT` in one trick to have a new primary key without moving old primary key to be a secondary index didn't work if the primary key name is a reserved SQL keyword. A `constraint already exists` error was returned. This patch fixed the bug, the trick now also works with primary key named as reserved keywords. Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: j82w <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The parameter
jobAdoptionStopFileinsqlServerArgsis not set properly for secondary tenants. It should.Epic: CRDB-14537
Jira issue: CRDB-20819
The text was updated successfully, but these errors were encountered: