-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-21.2: roachtest: fix multitenant-upgrade #71692
Conversation
In #71040 we added disk spilling for tenants which added the following call to the `mt start-sql` code path: https://github.com/cockroachdb/cockroach/blob/af5a5a5065ce80c5e6568b4b422bf5c3a179e173/pkg/cli/mt_start_sql.go#L90-L89 The defaults for the store match that of a regular CockroachDB node, and the tenant will thus attempt to clean up temp dirs for `cockroach-data` if no store is specified: https://github.com/cockroachdb/cockroach/blob/6999e5fded43f59eb5839dc8b943fd1e2a33a3fd/pkg/cli/start.go#L223-L227 In the `multitenant-upgrade` roachtest, as it happens there was actually a cockroach host instance running under `cockroach-data`, and so the tenant would fail to try to remove its (locked) temp dirs. Fix that by passing the `--store` flag to the tenants in this test. This is mildly annoying since the predecessor version doesn't understand it and so the test has to figure out when it is legal to pass it. Anyway, it is done now. I will point out that it isn't the greatest choice to have tenants default to `cockroach-data` as the resulting interaction with a CRDB server results in an unfortunate UX. I filed #71603 to that effect. Release note: None
3f12396
to
465113b
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)
Not sure what the rules for merging are these days, but going to keep this open until we've released. |
Hmm actually merging this would give us coverage of the upgrade process, so I should ask to merge this. |
Got approval from Isaac. |
Backport 1/1 commits from #71604 on behalf of @tbg.
Fixes #71269.
/cc @cockroachdb/release
In #71040 we added disk spilling for tenants which added the following
call to the
mt start-sql
code path:https://github.com/cockroachdb/cockroach/blob/af5a5a5065ce80c5e6568b4b422bf5c3a179e173/pkg/cli/mt_start_sql.go#L90-L89
The defaults for the store match that of a regular CockroachDB node, and
the tenant will thus attempt to clean up temp dirs for
cockroach-data
if no store is specified:
cockroach/pkg/cli/start.go
Lines 223 to 227 in 6999e5f
In the
multitenant-upgrade
roachtest, as it happens there was actuallya cockroach host instance running under
cockroach-data
, and so thetenant would fail to try to remove its (locked) temp dirs.
Fix that by passing the
--store
flag to the tenants in this test. Thisis mildly annoying since the predecessor version doesn't understand it
and so the test has to figure out when it is legal to pass it. Anyway,
it is done now.
I will point out that it isn't the greatest choice to have tenants
default to
cockroach-data
as the resulting interaction with a CRDBserver results in an unfortunate UX. I filed #71603 to that effect.
Closes #69920
Release note: None
Release justification: