roachprod: add COCKROACH_UPGRADE_TO_DEV_VERSION to DefaultEnvVars

[srosenberg]

Recent changes to cockroach_versions (logic) now require setting COCKROACH_UPGRADE_TO_DEV_VERSION environment variable in order to allow upgrading a stable release data-dir to a dev. version. The PR [1] which introduced this env. var. did not correctly backport the change to all the roachtests which perform this type of upgrade.

Since all roachtests which exercise upgrade paths are intended to test this upgrade scenario, we set COCKROACH_UPGRADE_TO_DEV_VERSION by default in roachprod instead of polluting the roachtests with more config. settings.

Consequently, MakeClusterSettings now returns the default ClusterSettings which includes COCKROACH_UPGRADE_TO_DEV_VERSION; generateStartCmd ensures it's passed into cockroach env. via start.sh.

Release note: None

Release justification: bug fix in roachtests.

Resolves:
#87675
#87687

[1] #87468

[cockroach-teamcity]

This change is 

[dt]

in #87826, @ajstorm has been arguing the opposite direction: we should be aiming to be more narrow in where we pass the env var, to more closely mirror how we expect a user to use it (i.e. only when you know you're doing a permanent stable->dev upgrade).

I'm mostly ambivalent; roachprod should only be used in dev and test cases anyway so I don't feel like it is particularly risky to default it as we're doing here, but it does mean we spend most of the time interacting with it in a way our users won't. 🤷

[srosenberg]

I'm mostly ambivalent; roachprod should only be used in dev and test cases anyway so I don't feel like it is particularly risky to default it as we're doing here, but it does mean we spend most of the time interacting with it in a way our users won't. 🤷

This new (upgrade safety valve) feature is rather controversial, imo. E.g., what are concrete test scenarios, other than upgrade, where you would explicitly need to unset COCKROACH_UPGRADE_TO_DEV_VERSION? If you just want to disable auto upgrade unconditionally, cluster.preserve_downgrade_option does that. The only roachtests which use the latter are those testing the upgrade paths.

As to Adam's comment, the intended user in this case is almost always an engineer; plus, more often than not, they would be using a dev. build. Roachprod defaults can be overridden if need be; it is very much an internal tool on which roachtests depend.

[ajstorm]

As to Adam's comment, the intended user in this case is almost always an engineer; plus, more often than not, they would be using a dev. build. Roachprod defaults can be overridden if need be; it is very much an internal tool on which roachtests depend.

That all makes sense to me. What I wonder though, is how many tests we'll have where the cluster setting is not set? I'd imagine these tests could only run on the release branch, but that we'd want to have some that are automated. Do those tests exist today?

[dt]

With this change, it'll just be set by default in all roachprod clusters, including those used by roachtest, all the time and individual tests won't need to set it or unset it at all.

[srosenberg]

What I wonder though, is how many tests we'll have where the cluster setting is not set? I'd imagine these tests could only run on the release branch, but that we'd want to have some that are automated. Do those tests exist today?

"cluster setting" is ambiguous in the context of this discussion. If you mean the new environment variable, then there aren't any to my knowledge. If you mean cluster.preserve_downgrade_option, then the roachtests,

pkg/cmd/roachtest/tests/autoupgrade.go
pkg/cmd/roachtest/tests/multitenant_upgrade.go
pkg/cmd/roachtest/tests/version.go
pkg/cmd/roachtest/tests/versionupgrade.go

and the unit tests in pkg/server/version_cluster_test.go (re)set it.

With this change, it'll just be set by default in all roachprod clusters, including those used by roachtest, all the time and individual tests won't need to set it or unset it at all.

That's correct. I prefer to set this as a global default in roachprod as opposed to having to sprinkle it across different roachtests. Each new configuration setting multiples the already exponential state space by 2. It's also quite easy to forget it (in a new upgrade test) and waste time debugging why the upgrade failed.

[renatolabs]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan)

[srosenberg]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[ajstorm]

If you mean the new environment variable, then there aren't any to my knowledge.

This is exactly my concern. Can we have at least one test that runs on the release branches which validates that the environment variable doesn't need to be set for the migration to complete successfully. It would suck if we didn't have this test and we accidentally broke the behaviour, causing all customers to have to set the env var (at least, until we fixed the bug).

[renatolabs]

Alternatively, it would also be nice if there was an easy way to build master as a "non-dev build" in the tests (currently I believe a code change is necessary). In the context of these tests, we want to pretend that master is the new release.

[srosenberg]

This is exactly my concern. Can we have at least one test that runs on the release branches which validates that the environment variable doesn't need to be set for the migration to complete successfully. It would suck if we didn't have this test and we accidentally broke the behaviour, causing all customers to have to set the env var (at least, until we fixed the bug).

This change is guarded by developmentBranch [1] which presumably would be automatically set to false on every subsequent release branch. (@celiala or @dt please confirm) In light of the guard, this env. var affects only the case of automated upgrades from a release to dev.

[1] https://github.com/cockroachdb/cockroach/blob/master/pkg/clusterversion/cockroach_versions.go#L481