server: introduce Test{ClusterConnectivity,JoinVersionGate}#54072
Merged
craig[bot] merged 4 commits intocockroachdb:masterfrom Sep 10, 2020
Merged
server: introduce Test{ClusterConnectivity,JoinVersionGate}#54072craig[bot] merged 4 commits intocockroachdb:masterfrom
craig[bot] merged 4 commits intocockroachdb:masterfrom
Conversation
And no need require callers re-supply it when starting up the cluster (we already need it when we create it). Just a drive by cleanup. Release justification: non-production code changes Release note: None
We pre-define a firstListener when creating a TestCluster in order to have the address accessible to other connecting servers that want to set up appropriate join flags. We generalize this idea to allow callers to install Listeners on a per server basis in order to set up arbitrary join flag "links" between nodes. We'll make use of this ability in a future test. Release note: None
Member
|
This change is |
415d98e to
d34e3a7
Compare
In a future commit we'll add a test that wants to read the node ID of a given server concurrently with it being set. This is because the test exercises initialization code paths where we'll start a test server in parallel with inspecting its state. If we consult the descriptor when retrieving the nodee ID, it's a data race (there's no mutex protecting its access, it was never needed). The rpcContext node ID container however is an atomic value, so it's safe for concurrent access. They're both set at the same points in the server start up codepaths, so it's safe to swap one our for another. Release note: None
d34e3a7 to
d27c908
Compare
Contributor
Author
|
(Gentle ping. This PR is not super important, but would still give me some peace of mind.) |
knz
approved these changes
Sep 10, 2020
Contributor
knz
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)
pkg/base/test_server_args.go, line 53 at r2 (raw file):
// manual control over how the join flags (`JoinAddr`) are populated, and // installs listeners manually to know which addresses to point to. Listener net.Listener
Curious: can you point me to the code responsible to close this?
(Maybe even include this pointer in the comment)
pkg/server/connectivity_test.go, line 176 at r4 (raw file):
// // NB: That aside, TestCluster very much wants to live on the main // goroutine running the test. THat's mostly to do with its internal
nit: typo
TestClusterConnectivity sets up an uninitialized cluster with custom join flags (individual nodes point to specific others, constructed using connectivity matrices). Included in the test configurations are the more "standard" ones where every node points to n1, or where there are bidirectional links set up between nodes. It tests various topologies and ensures that cluster IDs are distributed throughout connected components, and store/node IDs are allocated in the way we'd expect them to be. TestJoinVersionGate checks to see that improperly versioned cockroach nodes are not able to join a cluster. Release note: None
d27c908 to
78b5535
Compare
irfansharif
commented
Sep 10, 2020
Contributor
Author
irfansharif
left a comment
There was a problem hiding this comment.
Thanks for looking Rafa!
bors r+
Reviewable status:
pkg/base/test_server_args.go, line 53 at r2 (raw file):
Previously, knz (kena) wrote…
Curious: can you point me to the code responsible to close this?
(Maybe even include this pointer in the comment)
Was included in the follow-on commit. It's accessible when jumping to usages now, so I'll skip the comment.
pkg/server/connectivity_test.go, line 176 at r4 (raw file):
Previously, knz (kena) wrote…
nit: typo
Done.
Member
|
Ugh, sorry for letting this slip again. Had the tab open but got distracted.
…On Thu, Sep 10, 2020, 19:17 irfan sharif ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks for looking Rafa!
bors r+
*Reviewable
<https://reviewable.io/reviews/cockroachdb/cockroach/54072#-:-MGt0EjU0jR-Ij9hPKcU:bgicul6>*
status: [image:
] complete! 0 of 0 LGTMs obtained (waiting on @knz
<https://github.com/knz> and @tbg <https://github.com/tbg>)
------------------------------
*pkg/base/test_server_args.go, line 53 at r2
<https://reviewable.io/reviews/cockroachdb/cockroach/54072#-MGsplov4zIKiQSmnsaT:-MGt-leU0A_uriURmHOK:b82fo6i>
(raw file
<https://github.com/cockroachdb/cockroach/blob/c36463e5b071ae5ada393454f7b310da43924ac2/pkg/base/test_server_args.go#L53>):*
*Previously, knz (kena) wrote…*
Curious: can you point me to the code responsible to close this?
(Maybe even include this pointer in the comment)
It's here:
https://github.com/irfansharif/cockroach/blob/d27c908602763fe7edbfe09ba129b69e7c66d70a/pkg/server/connectivity_test.go#L135-L138
Was included in the follow-on commit. It's accessible when jumping to
usages now, so I'll skip the comment.
------------------------------
*pkg/server/connectivity_test.go, line 176 at r4
<https://reviewable.io/reviews/cockroachdb/cockroach/54072#-MGsq3w7DGlNSD49nDFx:-MGt-kzhCHlcEK9Gb-WZ:b-896fix>
(raw file
<https://github.com/cockroachdb/cockroach/blob/d27c908602763fe7edbfe09ba129b69e7c66d70a/pkg/server/connectivity_test.go#L176>):*
*Previously, knz (kena) wrote…*
nit: typo
Done.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54072 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZEXGEPDVRNJKNEKHMTSFEC4DANCNFSM4RAVFCOQ>
.
|
Contributor
|
Build succeeded: |
Contributor
Author
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-adding a few tests that I didn't check in while working on #52526.
TestClusterConnectivity sets up an uninitialized cluster with custom join
flags (individual nodes point to specific others, constructed using
connectivity matrices). Included in the test configurations are the more
"standard" ones where every node points to n1, or where there are
bidirectional links set up between nodes. It tests various topologies
and ensures that cluster IDs are distributed throughout connected
components, and store/node IDs are allocated in the way we'd expect them
to be.
TestJoinVersionGate checks to see that improperly versioned cockroach
nodes are not able to join a cluster.
Release note: None