rewrite limits to allow auto-scaling #48

marten-seemann · 2022-06-10T17:43:28Z

We want to be able to auto-scale our limits so that 99% of node operators don't need to worry about them.

To do so, this PR removes dynamic limits, which, to the best of our knowledge, have not been used. Using them would be challenging anyway due to complicated interactions between resource manager and connection manager.

The ScalingLimitConfig is what makes this auto-scaling possible (and configurable). It defines both the minimum resource requirements for a libp2p node (assuming 1 GB of system memory, of which 128 MB are reserved for libp2p), e.g. in SystemBaseLimit. In SystemLimitIncrease one then specifies how many connections / streams / memory is added _for every additional GB of memory.

As a convenience function, the ScalingLimitConfig defines a AutoScale function that queries the system for available memory / FDs. Light nodes (IPFS desktop for example) can use the Scale function and pass in lower values of memory / FDs, to obtain a more lightweight resource manager.

marten-seemann · 2022-06-12T09:55:06Z

@vyzo This should be ready for review now.

vyzo

see the comment.

limit.go

BigLep · 2022-06-18T20:26:50Z

@marten-seemann : what are the next steps here? Would it help to have @MarcoPolo review?

MarcoPolo

Looks okay. Just some minor nits. I feel like this is really only useful for helping set up good defaults, but I think users will probably use the fixed limits when tuning this themselves.

MarcoPolo · 2022-06-22T20:00:43Z

limit.go

 }

-func (l *BasicLimiter) GetServicePeerLimits(svc string) Limit {
-	pl, ok := l.ServicePeerLimits[svc]
+func (l *fixedLimiter) GetServiceLimits(svc string) Limit {


Not for this PR, but I would like a method for Limiter that iterates over all limits so that we can expose this information to the use (via opencensus or something else).

Maybe something like GetAllLimits(forEach func(scopeName string, Limit))

This would be good to log at the start as well since it can be a bit confusing what the final values are at the end of the day.

I would like this in to help debugging :)

limit_defaults.go

MarcoPolo · 2022-06-22T20:10:22Z

limit_defaults.go

-	StreamMemory    int64
+// Scale scales up a limit configuration.
+// memory is the amount of memory that the stack is allowed to consume,
+// for a full it's recommended to use 1/8 of the installed system memory.


Suggested change

// for a full it's recommended to use 1/8 of the installed system memory.

// for a node dedicated to this process it's recommended to use 1/8 of the installed system memory.

marten-seemann · 2022-06-29T09:17:50Z

@vyzo Could you please review this again? I'll be merging this PR tomorrow (Thu) by EOD.

vyzo

I still dont see how the user can do full manual specification, which is a feature we need to retain support for.
Auto* stuff have a tendency to do unpredictable things.

vyzo · 2022-06-29T13:32:48Z

limit.go

@@ -20,19 +23,6 @@ type Limit interface {
 	GetConnTotalLimit() int
 	// GetFDLimit returns the file descriptor limit.
 	GetFDLimit() int
-
-	// WithMemoryLimit creates a copy of this limit object, with memory limit adjusted to


can we please keep the prototype constructors for limit in some form?

These are not constructors. They’re methods on an interface, where they don’t belong.

they are prototype OO constructors hich allow you to easily adjust parts of the limit without knowing the concrete types.

They belong just fine, please provide them or a suitable replacement.

I haven't found any use for them. They conflate scaling and limit configuration. Disentangling those made this PR so invasive, but ultimately resulted in a cleaner structure.

marten-seemann · 2022-06-29T15:56:13Z

I still dont see how the user can do full manual specification, which is a feature we need to retain support for.
Auto* stuff have a tendency to do unpredictable things.

NewFixedLimiter takes a LimitConf struct. The LimitConf struct contains all the limits exactly as they'll be applied.
How you arrive at the LimitConf is your decision:

You can define it yourself, based on whatever algorithm you like (or read it from a config file)
You can define a ScalingLimitConf, then call Scale() on it, which produces a LimitConf.

vyzo · 2022-06-29T15:59:52Z

I can't approve this as is. This is too much of a breaking change without providing replacement for anything it is removing.

I want autoscaling too, but this is flat out throwing everything else away, including the baby with the bathwater.

marten-seemann · 2022-06-29T16:11:46Z

I don't understand what you're missing here. This builds with go-ipfs, proving that there's nothing left in the API.

BigLep · 2022-07-01T17:12:56Z

2022-07-01 conversation:

We need a README update that discusses:

What happens when someone passes the DefaultConfig.
How someone sets static limits
What happens when uses Scaled limits
How someone changes the Scaled limits
How someone overrides the computed scaled limits

We're also going to log the limits that the resource manager was configured with.

vyzo · 2022-07-01T17:18:27Z

and i must insist on having prototype adjustment in the limit interface.

Also, it must be made abundantly clear how we can do full manual control, the functionality that was previously provided and seems gone now.

As it is, it adds a desirable feature but on the same time regresses on fuma control.
I would like to exercise more care in not breaking existing code or removing existing functionality without replacement.

MarcoPolo

Let's get this in. I don't think it'll break existing use cases because:

Users can still pass in limits from a json config.
Users can define a custom limit config and merge it with the autoscale defaults.
Users can modify a limit by modifying the LimitConfig instead of the prototype methods.

And we get some new functionality around scaling. And net less code (nice work!).

I would like the log of what the limits are, but it's really not a blocker for this PR so feel free to do in a separate PR along with Steve's suggestions above.

marten-seemann · 2022-07-02T09:42:38Z

Rebased on top of master.

marten-seemann · 2022-07-02T13:14:39Z

Merging this PR. There'll be some follow up PRs for documentation and FD counting on Windows.

BigLep · 2022-07-05T06:44:41Z

@marten-seemann : are there issues for the followup work? I'm interested in the doc items for showing what the user experience is going to be like for anyone configuring the resource manager.

marten-seemann · 2022-07-05T08:38:28Z

@BigLep It's already done: #58 and #59. Merged and released.

marten-seemann requested a review from vyzo June 10, 2022 17:43

marten-seemann force-pushed the rework-limits branch from b86fdde to 07c38a5 Compare June 10, 2022 17:48

marten-seemann marked this pull request as draft June 10, 2022 17:55

marten-seemann force-pushed the rework-limits branch from 07c38a5 to c45d467 Compare June 12, 2022 09:52

marten-seemann marked this pull request as ready for review June 12, 2022 09:52

marten-seemann force-pushed the rework-limits branch from c45d467 to ec37448 Compare June 12, 2022 10:00

vyzo reviewed Jun 12, 2022

View reviewed changes

limit.go Show resolved Hide resolved

lidel mentioned this pull request Jun 12, 2022

docs(config): mark Swarm.ResourceMgr as experimental ipfs/kubo#9039

Merged

marten-seemann force-pushed the rework-limits branch 2 times, most recently from 8f2d1ba to 8593b43 Compare June 13, 2022 13:02

MarcoPolo mentioned this pull request Jun 19, 2022

feat: Allowlist #47

Merged

7 tasks

MarcoPolo reviewed Jun 22, 2022

View reviewed changes

marten-seemann force-pushed the rework-limits branch 2 times, most recently from 118a286 to 58fa159 Compare June 27, 2022 13:33

This was referenced Jun 28, 2022

Set data-driven default resource limits #32

Closed

go-libp2p v0.21.0 libp2p/go-libp2p#1514

Closed

marten-seemann requested a review from vyzo June 29, 2022 09:17

vyzo reviewed Jun 29, 2022

View reviewed changes

MarcoPolo mentioned this pull request Jul 1, 2022

Add examples for a custom go-libp2p-resource-manager with go-libp2p defaults applied. libp2p/go-libp2p#1628

Closed

MarcoPolo approved these changes Jul 1, 2022

View reviewed changes

MarcoPolo mentioned this pull request Jul 1, 2022

Add ability to configure allowlist limits #57

Merged

2 tasks

remove support for parsing limit configs from JSON

4d634e5

marten-seemann added 6 commits July 2, 2022 11:27

remove support for dynamic limits

7fbc5d2

implement scaling of limits

33bc8a8

add a function to automatically scale limits to system memory / FDs

83465e5

reimplement parsing of JSON limit configs

1e25007

log the values that the limiter is initialized with

df93b49

expose BaseLimitIncrease.Apply

7ceb0b8

marten-seemann force-pushed the rework-limits branch from 58fa159 to 7ceb0b8 Compare July 2, 2022 09:39

marten-seemann mentioned this pull request Jul 2, 2022

rcmgr: use autoscaling limits libp2p/go-libp2p#1637

Merged

marten-seemann merged commit d0a1694 into master Jul 2, 2022

ajnavarro mentioned this pull request Aug 24, 2022

Release 0.15 ipfs/kubo#9152

Closed

72 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite limits to allow auto-scaling #48

rewrite limits to allow auto-scaling #48

marten-seemann commented Jun 10, 2022 •

edited

Loading

marten-seemann commented Jun 12, 2022

vyzo left a comment

BigLep commented Jun 18, 2022

MarcoPolo left a comment

MarcoPolo Jun 22, 2022

MarcoPolo Jun 22, 2022

MarcoPolo Jul 1, 2022

MarcoPolo Jun 22, 2022

marten-seemann commented Jun 29, 2022

vyzo left a comment

vyzo Jun 29, 2022

marten-seemann Jun 29, 2022

vyzo Jun 29, 2022 •

edited

Loading

marten-seemann Jun 29, 2022

marten-seemann commented Jun 29, 2022

vyzo commented Jun 29, 2022

marten-seemann commented Jun 29, 2022

BigLep commented Jul 1, 2022

vyzo commented Jul 1, 2022 •

edited

Loading

MarcoPolo left a comment

marten-seemann commented Jul 2, 2022

marten-seemann commented Jul 2, 2022

BigLep commented Jul 5, 2022

marten-seemann commented Jul 5, 2022

	// for a full it's recommended to use 1/8 of the installed system memory.
	// for a node dedicated to this process it's recommended to use 1/8 of the installed system memory.

rewrite limits to allow auto-scaling #48

rewrite limits to allow auto-scaling #48

Conversation

marten-seemann commented Jun 10, 2022 • edited Loading

marten-seemann commented Jun 12, 2022

vyzo left a comment

Choose a reason for hiding this comment

BigLep commented Jun 18, 2022

MarcoPolo left a comment

Choose a reason for hiding this comment

MarcoPolo Jun 22, 2022

Choose a reason for hiding this comment

MarcoPolo Jun 22, 2022

Choose a reason for hiding this comment

MarcoPolo Jul 1, 2022

Choose a reason for hiding this comment

MarcoPolo Jun 22, 2022

Choose a reason for hiding this comment

marten-seemann commented Jun 29, 2022

vyzo left a comment

Choose a reason for hiding this comment

vyzo Jun 29, 2022

Choose a reason for hiding this comment

marten-seemann Jun 29, 2022

Choose a reason for hiding this comment

vyzo Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

marten-seemann Jun 29, 2022

Choose a reason for hiding this comment

marten-seemann commented Jun 29, 2022

vyzo commented Jun 29, 2022

marten-seemann commented Jun 29, 2022

BigLep commented Jul 1, 2022

vyzo commented Jul 1, 2022 • edited Loading

MarcoPolo left a comment

Choose a reason for hiding this comment

marten-seemann commented Jul 2, 2022

marten-seemann commented Jul 2, 2022

BigLep commented Jul 5, 2022

marten-seemann commented Jul 5, 2022

marten-seemann commented Jun 10, 2022 •

edited

Loading

vyzo Jun 29, 2022 •

edited

Loading

vyzo commented Jul 1, 2022 •

edited

Loading