-
-
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
feat: go-libp2p v0.21 (rcmgr auto scaling) #9074
Conversation
}) | ||
return result, err | ||
|
||
return result, mgr.ViewTransient(func(s network.ResourceScope) error { return getLimit(s) }) | ||
case strings.HasPrefix(scope, config.ResourceMgrServiceScopePrefix): |
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.
Do we have to worry about spans here?
core/node/libp2p/rcmgr_defaults.go
Outdated
defaultLimits.SystemBaseLimit.Conns = defaultLimits.SystemBaseLimit.ConnsOutbound + defaultLimits.SystemBaseLimit.ConnsInbound | ||
|
||
return defaultLimits | ||
// if cfg.ConnMgr.Type == "basic" { |
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.
This was weird. I would expect the resource manager be the ultimate arbiter, not connmgr.
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.
We need something analogous though. The connection manager limit needs to be in sync with resource manager limit.
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.
If these two are out of sync which should be the source of truth?
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.
Probably the rcmgr, just because it's configuration is so much more complex. If I remember correctly, there were plans to deprecate the connmgr watermark configuration options in go-ipfs.
31c3eaa
to
462224d
Compare
libp2p.SetDefaultServiceLimits(testLimiter) | ||
l := rcmgr.DefaultLimits | ||
libp2p.SetDefaultServiceLimits(&l) | ||
limits := l.AutoScale() |
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.
Note: AutoScale
dedicates 1/8 of the system memory to libp2p (which effects how many conns / streams / memory etc.) will be allowed. This might be too little for a standalone go-ipfs node, and too much for IPFS Desktop / Brave.
@lidel Do you have any "profile" setting that we can use here?
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.
We do have a server
profile which is used for adjusting specific config values as one-time change against Kubo config.
I think it is ok to go with AutoScale and revisit profile-specific defaults when in separate PR, as part of work towards enabling ResourceMgr by default (tracked in #8761)
Running this here: https://ipfs.marcopolo.io/d/MgmGIjjnk/resource-manager?orgId=1 |
There are two failures related to the new base256 emoji multibase: https://app.circleci.com/pipelines/github/ipfs/go-ipfs/7123/workflows/c0a0abf6-7f82-4965-8004-1413388e36fa/jobs/77371?invite=true#step-111-13053 I'm going to assign these bugs to @Jorropo since he added that multibase and the fix isn't super trivial since it involves updating some assumptions we've made in the sharness tests. |
c9887be
to
2484abb
Compare
rebased on v0.14.0 After tests pass I'll ask bifrost to deploy this on bank4 for smoke testing |
ebcda79
to
c94c60a
Compare
We should merge libp2p/go-libp2p-kad-dht#784 and make a new release first |
I randomly picked @aschmahmann for a reviewer here, but feel free to reassign. |
dd0035f
to
babea24
Compare
babea24
to
831135c
Compare
@lidel This is ready for a review, thanks! |
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.
Thank you @MarcoPolo, I believe this is ok to be included in 0.15-rc1
ResourceMgr.Enabled
defaults to false, which makes rcmgr-related changes in this PR low risk.
Work towards enabling it by default we will continue #8761
fysa small changes I've made:
- docs for
Swarm.ResourceMgr.Allowlist
- regression test so we can detect when rcmgr metrics disappear
- backed out goleveldb bump (rationale below)
go.mod
Outdated
github.com/syndtr/goleveldb v1.0.0 | ||
github.com/prometheus/common v0.35.0 // indirect | ||
github.com/stretchr/testify v1.8.0 | ||
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 |
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 am unsure why this was bumped:
- go-libp2p itself uses v.1.0.0?
- this impacts
plugin/plugins/levelds
, which still uses v1.0.0 – safer to use the same version- Ideally, we would remove this dependency, and use aliases similar to this one
Since we have no time to deal with this,
I've reverted to v1.0.0 in a42848a just to play this safe.
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.
Thanks for the flag, I’m also unsure why this was bumped
Updates to go-libp2p v0.21.
Updates the rcmgr.
Enables better metrics around the rcmgr.
Before merge: