LoadBalancer for bitswap (and later, more of libp2p)

[hannahhoward]

Goals

Implement a simple libp2p load balancer for booster bitswap based on https://pl-strflt.notion.site/Simple-Libp2p-Load-Balancer-a1f1d8c182f74ea5bba4b48ef5a4992d

The design was simplified significantly based on #801

I've also made the job of configuring the load balancer much simpler -- when you start booster-bitswap for the first time, it automatically sets the load balancers configured peer id for bitswap

Implementation

• add message types as IPLD serializable go types
• add utility functions for writing various message types to streams
• implement the load balancer based on the psuedo code in the load balancer design document. Load balancer:
  • takes a set peer config of expected service nodes and the protocols they will listen on
  • when service nodes connect, activates all protocols registered for service node
  • when service nodes disconnect, deactivates all protocols registered for service node
  • handles inbound and outbound forwarding requests
  • peer routing configuration can be updated as the cost of all service nodes disconnecting and reconnected
• implement a service node based on the psuedo code in the load balancer design document. Service node handles:
  • handling inbound and outbound forwarding requests
• config for bitswap peer id in boost
  • if this is set, go ahead and list in the available retrieval protocols
• add a module to start the load balancer within boost
  • only accept routing requests currently from set bitswap peer id
• add APIs to programmatically get/set the bitswap peer id
  • when set, updates the peer config in the load balancer
• in booster bitswap -
  • on start up, load or generate a peer key (previous, generated a new one every time)
  • query the boost for the currently set bitswap peer id
  • if peer id is not set, set it to the newly generated peer id
  • if peer id is set, verify it matches booster bitswaps peer id
    • if no match, error out, unless a special override-peer-id CLI option is passed, which tells booster-bitswap to replace the bitswap peer ID in Boost config with the current booster-bitswap peer

I have verified the load balancer works round trip -- I was able to get a bitswap block from boost's main peer ID

[dirkmc]

Thanks for putting this together Hannah.

I have some general questions about the load balancer implementation:

1. Can we assume that the booster-bitswap node has the same peer ID as the load balancer? If so, then I think it simplifies things.
2. Can the load balancer register routes as soon as it gets a connection from the booster-bitswap node, and remove the routes when the connection goes down? Then we wouldn't need a lot of the messages. For example we wouldn't need the renew / termination messages and logic.

The way I'd imagine this working:

In boostd config (maybe in a LoadBalancer section) we could have

• the protocols that the routed host serves
• the multiaddr of the routed host

When the load balancer gets a connection from the routed host, it starts listening on the protocols in config.
When the load balancer gets an inbound connection over these protocols

• if there is no connection to the routed host, it does a stream reset
• if there is a connection to the routed host it just passes data through to the routed host as-is

With respect to the forwarding protocol I think the general idea makes sense 👍
I wonder if we can use the forwarding protocol to just set up the connection, instead of sending messages for every stream? For example bitswap opens a new stream every time it sends a block.

[rvagg]

is this a bit too harsh for a "remove them all please!"? should we instead terminate any that exist but also return an ErrNotRegistered if any of those didn't exist rather than bail if just one doesn't exist?

[rvagg]

were they taking too long or did you forget to uncomment?

[hannahhoward]

it would appear so

[rvagg]

maybe optional too?

[rvagg]

there's probably a case for an inline union here with a success and failure subtype, keyed by code, something like:

type RoutingResponse union {
  | RoutingResponseOk "o"
  | RoutingResponseRejected "r"
} representation inline {
  discriminantKey "c"
}

type RoutingResponseOk struct {
  Message String (rename "m")
}

type RoutingResponseRejected struct {
  Expiry DateTime
}

... having written all of that; I don't think this is supported in bindnode, maybe it's not even in the current Go schema parser! I guess there's a TODO here because the union would give you good validation on the existence of the two different properties.

[hannahhoward]

I believe that Union types are supported by the parser (https://github.com/ipld/go-ipld-prime/blob/7548eb883bda4712355797547a0628a0ad1c00cb/node/bindnode/infer.go#L221) but the implementation for bindnode in go is pretty rough -- a wrapper struct with a set of members that are all nullable for each potential type in the union. Eek! Not sure what type safety you actually get out of that at least on the go-side, but seems like there are lots of ways to make said struct fail when actually serialized. It is enough that it makes me not want to use it.

[rvagg]

another case for a union here; we should look at doing this so we have protocol level enforcement of types

[hannahhoward]

Indeed. At the moment, the bindnode go union type implementation sucks enough that I don't want to deal with it. (I believe currently it's a struct with a bunch of nullable members?)

[rvagg]

shouldn't this be after the err check?

[dirkmc]

The "data-dir" flag doesn't seem to be defined in the run command.
I'd suggest using cmd.FlagRepo for consistency with the boost client.

[dirkmc]

If I understand correctly, the load balancer

• starts listening on bitswap protocols when the bitswap node connects to it
• stops listening on bitswap protocols when the bitswap node disconnects from it

From a client's perspective, whenever they open a stream to the load balancer over bitswap they will get a "protocol not supported" error. To a client this looks like a more long-term state: "This server doesn't support bitswap".

Would it make more sense for the client to get a stream reset instead? To a client that would look like a short term error state.

[dirkmc]

With respect to the mechanism of booster-bitswap automatically registering its peer ID with the load balancer, it may cause issues to close and restart the load balancer after it's already been started.

I'd suggest instead that we add a booster-bitswap init command (similar to boost init) that creates a peer ID. Then someone who wants to add a booster-bitswap node on their SP would:

1. Run booster-bitswap init to create a peer ID. If it has already been inited, it just outputs the already created peer ID.
2. Add the bitswap peer ID to boostd's config.toml
3. Restart boostd
4. Start booster-bitswap

[hannahhoward]

re: booster-bitswap init

It seems like it would be nice for it to use the API call to copy the value directly. What's stored on disk is the private key, not exactly obvious how to direct copy it. But I agree that it's reasonable to expect a restart rather than hot reload.

[hannahhoward]

I've updated the PR so that:

• configured routes where the service node is not connected return stream reset, instead of protocol not supported
• the peer id can be configured through the booster-bitswap init command
• no longer attempting to support hot reloading.

[dirkmc]

Now that the repo flag has a default value, I don't think we need this check

[dirkmc]

Looks like the --override-peer-id flag is no longer defined

[dirkmc]

This is looking really good 👍

What's stored on disk is the private key, not exactly obvious how to direct copy it.

I don't think you need to directly copy the private key. I'd suggest that the booster-bitswap init command outputs the peer ID (which is derived from the public key). Then the user can manually set the peer ID on the boostd node.

My concern is that if there is a booster-bitswap process on a diffferent node that can silently update a config value on the boostd node that might lead to confusing situations for the user. I think it's clearer for them to explicitly update the config values manually. At the moment it's a little tricky to follow the logic in the configureRepo method. The suggested change should simplify all that.

I'd suggest that

• booster-bitswap init creates a new peer ID, or prints out the existing peer ID if one was already created (take a look at boost init that does something similar)
• booster-bitswap run fails if booster-bitswap init has not yet been run

[hannahhoward]

@dirkmc changes made and hopefully ready to go!

[dirkmc]

Suggest changing this error message to `FlagRepo.name + " is a required flag"

[dirkmc]

If the peer key is not found, I think this will output an error like "setting up libp2p host: libp2p.key not found"
I'd suggest we show something more informative, eg

if os.IsNotExist(err) {
	return nil, fmt.Errorf("booster-bitswap has not been initialized. Run the booster-bitswap init command.")
}

[dirkmc]

Nice work! 🙌

I merged the PR, and left a couple of small UI things to address in a follow-up PR.

[dirkmc]

Let's add a detailed comment about what this config value is. The comment ends up being copied into the config file itself, so it should be easy for users to understand what the value is and know what to set it to.