roachtest: more cleanup in version-upgrade #47065
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 2 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @spaskob and @tbg)
pkg/cmd/roachtest/autoupgrade.go, line 1 at r2 (raw file):
// Copyright 2018 The Cockroach Authors.
thanks for splitting this out in a separate file - from a reader's point of view now it's easier to understand now.
Most of my comments are to aid readability for somebody vaguely familiar with how roachtests work.
pkg/cmd/roachtest/autoupgrade.go, line 47 at r2 (raw file):
// NB: remove startArgsDontEncrypt across this file once we're not running // roachtest against v2.1 any more (which would start a v2.0 cluster here).
For my understanding why do we test v2.1 - do we still have clients that we support running this version?
pkg/cmd/roachtest/autoupgrade.go, line 70 at r2 (raw file):
// in the pool and throws an error at the next client using it (instead of // transparently reconnecting). db.SetMaxIdleConns(0)
Do you mind running locally to check if this is still true.
pkg/cmd/roachtest/autoupgrade.go, line 104 at r2 (raw file):
} oldVersion, err = clusterVersion()
nit: Please add a comment on why we over-write the function arg here.
pkg/cmd/roachtest/autoupgrade.go, line 134 at r2 (raw file):
t.Fatal(err) } c.Put(ctx, cockroach, "./cockroach", c.Node(i))
what is the version of the new binary?
pkg/cmd/roachtest/autoupgrade.go, line 169 at r2 (raw file):
// Now decommission and stop n3. // The decommissioned nodes should not prevent auto upgrade.
nit: name the node since nodes-2 is used quite a bit further in the code and it relies on a comment, ie
decommissionedNode := nodes - 2
pkg/cmd/roachtest/autoupgrade.go, line 170 at r2 (raw file):
// Now decommission and stop n3. // The decommissioned nodes should not prevent auto upgrade. if err := decommissionAndStop(nodes - 2); err != nil {
nit: add a comment explaining what do we test by decomissioning this node.
pkg/cmd/roachtest/versionupgrade.go, line 218 at r2 (raw file):
} func (u *versionUpgradeTest) uploadVersion(ctx context.Context, t *test, newVersion string) option {
Do you think these may be re-used by other mixed-version roachtests?
Release note: None
The struct wrapping a fn was no longer needed. This was left over from supporting old versions, where the version could not be auto-detected. Release note: None
Avoid redundant code, improve logging Release note: None
The "upgrade" roachtest is renamed to "autoupgrade" and moved into a new file. The reason is that this test isn't particularly well-maintained or extensible, and so we want to "hide" it and favor the mixed-version test harness left in upgrade.go more prominently. Release note: None
Let the file name match the test name. Release note: None
Tickled my OCD bone. Release note: None
Add steps to abort each upgrade and finalize it only later. This reliably hit the error > pq: schema change cannot be initiated in this version until the > version upgrade is finalized from cockroachdb#44732 (comment). I fixed this by marking all DDL feature tests as 20.1+ only. Release note: None
This test was creating and closing SQL connections all the time, which took around a second each time. This really added up. Cache these conns, which speeds up this test quite a bit, requiring waits only for binary upload and restarts. Release note: None
2.1 is old and by skipping it we get to run this test with encryption at rest. Release note: None
Release note: None
It's in more dire need of reworking, but also it's unlikely that anyone will depend on it, so explaining it a little better give the best ROI for now. Release note: None
It's supposed to be called `autoupgrade` now. Release note: None
There was a problem hiding this comment.
TFTR @spaskob! Lmk if you have more comments and I'll send a follow-up.
Dismissed @spaskob from a discussion.
Reviewable status:
pkg/cmd/roachtest/autoupgrade.go, line 47 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
For my understanding why do we test v2.1 - do we still have clients that we support running this version?
Generally, as long as a release branch might see a new release cut from it, we will try to keep the roachtests runnable on them. Often, this is so tedious that we selectively skip some roachtests on old versions even though they haven't reached EOL yet. In this particular case, I think we do still have some kind of exception for v2.1, but the real reason you're still seeing this code is because this test hasn't been touched in a long time (I don't know if you realized, but the autoupgrade.go contents are pure movement - I haven't touched this test at all, though I will now in reaction to your comments).
pkg/cmd/roachtest/autoupgrade.go, line 70 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
Do you mind running locally to check if this is still true.
I don't think it is, or I would've needed it while reworking version-upgrade. Removed. (And tested)
pkg/cmd/roachtest/autoupgrade.go, line 104 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
nit: Please add a comment on why we over-write the function arg here.
I did add a comment to the best of my abilities (I didn't write or touch this test, or if I did then I forgot and rather not remember :-)) ).
pkg/cmd/roachtest/autoupgrade.go, line 134 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
what is the version of the new binary?
it's the version we're testing, so if you're running on master it will be 20.2, if you're on release-19.1 is 19.1.x. Added a comment.
pkg/cmd/roachtest/autoupgrade.go, line 170 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
nit: add a comment explaining what do we test by decomissioning this node.
Adjusted the comment to make it clearer.
pkg/cmd/roachtest/versionupgrade.go, line 218 at r2 (raw file):
Previously, spaskob (Spas Bojanov) wrote…
Do you think these may be re-used by other mixed-version roachtests?
Yes, the reason I am touching this code quite a bit is because I want to enable that, prompted by your use case.
|
bors r=spaskob |
Build succeeded |
The struct wrapping a fn was no longer needed. This was left over from
supporting old versions, where the version could not be auto-detected.
Avoid redundant code, improve logging.
Add steps to abort each upgrade and finalize it only later.
This reliably hit the error
from #44732 (comment).
I fixed this by marking all DDL feature tests as 20.1+ only.
Fixes #44732
Release note: None