clusterversion: allow tests to pretend to be a release branch #97595
Conversation
renatolabs
requested review from
herkolategan and
smg260
and removed request for
a team
|
This change is |
This commit adds a new environment variable that can be used by tests to force a development binary to treated as a release binary: `COCKROACH_TESTING_FORCE_RELEASE_BRANCH`. Previously, upgrade roachtests were setting the `COCKROACH_UPGRADE_TO_DEV_VERSION` variable to allow `master` to be deployed during tests. The main downside of this approach is that it is not the logic that real cluster would run when upgrading. In addition, it could lead to upgrades that seem to go "backwards", which can be quite confusing. With this change, upgrade roachtests bypass the version offsetting logic and are treated as the "next release" during tests. This commit also reverts cockroachdb#95904, as the workaround introduced there is no longer necessary. Resolves: cockroachdb#92608. Epic: CRDB-19321 Release note: None
renatolabs
force-pushed
the
rc/master-pretend-next-release-roachtest
branch
from
b47b06b to
a6c54b3
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @dt, @healthy-pod, @herkolategan, @renatolabs, and @smg260)
pkg/cmd/roachtest/tests/gossip.go line 434 at r1 (raw file):
defaultEnv := strings.Join(install.MakeClusterSettings().Env, " ") err := c.RunE(ctx, c.Node(1), defaultEnv+` ./cockroach start --insecure --background --store={store-dir} `+
Not entirely sure why that's necessary since this test doesn't participate in upgrade testing, right?
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/gossip.go line 434 at r1 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Not entirely sure why that's necessary since this test doesn't participate in upgrade testing, right?
It doesn't, but we're starting cockroach here with c.Run instead of c.Start (which is definitely not advisable), meaning that without defaultEnv, version offsetting will take place and cockroach will refuse to start because the existing store is deemed "too old".
|
TFTR! bors r=srosenberg |
|
Build succeeded: |
In cockroachdb#97595, we added the `COCKROACH_TESTING_FORCE_RELEASE_BRANCH` environment variable to the list of roachprod's "default" environment variables. These variables are exported and accessible to the cockroach process when `cockroach` is started using `c.Start()`. However, nothing stops tests from invoking cockroach using `c.Run("./cockroach ...")`. In such cases, if a cluster had previously been created with `c.Start()`, the existing store would be deemed "too old" by the cockroach started with `c.Run()`, since that process (without the environment variable) would perform version offsetting. To avoid these problems, we export roachprod's default environment variables to all commands run with `c.Run()`. The env vars available to cockroach are now consistent regardless of how the process is started, and the extra env vars should not affect unrelated commands run with `c.Run`. Fixes cockroachdb#98025. Fixes cockroachdb#98027. Fixes cockroachdb#98029. Fixes cockroachdb#98030. Fixes cockroachdb#98031. Epic: none Release note: None
98042: opt: fix incorrect results returned by non-recursive WITH RECURSIVE CTE r=rytaft a=rytaft Prior to this commit, queries that used a CTE marked as `WITH RECURSIVE` which did not reference itself (i.e., it was not actually recursive) could return incorrect results. This was because the `UNION ALL` in the CTE was incorrectly converted to a `UNION` by the optimizer. This commit fixes the problem by ensuring that `UNION ALL` is used if the original CTE contained `UNION ALL`. Fixes #93370 Release note (bug fix): Fixed a bug in which CTEs marked as `WITH RECURSIVE` which were not actually recursive could return incorrect results. This could happen if the CTE used a `UNION ALL`, because the optimizer incorrectly converted the `UNION ALL` to a `UNION`. This bug has existed since suppport for recursive CTEs was first added in v20.1, and has now been fixed. 98055: roachtest: use default env vars in `Run()` r=srosenberg a=renatolabs In #97595, we added the `COCKROACH_TESTING_FORCE_RELEASE_BRANCH` environment variable to the list of roachprod's "default" environment variables. These variables are exported and accessible to the cockroach process when `cockroach` is started using `c.Start()`. However, nothing stops tests from invoking cockroach using `c.Run("./cockroach ...")`. In such cases, if a cluster had previously been created with `c.Start()`, the existing store would be deemed "too old" by the cockroach started with `c.Run()`, since that process (without the environment variable) would perform version offsetting. To avoid these problems, we export roachprod's default environment variables to all commands run with `c.Run()`. The env vars available to cockroach are now consistent regardless of how the process is started, and the extra env vars should not affect unrelated commands run with `c.Run`. Fixes #98025. Fixes #98027. Fixes #98029. Fixes #98030. Fixes #98031. Epic: none Release note: None Co-authored-by: Rebecca Taft <[email protected]> Co-authored-by: Renato Costa <[email protected]>
|
The PR description here makes it sound like this was intended only to apply to upgrade roachtests, but it seems like this is indiscriminately being applied to all roachprod clusters, including ones started manually, outside roachtest? Was that intentional? |
|
I think this may have been intentional since roachprod is essentially for internal testing, and it helps prevent the confounding factor [1]. Has this change broken anything? |
|
I think if we added |
I don't see how setting That said, maybe the better non-roachtest roachprod default should be to set neither of these variables. I'll consider that change in an upcoming PR. |
This commit adds a new environment variable that can be used by tests to force a development binary to treated as a release binary:
COCKROACH_TESTING_FORCE_RELEASE_BRANCH.Previously, upgrade roachtests were setting the
COCKROACH_UPGRADE_TO_DEV_VERSIONvariable to allowmasterto be deployed during tests. The main downside of this approach is that it is not the logic that real cluster would run when upgrading. In addition, it could lead to upgrades that seem to go "backwards", which can be quite confusing.With this change, upgrade roachtests bypass the version offsetting logic and are treated as the "next release" during tests.
This commit also reverts #95904, as the workaround introduced there is no longer necessary.
Resolves: #92608.
Epic: CRDB-19321
Release note: None