feat!: rcmgr: Change LimitConfig to use LimitVal type

[MarcoPolo]

This solves the problem around the encoding having different semantics than the config that is passed into the resource manager context. This makes special values explicit (Unlimited, default, block all). And I think it's a more ergonomic API (create a struct with sane defaults, rather than having to copy some pre-initialized struct, then modify relevant values).

The high level idea is you have some custom configuration called PartialLimitConfig that gets constructed with a ConcreteLimitConfig (a configuration that has no special or missing values, zero means zero, max means max). The .Build method fills in any missing information from the PartialLimitConfig and returns a complete ConcreteLimitConfig. Another way to think about it is that PartialLimitConfig is a partial overrides config, and you add it to a complete ConcreteLimitConfig to get another ConcreteLimitConfig.

Also take a look at the README for some updates on the description there.

Changes:

• Introduces a new type LimitVal which can explicitly specify "use default", "unlimited", "block all", as well as any positive number. The zero value of LimitVal (the value when you create the object in Go) is "Use default".
  • The JSON marshalling of this is straightforward.
• Introduces a new ResourceLimits type which uses LimitVal instead of ints so it can encode the above for the resources.
• Changes LimitConfig to PartialLimitConfig and uses ResourceLimits. This along with the marshalling changes means you can now marshal the fact that some resource limit is set to block all.
  • Because the default is to use the defaults, this avoids the footgun of initializing the resource manager with 0 limits (that would block everything).

Before:

	cfg := rcmgr.DefaultLimits.AutoScale()
	cfg.System.ConnsInbound = 3
	cfg.System.ConnsOutbound = 1024
	cfg.System.Conns = 1024
	cfg.PeerDefault.Conns = 1
	cfg.PeerDefault.ConnsInbound = 1
	cfg.PeerDefault.ConnsOutbound = 1

After:

	cfg := rcmgr.PartialLimitConfig{
		System: &rcmgr.ResourceLimits{
			ConnsInbound:  3,
			ConnsOutbound: 1024,
			Conns:         1024,
		},
		PeerDefault: &rcmgr.ResourceLimits{
			ConnsInbound:  1,
			ConnsOutbound: 1,
			Conns:         1,
		},
	}.Build(rcmgr.DefaultLimits.AutoScale())

And the JSON encoding:

	bl := ResourceLimits{
		Streams:         DefaultLimit,
		StreamsInbound:  10,
		StreamsOutbound: BlockAllLimit,
		Conns:           10,
		// ConnsInbound:    DefaultLimit,
		ConnsOutbound: Unlimited,
		Memory:        Unlimited64,
	}

	jsonEncoded, err := json.Marshal(bl)
	require.NoError(t, err)
	require.Equal(t, string(jsonEncoded), `{"StreamsInbound":10,"StreamsOutbound":"blockAll","Conns":10,"ConnsOutbound":"unlimited","Memory":"unlimited"}`)

This doesn't affect the ScalingLimitConfig. That has a lot of knobs and represents the developer's knowledge of how the resource usage should scale. LimitConfig on the other hand is more like the manual tweaks an end-user would make. It's used in conjunction with the defaults created from a ScalingLimitConfig.

[ajnavarro]

LGTM. I'll get this PR and do some integration on Kubo to see how it looks.

[MarcoPolo]

after a sync conversation with @marten-seemann. 0 in the JSON should mean "block all". This is because a user who manually edits a limit from 1 to 0 would be surprised to see 0 actually mean default instead of block all.

[ajnavarro]

@MarcoPolo I have a question about how to integrate this when using rcmgr.ResourceScopeLimiter to set new limits. This is how I did it:

I had to copy infiniteBaseLimits because it is private:

var infiniteBaseLimit = rcmgr.BaseLimit{
	Streams:         math.MaxInt,
	StreamsInbound:  math.MaxInt,
	StreamsOutbound: math.MaxInt,
	Conns:           math.MaxInt,
	ConnsInbound:    math.MaxInt,
	ConnsOutbound:   math.MaxInt,
	FD:              math.MaxInt,
	Memory:          math.MaxInt64,
}

limit is a rcmgr.ResourceLimits object:

		limiter, ok := s.(rcmgr.ResourceScopeLimiter)
		if !ok { 
			return ...
		}

		baseLimit := limit.Build(infiniteBaseLimit)
		limiter.SetLimit(&baseLimit)

Shall we set infiniteBaseLimit public?

[ajnavarro]

added small things I saw wen integrating with Kubo here: ipfs/kubo#9612

[MarcoPolo]

func (l *ResourceLimits) Build(defaults BaseLimit) BaseLimit { Should instead take a Limit interface instead. This will make it nicer to use with the ResourceScopeLimiter interface.

[marten-seemann]

I'm still not fully convinced that ConcreteLimitConfig.ToPartialLimitConfig() is a good idea, but I don't want to block this PR on that.

@MarcoPolo Could you add a paragraph about the rcmgr changes to the v0.25 release issue please?

[marten-seemann]

Any reason not to make this math.MaxInt?

[MarcoPolo]

Purely preference. A -1 looks different than some large number 9223371036854775807 in case this is printed out. Is that large number a Max Int? (it isn't)

[marten-seemann]

Nit: a switch would be nicer here.

[BigLep]

Is there any API breaking changes with this work?

Regardless I didn't see any mention of the changes in https://github.com/libp2p/go-libp2p/releases/tag/v0.25.0

(it makes me think that having changelog files would be useful so that this text gets captured as part of the PR itself. Kubo follows this approach).

[BigLep]

Do you even have to specify rcmgr.DefaultLimit or are those just added for clarity?

[MarcoPolo]

Just added for clarity, and to show that default doesn't get serialized into json. You'll also notice Memory is implicitly default.

[BigLep]

Is the order that they serialize as? If we are in control, I think it would be good to go from outwards to in

Conns
ConnsInbound
CoonnsOutbound
Streams
StreamsInbound
StreamsOutbound

[BigLep]

To drive this home, maybe set a Memory limit in your example above so it shows up in the serialization?

[BigLep]

(minor) Maybe also mention the AutoScale method for added convenience?