feat(routing): Delegated Routing #8997

ajnavarro · 2022-05-27T13:53:55Z

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.Type is optional now (still work in progress until we add dht routes on delegated routing).
New Routing.Routes config param with documentation.
Deprecate dht commands (all but query)
New /routing HTTP commands endpoints:
- /routing
- /routing/put
- /routing/get
- /routing/findpeer
- /routing/findprovs
- /routing/provide
New routing command.
Routing system is split into two parts: TieredRouter with all routers, and routing.ContentRoutings, needed to generate a Tiered ContentRouting, used for Topic Discovery. All Delegated routers will be added to ContentRouters too.
Added a wrapper to be able to use github.com/ipfs/go-delegated-routing as routing.Routing implementations. Maybe we should do it in the library itself.

Things to do in next iterations:

Implement delegated DHT routing https://github.com/ipfs/go-ipfs/pull/8997/files#diff-ca7712f58faead361a24337520fcbf4c663a4fe6076f6505d92471f67249b3b1R76. (Routing: conform DHT to common Router interface #9079)
Add Methods field on Routing.Routes config param.

config/addresses.go

core/node/libp2p/routing.go

aschmahmann

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.

config/routing.go

docs/config.md

config/routing.go

core/commands/routing.go

docs/config.md

core/commands/routing.go

repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go

BigLep · 2022-06-28T14:38:37Z

2022-06-28 conversation:

Scope reduction: DHT configuration will have a migration in 0.15
Scope reduction: fewer options for what you can do with reframe (e.g., limiting the number of methods)

Missing items:

Add tests
Docs
Get all the tests pass (FX issues which @guseggert will help with)

aschmahmann

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?

aschmahmann · 2022-07-01T04:49:04Z

docs/config.md

+
+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]`


What does this mean?

I copy it from similar config param, Swarm.ResourceMgr.Limits I guess it means a map of objects.

docs/config.md

aschmahmann · 2022-07-01T04:51:20Z

go.mod

@@ -127,6 +127,7 @@ require (

 require (
 	github.com/benbjohnson/clock v1.3.0
+	github.com/ipfs/go-delegated-routing v0.2.2


has commits missing from master, and master is failing CI.

fix by @guseggert here: ipfs/go-delegated-routing#28. I'll update the dependency when merged.

routing/delegated.go

routing/wrapper.go

aschmahmann · 2022-07-01T09:26:24Z

core/node/groups.go

 		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")),


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?

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.

I added some godoc on the new methods. Ping me if I should add something more. Thx!

aschmahmann · 2022-07-01T09:38:23Z

config/routing.go

+	// Usually used for reframe Routers.
+	RouterParamAddress RouterParam = "address"
+
+	RouterParamPriority RouterParam = "priority"


@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.

@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).

If we add the mandatory flag we must stop using routinghelpers.TieredRouter and use some new implementation that supports that.

ack, this needs a design first.
We can "mandatory" decision/discussion to kubo 0.15 or later.

add an issue to track it: #9083

docs/config.md

routing/delegated_test.go

config/routing.go

aschmahmann · 2022-07-06T15:19:04Z

I found a new error on go-libp2p-routing-helpers. When we put a new value, TieredRouter should succeed if at least one Router added the value, but the logic is wrong here

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:

I want to do a put to the IPFS Public DHT and to a centralized database used by my application
If I only send the put to the centralized database and call that a success that's not great since it means users who don't have my centralized database configured won't see my put

Example 2:

I want to use a service that I delegate my routing requests to but fallback to the DHT if that fails
If I only send the put to the delegated routing service I'm ok calling that a success since it's supposed to do the put to the DHT on my behalf

aschmahmann · 2022-07-06T15:23:35Z

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

ajnavarro · 2022-07-06T15:40:07Z

I think this is debatable and probably more a matter of opinion than a bug.

@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 :)

CHANGELOG.md

BigLep · 2022-07-06T19:36:30Z

@ajnavarro : with the correspondence that has happened today synchronously and in chat, where are we at? What's remaining to get this landed?

As agreed in ipfs#8997 (comment)

lidel · 2022-07-06T23:29:20Z

@ajnavarro before you start work, please fetch my changes, should save you some time:

I've resolved conflicts that were caused by the repo rename and related refactors (refactor: rename to kubo #8958+fix: follow-up after repository rename #9098)
Renamed Reframe router's "address" to "Endpoint" as agreed here (2f57adf)

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 test/sharness/t0701-delegated-routing-reframe.sh (fwwiw we already have basic end-to-end finprovs test there).

ajnavarro · 2022-07-07T08:59:55Z

@ajnavarro : with the correspondence that has happened today synchronously and in chat, where are we at? What's remaining to get this landed?

Status:

Error on go-delegated-routing fixed and with PR: Fix: Handle correctly keys on Get/Put Value and Get/Put IPNS. go-delegated-routing#31, still on review.
Error from go-libp2p-routing-helpers might be considered a GoDoc misprint instead of a bug.
Tests that need Fix: Handle correctly keys on Get/Put Value and Get/Put IPNS. go-delegated-routing#31 will still fail until we merge that PR and update the dependency.
Thanks @lidel for the rebase with all the renaming changes!

lidel

nit: REFRAME spec link changed :^)

CHANGELOG.md

config/routing.go

docs/config.md

docs/changelogs/v0.14.md

core/node/provider.go

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>

ajnavarro requested review from guseggert and aschmahmann May 27, 2022 13:53

ajnavarro changed the title ~~Delegated Routing~~ feat(routing): Delegated Routing May 27, 2022

aschmahmann reviewed May 27, 2022

View reviewed changes

config/addresses.go Outdated Show resolved Hide resolved

aschmahmann reviewed May 27, 2022

View reviewed changes

core/node/libp2p/routing.go Show resolved Hide resolved

aschmahmann requested changes May 27, 2022

View reviewed changes