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

New node methods for tools and otherwise #265

Merged
merged 16 commits into from
Mar 5, 2019
Merged

Conversation

anacrolix
Copy link
Contributor

Adds several new methods, SetClientMode, to enter client mode after initialization, PeerId and PeerKey, and BootstrapSelf and BootstrapRandom. @ZenGround0 @jhiesey. All are used by https://github.com/anacrolix/go-libp2p-dht-tool and provide finer-grained access to node functionality.

@anacrolix anacrolix requested a review from raulk February 15, 2019 04:03
@ghost ghost assigned anacrolix Feb 15, 2019
@ghost ghost added the status/in-progress In progress label Feb 15, 2019
dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
dht_bootstrap.go Show resolved Hide resolved
@raulk
Copy link
Member

raulk commented Feb 15, 2019

@ZenGround0 @jhiesey clarification re: https://github.com/anacrolix/go-libp2p-dht-tool, note this tool fires up a separate libp2p host, which is not what we want. We want to call diagnostics, metrics and tracing on a running application.

But the APIs @anacrolix is introducing here and in subsequent PRs will enable Filecoin and other apps to extract these insights and offer them through porcelain in the form you prefer.

@raulk
Copy link
Member

raulk commented Feb 17, 2019

@anacrolix – looking good here. I just feel strongly about the mode switching enum as I see more modes coming soon (see comment thread). Once that and the unit test are sorted, I'm happy to approve and merge this PR.

@anacrolix
Copy link
Contributor Author

I don't foresee any other modes, but I can see that SetClientMode(bool) would mirror opts.Client(bool). That's yet more tests for something I don't want to use. Do you want me to add that?

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs fixing: Missing gx dependencies for xerrors and testify.

Re: SetClientMode discussion, we will likely have feature flags for DHT subsystems (content routing, peer routing, provider records), and SetClientMode can serve as a global switch.

@raulk
Copy link
Member

raulk commented Feb 26, 2019

Seeing that the requirement has now arisen for more granular knobs to turn specific features on/off in #274, my suggestion is as follows.

  1. Add a ClientOnlyModes config struct in the opts package with four bool fields: ContentRouting, PeerRouting, ValueStore, KeyStore. Zero value (false) means all features run in server mode (default). A true value means the feature runs in client only mode.
  2. Add an Option so that users can pass in a ClientOnlyModes object when constructing the DHT.
  3. Use this struct to enable/disable handlers.
  4. Remove SetClientMode in favour of setting the modes at construction (more correct).

@jhiesey
Copy link

jhiesey commented Feb 27, 2019

@raulk your proposal assumes we won't want to dynamically change a node from client to server or vice versa. I am not sure what the real requirements are, but I'd guess we want both an option to configure at construction time and dynamically.

As the daemon becomes more of a focus I think we're going to want as much as possible to be changed dynamically. Is there an idiomatic way of doing "reactive configuration" in go? By that I mean configuration that can be set either on construction or dynamically, with listeners to handle the change? Then ClientOnlyModes, and possibly other things, could be updated later without adding a bunch more methods ad hoc.

@raulk
Copy link
Member

raulk commented Feb 27, 2019

@jhiesey

your proposal assumes we won't want to dynamically change a node from client to server or vice versa.

I was tempted to model runtime mode changes, but intentionally kept it simple for now as this requirement hasn't really surfaced anywhere yet. In fact, in prior feedback to @anacrolix I argued that SetClientMode() creates asymmetry as you cannot revert back to server mode.

+1 on reactive configuration. In the Java world, netflix/archaius was a clever solution. In Go land I found:
https://github.com/spf13/viper
https://github.com/micro/go-config

Could you add your thoughts on this issue about system-wide configuration trees? libp2p/go-libp2p#526

Also, I propose you take this feature into account in your DHT rewrite proposal ;-)

@anacrolix
Copy link
Contributor Author

I only need the ability to dynamically downgrade the inbound message handling to be passive at the moment. Handling specific subsystems dynamically is well beyond the scope of this PR (not to mention orthogonal to the way I'm using "client" here). The project is now version 0, consider this an experimental API until the dust clears on other stuff, I'd like to just move forward.

@anacrolix
Copy link
Contributor Author

I'm not able to push xerrors to gx for some reason. The working repo is here: https://github.com/anacrolix/xerrors. Also whyrusleeping/gx#230. I've removed the SetClientMode, the workaround is here: anacrolix/go-libp2p-dht-tool@4c656e8.

@jhiesey
Copy link

jhiesey commented Feb 28, 2019

If a node is running in client-only mode, doesn't it need to indicate that in the RPCs it sends, so that the receivers of its FIND_NODE/FIND_PROVIDERS/etc. messages don't add the client to their routing tables?

If there is nothing doing that, then the client-only peer will be cluttering up other nodes' routing tables even though it won't answer queries.

@anacrolix
Copy link
Contributor Author

@jhiesey Yes it does in the BitTorrent DHT. It's not a hard requirement (since it will get evicted for not responding eventually anyway, and won't be able to barge into a correctly implemented routing table), but it's a nice to have. It's also out of scope for this PR. I've removed the SetClientMode method now anyway.

@anacrolix
Copy link
Contributor Author

This is ready for merging.

Copy link

@jhiesey jhiesey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, few nits on how the new public getters are used inconsistently.

Personally I'd perfer to fix the client-only routing table pollution issue (#281) before merging client-only mode (changed my mind on that), but it's not absolutely essential. @raulk what do you think?

dht_test.go Show resolved Hide resolved
dht.go Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
@anacrolix anacrolix requested review from jhiesey and raulk March 1, 2019 10:03
dht_net.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member

raulk commented Mar 3, 2019

@jhiesey client mode was already there. And I agree that client-only nodes should signal so, to avoid being included in routing tables, as they are useless. How about we merge this and we address #281 separately? Are you happy to submit a patch for that?

@anacrolix anacrolix requested a review from raulk March 3, 2019 23:08
@anacrolix anacrolix merged commit 08c34b4 into master Mar 5, 2019
@ghost ghost removed the status/in-progress In progress label Mar 5, 2019
@anacrolix anacrolix deleted the dht-tool-methods branch March 7, 2019 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants