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

feat: detect default limit changes in go-libp2p-resource-manager #8857

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

lidel
Copy link
Member

@lidel lidel commented Apr 7, 2022

Part of #8761, based on #8680

What

This PR removes hardcoded defaults and switches to ones from go-libp2p.

To safeguard against silent changes that could impact our users, adds a simple check that will scream loud and clear every time go-libp2p libraries change any of the limit defaults related to go-libp2p-resource-manager. This was suggested during recent go-ipfs sync.

How

  • relevant structs are serialized to JSON zand compared
  • if there are any unexpected changes, go-ipfs will refuse to start, forcing us to review impact of go-libp2p update
  • diff between expected and actual limits is returned in JSON Patch format
    • Initially, I just printed JSON for "before" and "after" and compared normalized strings,
      but given how many values we have it was difficult to spot the change.
    • Makes it easier to apply changes on top of expected limits in rcmgr_defaults.go

Demo

Assuming next version of go-libp2p changed:

The daemon will refuse to start until the changes are reviewed, and expected values are updated in rcmgr_defaults.go:

$ LIBP2P_RCMGR=1 ipfs daemon
Initializing daemon...
go-ipfs version: 0.13.0-dev-024bf885e-dirty
Repo version: 12
System version: amd64/linux
Golang version: go1.16.9
2022-04-07T23:47:00.068+0200	ERROR	p2pnode	libp2p/rcmgr_defaults.go:85	===> OOF! go-libp2p-resource-manager changed DefaultLimits
=> changes ('test' represents the old value):
  {"op":"test","path":"/SystemBaseLimit/FD","value":512}
  {"op":"replace","path":"/SystemBaseLimit/FD","value":1024}
  {"op":"test","path":"/SystemBaseLimit/StreamsInbound","value":4095}
  {"op":"replace","path":"/SystemBaseLimit/StreamsInbound","value":1024}
=> go-libp2p-resource-manager DefaultLimits update needs a review:
Please inspect if changes impact go-ipfs users, and update expectedDefaultLimits in rcmgr_defaults.go to remove this message
2022-04-07T23:47:00.068+0200	ERROR	p2pnode	libp2p/rcmgr_defaults.go:107	===> OOF! go-libp2p changed DefaultServiceLimits
=> changes ('test' represents the old value):
  {"op":"test","path":"/StreamLimits/Memory","value":16777216}
  {"op":"replace","path":"/StreamLimits/Memory","value":0}
  {"op":"test","path":"/SystemLimits/Streams","value":16384}
  {"op":"replace","path":"/SystemLimits/Streams","value":42}
=> go-libp2p SetDefaultServiceLimits update needs a review:
Please inspect if changes impact go-ipfs users, and update expectedDefaultServiceLimits in rcmgr_defaults.go to remove this message
2022-04-07T23:47:00.068+0200	FATAL	p2pnode	libp2p/rcmgr_defaults.go:115	daemon will refuse to run with the resource manager until this is resolved

@lidel lidel marked this pull request as draft April 7, 2022 22:05
@lidel lidel force-pushed the feat/detect-rcmgr-default-changes branch from 024bf88 to 5197088 Compare April 7, 2022 22:09
@lidel lidel marked this pull request as ready for review April 7, 2022 22:19
@BigLep
Copy link
Contributor

BigLep commented Apr 7, 2022

Good stuff @lidel ! Conceptually this makes sense to me. I'll let others approve from a code regard.

Base automatically changed from update-libp2p-v018 to master April 8, 2022 01:06
This adds simple check that will scream loud and clear every time
go-libp2p libraries change any of the implicit defaults
related to go-libp2p-resource-manager
@lidel lidel force-pushed the feat/detect-rcmgr-default-changes branch from 5197088 to 383ba08 Compare April 8, 2022 01:54
Copy link
Contributor

@guseggert guseggert left a comment

Choose a reason for hiding this comment

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

This is amazing, thanks!

@lidel lidel merged commit 5d71216 into master Apr 8, 2022
@lidel lidel deleted the feat/detect-rcmgr-default-changes branch April 8, 2022 15:43
@lidel lidel mentioned this pull request Apr 8, 2022
xrazis pushed a commit to xrazis/kubo that referenced this pull request Apr 14, 2022
This adds simple check that will scream loud and clear every time
go-libp2p libraries change any of the implicit defaults
related to go-libp2p-resource-manager
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants