feat(routing): Delegated Routing #8997
Conversation
There was a problem hiding this comment.
Added some comments about where things go in the config file and the remaining fx issues. lmk what you think and others may chime in as well.
We're also missing tests here. I think using Go tests will likely make your life easier here than writing sharness tests.
This comment was marked as outdated.
This comment was marked as outdated.
ajnavarro
force-pushed
the
feature/delegated-routing
branch
3 times, most recently
from
a4f38b4
to
276ccf0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ajnavarro
force-pushed
the
feature/delegated-routing
branch
from
276ccf0
to
b0da79c
Compare
ajnavarro
force-pushed
the
feature/delegated-routing
branch
from
b0da79c
to
24d979b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
2022-06-28 conversation:
Missing items:
|
ajnavarro
force-pushed
the
feature/delegated-routing
branch
2 times, most recently
from
0eb5007
to
0fbeac3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Looking good, most of my questions are not super substantial.
Two other things are:
- We may should start writing up some info on this change in CHANGELOG.md so we don't have to get it all done right before the release. It also serves to summarize the end state of things to the reviewer since their expectations should match what the you're telling the user in the docs/changelog
- Maybe it's ok to rely on go-delegated-routing having tests for the a single reframe router and go-libp2p-routing-helpers having tests for multiple routers in use. However, there's a bunch of complexity here coming from fx and plugging all the pieces together. I think it'd be useful to have some closer to end-to-end tests (whether in sharness or Go) to exercise this a bit. Sharness might be easier given that we don't have any demo writeable-reframe servers at the moment.
- @lidel WDYT, overkill or necessary?
|
|
||
| It can contain several Router implementations, such as DHT and delegated routing based on the [reframe protocol](https://github.com/ipfs/specs/blob/master/REFRAME.md). | ||
|
|
||
| Type: `object[string->object]` |
There was a problem hiding this comment.
I copy it from similar config param, Swarm.ResourceMgr.Limits I guess it means a map of objects.
go.mod
Outdated
| @@ -127,6 +127,7 @@ require ( | |||
|
|
|||
| require ( | |||
| github.com/benbjohnson/clock v1.3.0 | |||
| github.com/ipfs/go-delegated-routing v0.2.2 | |||
There was a problem hiding this comment.
has commits missing from master, and master is failing CI.
There was a problem hiding this comment.
fix by @guseggert here: ipfs/go-delegated-routing#28. I'll update the dependency when merged.
| fx.Provide(libp2p.Routing), | ||
| fx.Provide(libp2p.ContentRouting), | ||
|
|
||
| fx.Provide(libp2p.BaseRouting(cfg.Experimental.AcceleratedDHTClient)), | ||
| fx.Provide(libp2p.DelegatedRouting(cfg.Routing.Routers)), | ||
| maybeProvide(libp2p.PubsubRouter, bcfg.getOpt("ipnsps")), |
There was a problem hiding this comment.
This is a bit of a confusing mess of functions here related to routing. Might help to have some docs explaining what's going on.
We seem to have multiple sets of tiered routers, how are you thinking they should be used? Is it one set of tiered routers for everything other than the pubsub router and then another set that is everything including the pubsub router?
There was a problem hiding this comment.
Is it one set of tiered routers for everything other than the pubsub router and then another set that is everything including the pubsub router?
Yes. First, we created a list of content-routers that will be used on pubsub router. Second, we created a list of routers using all previous routers + pubsub router.
Where/How should I add that documentation? Can you point me to some example of that on the fx code? Thx.
There was a problem hiding this comment.
I added some godoc on the new methods. Ping me if I should add something more. Thx!
config/routing.go
Outdated
| // Usually used for reframe Routers. | ||
| RouterParamAddress RouterParam = "address" | ||
|
|
||
| RouterParamPriority RouterParam = "priority" |
There was a problem hiding this comment.
@lidel idk if you have thoughts here, but I'm not sure how much we want to expose here.
Might be useful info to have to understand some notion of user preference, but also we may want to just do more in parallel depending on our concerns about resource usage.
AFAICT the only areas where priority kicks in are for the GetValue, GetPublicKey and FindPeer functions (https://github.com/libp2p/go-libp2p-routing-helpers/blob/506670d53a31503f257980dbf354b3ea1b990fb7/tiered.go#L30). GetPublicKey is basically unused, FindPeer could probably be in parallel without issue, and GetValue sort of makes sense to have prioritization (there's some weird edge-cases around IPNS records since they're mutable and you're trying to find the latest ones but don't want to hang around forever checking every system) but IIRC SearchValue is generally used anyway which has parallelism.
There was a problem hiding this comment.
@aschmahmann If we talk about future where we have DHT routers here as well, I think we need two optional settings:
priority– allows setting the order (when it matters)mandatory– allows for marking some routers as mandatory and others as best-effort
With these, we will be able to model custom rules around parallelism and best-effort/mandatory routers in the future, but for now we can skip them in config (thus optional with implicit default).
There was a problem hiding this comment.
If we add the mandatory flag we must stop using routinghelpers.TieredRouter and use some new implementation that supports that.
There was a problem hiding this comment.
ack, this needs a design first.
We can "mandatory" decision/discussion to kubo 0.15 or later.
There was a problem hiding this comment.
add an issue to track it: #9083
I think this is debatable and probably more a matter of opinion than a bug. Depending on the configuration of the multiple systems you might want it to fail if only one of the systems succeeds. This relates to some of the more complex configuration options we may want to add to the config in the future. Example 1:
Example 2:
|
|
I suspect to roll this out the main thing we need is to deal with expected vs unexpected errors. In particular, if I add a reframe router that doesn't support something like "put" it should be fine as long as it returns the correct error given https://github.com/libp2p/go-libp2p-routing-helpers/blob/506670d53a31503f257980dbf354b3ea1b990fb7/parallel.go#L142 |
@aschmahmann I only consider that to be a bug because the documentation (GoDoc) describes a behavior that is not the one happening. Any behavior is fine for me :) |
|
@ajnavarro : with the correspondence that has happened today synchronously and in chat, where are we at? What's remaining to get this landed? |
|
@ajnavarro before you start work, please fetch my changes, should save you some time:
The failing gotest was already failing before my changes, so I assume it is still work-in-progress on your end. Up to you to decide if you want to fix them or add any additional sharness tests to |
Status:
|
ajnavarro
force-pushed
the
feature/delegated-routing
branch
from
fd9bfc9
to
79ed919
Compare
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
* Delegated Routing. Implementation of Reframe specs (https://github.com/ipfs/specs/blob/master/REFRAME.md) using go-delegated-routing library. * Requested changes. * Init using op string * Separate possible ContentRouters for TopicDiscovery. If we don't do this, we have a ciclic dependency creating TieredRouter. Now we can create first all possible content routers, and after that, create Routers. * Set dht default routing type * Add tests and remove uneeded code * Add documentation. * docs: Routing.Routers * Requested changes. Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com> * Add some documentation on new fx functions. * Add changelog entry and integration tests * test: sharness for 'dht' in 'routing' commands Since 'routing' is currently the same as 'dht' (minus query command) we need to test both, that way we won't have unnoticed divergence in the default behavior. * test(sharness): delegated routing via reframe URL * Add more tests for delegated routing. * If any put operation fails, the tiered router will fail. * refactor: Routing.Routers: Parameters.Endpoint As agreed in ipfs/kubo#8997 (comment) * Try to improve CHANGELOG entry. * chore: update reframe spec link * Update go-delegated-routing dependency * Fix config error test * use new changelog format * Remove port conflict * go mod tidy * ProviderManyWrapper to ProviderMany * Update docs/changelogs/v0.14.md Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com> Co-authored-by: Marcin Rataj <lidel@lidel.org> Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
Add a way to add any kind of router, starting with Reframe routers.
Reframe routers are implementing Reframe specs (https://github.com/ipfs/specs/blob/master/REFRAME.md) using the go-delegated-routing library.
Added on this PR:
Routing.Typeis optional now (still work in progress until we add dht routes on delegated routing).Routing.Routesconfig param with documentation.dhtcommands (all but query)/routingHTTP commands endpoints:/routing/routing/put/routing/get/routing/findpeer/routing/findprovs/routing/provideroutingcommand.TieredRouterwith all routers, androuting.ContentRoutings, needed to generate a TieredContentRouting, used for Topic Discovery. All Delegated routers will be added to ContentRouters too.github.com/ipfs/go-delegated-routingasrouting.Routingimplementations. Maybe we should do it in the library itself.Things to do in next iterations:
DHTrouting https://github.com/ipfs/go-ipfs/pull/8997/files#diff-ca7712f58faead361a24337520fcbf4c663a4fe6076f6505d92471f67249b3b1R76. (Routing: conform DHT to common Router interface #9079)Methodsfield onRouting.Routesconfig param.