Skip to content
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

sql: update upgrade unit tests to bootstrap from 23.1 (or 22.2) #103956

Closed
renatolabs opened this issue May 26, 2023 · 7 comments
Closed

sql: update upgrade unit tests to bootstrap from 23.1 (or 22.2) #103956

renatolabs opened this issue May 26, 2023 · 7 comments
Assignees
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@renatolabs
Copy link
Contributor

renatolabs commented May 26, 2023

Currently, all of our upgrade unit tests (mostly logic tests outside of the cockroach-go config) bootstrap the cluster at V22_2. Since the release-23.1 branch has been cut, master should now be considered 23.2 work (#103551). As such, we should be writing our upgrade tests to bootstrap the cluster from V23_1.

This is made more complex in this release, as we intend to keep BinaryMinSupportedVersionKey at V22_2 in release-23.2 [1]. This means that, ideally, we should start testing our upgrades both from V22_2 and from V23_1.

This work will involve creating initial_values for the 23.1 release: see [2].

Assigning @cockroachdb/sql-foundations, as a lot of our upgrade tests are logic tests. Migration and tenant upgrade tests are also among tests that should be updated. Corrresponding teams are free to create separate issues to handle those changes.

[1] https://cockroachlabs.atlassian.net/wiki/spaces/RE/pages/2762309867/Version+Skipping+to+Enable+Long-Term+Support+LTS
[2]

var initialValuesFnByKey = map[Tenant]map[clusterversion.Key]initialValuesFn{
SystemTenant: {
clusterversion.V22_2: initialValuesForSystemV222,
},
SecondaryTenant: {
clusterversion.V22_2: initialValuesForTenantV222,
},
}

Jira issue: CRDB-28269

@renatolabs renatolabs added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels May 26, 2023
@rafiss
Copy link
Collaborator

rafiss commented May 30, 2023

cc @postamar could you leave some instructions here with what we need to modify in the initial_values file? After that, we can figure out who else to assign this to, if needed.

@postamar
Copy link

postamar commented Jun 5, 2023

Ideally this should be TDD so that someone who isn't myself can update this straightforwardly: update all the config definitions for mixed-version tests and see what breaks and fix it.

In practice, some things are missing in the bootstrap package tests for that to be possible. I should add a test that checks that each supported major version has hardcoded initial values, instead of only the latest version. I'll file a PR which should, in turn, make it possible for anyone to address this present issue.

postamar pushed a commit to postamar/cockroach that referenced this issue Jun 5, 2023
This commit adds a test in the bootstrap package which checks that
initial values can be generated for each and every release which is
supported by the current version.

We expect this test, TestSupportedReleases, to fail loudly the next time
a release engineer bumps the major or minor version counter. While
getting the test to pass does increase the burden of the release
engineer, doing so should be a very mechanical process. It involves
copy-pasting the contents of the test data file for the data-driven
TestInitialValuesToString test in the new release branch.

This commit therefore also adds the hard-coded values for the 23.1
release which was until now not supported by the bootstrap package.

Informs cockroachdb#103956.

Release note: None.
craig bot pushed a commit that referenced this issue Jun 6, 2023
104336: boostrap: improve testing and support v23.1 r=postamar a=postamar

This commit adds a test in the bootstrap package which checks that initial values can be generated for each and every release which is supported by the current version.

We expect this test, TestSupportedReleases, to fail loudly the next time a release engineer bumps the major or minor version counter. While getting the test to pass does increase the burden of the release engineer, doing so should be a very mechanical process. It involves copy-pasting the contents of the test data file for the data-driven TestInitialValuesToString test in the new release branch.

This commit therefore also adds the hard-coded values for the 23.1 release which was until now not supported by the bootstrap package.

Informs #103956.

Release note: None.

Co-authored-by: Marius Posta <[email protected]>
@postamar
Copy link

postamar commented Jun 6, 2023

I'll unassign myself at this point, now that the initial values for 23.1 have been added. The remaining work in the scope of this issue and this team is adding and/or updating the following to involve the 23.1 release somehow:

  • mixed-version logic test configs
  • tenant creation/upgrade tests
  • roachtests involving mixed-version clusters that the team owns
  • declarative schema changer corpus tests

Feel free to edit this list accordingly

@postamar postamar removed their assignment Jun 6, 2023
@rafiss rafiss self-assigned this Jun 13, 2023
@rafiss
Copy link
Collaborator

rafiss commented Jun 13, 2023

@fqazi can you work on the corpus tests and mixed version schema change test? i will work on the mixed version logic tests.

@rafiss
Copy link
Collaborator

rafiss commented Jun 17, 2023

@postamar I still find myself a little bit lost. Do you know what it would take to unskip the mixed_version_can_login test?

When I try, it fails on this query due to getting the wrong results.

SELECT
          k,
          is_present_in,
          CASE
            WHEN k = '/Table/6/1/"version"/0'
            THEN '<volatile>' -- this value is volatile due to the "lastUpdated" column
            ELSE v
          END AS v
        FROM bootstrapped_tenant_data
        ORDER BY 1, 2

Do the expected results just need to be rewritten? (Aside: that test probably should be in a different file, since it's testing something much lower level than being able to login.)

@postamar
Copy link

Yes indeed a rewrite is all it takes, and you're right this doesn't really belong here. I squished it in there originally because I was being a bit lazy. I can't remember why, in any case I'm sorry about that.

@rafiss
Copy link
Collaborator

rafiss commented Oct 12, 2023

#112122 and #112149 address the remaining issues.

craig bot pushed a commit that referenced this issue Oct 13, 2023
112149: logictest: unskip some mixed version tests r=rafiss a=rafiss

This also removes old tests that are only relevant for testing clusters that bootstrap on 22.2.

informs #103956 - will be resolved by backport
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
@rafiss rafiss closed this as completed Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Archived in project
Development

No branches or pull requests

3 participants