roachtest: fix multitenant-upgrade stable to dev opt-in#87826
roachtest: fix multitenant-upgrade stable to dev opt-in#87826dt wants to merge 1 commit intocockroachdb:masterfrom
Conversation
Fixes cockroachdb#87689. Release note: none.
|
This change is |
ajstorm
left a comment
There was a problem hiding this comment.
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?
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? |
|
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
|
|
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
left a comment
There was a problem hiding this comment.
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:
|
Closing in favor of @srosenberg's change in #88005 |
Fixes #87689.
Release note: none.