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

refactor: Improve rendezvous protocol usage #399

Merged
merged 32 commits into from
Nov 7, 2023

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Oct 24, 2023

Description

This pull request implements the following features:

  • Add event handler cache implementation
  • Renew rendezvous registration at expiration
  • Re-discover from rendezvous servers on a set interval
  • Renew discovered peer registrations when they expire
  • Remove behavior where we add other peers to external_addresses
  • Add check to not dial ourselves on rendezvous discovery
  • Update dialing on rendezvous discovery to only dial peers we aren't already connected to
  • Add rendezvous server Dialing, PeerRegistered, and RegistrationExpired event debug logs
  • Separate enable_rendezvous config into enable_rendezvous_server and enable_rendezvous_client configs (server is opt-in, client is opt-out)
  • Add max_connected_peers config
  • Add rendezvous_registration_ttl and rendezvous_discovery_interval configs
  • Test rendezvous register, discover, and connect
  • Test disconnect after rendezvous connect
  • Test registration expires and re-registration on expiration
  • Test discovered registration expires and rediscovery attempted at expiration
  • Test rediscovery on rendezvous discovery interval
  • Add extract_timestamps_where and count_lines_where test utilities
  • Update libp2p deprecated Kademlia and SwarmBuilder interfaces
  • Update integration test fixtures to avoid overlapping ports

Link to issue

Implements #131.

Type of change

  • New feature (non-breaking change that adds functionality)
  • Refactor (non-breaking change that updates existing functionality)
  • Comments have been added/updated

Test plan (required)

We are adding tests to check existing functionality and a test to check registration renewals.

@bgins bgins added the networking Features, functionality involving networking label Oct 24, 2023
@bgins bgins self-assigned this Oct 24, 2023
@bgins bgins force-pushed the bgins/add-rendezvous-integration-tests branch from 15bb0a0 to 7fade98 Compare October 24, 2023 21:07
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #399 (0ea35d0) into main (fc542ef) will decrease coverage by 1.10%.
Report is 13 commits behind head on main.
The diff coverage is 21.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
- Coverage   71.91%   70.82%   -1.10%     
==========================================
  Files          71       72       +1     
  Lines        6955     7190     +235     
==========================================
+ Hits         5002     5092      +90     
- Misses       1953     2098     +145     
Files Coverage Δ
homestar-core/src/workflow/instruction_result.rs 90.24% <100.00%> (+0.12%) ⬆️
homestar-runtime/src/event_handler.rs 95.38% <100.00%> (+1.63%) ⬆️
homestar-runtime/src/logger.rs 92.00% <100.00%> (+0.10%) ⬆️
homestar-runtime/src/settings.rs 98.28% <100.00%> (+0.05%) ⬆️
homestar-core/src/unit.rs 37.50% <0.00%> (ø)
homestar-runtime/src/network/swarm.rs 61.20% <29.03%> (+1.08%) ⬆️
homestar-runtime/src/event_handler/cache.rs 22.50% <22.50%> (ø)
homestar-runtime/src/event_handler/event.rs 12.25% <0.00%> (-1.56%) ⬇️
homestar-runtime/src/event_handler/swarm_event.rs 19.05% <9.33%> (+3.48%) ⬆️

... and 8 files with indirect coverage changes

We only want to add addresses for ourself to external addresses.
Only dial when we are not at connected peer limit.
Only dial peers we are not already connected to.
Add cache with eviction listener to trigger discovery and renewal events
on an interval and at TTL respectively.
@bgins bgins force-pushed the bgins/add-rendezvous-integration-tests branch from 29d0527 to 5b37c6c Compare October 30, 2023 04:14
@bgins bgins force-pushed the bgins/add-rendezvous-integration-tests branch from 2de7817 to 4d7ad72 Compare October 30, 2023 21:14
@bgins bgins marked this pull request as ready for review October 31, 2023 03:29
@bgins bgins requested a review from a team as a code owner October 31, 2023 03:29
Copy link
Contributor

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

First pass. Need to look at the tests a little more, but this is looking great.

request_response_senders: FnvHashMap<RequestId, (RequestResponseKey, P2PSender)>,
rendezvous_registration_ttl: Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

@bgins thinking maybe we do have some substruct here: rendezvous, which has ttl and discovery interval and cookies? Trying to clean up this bit on length here. We may want the same for the two connected peers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Also added discovered_peers to the rendezvous one because it is not relevant for peers discovered through mDNS. We may revisit if we use the proposed ambient discovery protocol in the future.

Was slightly unsure about the naming for the connected peers one. Went with this struct:

// Connected peers configuration and state
struct Connections {
peers: FnvHashMap<PeerId, ConnectedPoint>,
max_peers: u32,
}

Which on the event handler is:

connections: Connections,

And when used we have event_handler.connections.peers and event_handler.connections.max_peers. In the first one, having connections and peers seems a bit redundant? I might be overthinking it.

homestar-runtime/src/event_handler.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler/swarm_event.rs Outdated Show resolved Hide resolved
@bgins bgins force-pushed the bgins/add-rendezvous-integration-tests branch from 1d4079b to c4bbd5f Compare October 31, 2023 17:09
@bgins bgins force-pushed the bgins/add-rendezvous-integration-tests branch from 2d6ab6f to 263a871 Compare October 31, 2023 23:15
@zeeshanlakhani zeeshanlakhani merged commit 5b5b2ff into main Nov 7, 2023
27 checks passed
@zeeshanlakhani zeeshanlakhani deleted the bgins/add-rendezvous-integration-tests branch November 7, 2023 02:17
This was referenced Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking Features, functionality involving networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants