feat: P2P Resource manager with predefined profiles via CLI option#4629
feat: P2P Resource manager with predefined profiles via CLI option#4629alexmylonas wants to merge 14 commits intosourcenetwork:developfrom
Conversation
| }, | ||
| }, | ||
| ResourceProfileServer: { | ||
| PeerDefault: rcmgr.ResourceLimits{ |
There was a problem hiding this comment.
Imo, server profile settings should be uncapped to leverage the autoscale depending on the hardware but pinning reasonable (and generous) limits at the peer level to provide peer fairness and protect against monopolization.
|
|
||
| replace github.com/wlynxg/anet => github.com/sourcenetwork/anet v0.0.0-20250417190629-7c87cba7799e | ||
|
|
||
| replace github.com/sourcenetwork/go-p2p => github.com/alexmylonas/go-p2p v0.0.0-20260310235021-0e7341cfd30b |
There was a problem hiding this comment.
TODO: remove after sourcenetwork/go-p2p#22 has been merged and tagged.
There was a problem hiding this comment.
Sorry for the mistake on the tag. v0.1.10 is now ready :)
| "peers": "net.peers", | ||
| "p2paddr": "net.p2paddresses", | ||
| "no-p2p": "net.p2pdisabled", | ||
| "resource-profile": "net.resourceprofile", |
There was a problem hiding this comment.
UX nit: I do not see any input validation at the CLI level (i.e. --resource-profile=typo would pass al the way to buildResourceManager at startup before erroring.)
There was a problem hiding this comment.
Yes, that's correct.
In general, I can see there is limited content validation for input namely, peers or document-acp-type which is closer to the input format, both fail downstream.
I would be happy to add it if there is strong preference.
There was a problem hiding this comment.
I think what you did is fine given the state of the start function. Although I don't think you should try to add validation specifically for the resource-profile flag, I would be curious to know what approach you would take to validate all the possible inputs to the start function generally.
There was a problem hiding this comment.
Imo, letting variables failing downstream at the package level instead of having a central validation isn't a bad direction and provides separation of concerns.
However, if we were to pursue that, I would add a Validate method to the NodeOptions which would run inside New() to do validation for all values related to node.
In addition, there are some cfg values not related to node like log.level which right now are not validated, so probably a Validate(cfg) for those to be run at the end of PersistentPreRunE
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements libp2p's Network Resource Manager integration, adding support for resource profiles ("limited" and "server") through a new CLI flag, config system, and P2P options layer that propagates resource limits into the node's P2P configuration during startup. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI/Config
participant NodeSetup as Node Startup
participant P2PBuilder as P2P Builder
participant ResourceMgr as Resource Manager
User->>CLI: Provide --resource-profile flag
CLI->>CLI: Load & bind config value
CLI->>NodeSetup: Initialize node with options
NodeSetup->>NodeSetup: Read resource profile from config
NodeSetup->>ResourceMgr: resourceLimitsForProfile(profile)
ResourceMgr-->>NodeSetup: Return p2p.ResourceLimits
NodeSetup->>P2PBuilder: SetResourceProfile(profile)
P2PBuilder->>P2PBuilder: Append WithResourceLimits option
P2PBuilder-->>NodeSetup: P2P configured with limits
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
node/resource_manager.go (1)
45-63: Minor: Streams total doesn't match sum of inbound + outbound.
Streams: 196appears in both Transient and PeerDefault, butStreamsInbound + StreamsOutbound = 64 + 128 = 192. While 196 > 192 means both can reach their max simultaneously, the value 196 seems arbitrary. Consider using192for consistency or256(next power of 2) if headroom is intended.Suggested fix
Transient: rcmgr.ResourceLimits{ ConnsInbound: 16, ConnsOutbound: 32, Conns: 48, StreamsInbound: 64, StreamsOutbound: 128, - Streams: 196, + Streams: 192, Memory: 16 << 20, FD: 32, }, PeerDefault: rcmgr.ResourceLimits{ ConnsInbound: 4, ConnsOutbound: 4, Conns: 8, StreamsInbound: 64, StreamsOutbound: 128, - Streams: 196, + Streams: 192, Memory: 64 << 20, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/resource_manager.go` around lines 45 - 63, The Streams totals in the Transient and PeerDefault ResourceLimits structs are inconsistent with StreamsInbound + StreamsOutbound (StreamsInbound + StreamsOutbound = 192 but Streams is set to 196); update the Streams field in rcmgr.ResourceLimits for both Transient and PeerDefault (or whichever is appropriate) to a consistent value (e.g., 192 to match the sum, or 256 if you want extra headroom) so Streams aligns with StreamsInbound and StreamsOutbound in the ResourceLimits definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 505: The README added a new section header "P2P Resource Management" but
the Table of Contents (TOC) wasn’t updated; add a TOC entry that links to the
"P2P Resource Management" section (match the header text and link anchor format
used elsewhere in the README) so clicking the TOC navigates to the new
section—update the TOC list and ensure the anchor/case/hyphenation matches the
"P2P Resource Management" heading exactly.
- Line 507: The intro sentence in the new section is grammatically incorrect;
update the sentence that currently reads "By default, DefraDB autoscale the
resource limits for p2p connections based on system availability." to "By
default, DefraDB autoscales the resource limits for p2p connections based on
system availability." — edit the README.md line containing that phrase (the same
sentence referencing the `--resource-profile` flag) to use "autoscales" so
subject-verb agreement is correct.
- Around line 509-511: The fenced code block containing the command "defradb
start --resource-profile limited" needs a language tag (e.g., shell) to satisfy
markdown linting and improve rendering; update the opening fence from ``` to
```shell for that specific block so the snippet is recognized as shell code.
---
Nitpick comments:
In `@node/resource_manager.go`:
- Around line 45-63: The Streams totals in the Transient and PeerDefault
ResourceLimits structs are inconsistent with StreamsInbound + StreamsOutbound
(StreamsInbound + StreamsOutbound = 192 but Streams is set to 196); update the
Streams field in rcmgr.ResourceLimits for both Transient and PeerDefault (or
whichever is appropriate) to a consistent value (e.g., 192 to match the sum, or
256 if you want extra headroom) so Streams aligns with StreamsInbound and
StreamsOutbound in the ResourceLimits definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b68e7b5d-fccd-48a4-826e-ec1813cfb66b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mdcli/config/config.gocli/config_test.gocli/start.goclient/options/node.gogo.modnode/node_p2p.gonode/resource_manager.gonode/resource_manager_test.go
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.21.0)
README.md
[warning] 509-509: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
cli/config_test.go (1)
60-60: LGTM!The new assertion correctly verifies the default value for
net.resourceprofilematches the empty string default defined inConfigDefaults.client/options/node.go (2)
116-119: LGTM!The
ResourceProfilefield is well-documented with valid values. The zero-value default (empty string) correctly enables autoscaling behavior.
514-519: LGTM!The setter follows the established builder pattern used by other P2P options in this file.
cli/config/config.go (2)
65-65: LGTM!The flag binding follows the established pattern for network-related configuration options.
95-95: LGTM!Empty string default correctly enables autoscaling behavior when no profile is explicitly specified.
go.mod (1)
469-470: Temporary fork dependency acknowledged.As noted in the existing comment, this replace directive should be removed once sourcenetwork/go-p2p#22 is merged and tagged. Ensure this is tracked before release.
node/node_p2p.go (1)
49-55: LGTM!The ResourceManager integration is well-structured:
- Error propagation is correct
- Empty profile check correctly falls back to autoscaling
- Placement in the initialization flow is appropriate
cli/start.go (2)
105-106: LGTM!The resource profile is correctly read from config and passed to the P2P options builder, following the established chaining pattern.
288-292: LGTM!The flag definition follows the established pattern and provides clear documentation of supported values.
node/resource_manager.go (1)
78-93: LGTM!The
buildResourceManagerfunction correctly:
- Validates the profile exists
- Returns a descriptive error for unknown profiles
- Applies profile overrides via
partial.Build(limits.AutoScale())- Wraps with
FixedLimiterfor consistent enforcementnode/resource_manager_test.go (4)
24-41: LGTM!Basic profile creation tests cover the happy paths and error case for unknown profiles.
43-66: LGTM!Good test coverage for transient inbound connection limits. The use of distinct IPs to avoid per-IP limits is a nice touch, and cleanup is properly handled.
68-95: LGTM!Correctly tests system-level inbound limits by moving connections out of transient scope via
SetPeer. Cleanup is properly handled.
97-130: LGTM!Good coverage of per-peer connection limits under the Server profile. The test correctly verifies that the 9th connection assignment to the same peer is rejected.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
node/resource_manager.go (1)
78-80:⚠️ Potential issue | 🟡 MinorFix the empty-profile contract in the comment.
Line 79 says
""returnsnil, but Lines 81-85 currently reject it as an unknown profile. Since the empty case is already guarded upstream, the implementation looks fine; the stale comment is the misleading part.✏️ Proposed fix
-// buildResourceManager constructs a resource manager from the given profile name. -// If profile is empty, nil is returned and go-p2p will use libp2p's autoscaled defaults. +// buildResourceManager constructs a resource manager for a predefined profile. +// Callers should skip this helper when no profile is configured.Also applies to: 81-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/resource_manager.go` around lines 78 - 80, The comment on buildResourceManager is stale: it claims an empty profile returns nil, but the function actually treats an empty or unknown profile as invalid and returns an error (the unknown-profile branch in buildResourceManager's switch/default). Update the function comment to remove the "If profile is empty, nil is returned" text and instead document that empty or unrecognized profile names are rejected and will produce an error, keeping the implementation (the switch handling in buildResourceManager) unchanged.
🧹 Nitpick comments (1)
README.md (1)
514-518: Consider mentioning theserverprofile's memory cap.The table only calls out connection and stream caps, but the profile also applies a per-peer memory limit. A short note here would keep the docs fully aligned with runtime behavior.
📝 Proposed wording
-| `server` | High-throughput nodes | Per-peer connection and stream caps; system limits remain autoscaled | +| `server` | High-throughput nodes | Per-peer connection, stream, and memory caps; system limits remain autoscaled |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 514 - 518, Update the README table row for the `server` profile to explicitly mention the per-peer memory cap: edit the Markdown table entry that currently lists only "Per-peer connection and stream caps" and append a short phrase such as "and per-peer memory limit" (or similar precise wording used elsewhere in docs/config) so the `server` profile description reflects connections, streams, and per-peer memory limits consistently across documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@node/resource_manager.go`:
- Around line 78-80: The comment on buildResourceManager is stale: it claims an
empty profile returns nil, but the function actually treats an empty or unknown
profile as invalid and returns an error (the unknown-profile branch in
buildResourceManager's switch/default). Update the function comment to remove
the "If profile is empty, nil is returned" text and instead document that empty
or unrecognized profile names are rejected and will produce an error, keeping
the implementation (the switch handling in buildResourceManager) unchanged.
---
Nitpick comments:
In `@README.md`:
- Around line 514-518: Update the README table row for the `server` profile to
explicitly mention the per-peer memory cap: edit the Markdown table entry that
currently lists only "Per-peer connection and stream caps" and append a short
phrase such as "and per-peer memory limit" (or similar precise wording used
elsewhere in docs/config) so the `server` profile description reflects
connections, streams, and per-peer memory limits consistently across
documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 510d4159-c096-4e84-8664-fc19a2b1c917
📒 Files selected for processing (2)
README.mdnode/resource_manager.go
alexmylonas
force-pushed
the
feat/resource-profiles
branch
from
4c04983 to
fccba40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/resource_manager.go`:
- Around line 65-75: The server profile's PeerDefault in ResourceProfileServer
uses rcmgr.ResourceLimits where Conns and Streams are set too low (equal to one
side) causing inbound+outbound to be artificially limited; update the
rcmgr.ResourceLimits for ResourceProfileServer.PeerDefault so that Conns =
ConnsInbound + ConnsOutbound (8 + 8 = 16) and Streams = StreamsInbound +
StreamsOutbound (512 + 1024 = 1536) to mirror the limited profile pattern and
avoid blocking legitimate bidirectional traffic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 467614fe-a98a-4e98-9695-6dafe3a5f0cd
📒 Files selected for processing (2)
README.mdnode/resource_manager.go
📜 Review details
🔇 Additional comments (3)
README.md (2)
40-40: TOC entry correctly added.The Table of Contents now includes the P2P Resource Management link with proper anchor formatting, consistent with other entries.
506-519: Documentation is clear and accurate.The section correctly describes:
- Default autoscaling behavior when no profile is set
- The
--resource-profileflag usage- Profile characteristics that align with the implementation (
limitedfor constrained hardware,serverfor per-peer fairness)Previous review feedback (grammar, code block language tag, TOC) has been addressed.
node/resource_manager.go (1)
78-92: Function implementation is correct and API-compatible with libp2p v0.47.0.The code correctly:
- Validates the profile against known values
- Applies default limits with service limits overlay
- Builds concrete limits using
partial.Build(limits.AutoScale())- Creates the ResourceManager with a FixedLimiter
- Handles errors appropriately for invalid profiles and resource manager creation failures
There was a problem hiding this comment.
🧹 Nitpick comments (2)
node/resource_manager.go (2)
82-83: Avoid hardcoding valid profile names in the error path.The error message duplicates profile names manually, which can drift when new profiles are added. Consider deriving this from a single source of truth.
♻️ Proposed refactor
import ( "fmt" + "strings" @@ ) +var validResourceProfiles = []string{ + ResourceProfileLimited, + ResourceProfileServer, +} + // resourceProfiles maps profile names to their PartialLimitConfig overrides. var resourceProfiles = map[string]rcmgr.PartialLimitConfig{ @@ partial, ok := resourceProfiles[profile] if !ok { - return nil, fmt.Errorf("unknown resource profile %q: valid values are %q, %q", - profile, ResourceProfileLimited, ResourceProfileServer) + return nil, fmt.Errorf("unknown resource profile %q: valid values are %s", + profile, strings.Join(validResourceProfiles, ", ")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/resource_manager.go` around lines 82 - 83, The error constructs a hardcoded list of valid profiles; update the code that returns the error (the fmt.Errorf call in resource_manager.go) to derive the valid profile names from a single source of truth (e.g., an exported slice/array or a helper like ValidResourceProfiles or ResourceProfiles() instead of embedding ResourceProfileLimited and ResourceProfileServer inline). Replace the manual duplication with code that joins that single source (using strings.Join or equivalent) so new profiles added to the authoritative list are automatically reflected in the error message; ensure you reference the existing constants ResourceProfileLimited and ResourceProfileServer only from that single list/provider.
87-90: Add profile context when returning constructor errors.On failure, the returned error currently loses useful context (which profile failed). Wrapping improves diagnosability.
♻️ Proposed refactor
rm, err := rcmgr.NewResourceManager(rcmgr.NewFixedLimiter(partial.Build(limits.AutoScale()))) if err != nil { - return nil, err + return nil, fmt.Errorf("build resource manager for profile %q: %w", profile, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/resource_manager.go` around lines 87 - 90, When NewResourceManager (called with NewFixedLimiter(partial.Build(limits.AutoScale()))) fails, wrap the returned error to include the profile identifier so callers know which profile construction failed; update the error return in the constructor around the rcmgr.NewResourceManager call to include profile context (e.g., fmt.Errorf("resource manager create for profile %s: %w", profileOrName, err)) or use the project's preferred error-wrapping helper, referencing NewResourceManager, NewFixedLimiter, partial.Build and limits.AutoScale() to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node/resource_manager.go`:
- Around line 82-83: The error constructs a hardcoded list of valid profiles;
update the code that returns the error (the fmt.Errorf call in
resource_manager.go) to derive the valid profile names from a single source of
truth (e.g., an exported slice/array or a helper like ValidResourceProfiles or
ResourceProfiles() instead of embedding ResourceProfileLimited and
ResourceProfileServer inline). Replace the manual duplication with code that
joins that single source (using strings.Join or equivalent) so new profiles
added to the authoritative list are automatically reflected in the error
message; ensure you reference the existing constants ResourceProfileLimited and
ResourceProfileServer only from that single list/provider.
- Around line 87-90: When NewResourceManager (called with
NewFixedLimiter(partial.Build(limits.AutoScale()))) fails, wrap the returned
error to include the profile identifier so callers know which profile
construction failed; update the error return in the constructor around the
rcmgr.NewResourceManager call to include profile context (e.g.,
fmt.Errorf("resource manager create for profile %s: %w", profileOrName, err)) or
use the project's preferred error-wrapping helper, referencing
NewResourceManager, NewFixedLimiter, partial.Build and limits.AutoScale() to
locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b03b3a80-af4d-4bac-816c-397b1e4c1a34
📒 Files selected for processing (1)
node/resource_manager.go
📜 Review details
🔇 Additional comments (2)
node/resource_manager.go (2)
25-30: Clean profile modeling and clear intent.The profile constants and the
resourceProfilesstructure are easy to follow, and thelimitedprofile limits are internally consistent (Conns/Streamstotals match inbound+outbound).Also applies to: 33-64
65-75: Server profile totals look correct now.
PeerDefault.ConnsandPeerDefault.Streamsare now aligned with inbound+outbound totals, which avoids unintended directional starvation.
| cmd.PersistentFlags().String( | ||
| "resource-profile", | ||
| cfg.GetString(config.ConfigFlags["resource-profile"]), | ||
| "Set the resource manager profile for the p2p network (supported: limited, server) (default \"\")", |
There was a problem hiding this comment.
suggestion: No need to specify the default here as you've already done that in config/config.go and the docs will pick up on that automatically.
There was a problem hiding this comment.
I removed as it doesn't have a significant contextual meaning but it's not picked but it's not picked up automatically cause it's the zero value. If the default value was limited it would have been picked up as expected.
| "peers": "net.peers", | ||
| "p2paddr": "net.p2paddresses", | ||
| "no-p2p": "net.p2pdisabled", | ||
| "resource-profile": "net.resourceprofile", |
There was a problem hiding this comment.
I think what you did is fine given the state of the start function. Although I don't think you should try to add validation specifically for the resource-profile flag, I would be curious to know what approach you would take to validate all the possible inputs to the start function generally.
There was a problem hiding this comment.
thougth: Now that I see it I regret my choice of option A2 😅. To be fair the code here is correct and I think the different limits are reasonable.
I didn't consider that rcmgr was going to be the resource manager from libp2p itself. The reason for go-p2p to exist is to try to isolate Defra from the specifics of libp2p itself and simply require that the p2p stack used implements the client/Host interface. This would allow users to provide their own p2p implementation (something other than libp2p) if they wanted to. This means that the code in the file should be moved to go-p2p.
I think it's fine for go-p2p to define these different resource profiles even though they are technically made for Defra. They are optional though and sensible so it makes sense, in my view, for them to exists over there. Also, given that go-p2p is made explicitly with libp2p, it's totally ok for the WithResourceManager option to continue existing in its current form. We would also have a WithResourceProfile option.
There was a problem hiding this comment.
Yes, I agree that importing a specific library of libp2p could create a lock-in, I should have pushed harder for A1 myself :)
So what we are thinking is leave the WithResourceManager in go-p2p if some1 needs something custom they can add it via defradb code, by adding a line and building a ResourceManager which would allow customization.
Add WithResourceProfile to go-p2p and move the code from resource-manager.go there and just a pass a profile string downstream.
There is also a third alternative but with less direct control over specifics limits. Instead of using a profile we can define a memory limit for p2p like MaxMemory, then we can derive specific limits from that i.e. 1 connection per 1 MB.
Very similarly to how kubo handles it by defining the user inputs here and deriving some limits based on memory and FD here
So defradb will pass options via WithResourceLimits and go-p2p will translate to libp2p specifics.
What do you think?
There was a problem hiding this comment.
I like that third option. It makes it nicely applicable to any other p2p implementation also.
There was a problem hiding this comment.
Sounds good, so the plan changes:
- We are creating
WithResourceLimitsingo-p2p, accepting aResourceLimits{MaxMemory, MaxFileDescriptors}struct go-p2pis translating/deriving connections limits from input and building the libp2p resource manager- In
defradb, we are callingWithResourceLimitsinstead ofWithResourceManager
Some open questions related to defradb inputs:
- Do we want to allow users to also specify
MaxMemoryPerPeer(and FD) ? - Do we still want to use the profiles instead of something like
--p2p-max-memory? I think at this point it only make sense if we allowMaxMemoryPerPeer(server profile) - If we use
--p2p-max-memoryflag are we going accept raw bytes or humanize form like 4GiB
There was a problem hiding this comment.
I think we can keep it simple for now and see what feedback we get from users. If it turns out that fine tuning is necessary then we can revisit this at that time.
There was a problem hiding this comment.
Hi @fredcarle, I have pushed a new PR sourcenetwork/go-p2p#25 in go-p2p that moves the Resource Manager there.
DefraDB now only translates resource profile to ResourceLimits and passes them downstream.
Let me know what you think!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
node/resource_profile_test.go (1)
36-39: Consider asserting the specific unknown-profile error.
assert.Erroris a bit loose here; validating the returned error shape/message would make this test more regression-resistant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/resource_profile_test.go` around lines 36 - 39, The test TestResourceLimitsForProfile_Unknown should assert the specific unknown-profile error instead of just assert.Error; update the test to call resourceLimitsForProfile("unknown") and then either use assert.EqualError(t, err, "<expected error message>") with the exact message produced (e.g. "unknown profile: unknown") or use errors.Is/require.True with the package sentinel error (e.g. ErrUnknownProfile) to verify the error shape; reference resourceLimitsForProfile and ErrUnknownProfile (or the exact message) when making the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/resource_profile_test.go`:
- Around line 30-34: The test TestResourceLimitsForProfile_Server expects 16<<30
but ResourceProfileServer currently defines MaxMemory as 8<<30; make them
consistent by updating the test expectation in
TestResourceLimitsForProfile_Server to assert.Equal(t, int64(8<<30),
limits.MaxMemory) (or alternatively change ResourceProfileServer.MaxMemory
inside the ResourceProfileServer definition returned by resourceLimitsForProfile
to 16<<30 if the intended limit is 16 GiB); modify the symbol
TestResourceLimitsForProfile_Server to match the value provided by
ResourceProfileServer used by resourceLimitsForProfile.
In `@node/resource_profile.go`:
- Around line 30-39: The Server profile MaxMemory is wrong: update
resourceProfiles' entry for ResourceProfileServer to use MaxMemory: 16<<30 (16
GiB) to match tests, and ensure other resource fields aren't left as zero by
explicitly merging defaults before passing to p2p.WithResourceLimits; locate the
resourceProfiles map and change ResourceProfileServer, then create/use a small
merge step (e.g., build a p2p.ResourceLimits value that starts from
p2p.DefaultResourceLimits or your own defaults and overwrites MaxMemory and
MaxFileDescriptors from the map) so p2p.WithResourceLimits never receives a
partially zeroed struct and connection/file-descriptor limits get intended
defaults.
---
Nitpick comments:
In `@node/resource_profile_test.go`:
- Around line 36-39: The test TestResourceLimitsForProfile_Unknown should assert
the specific unknown-profile error instead of just assert.Error; update the test
to call resourceLimitsForProfile("unknown") and then either use
assert.EqualError(t, err, "<expected error message>") with the exact message
produced (e.g. "unknown profile: unknown") or use errors.Is/require.True with
the package sentinel error (e.g. ErrUnknownProfile) to verify the error shape;
reference resourceLimitsForProfile and ErrUnknownProfile (or the exact message)
when making the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ce5cf7-7fb7-4d19-a68c-9bca2e263705
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cli/start.goclient/options/node.gogo.modnode/errors.gonode/node_p2p.gonode/resource_profile.gonode/resource_profile_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/start.go
- client/options/node.go
- go.mod
📜 Review details
🔇 Additional comments (2)
node/node_p2p.go (1)
49-55: Profile-to-limits wiring looks correct.The conditional lookup and early return on invalid profile are clean, and applying
p2p.WithResourceLimits(limits)only when configured preserves default behavior otherwise.node/errors.go (1)
40-42: Good addition of a typed constructor for unknown profiles.This keeps error creation consistent and preserves profile context for callers/logging.
| // resourceProfiles maps profile names to their resource limits. | ||
| // Connection limits are derived automatically from MaxMemory. | ||
| var resourceProfiles = map[string]p2p.ResourceLimits{ | ||
| ResourceProfileLimited: { | ||
| MaxMemory: 128 << 20, // 128 MiB | ||
| MaxFileDescriptors: 256, | ||
| }, | ||
| ResourceProfileServer: { | ||
| MaxMemory: 8 << 30, // 8 GiB | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`For github.com/sourcenetwork/go-p2p version v0.0.0-20260320165929-6c5461abc636:
- How does WithResourceLimits handle zero/unset fields in ResourceLimits?
- Are unspecified system/transient/peer stream/connection limits merged with autoscale defaults, or treated as literal zero/unbounded values?
- What is the recommended way to define partial custom profiles safely?`
💡 Result:
The repository github.com/sourcenetwork/go-p2p at the specific version v0.0.0-20260320165929-6c5461abc636 could not be located in public sources. Source Network's main repositories like defradb use libp2p for P2P networking, which employs the rcmgr (resource manager) package from github.com/libp2p/go-libp2p/p2p/host/resource-manager. go-p2p is likely an internal or forked libp2p variant, but WithResourceLimits and ResourceLimits refer to libp2p's rcmgr APIs. 1) WithResourceLimits is a libp2p option (libp2p.WithResourceManager) that configures the resource manager with custom limits. Zero/unset fields in ResourceLimits are handled by the Apply method: when applying partial configs (e.g., PartialLimitConfig), zero values mean "not set" and inherit from parent/default limits (e.g., autoscaled limits from DefaultLimits.AutoScale). Explicit zero sets a literal limit of zero. 2) Unspecified limits in system/transient/peer configs are merged with autoscale defaults via PartialLimitConfig and Apply/Build. They are not literal zero (which blocks resources) or unbounded; zeros during config application skip/leave as default. Final limits use autoscaled values unless overridden. 3) Recommended safe partial custom profiles: Start from rcmgr.DefaultLimits, use libp2p.SetDefaultServiceLimits, then rcmgr.PartialLimitConfig for tweaks (e.g., System: rcmgr.ResourceLimits{StreamsOutbound: rcmgr.Unlimited}), apply with cfg.Build(scaledLimits), create FixedLimiter, then NewResourceManager(limiter). Use pointers (*ResourceLimits) for partial to explicitly unset/avoid zero-overrides. Enable metrics for tuning: rcmgr.WithMetricsEnabled or WithTraceReporter. Examples from libp2p docs: scalingLimits := rcmgr.DefaultLimits libp2p.SetDefaultServiceLimits(&scalingLimits) cfg := rcmgr.PartialLimitConfig{System: &rcmgr.ResourceLimits{StreamsOutbound: rcmgr.Unlimited}} limits := cfg.Build(scalingLimits.AutoScale(...)) limiter := rcmgr.NewFixedLimiter(limits) rm, _ := rcmgr.NewResourceManager(limiter) host, _ := libp2p.New(libp2p.ResourceManager(rm))
Citations:
- 1: https://pkg.go.dev/github.com/libp2p/go-libp2p/p2p/host/resource-manager
- 2: https://github.com/libp2p/go-libp2p/blob/master/p2p/host/resource-manager/README.md
- 3: Fix: Ignore zero values when marshalling Limits. libp2p/go-libp2p#1998
- 4: https://github.com/libp2p/go-libp2p-resource-manager
- 5: https://docs.btfs.io/docs/the-libp2p-network-resource-manager
- 6: https://pkg.go.dev/github.com/sourcenetwork/defradb@v0.18.0
- 7: https://docs.source.network/defradb
- 8: https://docs.source.network/defradb/guides/peer-to-peer
🏁 Script executed:
# First, let's understand the codebase structure
git ls-files | grep -E "(resource|profile)" | head -20Repository: sourcenetwork/defradb
Length of output: 792
🏁 Script executed:
# Search for usage of resourceProfiles
rg "resourceProfiles" --type go -B 2 -A 2Repository: sourcenetwork/defradb
Length of output: 903
🏁 Script executed:
# Check imports in resource_profile.go to understand p2p.ResourceLimits
head -50 node/resource_profile.go | grep -E "^(package|import)"Repository: sourcenetwork/defradb
Length of output: 86
🏁 Script executed:
# Search for WithResourceLimits or resource limit handling
rg "WithResourceLimits|ResourceLimits" --type go -l | head -10Repository: sourcenetwork/defradb
Length of output: 136
🏁 Script executed:
# Read the full resource_profile.go file
cat node/resource_profile.goRepository: sourcenetwork/defradb
Length of output: 1479
🏁 Script executed:
# Check node_p2p.go to see how limits are used
cat node/node_p2p.goRepository: sourcenetwork/defradb
Length of output: 1944
🏁 Script executed:
# Search for how resourceLimitsForProfile is called
rg "resourceLimitsForProfile" --type go -B 3 -A 3Repository: sourcenetwork/defradb
Length of output: 1968
🏁 Script executed:
# Check go.mod for go-p2p dependency
grep "go-p2p" go.modRepository: sourcenetwork/defradb
Length of output: 212
🏁 Script executed:
# Read the test file to understand what's expected
cat node/resource_profile_test.goRepository: sourcenetwork/defradb
Length of output: 1162
🏁 Script executed:
# Search for any go-p2p module or vendored code in the repo
find . -path ./vendor -prune -o -name "go-p2p" -type d -print 2>/dev/null | headRepository: sourcenetwork/defradb
Length of output: 47
🏁 Script executed:
# Search for WithResourceLimits implementation or documentation in the repo
rg "WithResourceLimits" --type go -B 5 -A 10Repository: sourcenetwork/defradb
Length of output: 744
🏁 Script executed:
# Search for any documentation or comments about go-p2p WithResourceLimits
rg "go-p2p|p2p.ResourceLimits" --type go -B 3 -A 3 | head -50Repository: sourcenetwork/defradb
Length of output: 2070
🏁 Script executed:
# Check if there's a vendor directory with go-p2p source
find . -type f -path "*/go-p2p/*.go" 2>/dev/null | head -5Repository: sourcenetwork/defradb
Length of output: 47
🌐 Web query:
alexmylonas/go-p2p v0.0.0-20260320165929-6c5461abc636 WithResourceLimits ResourceLimits implementation
💡 Result:
No public information found on the specific repository "alexmylonas/go-p2p" at version v0.0.0-20260320165929-6c5461abc636 or its ResourceLimits and WithResourceLimits implementation. The repository does not appear in GitHub search results or pkg.go.dev. Alex Mylonas has a GitHub profile but no matching public go-p2p repo. The commit hash 6c5461abc636 and version from today's date (2026-03-20 16:59:29 UTC) suggest it may be a private fork or recent unpublished module, likely based on libp2p/go-libp2p given the context of ResourceLimits (from github.com/libp2p/go-libp2p/p2p/host/resource-manager/rcmgr). In libp2p, ResourceLimits is a struct defining limits for streams, conns, FD, memory (e.g., type ResourceLimits struct { Streams LimitVal; ... }). WithResourceLimits is not explicitly documented but aligns with options like libp2p.ResourceManager(rm) where rm uses limits. No specific implementation details for this exact module/version available in public sources.
Citations:
- 1: https://pkg.go.dev/github.com/libp2p/go-libp2p/p2p/host/resource-manager
- 2: https://github.com/alexmylonas
- 3: https://github.com/libp2p/go-libp2p-resource-manager
🌐 Web query:
sourcenetwork/go-p2p WithResourceLimits zero value fields handling
💡 Result:
No specific repository named "sourcenetwork/go-p2p" was found across GitHub searches or Source Network's repositories. Source Network (sourcenetwork) primarily uses libp2p for P2P functionality in projects like DefraDB (github.com/sourcenetwork/defradb), which explicitly uses libp2p/go-libp2p and mentions creating a libp2p host on startup. In libp2p (go-libp2p), resource limits are configured via the ResourceManager from github.com/libp2p/go-libp2p/p2p/host/resource-manager. Zero value fields in limit structs (e.g., LimitVal=0 as DefaultLimit) are handled as default limits derived from rcmgr.DefaultLimits and ScalingLimitConfig.AutoScale, which scales based on system memory (assuming 1/8th allocated to libp2p). Explicit zero typically means "use default context-dependent values", not unlimited (-1) or blocked. WithResourceLimits likely refers to libp2p host constructor options like libp2p.ResourceManager(rm), where rm is built from limits with zero fields falling back to defaults. No custom "WithResourceLimits" function or zero-value handling specific to sourcenetwork/go-p2p was identified; it follows standard libp2p behavior. Example configuration: scalingLimits := rcmgr.DefaultLimits // Zero fields use defaults rm, _ := rcmgr.NewResourceManager(rcmgr.NewFixedLimiter(limits.AutoScale)) h, _ := libp2p.New(libp2p.ResourceManager(rm))
Citations:
- 1: https://github.com/libp2p/go-libp2p/blob/master/p2p/host/resource-manager/README.md
- 2: https://github.com/libp2p/go-libp2p-resource-manager
- 3: https://pkg.go.dev/github.com/libp2p/go-libp2p/p2p/host/resource-manager
- 4: https://github.com/sourcenetwork
- 5: https://github.com/libp2p/go-libp2p
- 6: https://pkg.go.dev/github.com/seqsy/go-libp2p
- 7: https://github.com/sourcenetwork/defradb
- 8: https://docs.source.network/defradb/guides/peer-to-peer/
Fix the Server profile MaxMemory mismatch and verify ResourceLimits field handling before shipping.
The test expects ResourceProfileServer to have MaxMemory: 16<<30 (16 GiB), but the code defines 8<<30 (8 GiB). One of these is incorrect and must be fixed.
Additionally, the code passes partial ResourceLimits structs directly to p2p.WithResourceLimits without explicit field merging. The inline comment states "Connection limits are derived automatically from MaxMemory," which implies the go-p2p library handles unset zero fields. However, verify that unspecified fields (e.g., connection limits, file descriptor limits) receive intended defaults rather than zero/unbounded values, as this directly impacts resource fairness and exhaustion behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node/resource_profile.go` around lines 30 - 39, The Server profile MaxMemory
is wrong: update resourceProfiles' entry for ResourceProfileServer to use
MaxMemory: 16<<30 (16 GiB) to match tests, and ensure other resource fields
aren't left as zero by explicitly merging defaults before passing to
p2p.WithResourceLimits; locate the resourceProfiles map and change
ResourceProfileServer, then create/use a small merge step (e.g., build a
p2p.ResourceLimits value that starts from p2p.DefaultResourceLimits or your own
defaults and overwrites MaxMemory and MaxFileDescriptors from the map) so
p2p.WithResourceLimits never receives a partially zeroed struct and
connection/file-descriptor limits get intended defaults.
3 participants
Relevant issue(s)
Resolves #421
Depends on sourcenetwork/go-p2p#22
Description
This PR integrates the libp2p resource manager into DefraDB's P2P networking stack, by providing two distinct profile (
limited&server) options via--resource-profileCLI flag.limitedprofileTargeting hardware constraint nodes by pinning limits to half of the baseline, for system, transient and peer connections and streams.
serverprofileAdds reasonable limits only to
Peerconnections to ensure fairness and avoid peer monopolization of resourcesno profile
Fallbacks to existing autoscale resource manager as provided by libp2p.
Tasks
How has this been tested?
Added unit tests for profile setting and showcasing limit hits for connections and peers
Specify the platform(s) on which this was tested: