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

roachtest: fix network/authentication/nodes=4 #138816

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

tbg
Copy link
Member

@tbg tbg commented Jan 10, 2025

For reasons, the test starts a three node cluster, then immediately
stops it, then restarts n1 and {n2,n3} separately. It was previously
restarting n1 first, followed by {n2,n3}. Due to recent change
PR #138109, this no longer works since n1 doesn't have quorum and
so won't signal SQL readiness.

There's an ongoing discussion on whether this new behavior is desired,
but either way, this PR changes the test to restart {n2,n3} first (which
does have quorum, assuming we wait for full replication first, which we
now do as well), followed by n1.

Closes #138806.

Epic: none
Release note: None

For reasons, the test starts a three node cluster, then immediately
stops it, then restarts n1 and {n2,n3} separately. It was previously
restarting n1 first, followed by `{n2,n3}`. Due to recent change
PR cockroachdb#138109, this no longer works since n1 doesn't have quorum and
so won't signal SQL readiness.

There's an ongoing discussion on whether this new behavior is desired,
but either way, this PR changes the test to restart {n2,n3} first (which
does have quorum, assuming we wait for full replication first, which we
now do as well), followed by n1.

Closes cockroachdb#138806.

Epic: none
Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from srosenberg January 10, 2025 10:27
@tbg
Copy link
Member Author

tbg commented Jan 10, 2025

Successful run here

@tbg tbg marked this pull request as ready for review January 10, 2025 13:25
@tbg tbg requested a review from miraradeva January 10, 2025 15:46
@srosenberg
Copy link
Member

For reasons, the test starts a three node cluster, then immediately stops it, then restarts n1 and {n2,n3} separately. It was previously restarting n1 first, followed by {n2,n3}. Due to recent change PR #138109, this no longer works since n1 doesn't have quorum and so won't signal SQL readiness.

Sorry for causing a regression; I thought I had smoke tested everything, but this one somehow eluded the smoke tests. We could make an opt-out from waiting for sql for special cases like this one. (There is one already for some acceptance tests which don't use cluster init on startup.)

@tbg
Copy link
Member Author

tbg commented Jan 14, 2025

bors r+

@craig craig bot merged commit f87bf4d into cockroachdb:master Jan 14, 2025
22 checks passed
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.

roachtest: network/authentication/nodes=4 failed
4 participants