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

Fix: RM: Improve init RM message and fix final memory value. #9470

Conversation

ajnavarro
Copy link
Member

@ajnavarro ajnavarro commented Dec 7, 2022

Improved init message explaining what RM is doing. Output example:

Initializing daemon...
Kubo version: 0.18.0-dev-a3913d9f9-dirty
Repo version: 12
System version: amd64/linux
Golang version: go1.19.3

Computing default go-libp2p Resource Manager limits based on: 
    - 'Swarm.ResourceMgr.MaxMemory': "8GB"
    - 'Swarm.ResourceMgr.MaxFileDescriptors': 524288

Applying any user-supplied overrides on top.
Run 'ipfs swarm limit all' to see the resulting limits.

Swarm listening on /ip4/127.0.0.1/tcp/4001
Swarm listening on /ip4/127.0.0.1/udp/4001/quic
Swarm listening on /ip4/16.0.0.1/tcp/4001
Swarm listening on /ip4/16.0.0.1/udp/4001/quic
Swarm listening on /ip4/16.1.0.1/tcp/4001
Swarm listening on /ip4/16.1.0.1/udp/4001/quic
Swarm listening on /ip4/16.10.0.1/tcp/4001
Swarm listening on /ip4/16.10.0.1/udp/4001/quic
Swarm listening on /ip4/16.11.0.1/tcp/4001
Swarm listening on /ip4/16.11.0.1/udp/4001/quic
Swarm listening on /ip4/16.12.0.1/tcp/4001
Swarm listening on /ip4/16.12.0.1/udp/4001/quic

Also fixed the computed memory value. When calling Scale(int64(maxMemory), int(numFD)) method we were adding that memory to the previously configured one at System level. Setting System memory and FD to libp2p default ones. Output example using 8GB as set memory now:

ipfs swarm limit system
{
  "Conns": 4611686018427388000,
  "ConnsInbound": 540,
  "ConnsOutbound": 4611686018427388000,
  "FD": 524288,
  "Memory": 8133804032,
  "Streams": 4611686018427388000,
  "StreamsInbound": 8653,
  "StreamsOutbound": 4611686018427388000
}

Signed-off-by: Antonio Navarro Perez antnavper@gmail.com

Copy link
Contributor

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Thanks @ajnavarro. I'm wondering if we should have a message where the resource manager is enabled or not. Something like:

"go-libp2p resource protection disabled"

It emphasizes the purpose (node protection) and makes it clear when someone is forgoing that protectin.

core/node/libp2p/rcmgr_defaults.go Outdated Show resolved Hide resolved
@ajnavarro ajnavarro force-pushed the ajnavarro/fix/better-RM-startup-message-and-fix-memory-values branch from 682be6b to 0c6f669 Compare December 7, 2022 14:31
@ajnavarro
Copy link
Member Author

@BigLep added a message when starting the daemon and RM is disabled.

ajnavarro and others added 6 commits December 7, 2022 17:37
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Piotr Galar <piotr.galar@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro force-pushed the ajnavarro/fix/better-RM-startup-message-and-fix-memory-values branch from 0c6f669 to cef695f Compare December 7, 2022 16:39
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro force-pushed the ajnavarro/fix/better-RM-startup-message-and-fix-memory-values branch from cef695f to 6b0b7a0 Compare December 7, 2022 16:42
test/sharness/t0060-daemon.sh Outdated Show resolved Hide resolved
core/node/libp2p/rcmgr_defaults.go Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

SGTM

@ajnavarro ajnavarro merged commit a10d012 into master Dec 7, 2022
@ajnavarro ajnavarro deleted the ajnavarro/fix/better-RM-startup-message-and-fix-memory-values branch December 7, 2022 17:30
@lidel
Copy link
Member

lidel commented Dec 7, 2022

(unrelated) @ajnavarro if you have multiple "wip" commits like https://github.com/ipfs/kubo/pull/9470/commits it is perfectly fine to just squash-merge :)

(makes eyeballing https://github.com/ipfs/kubo/commits/master and inspecting release diff a bit easier for person making the release dance)

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.

5 participants