Skip to content

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

Merged
lidel merged 1 commit intomasterfrom
feat/detect-rcmgr-default-changes
Apr 8, 2022
Merged

feat: detect default limit changes in go-libp2p-resource-manager#8857
lidel merged 1 commit intomasterfrom
feat/detect-rcmgr-default-changes

Conversation

@lidel
Copy link
Copy Markdown
Member

@lidel lidel commented Apr 7, 2022 •
edited
Loading

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
Copy Markdown
Contributor

BigLep commented Apr 7, 2022

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

@lidel lidel mentioned this pull request Apr 7, 2022
Merged
6 tasks
Base automatically changed from update-libp2p-v018 to master April 8, 2022 01:06
@lidel
feat: detect changes in go-libp2p-resource-manager
383ba08 
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
guseggert
guseggert approved these changes Apr 8, 2022
View reviewed changes
Copy link
Copy Markdown
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
Draft
xrazis pushed a commit to xrazis/kubo that referenced this pull request Apr 14, 2022
@lidel @xrazis
feat: detect changes in go-libp2p-resource-manager (ipfs#8857)
f4d7cbf 
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 mentioned this pull request May 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guseggert guseggert guseggert approved these changes

@BigLep BigLep Awaiting requested review from BigLep

@aschmahmann aschmahmann Awaiting requested review from aschmahmann

Assignees

No one assigned

Labels

None yet

Projects

No open projects
IPFS Shipyard Team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lidel @BigLep @guseggert