relay: turn autorelay into a service and always filter out relay addresses #578

Stebalien · 2019-04-05T21:18:53Z

Also, remove the "RelayHost".

Stebalien · 2019-04-05T21:19:12Z

config/config.go

+			// advertise ourselves
+			// TODO: Why do we only do this when EnableAutoRelay is
+			// set? This has absolutely _nothing_ to do with
+			// autorelay.


Sure it has to do with autorelay -- it's the advertisement of relay services for autorelayed hosts!

If you look at this from an "I'm enabling the autorelay subsystem" perspective, yes. However, users will just see an autorelay option. When looking at these as options,

If I enable relay, I expect my node to make use of relays and accept relay connections.

If I enable relay hop, I expect my node to act as a relay. That includes telling other users that I'm willing to be a relay.

If I enable autorelay, I expect my node to automatically pick and advertise a relay.

Having to enable both relay hop + autorelay to get other users to use me as a relay is really not what I would expect.

We do need a mechanism to allow users to enable relay for their own purposes (ie their own little fleet of nodes) but without having to be inundated by the torrent of traffic that comes when announcing the relay functionality.

Sure, but that should really be an option on the relay service. I'll try to document all this to be less confusing but I know we're still going to be confusing users.

Stebalien · 2019-04-05T21:46:07Z

Hm. Actually, I'm going to try to simply remove both of these custom hosts. I have a feeling we don't actually need them.

We don't need the host wrappers, we can just replace the filter AddrsFactory. Also, always filter out relay addresses.

Stebalien · 2019-04-06T02:57:18Z

p2p/host/relay/autorelay.go

+	if ar.host.Network().Connectedness(p) == inet.Connected {
+		// We have a second connection.
+		return
+	}


Fixes a small race.

config/config.go

vyzo

We need to return/wrap the host constructed by AutoRelay.

vyzo · 2019-04-06T07:43:32Z

config/config.go

+			// advertise ourselves
+			// TODO: Why do we only do this when EnableAutoRelay is
+			// set? This has absolutely _nothing_ to do with
+			// autorelay.


Sure it has to do with autorelay -- it's the advertisement of relay services for autorelayed hosts!

vyzo · 2019-04-06T07:45:54Z

config/config.go

+			// 2. Introduce a service management system (e.g.,
+			// uber's fx) so we can actually manage the lifetime of
+			// this service.
+			_ = relay.NewAutoRelay(swrm.Context(), h, discovery, router)


we need to keep the autorelay host, otherwise we will wrap/return the basic host and all the address manipulation will be voided.

actually it's probably ok, we are overriding the AddressFactory at the end of the day.

Also, it's not really an abuse of context. Contexts are the only construct go has for denoting dynamic scopes, so it's ok to use it for long-lived ones.

Context in go are rather... overloaded. They are the only construct for dynamic scopes so, in that sense, it's correct to pass them into constructors. However, they're more commonly (e.g., here) used for cancellation and that's where we get into trouble: contexts don't allow waiting for something to finish.

They're useful for requests where either:

You pass the context and then wait on a returned channel. This channel allows you to actually wait for the request.

You pass the context to a synchronous request.

You just don't care about waiting.

However, services should really just provide a Stop() or Close() function.

I bring this up because it prevents orderly shutdown in go-ipfs. For example, I'd like to be able to say:

Shut down all services using the datastore.

Shut down the datastore.

However, when we use contexts to manage services, we can't wait for 1 to finish before we move on to 2.

And how are we going to implement Stop or Close when there are background goroutines doing work? Why, with contextx of course!
So we can't really get rid of them and I think it's better to have one global context for the entire host and tree of services rather than have each service make its own background one with cancel.

I am not arguing against Close methods; all I am saying is that contexts will inevitably be used for this and it's convenient to have a global context with user cancellation.
When we go services, we might have GlobalContext service dependency that provides this context.

And I'm explicitly arguing against using contexts for canceling services at all because:

It causes all services to shut down all at once. This has been causing real and really annoying problems in go-ipfs where we'll, e.g., close the blockservice before we stop all services trying to write blocks.

It makes it really easy to incorrectly implement services. Before the transport refactor, we had quite a few services that wouldn't actually stop unless you both canceled the context and called Close.

Having a Close method for services is undeniably a useful abstraction, as it could handle orderly dependency shutdown as well (but it won't be easy).
The global context could be used by services that need a context to manager their internal goroutines, there is nothing stopping them from wrapping this context in a WithCancel.

The issue is that canceling this context will cause the service to start shutting down before close is called, throwing any orderly shutdown out the window. Now, of course, we could just say "don't cancel this context till you've finished the orderly shutdown" but that's not exactly user-friendly.

Let's put together a requirements/design document outlining what we actually need from the refactor. That will help us drive alignment and consensus.

I sense we all have slightly different views, understandings and wishes, and it'll cause frustration if we jump into technical solutions before we decide what problems we want to solve.

Things we have discussed:

dependency injection

service lifecycles

hot replacement of runtime services

service tree (lifecycle dependencies)

eventbus

etc.

vyzo · 2019-04-06T07:47:58Z

So why are we killing RelayHost? I think we should keep it instead of having the advertisement hiding in the constructor.

p2p/host/relay/autorelay.go

vyzo

I think it's ok to not wrap the hosts and go back to the basic host, we are overriding the address factory at the end of the day.

vyzo

yeah, on second (and third) reading, this is all fine; it simplifies a bunch by turning the host into a service.

config/config.go

vyzo · 2019-04-06T08:57:55Z

config/config.go

+			// 2. Introduce a service management system (e.g.,
+			// uber's fx) so we can actually manage the lifetime of
+			// this service.
+			_ = relay.NewAutoRelay(swrm.Context(), h, discovery, router)


Also, it's not really an abuse of context. Contexts are the only construct go has for denoting dynamic scopes, so it's ok to use it for long-lived ones.

vyzo · 2019-04-06T08:58:31Z

p2p/host/relay/autorelay.go

-// AutoRelayHost is a Host that uses relays for connectivity when a NAT is detected.
-type AutoRelayHost struct {
-	*basic.BasicHost
+// AutoRelay is a Host that uses relays for connectivity when a NAT is detected.


it's no longer a host, but rather a service.

p2p/host/relay/autorelay.go

vyzo · 2019-04-06T09:04:20Z

p2p/host/relay/doc.go

-  `libp2p.New(libp2p.EnableRelay(circuit.OptHop), libp2p.Routing(makeDHT))`.
-  They provide Relay Hop services, and advertise through the DHT
-  in the `/libp2p/relay` namespace
-
 - `AutoRelayHost`s are constructed with `libp2p.New(libp2p.Routing(makeDHT))`


this needs an update too, it's no longer a host.

vyzo · 2019-04-06T09:05:54Z

The documentation has become obsolete.

vyzo · 2019-04-06T09:17:46Z

p2p/host/relay/autorelay.go

-	need := DesiredRelays - len(h.relays)
-	h.mx.Unlock()
+	need := DesiredRelays - len(ar.relays)
+	ar.mx.Unlock()

 	limit := 20


can we bump this limit to 50? We have 25 relays deployed plus whatever user-run relays, so we should try to see them all.

Pushed a commit that bumps that.

These are issues for another day.

Stebalien · 2019-04-06T14:45:35Z

options.go

+//    as a public relay using the provided routing system.
+// 2. When this libp2p node is _not_ configured as a relay "hop", it will
+//    automatically if it is unreachable (e.g., behind a NAT). If so, it will
+//    find, configure, and announce a set of public relays.


@vyzo, this is why I don't like this option design. I now have to explain to the user that the "AutoRelay" option can do two very different things depending on how they've configured their relay.

AutoRelay is a system service.
If you are a hop relay, then the functionality is to advertise relay services. If you are a client, then the functionality is to use automatically use the services advertised by hop relays.

What service? There are, and always have been, two completely separate and orthogonal services: one for "hop" relays and one for clients. That's why this is so damn confusing.

I tend to think of autorelay as a single service. But yes, at some level you are right that these are two seperate services and that could potentially be confusing.

There are, and always have been, two completely separate and orthogonal services: one for "hop" relays and one for clients.

Actually, it's even more confusing than that. A client can be a consumer of a relay service, or a target of a relayed connection. Currently, all of these flow over the same thick protocol. I really have a hard time wrapping my head around this, and so do our users.

I'd like to segregate this protocol if possible into at least two:

/libp2p/circuit/relay/hop/v0.0.0: exposed only by relay nodes. Used inside the initiator -> relay connection.

/libp2p/circuit/relay/target/v0.0.0: exposed by clients accepting relayed connections. Used in the relay -> target connection.

This would allow for better implicit observability too. Would've helped a lot in the incident we're analysing.

vyzo · 2019-04-06T15:08:15Z

options.go

+//    (circuit.OptHop is passed to EnableRelay), this node will advertise itself
+//    as a public relay using the provided routing system.
+// 2. When this libp2p node is _not_ configured as a relay "hop", it will
+//    automatically if it is unreachable (e.g., behind a NAT). If so, it will


missing "detect" word.

vyzo

LGTM

raulk

No code review; just contributions to the great discussion.

raulk · 2019-04-06T16:14:55Z

options.go

+//    as a public relay using the provided routing system.
+// 2. When this libp2p node is _not_ configured as a relay "hop", it will
+//    automatically if it is unreachable (e.g., behind a NAT). If so, it will
+//    find, configure, and announce a set of public relays.


There are, and always have been, two completely separate and orthogonal services: one for "hop" relays and one for clients.

Actually, it's even more confusing than that. A client can be a consumer of a relay service, or a target of a relayed connection. Currently, all of these flow over the same thick protocol. I really have a hard time wrapping my head around this, and so do our users.

I'd like to segregate this protocol if possible into at least two:

/libp2p/circuit/relay/hop/v0.0.0: exposed only by relay nodes. Used inside the initiator -> relay connection.

/libp2p/circuit/relay/target/v0.0.0: exposed by clients accepting relayed connections. Used in the relay -> target connection.

This would allow for better implicit observability too. Would've helped a lot in the incident we're analysing.

raulk · 2019-04-06T16:19:24Z

config/config.go

+			// 2. Introduce a service management system (e.g.,
+			// uber's fx) so we can actually manage the lifetime of
+			// this service.
+			_ = relay.NewAutoRelay(swrm.Context(), h, discovery, router)


Let's put together a requirements/design document outlining what we actually need from the refactor. That will help us drive alignment and consensus.

I sense we all have slightly different views, understandings and wishes, and it'll cause frustration if we jump into technical solutions before we decide what problems we want to solve.

Things we have discussed:

dependency injection

service lifecycles

hot replacement of runtime services

service tree (lifecycle dependencies)

eventbus

etc.

ghost assigned Stebalien Apr 5, 2019

ghost added the status/in-progress In progress label Apr 5, 2019

Stebalien requested a review from vyzo April 5, 2019 21:19

Stebalien commented Apr 5, 2019

View reviewed changes

Stebalien added 2 commits April 5, 2019 19:54

relay: no more wrapping

80ada8a

We don't need the host wrappers, we can just replace the filter AddrsFactory. Also, always filter out relay addresses.

relay: only stop advertising relay addresses if we fully disconnect

5b66564

Stebalien force-pushed the fix/relay-host-stuff branch from 1d096af to 5b66564 Compare April 6, 2019 02:55

Stebalien commented Apr 6, 2019

View reviewed changes

vyzo requested changes Apr 6, 2019

View reviewed changes

vyzo reviewed Apr 6, 2019

View reviewed changes

p2p/host/relay/autorelay.go Show resolved Hide resolved

vyzo reviewed Apr 6, 2019

View reviewed changes

p2p/host/relay/autorelay.go Show resolved Hide resolved

vyzo reviewed Apr 6, 2019

View reviewed changes

vyzo approved these changes Apr 6, 2019

View reviewed changes

vyzo reviewed Apr 6, 2019

View reviewed changes

vyzo changed the title ~~relay: always filter out relay addresses~~ relay: turn autorelay into a service and always filter out relay addresses Apr 6, 2019

ghost assigned vyzo Apr 6, 2019

vyzo and others added 3 commits April 6, 2019 12:22

bump relay record limit to 50

45df977

doc: document relay options

1d8efc3

relay: drop design comments

c580255

These are issues for another day.

Stebalien commented Apr 6, 2019

View reviewed changes

vyzo reviewed Apr 6, 2019

View reviewed changes

vyzo and others added 3 commits April 6, 2019 18:11

add missing word

fee7b01

relay: update documentation

7093262

relay: allow users to explicitly add relay addresses

87d57b5

Stebalien requested a review from vyzo April 6, 2019 15:37

vyzo approved these changes Apr 6, 2019

View reviewed changes

Stebalien merged commit a5ef3bd into master Apr 6, 2019

Stebalien deleted the fix/relay-host-stuff branch April 6, 2019 15:53

ghost removed the status/in-progress In progress label Apr 6, 2019

raulk reviewed Apr 6, 2019

View reviewed changes

raulk mentioned this pull request Apr 6, 2019

Revise the relay protocol libp2p/go-libp2p-circuit#68

Closed

relay: turn autorelay into a service and always filter out relay addresses #578

relay: turn autorelay into a service and always filter out relay addresses #578

Conversation

Stebalien commented Apr 5, 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

Stebalien commented Apr 5, 2019

Choose a reason for hiding this comment

vyzo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Apr 6, 2019

vyzo left a comment

Choose a reason for hiding this comment

vyzo left a 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

vyzo commented Apr 6, 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

Choose a reason for hiding this comment

raulk Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

raulk Apr 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Apr 6, 2019 •

edited

Loading

raulk Apr 6, 2019 •

edited

Loading