logictest: support mixed binary version tests with upgrades #91881

rafiss · 2022-11-14T23:12:44Z

The logictest framework can now use the cockroach-go testserver to test a multi-node cluster in which each node is running a different binary version.

Previously, only mixed logical versions could be tested.

Release note: None

cockroach-teamcity · 2022-11-14T23:12:51Z

This change is

ZhouXing19

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

pkg/sql/logictest/logic.go line 1236 at r1 (raw file):

}

func (t *logicTest) newTestServerCluster(bootstrapBinaryPath string, upgradeBinaryPath string) {

nit: maybe add a comment that "upgradeBinaryPath is set according to cfg.CockroachGoUpgradeVersion. If the latter is unset, upgradeBinaryPath is of the same value as bootstrapBinaryPath"

pkg/sql/logictest/logic.go line 1251 at r1 (raw file):

		testserver.StoreOnDiskOpt(),
		testserver.CockroachBinaryPathOpt(bootstrapBinaryPath),
		testserver.UpgradeCockroachBinaryPathOpt(upgradeBinaryPath),

This may be a bit random, but just curious if we would ever want a mix of 3 versions in a test?

pkg/sql/logictest/logic.go line 3018 at r1 (raw file):

				return errors.Errorf(`could not perform "upgrade", not a cockroach-go/testserver cluster`)
			}
			nodeNum, err := strconv.Atoi(fields[1])

nit: nodeIdx maybe better?

pkg/sql/logictest/logic.go line 3045 at r1 (raw file):

			}

			if err := t.testserverCluster.WaitForInitFinishForNode(nodeNum); err != nil {

Do we still need this WaitForInitFinishForNode? Look like it's doing the same thing as the for loop above.

pkg/sql/logictest/logic.go line 3871 at r1 (raw file):

	// Ensure that all of the created descriptors can round-trip through json.
	{
		// If `useCockroachGoTestserver` is true and we do an upgrade,

Not requesting for changes, but wanted to know whether this check for descriptors in necessary for all tests? When using cockroach-go test server, will we want to wait till migrations finish then run this check?

pkg/sql/logictest/testdata/logic_test/testserver_upgrade_node line 1 at r1 (raw file):

# LogicTest: cockroach-go-testserver-22.2-master

nit: Would this name be a bit confusing once the master moves forward to a later version? Would it be better if it's something like testserver-22.2-single-version?

rafiss

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

pkg/sql/logictest/logic.go line 1236 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: maybe add a comment that "upgradeBinaryPath is set according to cfg.CockroachGoUpgradeVersion. If the latter is unset, upgradeBinaryPath is of the same value as bootstrapBinaryPath"

done - with a small correction

pkg/sql/logictest/logic.go line 1251 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

This may be a bit random, but just curious if we would ever want a mix of 3 versions in a test?

no, we shouldn't need that, since CRDB only supports upgrading from one version to the next

pkg/sql/logictest/logic.go line 3018 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: nodeIdx maybe better?

done

pkg/sql/logictest/logic.go line 3045 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Do we still need this WaitForInitFinishForNode? Look like it's doing the same thing as the for loop above.

yeah, i'll remove this one, nice catch

pkg/sql/logictest/logic.go line 3871 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Not requesting for changes, but wanted to know whether this check for descriptors in necessary for all tests? When using cockroach-go test server, will we want to wait till migrations finish then run this check?

i don't think it's necessary for all tests - it's kind of like a defensive check, and doesn't seem worth it to make it work during version upgrades

pkg/sql/logictest/testdata/logic_test/testserver_upgrade_node line 1 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: Would this name be a bit confusing once the master moves forward to a later version? Would it be better if it's something like testserver-22.2-single-version?

when master moves forward to a new version, this test should start failing, so then the test configs will be update or removed as needed

ZhouXing19

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

pkg/sql/logictest/logic.go line 3045 at r5 (raw file):

				t.Fatal(err)
			}
			for i := 0; i < 3; i++ {

Hmm, so now with the ci green, seems that though we upgrade a single node (by killing the process and restart a new one), we still need to wait till all nodes in the cluster are ping-able. 🤔

rafiss

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

pkg/sql/logictest/logic.go line 3045 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Hmm, so now with the ci green, seems that though we upgrade a single node (by killing the process and restart a new one), we still need to wait till all nodes in the cluster are ping-able. 🤔

yeah, i wanted to experiment with this to see if that would fix it. i believe this is correct, since the testserver uses Kill when upgrading a node, which means that there could be temporary cluster unavailability (for example, if the leaseholder for the system database is killed)

rafiss

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

pkg/sql/logictest/logic.go line 3045 at r5 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yeah, i wanted to experiment with this to see if that would fix it. i believe this is correct, since the testserver uses Kill when upgrading a node, which means that there could be temporary cluster unavailability (for example, if the leaseholder for the system database is killed)

i've also made cockroachdb/cockroach-go#154 so that this logic is part of the testserver

ZhouXing19 · 2022-11-23T19:00:15Z

pkg/sql/logictest/logic.go line 1272 at r5 (raw file):

		t.Fatal(err)
	}
	testutils.SucceedsSoon(t.rootT, func() error {

Do we want to replace here with

for i := 0; i < 3; i ++ {
  if err := ts.WaitForInitFinishForNode(i); err != nil {
    return err
  }
}

as well?

This makes the next commit for mixed-version testing easier. Release note: None

The logictest framework can now use the cockroach-go testserver to test a multi-node cluster in which each node is running a different binary version. Previously, only mixed logical versions could be tested. Release note: None

This prevents flakiness that is caused by temporary cluster unavailability after a node is killed and restarted. Release note: None

rafiss

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

pkg/sql/logictest/logic.go line 1272 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Do we want to replace here with

for i := 0; i < 3; i ++ {
  if err := ts.WaitForInitFinishForNode(i); err != nil {
    return err
  }
}

as well?

i think this one is fine as-is, since during the initial startup there is no non-graceful killing

ZhouXing19

!! Thanks for answering my questions

Reviewed 5 of 15 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained

rafiss · 2022-11-24T04:58:40Z

tftr!

bors r=ZhouXing19

craig · 2022-11-24T06:34:57Z

Build failed:

Bazel Essential CI (Cockroach)

rafiss · 2022-11-24T16:36:20Z

bors r=ZhouXing19

craig · 2022-11-24T17:25:56Z

Build succeeded:

Bazel Essential CI (Cockroach)

rafiss requested a review from a team November 14, 2022 23:12

rafiss requested a review from a team as a code owner November 14, 2022 23:12

rafiss mentioned this pull request Nov 14, 2022

testlogic: support running logic tests with cockroach-go/testserver #83296

Closed

rafiss force-pushed the logic-test-mixed-version branch from ba973ea to 5886ffd Compare November 15, 2022 19:59

rafiss requested a review from ZhouXing19 November 15, 2022 19:59

rafiss force-pushed the logic-test-mixed-version branch from 5886ffd to 06fcb6c Compare November 15, 2022 21:36

ZhouXing19 requested changes Nov 17, 2022

View reviewed changes

rafiss commented Nov 21, 2022

View reviewed changes

rafiss force-pushed the logic-test-mixed-version branch 3 times, most recently from efce051 to 3cee6b6 Compare November 22, 2022 01:15

rafiss requested a review from a team November 22, 2022 01:15

andyyang890 mentioned this pull request Nov 22, 2022

sql: add mixed version logic tests for role id migrations #92342

Closed

rafiss force-pushed the logic-test-mixed-version branch from 3cee6b6 to 01bece1 Compare November 22, 2022 20:47

rafiss requested review from a team as code owners November 22, 2022 20:47

rafiss requested review from benbardin and removed request for a team and benbardin November 22, 2022 20:47

ZhouXing19 reviewed Nov 23, 2022

View reviewed changes

rafiss commented Nov 23, 2022

View reviewed changes

rafiss added 3 commits November 23, 2022 14:31

*: remove gates for V22_2AddSystemUserIDColumn

0390d01

This makes the next commit for mixed-version testing easier. Release note: None

logictest: support mixed binary version tests with upgrades

8550794

The logictest framework can now use the cockroach-go testserver to test a multi-node cluster in which each node is running a different binary version. Previously, only mixed logical versions could be tested. Release note: None

logictest: improve check after starting/upgrading cluster

acdc9ba

This prevents flakiness that is caused by temporary cluster unavailability after a node is killed and restarted. Release note: None

rafiss force-pushed the logic-test-mixed-version branch from 05480d0 to acdc9ba Compare November 23, 2022 20:03

rafiss commented Nov 23, 2022

View reviewed changes

rafiss requested a review from ZhouXing19 November 23, 2022 21:38

ZhouXing19 reviewed Nov 24, 2022

View reviewed changes

ZhouXing19 approved these changes Nov 24, 2022

View reviewed changes

craig bot merged commit 4ae94a5 into cockroachdb:master Nov 24, 2022

rafiss deleted the logic-test-mixed-version branch November 29, 2022 02:02

matthewtodd mentioned this pull request Jan 23, 2023

create migration tests #71536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logictest: support mixed binary version tests with upgrades #91881

logictest: support mixed binary version tests with upgrades #91881

rafiss commented Nov 14, 2022

cockroach-teamcity commented Nov 14, 2022

ZhouXing19 left a comment

rafiss left a comment

ZhouXing19 left a comment

rafiss left a comment

rafiss left a comment

ZhouXing19 commented Nov 23, 2022

rafiss left a comment

ZhouXing19 left a comment

rafiss commented Nov 24, 2022

craig bot commented Nov 24, 2022

rafiss commented Nov 24, 2022

craig bot commented Nov 24, 2022

logictest: support mixed binary version tests with upgrades #91881

logictest: support mixed binary version tests with upgrades #91881

Conversation

rafiss commented Nov 14, 2022

cockroach-teamcity commented Nov 14, 2022

ZhouXing19 left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ZhouXing19 left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ZhouXing19 commented Nov 23, 2022

rafiss left a comment

Choose a reason for hiding this comment

ZhouXing19 left a comment

Choose a reason for hiding this comment

rafiss commented Nov 24, 2022

craig bot commented Nov 24, 2022

rafiss commented Nov 24, 2022

craig bot commented Nov 24, 2022