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

feat: sharded peer manager #2151

Merged
merged 16 commits into from
Dec 7, 2023
Merged

feat: sharded peer manager #2151

merged 16 commits into from
Dec 7, 2023

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Oct 23, 2023

Description

Peer manager must track peers per shard. Each with a target number of in/out peers and then connect/prune connections accordingly.

Changes

  • Per shard pruning of connections
  • Per shard management of relay in/out connections
  • Refactor/clean-up

Tracking #1940

@SionoiS SionoiS added the E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details label Oct 23, 2023
@SionoiS SionoiS self-assigned this Oct 23, 2023
@github-actions
Copy link

github-actions bot commented Oct 23, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2151

Built from ec08762

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

thanks! left some comments

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 26, 2023

I'm still looking at the tests. Some are easier to update than others.

@alrevuelta alrevuelta self-requested a review October 27, 2023 08:10
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for the changes.

Since it involves some networking changes, sometimes difficult to unit test, I would verify with waku-simulator (or related setup) that connectivity is working fine.

@SionoiS
Copy link
Contributor Author

SionoiS commented Oct 27, 2023

lgtm! Thanks for the changes.

Since it involves some networking changes, sometimes difficult to unit test, I would verify with waku-simulator (or related setup) that connectivity is working fine.

Yes I won't be merging this without some simulations. These changes could have unforeseen consequences.

@SionoiS SionoiS force-pushed the feat--sharded-peer-manager branch 2 times, most recently from eb0f52b to 2947dab Compare October 27, 2023 19:06
@SionoiS SionoiS requested a review from jm-clius October 27, 2023 19:36
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Great job. Approving as not to be a blocker, though I think the current targets per shard may quickly cause issues if underlying max-connections for the libp2p switch does not account for the total number of connections. Comment re this below.

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Show resolved Hide resolved
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

I'm late to this PR. Just added a comment aiming to bring more debug detail. No blocking

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS force-pushed the feat--sharded-peer-manager branch 2 times, most recently from 256ecd9 to bfc4e13 Compare November 8, 2023 15:59
@SionoiS
Copy link
Contributor Author

SionoiS commented Nov 22, 2023

In order to unblock this work I've opened #2237

Fix possible out of bound & logic error

Filter peers per protocol & rename proc

Fix out of bound & refactor dialling

Fix catching raise VS timeout & tests fixes

Fix test to connect to all peer per proto

Fix test

Div target per shard count

Logging & stuff

Fixes

Log peer count

More logs

Remove protobook override & clean up

Fix relay peer management & logs

Mics Fixes

Fixes
@SionoiS
Copy link
Contributor Author

SionoiS commented Dec 1, 2023

I've done simulations and this should be the final version of this feature. Sorry for the re-review but it's critical to get this right.

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

great work on this thanks! lgtm!
one important comment though regarding modificating the ProtoBook table, which is already modified by identify.

waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM!

log.txt Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
waku/node/peer_manager/peer_manager.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS merged commit dba9820 into master Dec 7, 2023
9 of 10 checks passed
@SionoiS SionoiS deleted the feat--sharded-peer-manager branch December 7, 2023 11:48
Ivansete-status added a commit that referenced this pull request Dec 20, 2023
This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish
connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit:
"same cluster, different shard: nodes connect"
Ivansete-status added a commit that referenced this pull request Dec 20, 2023
This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish
connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit:
"same cluster, different shard: nodes connect"

* waku_lightpush/protocol.nim: minor changes to make it compile after revert
@SionoiS SionoiS restored the feat--sharded-peer-manager branch January 3, 2024 16:47
Ivansete-status added a commit that referenced this pull request Jan 9, 2024
This reverts commit dba9820.

We need to revert this commit because
the waku-simulator stopped working. i.e. the nodes couldn't establish
connections among them: https://github.com/waku-org/waku-simulator/tree/054ba9e33f4fdcdb590bcfe760a5254069c5cb9f

Also, the following js-waku test fails due to this commit:
"same cluster, different shard: nodes connect"

* waku_lightpush/protocol.nim: minor changes to make it compile after revert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants