roachtest: fix multitenant-upgrade by tbg · Pull Request #71604 · cockroachdb/cockroach

tbg · 2021-10-15T09:11:58Z

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

    
           recordPath := filepath.Join(spec.Path, server.TempDirsRecordFilename) 
        
           if err := storage.CleanupTempDirs(recordPath); err != nil { 
        
           	return base.TempStorageConfig{}, errors.Wrap(err, 
        
           		"could not cleanup temporary directories from record file") 
        
           }

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.

Closes #69920

Release note: None

In cockroachdb#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 cockroachdb#71603 to that effect. Release note: None

cockroach-teamcity · 2021-10-15T09:12:08Z

This change is

jaylim-crl

Thanks for the quick fix!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

tbg · 2021-10-19T08:16:39Z

bors r=jaylim-crl

craig · 2021-10-19T08:53:37Z

Build succeeded:

GitHub CI (Cockroach)

tbg · 2021-10-19T10:31:25Z

blathers backport 21.2

blathers-crl · 2021-10-19T10:31:27Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating backport branch refs/heads/blathers/backport-release-21.2-71604: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 21.2 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

Previously, the `multitenant-upgrade` test had several workarounds and branches for dealing with predecessor versions less than v21.2, particularly the fact that these versions did not support the `--store` flag passed on the `mt start-sql` CLI command. This was introduced in cockroachdb#71040, and subsequently incorporated into the roachtest (including workarounds) in cockroachdb#71604. Now that the current version is v22.1, and thus the predecessor version supports the `--store` flag, it is necessary to remove the workaround in order to ensure proper functionality of the test. Additionally, this change cleans up an even earlier workaround for validating the cluster version for predecessor versions less than v21.1.2. Fixes cockroachdb#72971. Release note: None

72948: ui: add index stats to table details page r=lindseyjin a=lindseyjin Resolves #67647, #72842 Previously, there was no way to view and clear index usage stats from the frontend db console. This commit adds Index Stats tables for each table on the Table Detail pages, allowing users to view index names, total reads, and last used statistics. This commit also adds the functionality of clearing all index stats as a button on the Index Stats tables. ![image](https://user-images.githubusercontent.com/29153209/142700094-cad60f81-8c83-48cc-b1f4-970e80d951ee.png) Release note (ui change): Add index stats table and button to clear index usage stats on the Table Details page for each table. 73145: roachtest: fix flags used in multitenant-upgrade for versions >= v21.2 r=AlexTalks a=AlexTalks Previously, the `multitenant-upgrade` test had several workarounds and branches for dealing with predecessor versions less than v21.2, particularly the fact that these versions did not support the `--store` flag passed on the `mt start-sql` CLI command. This was introduced in #71040, and subsequently incorporated into the roachtest (including workarounds) in #71604. Now that the current version is v22.1, and thus the predecessor version supports the `--store` flag, it is necessary to remove the workaround in order to ensure proper functionality of the test. Additionally, this change cleans up an even earlier workaround for validating the cluster version for predecessor versions less than v21.1.2. Fixes #72971. Release note: None Co-authored-by: Lindsey Jin <lindsey.jin@cockroachlabs.com> Co-authored-by: Alex Sarkesian <sarkesian@cockroachlabs.com>

tbg requested a review from jaylim-crl October 15, 2021 09:11

jaylim-crl approved these changes Oct 15, 2021

View reviewed changes

tbg added the backport-21.2.x label Oct 19, 2021

craig bot merged commit 319256c into cockroachdb:master Oct 19, 2021

blathers-crl bot mentioned this pull request Oct 19, 2021

release-21.2: roachtest: fix multitenant-upgrade #71692

Merged

tbg deleted the fix-multiten-upgrade branch October 19, 2021 10:31

tbg mentioned this pull request Nov 16, 2021

multitenant: unable to start secondary tenant pod with single node cluster #72518

Closed

AlexTalks mentioned this pull request Nov 24, 2021

roachtest: fix flags used in multitenant-upgrade for versions >= v21.2 #73145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: fix multitenant-upgrade#71604

roachtest: fix multitenant-upgrade#71604
craig[bot] merged 1 commit intocockroachdb:masterfrom
tbg:fix-multiten-upgrade

tbg commented Oct 15, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 15, 2021

Uh oh!

jaylim-crl left a comment

Uh oh!

tbg commented Oct 19, 2021

Uh oh!

craig bot commented Oct 19, 2021

Uh oh!

tbg commented Oct 19, 2021

Uh oh!

blathers-crl bot commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	recordPath := filepath.Join(spec.Path, server.TempDirsRecordFilename)
	if err := storage.CleanupTempDirs(recordPath); err != nil {
	return base.TempStorageConfig{}, errors.Wrap(err,
	"could not cleanup temporary directories from record file")
	}

Conversation

tbg commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 15, 2021

Uh oh!

jaylim-crl left a comment

Choose a reason for hiding this comment

Uh oh!

tbg commented Oct 19, 2021

Uh oh!

craig bot commented Oct 19, 2021

Uh oh!

tbg commented Oct 19, 2021

Uh oh!

blathers-crl bot commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tbg commented Oct 15, 2021 •

edited

Loading