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

ResourceMgr UX improvements #8858

Closed
4 tasks
lidel opened this issue Apr 7, 2022 · 11 comments · Fixed by #8901
Closed
4 tasks

ResourceMgr UX improvements #8858

lidel opened this issue Apr 7, 2022 · 11 comments · Fixed by #8901
Assignees
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress topic/libp2p Topic libp2p
Milestone

Comments

@lidel
Copy link
Member

lidel commented Apr 7, 2022

Description

Part of #8761 (extracted tasks from #8680)

This is a grab-bag for any UX work or open questions related to

  • Swarm.ResourceMgr config
  • ipfs swarm limit|stats CLI commands

👉 Not urgent, but we should figure these out before we ship ResourceMgr enabled by default.

TODO

  • Swarm.ResourceMgr.Limits config should allow users to persist rules across restarts
    • the format should be compatible with the output of ipfs swarm limit
    • see if we can be also compatible with limit.json used by lotus/go-libp2p team
  • Add ipfs swarm limit -s that persists limit into Swarm.ResourceMgr.Limits config
  • TBD do we need Optional configuration for memory-adjusted defaults?
  • TBD there are limits for "peer" "service" and we have related scopes, but we don't have scope for "servicepeer" limits (need to add `svcpeer" ? anything else missing?)
@lidel lidel added kind/enhancement A net-new feature or improvement to an existing feature topic/libp2p Topic libp2p labels Apr 7, 2022
@lidel lidel added this to the go-ipfs 0.13 milestone Apr 7, 2022
@lidel
Copy link
Member Author

lidel commented Apr 8, 2022

Additional thoughts on this:

  • simplify: make the outputs of ipfs swarm limit compatible with that we expect in Swarm.ResourceMgr.Limits, and with limit.json too
    • I already created the config.ResourceMgrLimitsConfig struct which is how limit.json from lib2p team looks like
    • ipfs swarm limit could operate on the same struct, making it easy to apply limit.json from libp2p via ipfs swarm limit -s limit.json.
      • This way, we can remove support for $IPFS_PATH/limit.json from core/node/libp2p/rcmgr.go

@guseggert guseggert modified the milestones: go-ipfs 0.13, go-ipfs 0.14 Apr 8, 2022
@guseggert guseggert self-assigned this Apr 8, 2022
@guseggert guseggert modified the milestones: go-ipfs 0.14, go-ipfs 0.13 Apr 8, 2022
@guseggert
Copy link
Contributor

What's the use case for not persisting limit changes by default?

@lidel
Copy link
Member Author

lidel commented Apr 11, 2022

@guseggert

  • official: testing new limit before fully committing to it (if you mess up, you can restart)
  • unofficial :trollface:: aligning with other commands like ipfs swarm peering add (last time i checked it did not persist to config, but that could just be a bug?)

Personally, I would always persist to config. Removes surprises. If you feel the same way, just do that.

@guseggert
Copy link
Contributor

Yeah I like just persisting by default, and we can always add a flag later not to if we need it.

@lidel lidel added the status/in-progress In progress label Apr 15, 2022
@guseggert
Copy link
Contributor

Fixed libp2p/go-libp2p-resource-manager#19 as part of this

@guseggert
Copy link
Contributor

Once constructed, the existing RM limiter can't distinguish between default limits and configured limits, so persisting changes means we'd persist defaults too, which isn't what we want, since it will silently "freeze" the defaults and prevent new defaults from being applied. This is complicated by the fact that we want the limits in both the IPFS config and in a limit.json in the IPFS_PATH, and also due to RM itself not caring what concrete limiter is used (while we do care, because we are configuring a concrete limiter). Currently I'm trying to work out how to make changes, ideally to go-libp2p-resource-manager, to support these use cases.

@aschmahmann
Copy link
Contributor

If there's a bunch of complexity here that looks application specific around defaults it might be worth just handling things in the go-ipfs config file and not bothering with limit.json. go-libp2p doesn't really have other examples of this config file business with defaults and such, so if making changes in go-libp2p-resource-manager is a problem we should be able to handle it here.

That being said I wouldn't be surprised if other go-libp2p applications with config files (like lotus) might want (or maybe already have?) something similar so doing this in go-libp2p-resource-manager might make sense if it's not a pain.

@guseggert
Copy link
Contributor

Yeah that's the idea, might as well do it in a way that can be used by other folks too.

@guseggert
Copy link
Contributor

guseggert commented Apr 21, 2022

(need to add `svcpeer" ? anything else missing?)

There's also "protopeer" and "transientpeer". These aren't going to be easy to add, they will require changes to resource manager.

@guseggert
Copy link
Contributor

We decided in standup today to run with the suboptimal UX, because fixing it is too time-consuming.

I've impl'd that in this PR: #8901

@guseggert
Copy link
Contributor

guseggert commented Apr 21, 2022

Also exported the config schema from resource manager so we can reuse it in go-ipfs and released RM v0.3.0 here: libp2p/go-libp2p-resource-manager#21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress topic/libp2p Topic libp2p
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants