roachtest: use default env vars in Run()
#98055
Conversation
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
renatolabs
requested review from
srosenberg and
smg260
and removed request for
a team
|
This change is |
|
@renatolabs does this PR fix #98026, #98017, and #98018 as well? |
While I also initially suspected that (as the timelines for the failures are suggestive), I don't think that's the case. The error message for those decommission failures are different (there's nothing about store version incompatibility). I also ran one of those decommission tests (randomized) off master with #97595 reverted and the failure persisted. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)
pkg/roachprod/install/cluster_synced.go line 721 at r1 (raw file):
// reveal that the sleep command is running on the cluster. envVars := append([]string{ fmt.Sprintf("ROACHPROD=%s", c.roachprodEnvValue(node)), "GOTRACEBACK=crash",
We should think about separating the API for crdb vs. other commands. It would reduce the divergence wrt env.; e.g., generateStartCmd also sets COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING=1 to effectively disable sentry uploads.
There was a problem hiding this comment.
Reviewable status:
pkg/roachprod/install/cluster_synced.go line 721 at r1 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
We should think about separating the API for crdb vs. other commands. It would reduce the divergence wrt env.; e.g.,
generateStartCmdalso setsCOCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING=1to effectively disable sentry uploads.
Good point, and I agree. Perhaps instead of splitting the API, isolating the logic currently in generateStartCmd and reusing it on Start and Run might be good enough.
I'll defer that to a future PR to unblock the loq and inconsistency tests, but I think that change would make sense.
|
bors r=srosenberg |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
In #97595, we added the
COCKROACH_TESTING_FORCE_RELEASE_BRANCHenvironment variable to the list of roachprod's "default" environment variables. These variables are exported and accessible to the cockroach process whencockroachis started usingc.Start(). However, nothing stops tests from invoking cockroach usingc.Run("./cockroach ..."). In such cases, if a cluster had previously been created withc.Start(), the existing store would be deemed "too old" by the cockroach started withc.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 withc.Run.Fixes #98025.
Fixes #98027.
Fixes #98029.
Fixes #98030.
Fixes #98031.
Epic: none
Release note: None