-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: Prevent Routing.Type=auto from enabling dhtserver mode if have too low of limits #9561
Conversation
39ba963
to
635efcb
Compare
…oo low of limits Closes #9548 Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
635efcb
to
3aa60c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks good to me 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's other changes going on here than I would have expected but I assume that's because you're pulling in changes from #9555 ? Can you just base on top of that so your diff is clearer?
cmd/ipfs/daemon.go
Outdated
// Check limits to see if we have enough to run DHT as a server. | ||
var sysConnInbound int | ||
var sysStreamInbound int | ||
var dhtStreamsInbound int | ||
if cfg.Swarm.ResourceMgr.Limits != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right (but maybe I have it wrong).
We want to look at the limits that were set on the resource manager (it's it's enabled). The limits that are set is the combination of computed defaults and user-supplied overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a conversation with @Jorropo:
Jorropo: "just" reading the *config.Config object and select the DHT
Antonio: the config is not giving you the final limits
Jorropo: the scalled limits are always good
So I'm just reading the custom resource manager configuration added by the user as the result.
This validation should be done only if resourceManager is active, adding that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch routingOption { | ||
case routingOptionSupernodeKwd: | ||
return errors.New("supernode routing was never fully implemented and has been removed") | ||
case routingOptionDefaultKwd, routingOptionAutoKwd: | ||
mode := dht.ModeAuto | ||
if (sysConnInbound != 0 && sysConnInbound <= config.DefaultResourceMgrMinInboundConns) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not let the "zero" weirdness here in here. When we're looking at the actual limits that the resource manager is using, zero means zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are limits coming from the configuration, so zero means not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't really do better without libp2p/go-libp2p#2000 yet.
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Per 2023-01-19 standup, we're going to focus on doing this right rather than rushing something. This means having a way to authoritatively know what limits the resource manager is actually configured with. |
Closing because we are changing how RM is used (limiting by memory and FDs) so this is now a low priority. #9580 |
Depends on #9555
Fixes #9548