Skip to content
This repository has been archived by the owner on Apr 21, 2022. It is now read-only.

Add peer protection capability (implementation) #36

Merged
merged 3 commits into from
Mar 29, 2019
Merged

Conversation

raulk
Copy link
Member

@raulk raulk commented Mar 27, 2019

Users can call Protect() to save a peer from pruning, and Unprotect() to remove the protection.

Protected peers don't count towards the target quantity to prune during a connection manager cycle. That is, if we have 10 peers, and we need to prune 5, where 3 are protected but would've otherwise been selected, we will still prune 5 peers, saving the protected 3 from pruning.

This is the implementation for libp2p/go-libp2p-interface-connmgr#14.

Enables ipfs/kubo#6097.


NOTE: this will need a gomod release on the interface repo, and an upgrade here.

Users can call Protect() to save a peer from pruning, and Unprotect() to
remove the protection. Protected peers don't count towards the target
quantity to prune during a connection manager cycle.
connmgr.go Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Let's refcount the protections.

connmgr.go Outdated Show resolved Hide resolved
connmgr.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member Author

raulk commented Mar 28, 2019

I don’t think we want to do refcounting here as this is intented to behave like a whitelist. Refcounting will be needed for temporary connection locking when we introduce it IMO.

@vyzo
Copy link
Contributor

vyzo commented Mar 28, 2019

I can easily see this being used for other purposes. If it is a whitelist, then it should be documented as such and possibly not provide Unprotect.

The concern is the application calling Protect twice and then calling Unprotect once, which would leave the peer unprotected while the intent would be the opposite.

@raulk
Copy link
Member Author

raulk commented Mar 28, 2019

@vyzo agree that's confusing. I tend to prefer intuitive method signatures. I can add an error return value, and two exported errors ErrAlreadyProtected & ErrNotProtected that get returned in such circumstances. As well as a note in the godoc.

Also slightly reworked the loop that selects candidates to prune to make
it better behaved. Before, if we needed to prune N connections, we
preselected N and ended up doing nothing if they happened to be in the
grace period. Now we skip over them and prune ungraced connections until
we meet our target.
@raulk
Copy link
Member Author

raulk commented Mar 29, 2019

@vyzo: I picked @Stebalien's brain yesterday. We definitely expect this API to behave idempotently, so we don't want to do refcounting (there will be an API for locking connections transactionally in the near future).

However, we did realise that different subsystems could interfere with each other if they were managing protections. I think this might be what you were getting at.

So I've just pushed a new version that scopes protections by tag, both here and in the interface repo: libp2p/go-libp2p-interface-connmgr#14.

Also enhanced the docs to explain the behaviour clearly.

go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

that's acceptable.

@raulk raulk merged commit a3dceaf into master Mar 29, 2019
@raulk raulk deleted the feat/protect branch March 29, 2019 19:43
@ghost ghost removed the status/in-progress In progress label Mar 29, 2019
@raulk raulk mentioned this pull request Apr 3, 2019
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants