roachtest: add acceptance/version-upgrade

[petermattis]

Move the version-upgrade acceptance test to a new
acceptance/version-upgrade roachtest.

See #29151

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

The first 6 commits are #29465.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/upgrade.go, line 469 at r7 (raw file):

		binaryVersionUpgrade("HEAD", nodes, false /* wipe */),
		clusterVersionUpgrade(clusterversion.BinaryServerVersion.String(), false /* manual */),

TODO(peter): this test is currently failing on this last cluster version upgrade which doesn't seem to be working. The old TestVersionUpgrade has the exact same logic (automatic upgrade for the last cluster version upgrade) and it seems to work fine. Anyone have any idea what the difference might be? I haven't had a chance to investigate yet so it might be something stupid in my translation of the test.

[benesch]

Reviewed 3 of 4 files at r3, 1 of 1 files at r5, 1 of 1 files at r6, 3 of 3 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/upgrade.go, line 451 at r7 (raw file):
FYI, from our style guide:

Note: For bool constants, like for all literals, the comment should indicate
the name of the parameter and does not depend on the argument value.
Do not put a bang in the comment when commenting the false constant. Also,
do not adapt the comment to negate the name of the parameter.

---

pkg/cmd/roachtest/upgrade.go, line 469 at r7 (raw file):

Previously, petermattis (Peter Mattis) wrote…

TODO(peter): this test is currently failing on this last cluster version upgrade which doesn't seem to be working. The old TestVersionUpgrade has the exact same logic (automatic upgrade for the last cluster version upgrade) and it seems to work fine. Anyone have any idea what the difference might be? I haven't had a chance to investigate yet so it might be something stupid in my translation of the test.

Not seeing anything obvious. @nvanbenschoten might know; I think he was the last person to seriously touch this test.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cmd/roachtest/upgrade.go, line 451 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

FYI, from our style guide:

Note: For bool constants, like for all literals, the comment should indicate
the name of the parameter and does not depend on the argument value.
Do not put a bang in the comment when commenting the false constant. Also,
do not adapt the comment to negate the name of the parameter.

Not sure what this comment was recommending I do, but I thought I was following the style guide. , I've removed the wipe parameter, so the comment is probably not relevant anymore.

---

pkg/cmd/roachtest/upgrade.go, line 469 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Not seeing anything obvious. @nvanbenschoten might know; I think he was the last person to seriously touch this test.

Well, the problem stems from wiping node 3 a few lines higher. Comment out that line and the automatic upgrade succeeds. Leave it in and the automatic upgrade cannot pass. But this series of upgrade steps is taken directly from TestVersionUpgrade. Perhaps the wiping was broken in that acceptance test. I'm tracking down what is going on. From what I can tell, TestVersionUpgrade only ever sees 3 nodes in the cluster (n1, n2 and n3), while the new acceptance/version-upgrade sees 4 nodes with the 4th appearing after n3 is wiped.

Question for @nvanbenschoten (or perhaps @tschottdorf) is it more useful for this test to checking wiping a node, or to exercise the automatic upgrade path?

Update: Ok, found the bug:

// RemoveNodeData removes the given node's data directory.
func (c *Cluster) RemoveNodeData(nodeIdx int) error {
	dir := c.Cfg.PerNodeCfg[nodeIdx].DataDir
	return os.RemoveAll(dir)
}

In TestVersionUpgradeTest, c.Cfg.PerNodeCfg.DataDir is empty so we don't end up removing anything. I've removed the support for wiping nodes since I think we're testing that automatic upgrades are disabled in the "upgrade/oldVersion=..." roachtest.

PTAL

[nvanbenschoten]

Reviewed 2 of 3 files at r7, 1 of 2 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cmd/roachtest/upgrade.go, line 462 at r8 (raw file):

		binaryVersionUpgrade("HEAD", nodes),
		clusterVersionUpgrade(clusterversion.BinaryServerVersion.String(), false /* manual */),

How are you expecting this test to interact with release-2.0 roachtests?

---

pkg/cmd/roachtest/upgrade.go, line 530 at r8 (raw file):

	// of cockroach.
	c.Run(ctx, c.Node(1), "mkdir -p {store-dir} && touch {store-dir}/settings-initialized")
	c.Start(ctx, nodes, args, startArgs("--sequential"))

Is --sequential necessary or does it just assist debugging?

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/cmd/roachtest/upgrade.go, line 462 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How are you expecting this test to interact with release-2.0 roachtests?

Hmm, good point. I've fixed this by removing this versionStep and replaced it with custom code at the end of the test which retrieves crdb_internal.node_executable_version() and compares it against the most recent cluster upgrade value. PTAL.

---

pkg/cmd/roachtest/upgrade.go, line 530 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is --sequential necessary or does it just assist debugging?

Unnecessary. Left over from some debugging. Removed.

[petermattis]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)