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

rework mapsession #1791

Merged
merged 4 commits into from
Apr 15, 2024
Merged

rework mapsession #1791

merged 4 commits into from
Apr 15, 2024

Conversation

kradalby
Copy link
Collaborator

@kradalby kradalby commented Feb 23, 2024

This commit restructures the map session in to a struct
holding the state of what is needed during its lifetime.

For streaming sessions, the event loop is structured a
bit differently not hammering the clients with updates
but rather batching them over a short, configurable time
which should significantly improve cpu usage, and potentially
flakyness.

The use of Patch updates has been dialed back a little as
it does not look like its a 100% ready for prime time. Nodes
are now updated with full changes, except for a few things
like online status.

In addition, there is a lot of simplifications, removing internal states in the mappers,
lots of error cleanups and more tests.

Signed-off-by: Kristoffer Dalby [email protected]

@kradalby kradalby force-pushed the mapper-refac branch 3 times, most recently from bb0ff13 to db69083 Compare March 4, 2024 09:01
@kradalby
Copy link
Collaborator Author

kradalby commented Mar 4, 2024

Known TODOs:

  • There is a bug with Expiry, its sent very often.
  • If a streaming mapsession is open, and a new one is attempted, the old one should be closed.

This commit restructures the map session in to a struct
holding the state of what is needed during its lifetime.

For streaming sessions, the event loop is structured a
bit differently not hammering the clients with updates
but rather batching them over a short, configurable time
which should significantly improve cpu usage, and potentially
flakyness.

The use of Patch updates has been dialed back a little as
it does not look like its a 100% ready for prime time. Nodes
are now updated with full changes, except for a few things
like online status.

Signed-off-by: Kristoffer Dalby <[email protected]>
Replace a lot of occurences of log.Error with fmt.Errorf,
bubbling the error up the chain instead.
@kradalby kradalby marked this pull request as ready for review April 12, 2024 15:47
@juanfont juanfont merged commit 60f0cf9 into juanfont:main Apr 15, 2024
100 checks passed
kradalby added a commit that referenced this pull request May 24, 2024
This PR removes the complicated session management introduced in #1791 which kept track of the sessions in a map, in addition to the channel already kept track of in the notifier.

Instead of trying to close the mapsession, it will now be replaced by the new one and closed after so all new updates goes to the right place.

The map session serve function is also split into a streaming and a non-streaming version for better readability.

RemoveNode in the notifier will not remove a node if the channel is not matching the one that has been passed (e.g. it has been replaced with a new one).

A new tuning parameter has been added to added to set timeout before the notifier gives up to send an update to a node.

Add a keep alive resetter so we wait with sending keep alives if a node has just received an update.

In addition it adds a bunch of env debug flags that can be set:

- `HEADSCALE_DEBUG_HIGH_CARDINALITY_METRICS`: make certain metrics include per node.id, not recommended to use in prod. 
- `HEADSCALE_DEBUG_PROFILING_ENABLED`: activate tracing 
- `HEADSCALE_DEBUG_PROFILING_PATH`: where to store traces 
- `HEADSCALE_DEBUG_DUMP_CONFIG`: calls `spew.Dump` on the config object startup
- `HEADSCALE_DEBUG_DEADLOCK`: enable go-deadlock to dump goroutines if it looks like a deadlock has occured, enabled in integration tests.

Signed-off-by: Kristoffer Dalby <[email protected]>
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.

2 participants