Skip to content

cli: conditionally allow version skipping#121011

Closed
celiala wants to merge 1 commit intocockroachdb:masterfrom
celiala:version-skipping
Closed

cli: conditionally allow version skipping#121011
celiala wants to merge 1 commit intocockroachdb:masterfrom
celiala:version-skipping

Conversation

@celiala
Copy link
Copy Markdown
Collaborator

@celiala celiala commented Mar 25, 2024

This PR revives #113695, where we want to conditionally allow version skipping:

Currently the code allows upgrading from TWO previous releases (23.1 and 23.2); however, for now we only want to allow skipping a version experimentally.

This commit changes the cli code to set the minimum supported version to PreviousRelease by default, or to MinSupported when a special env var is used.

Epic: none
Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Mar 26, 2024

These tests are failing:

The error message for all of these is something like panic: empty connection string

  • (1 test) pkg/ccl/logictestccl/tests/cockroach-go-testserver-23.1/cockroach-go-testserver-23_1_test:
    • TestCCLLogic_mixed_version_plpgsql
  • (19 tests) pkg/sql/logictest/tests/cockroach-go-testserver-23.1/cockroach-go-testserver-23_1_test:
    • TestLogic_mixed_version_insights_queries
    • TestLogic_mixed_version_partially_visible_index
    • TestLogic_mixed_version_procedure
    • TestLogic_mixed_version_refcursor
    • TestLogic_mixed_version_refcursor/all_old_cast
    • ...

The error message for the below tests is more like

store last used with cockroach version v1000023.1 is too old for running version v1000023.2-upgrading-to-1000024.1-step-020 (which requires data from v1000023.2 or later)
  • (4 tests) github.com/cockroachdb/cockroach/pkg/cli:
    • TestDebugKeysHex
    • TestOpenReadOnlyStore
    • TestOpenReadOnlyStore/readOnly=false
    • TestOpenReadOnlyStore/readOnly=true

@celiala
Copy link
Copy Markdown
Collaborator Author

celiala commented Mar 26, 2024

@RaduBerinde wondering if you might have any ideas on how to fix these failing tests?

@RaduBerinde
Copy link
Copy Markdown
Member

In all these cases, we are probably executing a binary and we need to pass the new env var.

The diff below fixes the logic tests. The other tests should be similar - let me know if you get stuck.

--- a/pkg/sql/logictest/logic.go
+++ b/pkg/sql/logictest/logic.go
@@ -1317,8 +1317,19 @@ func (t *logicTest) newTestServerCluster(bootstrapBinaryPath, upgradeBinaryPath
                }
        }
 
-       // During config initialization, NumNodes is required to be 3.
+       var envVars []string
+       if strings.Contains(upgradeBinaryPath, "cockroach-short") {
+               // If we're using a cockroach-short binary, that means it was
+               // locally built, so we need to opt-out of version offsetting to
+               // better simulate a real upgrade path.
+               envVars = append(envVars, "COCKROACH_TESTING_FORCE_RELEASE_BRANCH=true")
+               // The build is made during testing, so it has metamorphic constants.
+               // We disable them here so that the test is more stable.
+               envVars = append(envVars, "COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true")
+       }
+       envVars = append(envVars, "COCKROACH_ALLOW_VERSION_SKIPPING=true")
        opts := []testserver.TestServerOpt{
+               // During config initialization, NumNodes is required to be 3.
                testserver.ThreeNodeOpt(),
                testserver.StoreOnDiskOpt(),
                testserver.CacheSizeOpt(0.1),
@@ -1326,17 +1337,7 @@ func (t *logicTest) newTestServerCluster(bootstrapBinaryPath, upgradeBinaryPath
                testserver.UpgradeCockroachBinaryPathOpt(upgradeBinaryPath),
                testserver.PollListenURLTimeoutOpt(120),
                testserver.CockroachLogsDirOpt(logsDir),
-       }
-       if strings.Contains(upgradeBinaryPath, "cockroach-short") {
-               opts = append(opts, testserver.EnvVarOpt([]string{
-                       // If we're using a cockroach-short binary, that means it was
-                       // locally built, so we need to opt-out of version offsetting to
-                       // better simulate a real upgrade path.
-                       "COCKROACH_TESTING_FORCE_RELEASE_BRANCH=true",
-                       // The build is made during testing, so it has metamorphic constants.
-                       // We disable them here so that the test is more stable.
-                       "COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true",
-               }))
+               testserver.EnvVarOpt(envVars),

Currently the code allows upgrading from TWO previous releases (23.1
and 23.2); however, for now we only want to allow skipping a version
experimentally.

This commit changes the cli code to set the minimum supported version
to `PreviousRelease` by default, or to `MinSupported` when a special
env var is used.

Epic: none
Release note: None
@RaduBerinde
Copy link
Copy Markdown
Member

Closing in favor of #121952.

@RaduBerinde RaduBerinde closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants