Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logictest: fix flakes from mixed version testing #94458

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Dec 30, 2022

fixes #92637

The main fix was to wait for each node to be reachable before beginning the upgrade process. This also includes a version bump that has better logging to make it easier to debug.

Release note: None

@rafiss rafiss requested a review from a team December 30, 2022 19:55
@rafiss rafiss requested a review from a team as a code owner December 30, 2022 19:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss requested a review from ZhouXing19 January 3, 2023 14:58
Copy link
Collaborator

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :lgtm: mod a nit

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


pkg/sql/logictest/logictestbase/logictestbase.go line 479 at r1 (raw file):

		NumNodes:                    3,
		CockroachGoBootstrapVersion: "v22.2.1",
		BootstrapVersion:            roachpb.Version{Major: 22, Minor: 2},

nit: I'm wondering why we need this BootstrapVersion field. Seems to me that it's used in logicTest.newCluster(), which I think should not be called when we're using a testserver

The main fix was to wait for each node to be reachable before beginning
the upgrade process. This also includes a version bump that has better
logging to make it easier to debug.

Release note: None
@rafiss rafiss force-pushed the fix-mixed-node-logic-test branch from 10815c0 to b4f40e4 Compare January 3, 2023 16:15
Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tftr!

bors r=ZhouXing19

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ZhouXing19)


pkg/sql/logictest/logictestbase/logictestbase.go line 479 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: I'm wondering why we need this BootstrapVersion field. Seems to me that it's used in logicTest.newCluster(), which I think should not be called when we're using a testserver

nice catch, it's not needed

@craig
Copy link
Contributor

craig bot commented Jan 3, 2023

Build succeeded:

@craig craig bot merged commit 291a43b into cockroachdb:master Jan 3, 2023
@rafiss rafiss deleted the fix-mixed-node-logic-test branch January 3, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logictest: mixed-version testserver tests are too flaky
3 participants