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

Pull hostmap and pending hostmap apart, remove unused functions #843

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

nbrownus
Copy link
Collaborator

@nbrownus nbrownus commented Apr 5, 2023

The main HostMap has concerns that the pending host map does not and has led to bugs in recent changes.

This change puts HandshakeManager in charge of its own dedicated maps that share some similarity with the main HostMap but lacks all the extra cruft. This also removes some unused code from the main HostMap as a result of the split.

My main concern is: HandshakeManager.AddVpnIp will go for a write lock immediately, Interface.getOrHandshake won't attempt check the pending hostmap before making a call to this function. This avoids a read lock to write lock trading race that lead to the overcomplication of Interface.getOrHandshake long ago. The concern is when restarting nebula on a busy host with many tunnels, there will be a period of strong write lock contention until the tunnels form or the services using the overlay slow down transmitting until replies come in.

If we believe this is an issue I can go back to behavior similar to the old way that still sets us up to simplify Interface.getOrHandshake in the future with a read lock check and a write lock check before proceeding to add.

@maggie44
Copy link

maggie44 commented Apr 6, 2023

I am looking to poll the hostmap every 4 seconds to be able to keep an up to date list of connected devices for use on a dashboard. Would that be blocked by these locks or create a lock itself that would prevent devices joining?

@nbrownus
Copy link
Collaborator Author

nbrownus commented Apr 6, 2023

That would require a read lock and could slow down a few interactions but its probably fine. Control.ListHostmapHosts or Control.ListHostmapIndexes would be your best bet at accessing that information as well.

@maggie44
Copy link

maggie44 commented Apr 6, 2023

I've been using hostMap := Ctrl.ListHostmap(false) exposed by *nebula.Control, but looks like it is doing the same thing (not accounting for anything in this PR mind).

It could be 100,000s of nodes, but presumably it is just a lookup of data already gathered, so thinking it shouldn't be a noticeable delay or lock 🤞

@wadey wadey self-assigned this Jul 19, 2023
@wadey wadey added the needs-slack-review Needs review from a Slack team member label Jul 19, 2023
outside.go Show resolved Hide resolved
Copy link
Member

@wadey wadey left a comment

Choose a reason for hiding this comment

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

Looks good except one comment

@nbrownus nbrownus merged commit a10baee into master Jul 24, 2023
6 checks passed
@nbrownus nbrownus deleted the pending-hostmap branch July 24, 2023 17:37
@wadey wadey added this to the v1.8.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed needs-slack-review Needs review from a Slack team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants