Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Routing: conform DHT to common Router interface #9079

Closed
1 of 5 tasks
ajnavarro opened this issue Jul 4, 2022 · 12 comments · Fixed by #9274
Closed
1 of 5 tasks

Routing: conform DHT to common Router interface #9079

ajnavarro opened this issue Jul 4, 2022 · 12 comments · Fixed by #9274
Assignees
Labels
kind/feature A new feature

Comments

@ajnavarro
Copy link
Member

ajnavarro commented Jul 4, 2022

Description

Delegated routing implementation (#8997) currently only supports reframe routers. Implement DHT routers so they can be added as a new type on delegated routing config list. Actual DHT config initialization and behavior must work as before.

New design document here: https://hackmd.io/@ajnavarro/HJr059wC9

  • Config documentation: Add a new routing type and explain different possible parameters
  • Tests: add new tests on delegatred_test.go
  • Deprecate any old configuration param used to create the DHT (previous, simpler configuration will not be deprecated)
  • Support any DHT kind that is already supported today (including experimental DHT clients)
  • Use special routers to support the different router behaviors (New Routing helpers: parallel and sequential. libp2p/go-libp2p-routing-helpers#56)
@BigLep
Copy link
Contributor

BigLep commented Jul 25, 2022

@ajnavarro : Is this really about "delegated routing", or about having all our routing options (ipfs/go-delegated-routing and libp2p/go-libp2p-kad-dht) conforming to common pattern/interface?

@ajnavarro
Copy link
Member Author

It's the latter, having all our routing options conforming to a common pattern.

@BigLep BigLep changed the title Delegated Routing: Implement DHT Routing: conform DHT to common Router interface Jul 26, 2022
@BigLep BigLep moved this to 🥞 Todo in IPFS Shipyard Team Jul 26, 2022
@ajnavarro
Copy link
Member Author

Example of delegated routing configuration with DHT router:

{
  "Type": "reframe",
  "Enabled": true,
  "Parameters": {
    "Endpoint": "https://cid.contact/reframe",
    "Priority": 100
  }
},
{
  "Type": "dht",
  "Enabled": true,
  "Parameters": {
    "Priority": 200,
    "DHTMode": "dhtclient", # It can also be "dht", "dhtserver" and "none" too. "dht" by default.
    "TrackFullNetwork": false, # experimental dht
    "Concurrency": 10 # we can expose internal configuration params to tweak DHTs.
    "Prefix": "/ipfs" # shall we expose used prefix?
  }
}

@ajnavarro
Copy link
Member Author

ajnavarro commented Aug 2, 2022

I'm having some problems refactoring how DHT is created on Kubo. The main DHT Router is created at the same time than Host here:

opts = append(opts, libp2p.Routing(func(h host.Host) (routing.PeerRouting, error) {
r, err := params.RoutingOption(
ctx, h,
params.Repo.Datastore(),
params.Validator,
bootstrappers...,
)
out.Routing = r
return r, err
}))

It uses RoutingOption and a list of libp2p.Option.

I feel that everything is more complicated than it should be, so I'm tempted to refactor all DHT-related code to make it easier to update in the future. Maybe we don't want to spend time on this, so @lidel @guseggert @aschmahmann @Jorropo I would love to have some feedback about the following possible solutions:

  1. Refactor all code related to RoutingOption to make it simpler. The previous DHT configuration will still work as before. New DHT routers created by the new configuration will use global libp2p.Options like ForceReachability and ConnectionManager
  2. Leave the main DHT as it is. Make it possible to add more DHT Routers apart from the main DHT. AFAIK the only part that is making direct use of the DHT is AutoRelayFeeder. We can add a configuration flag that will make that DHT to be used on AutoRelayFeeder if needed.
  3. Other proposed solutions.

WDYT?

@ajnavarro ajnavarro self-assigned this Aug 2, 2022
@ajnavarro
Copy link
Member Author

ajnavarro commented Aug 4, 2022

As we discussed, I will make a comparison between config files before, and after the change (I'll remove the irrelevant parts to have less noise):

Before:

Config file
{
  "Identity": {
    [...]
  },
  "Datastore": {
    [...]
  },
  "Addresses": {
    "Swarm": [
      "/ip4/0.0.0.0/tcp/4001",
      "/ip6/::/tcp/4001",
      "/ip4/0.0.0.0/udp/4001/quic",
      "/ip6/::/udp/4001/quic"
    ],
    "Announce": [],
    "AppendAnnounce": [],
    "NoAnnounce": [],
    "API": "/ip4/127.0.0.1/tcp/5001",
    "Gateway": "/ip4/127.0.0.1/tcp/8080"
  },
  "Discovery": {
    "MDNS": {
      "Enabled": true
    }
  },
  "Routing": {
    "Type": "dht",
    "Routers": {}
  },
  "Bootstrap": [
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmQCU2EcMqAqQPR2i9bChDtGNJchTbq5TbXJJ16u19uLTa",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt",
    "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ",
    "/ip4/104.131.131.82/udp/4001/quic/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"
  ],
  "Gateway": {
    [...]
  },
  "API": {
    "HTTPHeaders": {}
  },
  "Swarm": {
    "AddrFilters": null,
    "DisableBandwidthMetrics": false,
    "DisableNatPortMap": false,
    "RelayClient": {},
    "RelayService": {},
    "Transports": {
      "Network": {},
      "Security": {},
      "Multiplexers": {}
    },
    "ConnMgr": {
      "Type": "basic",
      "LowWater": 600,
      "HighWater": 900,
      "GracePeriod": "20s"
    },
    "ResourceMgr": {}
  },
  "AutoNAT": {},
  "Pubsub": {
    "Router": "",
    "DisableSigning": false
  },
  "Peering": {
    "Peers": null
  },
  "DNS": {
    "Resolvers": {}
  },
  "Migration": {
    [...]
  },
  "Provider": {
    "Strategy": ""
  },
  "Reprovider": {
    [...]
  },
  "Experimental": {
    [...]
    "AcceleratedDHTClient": false
  },
  "Plugins": {
    [...]
  },
  "Pinning": {
    [...]
  },
  "Internal": {}
}
  • Bootstrap is generic for all DHTs
  • Routing.Type is where dht is specified.
  • AcceleratedDHTClient is on Experimental flags.

After:

Config file
{
  "Identity": {
    [...]
  },
  "Datastore": {
    [...]
  },
  "Addresses": {
    "Swarm": [
      "/ip4/0.0.0.0/tcp/4001",
      "/ip6/::/tcp/4001",
      "/ip4/0.0.0.0/udp/4001/quic",
      "/ip6/::/udp/4001/quic"
    ],
    "Announce": [],
    "AppendAnnounce": [],
    "NoAnnounce": [],
    "API": "/ip4/127.0.0.1/tcp/5001",
    "Gateway": "/ip4/127.0.0.1/tcp/8080"
  },
  "Discovery": {
    "MDNS": {
      "Enabled": true
    }
  },
  "Routing": {
    "Routers": {
	 "storetheindex": {
	    "Type": "reframe",
	    "Enabled": true,
	    "Parameters": {
	      "Endpoint": "https://cid.contact/reframe",
	      "Priority": 100
	    }
	  },
	 "ipfs-public-dht" : {
	 	"Type" : "dht"
		"Enabled" : "true"
		"Methods" : ["put-ipns", "get-p2p-providers", ...]
		"Parameters" : {
			"Mode" : "auto"
			"TrackFullNetworkDHT": true,
			"ProtocolID" : "/ipfs/kad/1.0.0"
			"Bootstrappers" : [
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmQCU2EcMqAqQPR2i9bChDtGNJchTbq5TbXJJ16u19uLTa",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb",
    "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt",
    "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ",
    "/ip4/104.131.131.82/udp/4001/quic/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"
  ]
				"Public-IP-Network" : "true"
				}
		}
		"ipfs-lan-dht" : {
				"Type" : "dht"
				"Enabled" : "false"
				"Methods" : ["put-ipns", "get-p2p-providers", ...]
				"Parameters" : {
					"Mode" : "server"
					"ProtocolID" : "/ipfs/lan/kad/1.0.0"
					"Bootstrappers" : []
					"Public-IP-Network" : "false"
				}
		}
	}
  },
  "Gateway": {
    [...]
  },
  "API": {
    "HTTPHeaders": {}
  },
  "Swarm": {
    "AddrFilters": null,
    "DisableBandwidthMetrics": false,
    "DisableNatPortMap": false,
    "RelayClient": {},
    "RelayService": {},
    "Transports": {
      "Network": {},
      "Security": {},
      "Multiplexers": {}
    },
    "ConnMgr": {
      "Type": "basic",
      "LowWater": 600,
      "HighWater": 900,
      "GracePeriod": "20s"
    },
    "ResourceMgr": {}
  },
  "AutoNAT": {},
  "Pubsub": {
    "Router": "",
    "DisableSigning": false
  },
  "Peering": {
    "Peers": null
  },
  "DNS": {
    "Resolvers": {}
  },
  "Migration": {
    [...]
  },
  "Provider": {
    "Strategy": ""
  },
  "Reprovider": {
    [...]
  },
  "Experimental": {
    [...]
  },
  "Plugins": {
    [...]
  },
  "Pinning": {
    [...]
  },
  "Internal": {}
}
  • Global Bootstrap config param disappear.
  • Routing.Type is removed.
  • AcceleratedDHTClient is replaced by TrackFullNetworkDHT

Steps:

  • Refactor how DHT is created and how it interacts with other modules to make it possible to create several ones.
  • Add a new default config that works as before, adding the WAN and LAN DHTs on Routers config.
  • Add tests for everything (wrong config params validation, new default config === old default config...)
    (2 days - getting tests to pass)
  • Create a migration. I need to familiarize with migrations repo
  • Test migration
    (challenge here is that need to make sure don't screw anything u. Really need to scrutinize for corner cases. Not so bad here because we're doing config file migration. 1 week)
    (we have other stuff we want to include in migration code)

@BigLep BigLep moved this from 🥞 Todo to 🏃‍♀️ In Progress in IPFS Shipyard Team Aug 4, 2022
@BigLep
Copy link
Contributor

BigLep commented Aug 10, 2022

@ajnavarro : I'm not going to be able to engage thoroughly here, but wanted to drop a couple of thoughts:

  1. Either here or somewhere else, I want us to be clear about the configuration we expect bifrost/ipfs.io gateway to use since that is the main usecase we're focused on.
  2. Why doesn't "storetheindex" have "Methods"?
  3. What's the full definition of supported "Methods"

@BigLep
Copy link
Contributor

BigLep commented Aug 10, 2022

@ajnavarro : As I was looking at this and thinking about #9083, I was wondering if we should generalize a bit more here.

This should have more design but throwing this out as a starting point. Feel free to disregard if it's not useful, not moving in the right direction, is overcomplicating, etc.

Basically I'm wondering if our routing configuration should look more like:

"Routing": {
  "Routers": {
    "storetheindex": {...}, # what you already had
    "ipfs-public-dht" : {...}, # what you already had
    "ipfs-lan-dht" : {...}, # what you already had
    "peer-routing" : {
      "Type" : "bitswap-peer", 
      "Enabled" : true,
      "Parameters" : {...}
    },
    "parallel-get-p2p-providers" : {
      "Type" : "parallel", # A special router we create that can encapsulate other Routers
      "Parameters" : {
        "Routers" : [
          {
            "Name" : "storetheindex",
            "TimeoutMs" : 200, # Optional - can rely on the outer timeout as well.  Not sure how useful this is.  
          }, {
            "Name" : "ipfs-public-dht", 
          }
        ],
        "TimeoutMs" : "500",
        "ReturnStrategy" : "BestEffort" 
                           # This could be one of
                           # "First": return the response from the router whose response we get first
                           # "BestEffort": Combine all the results we get by the outer timeout value.
                           # "AllRequired": Only return a result within the timeout if we get a response from all the routers.  Responses are merged.  
      }
    },
    "get-p2p-providers-chain" : {
      "Type" : "parallel",
      "Parameters" : {
        "Routers" : [
          {
            "Name" : "peer-routing",
            "DelayMs" : 0
          }, {
            "Name" : "parallel-get-p2p-providers",
            "DelayMs" : 500 # Wait 500ms before starting this.  If we get a response from peer-routing sooner, we'll have already returned because of the "First" return strategy.
          },
        ],
        "TimeoutMs" : 2000,
        "ReturnStrategy" : "First",
      }
    }

  },
  "MethodToRouterMap" : {
    "put-ipns" : "ipfs-public-dht",
    "get-p2p-providers" : "get-p2p-providers-chain",
    ... # There should be a mapping from every method to a router
  }
}

@ajnavarro
Copy link
Member Author

ajnavarro commented Aug 11, 2022

@BigLep so the main idea is to create a special router that can encapsulate other routers defining how and when we call them.
Main problems I can see here:

  • How do we call root Routers? All in parallel? Shall we create a Routers list and also a RoutingStrategies list? this will allow us to compose Callers using Routers.
  • Do we apply the same strategy to all Router methods? maybe that won't make sense.
  • Where should we limit methods that will be called per router? I think we should move that to the Strategy definition, because depending on the strategy, maybe we want to use some methods or others.

@BigLep
Copy link
Contributor

BigLep commented Aug 11, 2022

@ajnavarro : I'll quickly engage to try and get the idea across. Please don't take this as direction for what we have to go.

My idea here is that we should have a map of method to router to use for that method (MethodToRouterMap). This map should define the router for every method. The router for a method can be a "base" router that actually makes network calls (e.g., "store-the-index", "ipfs-public-dht" in my example config) or it can be a composite router like "get-p2p-providers-chain" (which itself is composed of a "base" router "peer-routing" and a composite router "parallel-get-p2p-providers"). I think a picture would help here a lot.

Do we apply the same strategy to all Router methods? maybe that won't make sense.

The "strategy" used for a method depends on the router that is specified for that method. For a specific method, one can create a new router composing a different "base" router and strategy if needed.

Where should we limit methods that will be called per router?

I think that lives in MethodToRouterMap. If a router isn't listed for a given method, then it won't be invoked for that method. This would get rid of the "Methods" map key you have in #9079 (comment).

@BigLep
Copy link
Contributor

BigLep commented Aug 12, 2022

@ajnavarro : concerning the done criteria for this issue, it doesn't specify how multiple routers will be combined in terms of execution ordering and result merging. Will this use the "Routers Wrapper" mentioned in #9083 (comment) ?

@BigLep
Copy link
Contributor

BigLep commented Aug 12, 2022

Will this use the "Routers Wrapper" mentioned in #9083 (comment) ?

Per 2022-08-12 conversation, yes, this work is using https://github.com/libp2p/go-libp2p-routing-helpers

@BigLep
Copy link
Contributor

BigLep commented Aug 12, 2022

@ajnavarro : per 2022-08-12 conversation, here's the issue about designing comprehensive routing config: #9188 . We're going to pause this current work to do some design to make sure we're aligned on our end state.

@BigLep BigLep moved this from 🏃‍♀️ In Progress to 🛑 Blocked in IPFS Shipyard Team Aug 12, 2022
@ajnavarro ajnavarro moved this from 🛑 Blocked to 🥞 Todo in IPFS Shipyard Team Aug 22, 2022
@ajnavarro ajnavarro moved this from 🥞 Todo to 🏃‍♀️ In Progress in IPFS Shipyard Team Sep 12, 2022
Repository owner moved this from 🏃‍♀️ In Progress to 🎉 Done in IPFS Shipyard Team Sep 22, 2022
ajnavarro added a commit that referenced this issue Sep 22, 2022
New multi-router configuration system based on https://hackmd.io/G1KRDEX5T3qyfoBMkIrBew#Methods

- Added a new routing type: "custom"
- Added specific struct types for different Routers (instead of map[string]interface{})
- Added `Duration` config type, to make easier time string parsing
- Added config documentation.
- Use the latest go-delegated-routing library version with GET support.
- Added changelog notes for this feature.

It:
- closes #9157
- closes #9079
- closes #9186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A new feature
Projects
No open projects
Archived in project
2 participants