storageccl,cli: add external IO dir cli flag #19725
Conversation
|
This change is |
|
Review status: 0 of 14 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. pkg/ccl/storageccl/export_storage.go, line 227 at r1 (raw file):
I think it is incorrect to have a per-node setting that a user is then required to know, and be the same across the entire cluster, for them to use this feature. I think we should instead require relative paths so that clusters can be configured however they need to be, possibly with differing settings, and the user's query won't have to change. pkg/cli/start.go, line 410 at r1 (raw file):
"disabled" looks like leftover from testing pkg/cli/start.go, line 411 at r1 (raw file):
debug leftover? maybe you already know about it... pkg/cli/cliflags/flags.go, line 483 at r1 (raw file):
May need to specify that this only applies to nodelocal and nfs. Comments from Reviewable |
dt
force-pushed
the
extern
branch
4 times, most recently
from
0dbe254
to
45bf8d8
Compare
|
okay, I've added tests that this does what it says on the tin. After discussion in person with mjibson though, I'm planning to move forward with not just restricting access to just the local dir, but rather prefixing all accessed paths with it -- as a cli-flag it is node-specific, it's just generally not something we want specified in a query that is executed across many nodes. The biggest downside to automatically prefixing is that it could be confusing in the case where you say "backup db to file:///home/david/desktop/bar" and then don't find "bar" on your desktop. We discussed a few ways we might try to prevent this confusion:
In the end, we don't like any of these. We decided to just auto-prefix. If you try to read that doesn't exist after prefixing, we'll make sure the error makes it clear that the file was searched for in the external IO dir. |
dt
force-pushed
the
extern
branch
2 times, most recently
from
bc3e653
to
0dbe254
Compare
|
Separate is my preference. |
|
okay, it's here dt/cockroach@extern...dt:extern-prefix, but I'll wait for this one to land then before opening a PR for it. |
|
Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions. pkg/ccl/storageccl/export_storage.go, line 227 at r1 (raw file): Previously, mjibson (Matt Jibson) wrote…
As discussed, the big change to switch to relative IO paths will be a follow-up CL. pkg/cli/start.go, line 410 at r1 (raw file): Previously, mjibson (Matt Jibson) wrote…
We'll want a way to completely disable local disk IO, so if "" defaults to first store dir, we'll need a placeholder value like "disabled". There's no risk of collisions with an intended path since actual paths must be absolute. Added a cli test that "disabled" prints the right message. pkg/cli/start.go, line 411 at r1 (raw file): Previously, mjibson (Matt Jibson) wrote…
whoops, yes, thanks, pkg/cli/cliflags/flags.go, line 483 at r1 (raw file): Previously, mjibson (Matt Jibson) wrote…
Done. Comments from Reviewable |
|
Review status: 0 of 15 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful. pkg/ccl/storageccl/export_storage_test.go, line 224 at r2 (raw file):
newline here Comments from Reviewable |
|
Review status: 0 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending. pkg/ccl/storageccl/export_storage_test.go, line 224 at r2 (raw file): Previously, mjibson (Matt Jibson) wrote…
Done. Comments from Reviewable |
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
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]>
okay, still need to test the CLI side of this one (need to figure out how to do that in ccl tree), but wanted to get some early feedback and see what the CI thinks on all this plumbing.