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

gossip: do not recommend self as node to forward to #5901

Merged

Conversation

petermattis
Copy link
Collaborator

This caused a failure to connect to a gossip network when bootstrapping
if the bootstrap node told the other side to forward to itself. The
client would see the forward address but refuse to do anything with it
because it was already connected to the address it was being told to
forward to. And since the bootstrap node was the only bootstrap address
and this address was now exhausted we would never connect to the gossip
network.

Fixes #5879.


This change is Reviewable

This caused a failure to connect to a gossip network when bootstrapping
if the bootstrap node told the other side to forward to itself. The
client would see the forward address but refuse to do anything with it
because it was already connected to the address it was being told to
forward to. And since the bootstrap node was the only bootstrap address
and this address was now exhausted we would never connect to the gossip
network.

Fixes cockroachdb#5879.
@petermattis
Copy link
Collaborator Author

Curious why we only try each bootstrap address once? That seems fragile.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


gossip/server.go, line 189 [r1] (raw file):
I'm not very familiar with the gossip code. Is it possible for this panic to happen if there is only one entry in the nodeMap? But if there is only one entry, wouldn't s.incoming.hasSpace return true?


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

cc @spencerkimball

@tamird
Copy link
Contributor

tamird commented Apr 6, 2016

:lgtm:

how painful is writing a test for this going to be?


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


gossip/server.go, line 189 [r1] (raw file):
Yeah, I think that's right.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

Well, there is a test: TestConvergence. It just doesn't trigger every time, but make stress does reproduce the problem rather quickly. Let think about whether we can add something more targeted.


Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@spencerkimball
Copy link
Member

We should always be retrying bootstrap addresses as long as there's not a connection outstanding. I made a comment on another PR from @tamird, but haven't had much time this week to look at what's really going on in there. I was a little nervous about the changes to gossip because not adding self to the bootstrap addresses was a carefully negotiated compromise to handle some tricky edge cases. There should have been better testing / comments in there, but alas.


Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

I'm going to merge this sans-test as this fixes a bunch of acceptance test flakiness we've been seeing recently. See #5912.

@petermattis
Copy link
Collaborator Author

@spencerkimball You've got #5902 assigned to you. I'm not sure about the history of IsExhaustive, but that mechanism seems fraught. Simply commenting out the IsExhaustive conditional seems to work fine.

@petermattis petermattis merged commit 5149e50 into cockroachdb:master Apr 7, 2016
@petermattis petermattis deleted the pmattis/test-convergence branch April 7, 2016 00:26
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.

3 participants