backupccl: allow backupRestoreTestSetup[Empty]WithParams to test within a tenant #88271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
from
855f385 to
efb6ee8
Compare
This was referenced Sep 21, 2022
Closed
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
from
efb6ee8 to
b069fe8
Compare
msbutler
requested review from
benbardin,
stevendanna and
a team
and removed request for
a team and
benbardin
msbutler
commented
Sep 22, 2022
| @@ -7203,7 +7216,9 @@ func TestBackupExportRequestTimeout(t *testing.T) { | |||
|
|
|||
| allowRequest := make(chan struct{}) | |||
| defer close(allowRequest) | |||
| params := base.TestClusterArgs{} | |||
|
|
|||
There was a problem hiding this comment.
nvm, I figured this flake out. Will amend the commit after this current CI run
adityamaru
suggested changes
Sep 22, 2022
pkg/ccl/backupccl/testdata/backup-restore/external-connections-nodelocal
Show resolved
Hide resolved
| @@ -180,31 +180,14 @@ query-sql | |||
| WITH descs AS ( | |||
| SHOW BACKUP LATEST IN 'nodelocal://0/test/' | |||
| ) | |||
| SELECT database_name, parent_schema_name, object_name, object_type, is_full_cluster FROM descs | |||
| SELECT | |||
There was a problem hiding this comment.
got rid of the tenant_settings system table. I also don't think the test needed to show all these other rows anyway.
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
from
b069fe8 to
4113542
Compare
msbutler
commented
Sep 22, 2022
pkg/ccl/backupccl/testdata/backup-restore/external-connections-nodelocal
Show resolved
Hide resolved
| @@ -180,31 +180,14 @@ query-sql | |||
| WITH descs AS ( | |||
| SHOW BACKUP LATEST IN 'nodelocal://0/test/' | |||
| ) | |||
| SELECT database_name, parent_schema_name, object_name, object_type, is_full_cluster FROM descs | |||
| SELECT | |||
There was a problem hiding this comment.
got rid of the tenant_settings system table. I also don't think the test needed to show all these other rows anyway.
stevendanna
reviewed
Sep 23, 2022
stevendanna
reviewed
Sep 23, 2022
|
#88557 might help for some of the tests that aren't working because of testing hooks. |
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
3 times, most recently
from
a047ef3 to
710d680
Compare
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
3 times, most recently
from
ecaf793 to
c321fef
Compare
|
RFAL! |
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
3 times, most recently
from
3117472 to
81fc39c
Compare
|
@stevendanna friendly ping on this! |
adityamaru
approved these changes
Oct 19, 2022
…in tenant Previously, all backup unit tests that called backupRestoreTestSetupWithParams and backupRestoreTestSetupEmptyWithParams were disabled to run within tenants, signficantly decreasing test coverage. This patch allows tests that call these helper methods to be probabilistically run within tenants (including most data driven tests). This patch also manually disables some of these tests to be run within tenants. Future work should continue to enable bulk unit tests to run within tenants by default. Fixes cockroachdb#88381, cockroachdb#88527, cockroachdb#88453, cockroachdb#88380 Release note: None
msbutler
force-pushed
the
butler-tenant-unit-tests
branch
from
81fc39c to
fd7f48d
Compare
|
TFTRs!!! bors r=adityamaru |
|
Build succeeded: |
|
@msbutler seeing TestDataDriven get pretty flaky and I suspect its this PR - https://teamcity.cockroachdb.com/viewLog.html?buildId=7032391&tab=buildResultsDiv&buildTypeId=Cockroach_Ci_TestsAwsLinuxArm64_UnitTests |
|
Flaky behaviour started around 1300 today https://teamcity.cockroachdb.com/project.html?projectId=Cockroach_Ci_TestsAwsLinuxArm64BigVm&testNameId=-4898685845226892981&tab=testDetails&branch_Cockroach_Ci_TestsAwsLinuxArm64BigVm=%3Cdefault%3E which roughly lines up with when this was merged. Looks like when run as a tenant the DD test is timing out the package. |
adityamaru
added a commit
to adityamaru/cockroach
that referenced
this pull request
Oct 20, 2022
In cockroachdb#88271 we enabled most DD tests to run as tenants but we saw an uptick in timeouts on CI. This change disables these tests from being run as tenants while we dig into the timeouts. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Oct 20, 2022
90302: backupccl: disable tenants in datadriven test r=stevendanna a=adityamaru In #88271 we enabled most DD tests to run as tenants but we saw an uptick in timeouts on CI. This change disables these tests from being run as tenants while we dig into the timeouts. Release note: None Epic: none Co-authored-by: adityamaru <[email protected]>
|
hmmm, i'll try stressing the DD test with MT on my gce worker to see if anything shakes out. |
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Oct 21, 2022
TestDataDriven unit tests were recently enabled to run within tenants(cockroachdb#88271). This was quickly disabled as it caused TestDataDriven/restore-grants to flake (tracked in cockroachdb#90444). This patch re-enables TestDataDriven unit tests to run within a tenant, except for TestDataDriven/restore-grants. Further investigation is needed to understand why restore-grants hangs when run within a tenant. Informs cockroachdb#90444 Release note: None
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Oct 24, 2022
TestDataDriven unit tests were recently enabled to run within tenants(cockroachdb#88271). This was quickly disabled as it caused TestDataDriven/restore-grants to flake (tracked in cockroachdb#90444). This patch re-enables TestDataDriven unit tests to run within a tenant, except for TestDataDriven/restore-grants. Further investigation is needed to understand why restore-grants hangs when run within a tenant. Informs cockroachdb#90444 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
90446: backupccl: re-enable data driven tests to run within a tenant r=adityamaru,stevendanna a=msbutler TestDataDriven unit tests were recently enabled to run within tenants(#88271). This was quickly disabled as it caused TestDataDriven/restore-grants to flake (tracked in #90444). This patch re-enables TestDataDriven unit tests to run within a tenant, except for TestDataDriven/restore-grants. Further investigation is needed to understand why restore-grants hangs when run within a tenant. Informs #90444 Release note: None Co-authored-by: Michael Butler <[email protected]>
THardy98
pushed a commit
to THardy98/cockroach
that referenced
this pull request
Nov 2, 2022
In cockroachdb#88271 we enabled most DD tests to run as tenants but we saw an uptick in timeouts on CI. This change disables these tests from being run as tenants while we dig into the timeouts. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, all backup unit tests that called backupRestoreTestSetupWithParams
and backupRestoreTestSetupEmptyWithParams were disabled to run within tenants,
signficantly decreasing test coverage.
This patch allows tests that call these helper methods to be probabilistically
run within tenants (including most data driven tests). This patch also manually
disables some of these tests to be run within tenants. Future work should
continue to enable bulk unit tests to run within tenants by default.
Fixes #88381, #88527, #88453, #88380
Release note: None