Dialer v2: modular and composable dialer #122

raulk · 2019-05-07T22:19:45Z

This PR deprecates #88.

Description

Here we introduce a Pipeline construction for the dialer. Copied from the godoc:

A Pipeline assembles a set of modular components, each with a clearly-delimited responsibility, into a dialing engine. Consumers can replace, delete, add components to customize the dialing engine's behavior.

The Pipeline comprises five components. We provide brief descriptions below. For more detail, refer to the respective godocs of each interface:

Preparer: runs preparatory actions prior to the dial execution. Actions may include: deduplicating, populating timeouts, circuit breaking, validating the peer ID, etc.

AddressResolver: populates the set of addresses for a peer, either from the peerstore and/or from other sources, such as a discovery process.

Planner: schedules dials in time, responding to events from the environment, such as new addresses discovered, or dial jobs completing.

Throttler: throttles dials based on resource usage or other factors.

Executor: actually carries out the network dials.

Recommendations for review

Here's the recommended walkthrough to review this PR:

Read the package-level godocs on dial/doc.go for an overview of the solution.
Go through the godocs of the interfaces on dial/interfaces.go.
Explore the Request and Job entities in dial/request.go and dial/job.go.
Review the godocs on the Pipeline, as well as the pipeline logic itself in dial/pipeline.go.
Review the implementations of the interfaces in the rest of the files.

raulk · 2019-05-08T11:55:06Z

dial/error.go

@@ -12,14 +12,14 @@ import (
 const maxDialDialErrors = 16

 // DialError is the error type returned when dialing.
-type DialError struct {
+type Error struct {


Sidenote: we could borrow the technique that multierror uses to defer allocs until/if an error is recorded.

var err *dial.Error err.recordErr(addr, err) // does new(dial.Error) if the pointer is nil

magik6k

Digging into this a bit I don't see anything I'd do differently, this looks like a big, big improvement over the current system.

Random notes/thoughts:

Would be nice to test in go-ipfs (sharness / real network / gateways)
If we'd have a way to suspend pipelines, and an additional transport layer, we might be able to implement connection migration quite easily. (leave this for the future)
I like this, it seems to provide a good base for further development

magik6k · 2019-05-17T23:29:39Z

dial/common.go

+	case StatusBlocked:
+		return "Status(Blocked)"
+	default:
+		return fmt.Sprintf("Status(%d)", s)


I'd use %x (or %b) for readability.

magik6k · 2019-05-18T00:13:14Z

dial/throttler.go

+	released <- job
+}
+
+func (t *throttler) throttleFd(job *Job) (throttle bool) {


In future it might be nice to expose this (and other throttler info) to planners so e.g. we won't defer dials to relays and/or defer dialing expensive transports

raulk · 2019-05-22T17:48:10Z

@Stebalien @vyzo we probably want to start moving on this soon.

raulk · 2019-05-28T13:30:28Z

Calling out @vyzo @bigs specifically for a first round of review. A bunch of things are now depending on the dialler changes (self-dial, prioritising non-relay addresses, etc.), so we should move on this urgently to avoid dropping the ball.

@magik6k thanks for the review! 🎉

Would be nice to test in go-ipfs (sharness / real network / gateways)

Agree. I've so far tested with the DHT crawler, which is the most dial-intensive thing we have, and it looks good memory and CPU wise. But I agree we should integrate into IPFS, and test.

If we'd have a way to suspend pipelines, and an additional transport layer, we might be able to implement connection migration quite easily. (leave this for the future)

Could you elaborate on this? Suspending pipelines was on my mind but for a different use case (e.g. activating transports only if we need to dial peers with those transports). It allows for a more dynamic environment, but not an immediate requirement.

I like this, it seems to provide a good base for further development

Yeah, optional self-dial can now be added by removing the default Validator, and adding one that recognises self-dials and returns a singleton in-memory net.Pipe (see libp2p/go-libp2p#638 (review)). Also, we can order, prioritise and stagger dials for transports we'd like to avoid (e.g. relay).

vyzo

This is going to take a while to review properly.

One first observation, this all seems a little static.
I would like to have a mechanism to supply at a minimum the address resolver for individual dial jobs.

Use case in mind: direct connection upgrade.
We will want to dial while already having an existing (relay) connection. For that to happen we will have to bypass the peerstore completely and only dial using user-specified addresses.
Can this be done with this design? Hard to say; if not, something must change.

raulk · 2019-08-14T10:34:44Z

@bigs has stepped up to move this PR over the finish line :-)

bigs · 2019-08-15T21:09:08Z

@raulk @Stebalien just rebased this and updated the branch to reflect the core refactor, amongst other things. tests passing, this should be ready for a review.

bigs · 2019-08-23T17:25:25Z

dial/interfaces.go

+//
+// Likewise, the background discovery process must stop when the provided context is closed.
+type AddressResolver interface {
+	Resolve(req *Request) (known []ma.Multiaddr, more <-chan []ma.Multiaddr, err error)


@raulk this could probably just return a buffered channel, with size len(known). would simplify the api a bit. thoughts?

Good idea! Mind creating a branch with that change, and sending a WIP PR to this branch to see how the code simplifies?

bigs · 2019-08-23T17:28:47Z

dial/interfaces.go

+	// channel to signal it has finished planning.
+	//
+	// The lifetime of asynchronous processes should obey the request context.
+	NewPlan(req *Request, initial []ma.Multiaddr, out chan<- []*Job) (Plan, error)


furthermore, it would remove an argument here

bigs · 2019-08-23T17:29:36Z

dial/interfaces.go

+	JobComplete(completed *Job)
+
+	// ResolutionDone is called when the AddressResolver has finished.
+	ResolutionDone()


should this become an event on the eventbus? perhaps a future change.

bigs · 2019-08-23T17:31:08Z

dial/interfaces.go

+	// Run starts the throttling process. It receives planned jobs via the incoming channel, and emits jobs to dial via
+	// the released channel. Both channels are provided by the caller, who should abstain from closing either during
+	// normal operation to avoid panics. To safely shut down a throttler, first call Close() and await return.
+	Run(incoming <-chan *Job, released chan<- *Job)


since this also implements Closer, maybe it makes sense to have the Throttler own the release channel and, thus, be responsible for closing it. this would require an API change to

Run(incoming <-chan *Job) (released chan<- *Job)

raulk · 2020-02-24T20:28:40Z

Resuming this thread.

@vyzo:

I would like to have a mechanism to supply at a minimum the address resolver for individual dial jobs.

IMO, that would be too complex. We really don't want users of the swarm/network providing address resolvers per-dial; that leaks an abstraction. Instead, the address resolver can take contextual decisions based on the dial that it's processing.

raulk · 2020-02-24T20:34:15Z

If this becomes a common case, we introduce an higher-level AddressResolverDispatcher that conforms to the AddressResolver interface and proxies a bunch of address resolvers behind a set of predicates. The dispatcher evaluates predicates sequentially against the incoming dial request, and chooses the address resolver that matches, or errors if none match.

ghost assigned raulk May 7, 2019

ghost added the status/in-progress In progress label May 7, 2019

raulk mentioned this pull request May 7, 2019

WIP Dialer v2: Pluggable and composable dialer #88

Closed

8 tasks

raulk requested review from Stebalien, vyzo, whyrusleeping, marten-seemann and bigs May 7, 2019 22:21

raulk force-pushed the feat/dialerv2-it2 branch from 98956cd to 8ef0c2f Compare May 7, 2019 22:39

magik6k self-requested a review May 8, 2019 11:40

raulk commented May 8, 2019

View reviewed changes

magik6k reviewed May 18, 2019

View reviewed changes

prestonvanloon mentioned this pull request May 24, 2019

When libp2p fails, node never re-attempts to get connection prysmaticlabs/prysm#2674

Closed

vyzo reviewed May 28, 2019

View reviewed changes

raulk assigned bigs Aug 14, 2019

raulk and others added 7 commits August 15, 2019 16:36

dialer v2: new iteration.

b8a515a

error aliases for backwards compatibility.

31800c2

handle state of completed/cancelled jobs.

c3fb7bf

add Request and Job-scoped log funcs.

c9658be

document and improve pipeline API.

376aa33

reorg code; harden; godocs.

74cb76f

Complete core migration

84fd679

bigs force-pushed the feat/dialerv2-it2 branch from 8ef0c2f to 84fd679 Compare August 15, 2019 21:08

bigs suggested changes Aug 27, 2019

View reviewed changes

Stebalien mentioned this pull request Nov 8, 2019

Prioritize connection via /ip4/ over /p2p-circuit/ ipfs/kubo#6755

Closed

bigs removed their assignment Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialer v2: modular and composable dialer #122

Dialer v2: modular and composable dialer #122

raulk commented May 7, 2019

raulk May 8, 2019

magik6k left a comment

magik6k May 17, 2019

magik6k May 18, 2019

raulk commented May 22, 2019

raulk commented May 28, 2019

vyzo left a comment •

edited

Loading

raulk commented Aug 14, 2019

bigs commented Aug 15, 2019

bigs Aug 23, 2019

raulk Aug 27, 2019

bigs Aug 23, 2019

bigs Aug 23, 2019

bigs Aug 23, 2019

raulk commented Feb 24, 2020

raulk commented Feb 24, 2020

Dialer v2: modular and composable dialer #122

Are you sure you want to change the base?

Dialer v2: modular and composable dialer #122

Conversation

raulk commented May 7, 2019

Description

Recommendations for review

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented May 22, 2019

raulk commented May 28, 2019

vyzo left a comment • edited Loading

Choose a reason for hiding this comment

raulk commented Aug 14, 2019

bigs commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented Feb 24, 2020

raulk commented Feb 24, 2020

vyzo left a comment •

edited

Loading