fix: coordinator node update race #7345
Merged
Conversation
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
Signed-off-by: Spike Curtis <spike@coder.com>
kylecarbs
reviewed
May 1, 2023
tailnet/coordinator.go
Outdated
| @@ -159,8 +158,30 @@ type coordinator struct { | |||
| agentNameCache *lru.Cache[uuid.UUID, string] | |||
| } | |||
|
|
|||
| func NewCore(logger slog.Logger) *Core { | |||
There was a problem hiding this comment.
Thoughts on putting the coordinator in it's own package? The name NewCore and Core doesn't make much sense in the context of tailnet itself for providing connections.
An alternative would be putting the in-memory coordinator in it's own package.
There was a problem hiding this comment.
I originally exported those names thinking we'd want the Core for the haCoordinator but I'm now thinking they won't be that reusable. So, if the concern is that the public tailnet API contains these and it's not clear, we could just make them private.
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
kylecarbs
approved these changes
May 2, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This fixes the most pressing bug on #7295 but leaves the
haCoordinatormainly as is, meaning that we're still subject to various distributed races leaving us in a bad state where clients can't connect.A full fix for
haCoordinatoris quite a large change, so I plan to write an RFC for it.The main idea of this fix is that we establish an output queue of updates on each connection. This allows us to queue up the initial node update while holding the lock, ensuring we don't miss any updates coming from the other side just as we are connecting. The queue is buffered, and actual I/O on the connection is done on a separate goroutine, where it will never contend for the lock.
There was also a lot of complex manipulation of locking and unlocking the mutex in the existing routines, so I refactored the
coordinatorto contain a mutex-protected memory "core". All operations on thiscoreare via methods thatso it should be much easier to follow what things are holding the lock and what things aren't (
coreoperations vs the rest of thecoordinatormethods).