roachtest: fix multitenant-upgrade stable to dev opt-in

[dt]

Fixes #87689.

Release note: none.

[cockroach-teamcity]

This change is 

[ajstorm]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @dt)

---

pkg/cmd/roachtest/tests/multitenant_upgrade.go line 75 at r1 (raw file):

	settings := install.MakeClusterSettings(install.BinaryOption(predecessorBinary), install.SecureOption(true))
	settings.Env = append(settings.Env, "COCKROACH_UPGRADE_TO_DEV_VERSION=true")

Nit: Should this be conditional based on the binary type? Once we drop the dev designation in a release, wouldn't we not need this?

[dt]

wouldn't we not need this?

We will always need this on master; it is always dev. We don't need it on a release branch, but it also doesn't hurt anything to be there -- the test is just saying "look, I know that I might be doing a one-way upgrade here (but I might not) and I'm okay with that; this isn't critical production cluster that shouldn't be tainted". Doesn't seem worth adding more complexity to duplicate the upgrade checks just to decide when we're in the case where we'll hit them or not does it?

[ajstorm]

I'll let you make the call. I was worried about the case where we break the release checking logic and now on the release branch, we require setting the cluster setting when it's not strictly necessary. We wouldn't catch that case with the way the test is written now. Clearly a simple workaround, but would be poor optics. That being said, I'd _hope_ we'd catch this elsewhere in testing before we shipped.

On Mon, Sep 12, 2022 at 11:22 AM David Taylor ***@***.***> wrote: wouldn't we not need this? We will always need this on master; it is always dev. We don't need it on a release branch, but it also doesn't hurt anything to be there -- the test is just saying "look, I know that I might be doing a one-way upgrade here (but I might not) and I'm okay with that; this isn't critical production cluster that shouldn't be tainted". Doesn't seem worth adding more complexity to duplicate the upgrade checks just to decide when we're in the case where we'll hit them or not does it? — Reply to this email directly, view it on GitHub <#87826 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMXVORCUGHECOU3BMA7WVTV55DCXANCNFSM6AAAAAAQKQZX6Q> . You are receiving this because your review was requested.Message ID: ***@***.***>

-- Thanks, Adam

[dt]

My feeling is that we should make the test stop failing so we get the signal we want from it -- that when you know you're doing release to dev upgrades, it works. We don't need to backport to the any current release branch since the current release branches don't think they are a dev versions anyway. That means we have several months to decide if we want to runbook this so that the PR that removes the "is dev" bit on the release branch next time (23.1) also removes the env var.

[ajstorm]

That all SGTM. Do we want a follow-up issue for this? Or were you thinking that we'd rely on the test failure on 23.1 branch cut to "remind" us?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @dt)

[dt]

Closing in favor of @srosenberg's change in #88005