fix: check ResourceMgr default limits during tests, not runtime #8954
Conversation
ajnavarro
force-pushed
the
fix/resource-manager-config-checking
branch
from
fc31f52
to
4e332df
Compare
ajnavarro
requested review from
lidel,
aschmahmann and
guseggert
and removed request for
lidel
| require.Equal(expectedDefaultLimits, rcmgr.DefaultLimits, "===> OOF! go-libp2p-resource-manager changed DefaultLimits") | ||
| } | ||
|
|
||
| // https://github.com/libp2p/go-libp2p-resource-manager/blob/v0.1.5/limit_defaults.go#L49 |
There was a problem hiding this comment.
Is there a clean way (maybe not) in Go to also import this version instead of duplicating its constants?
There was a problem hiding this comment.
you can scale it with WithSystemMemory or something like that.
Badically it gives you default limits, memory adjusted and you can modify ehat you want.
There was a problem hiding this comment.
@vyzo I don't think I understood you. Do you mean to use DefaultLimitConfig.WithSystemMemory for something? DefaultLimitConfig is not the problem here, the problem is coming where we create a BasicLimiter here: https://github.com/libp2p/go-libp2p-resource-manager/blob/617d17d0f43e7ede1ca630a8d2cdd71f2eb4a8cd/limit_static.go#L84 , because we are using the total physical memory as a value for the BasicLimiter.
| }` | ||
|
|
||
| // https://github.com/libp2p/go-libp2p/blob/v0.18.0/limits.go#L17 | ||
| const expectedDefaultServiceLimits = `{ |
There was a problem hiding this comment.
It was checking the output of ´NewStaticLimiter´:
As you can see, it is creating a new structure from the already checked ´DefaultLimitConfig´, so, in my opinion, we can leave it out of our checks.
There was a problem hiding this comment.
@ajnavarro these are generic limits for all services, but there are additional per specific service defaults defined in https://github.com/libp2p/go-libp2p/blob/v0.18.0/limits.go#L17 (e.g. /libp2p/circuit/relay/0.2.0/hop)
We did guard against them in the old version, mind adding a check for them back?
(Even if current values are the same as global defaults and this feels redundant, these values may change to different defaults per service, and we need to know when that happens)
There was a problem hiding this comment.
I move back to use the previous checking logic, I just added a list of paths that might vary and shouldn't be checked.
There was a problem hiding this comment.
Added all previous checks.
There was a problem hiding this comment.
👍 for moving this from runtime to a test, thanks @ajnavarro!
Before we merge, please ensure we are testing the same set of go-libp2p defaults as we did before (details below), and can quickly identify what changed (not a blocker, just good to have).
| }` | ||
|
|
||
| // https://github.com/libp2p/go-libp2p/blob/v0.18.0/limits.go#L17 | ||
| const expectedDefaultServiceLimits = `{ |
There was a problem hiding this comment.
@ajnavarro these are generic limits for all services, but there are additional per specific service defaults defined in https://github.com/libp2p/go-libp2p/blob/v0.18.0/limits.go#L17 (e.g. /libp2p/circuit/relay/0.2.0/hop)
We did guard against them in the old version, mind adding a check for them back?
(Even if current values are the same as global defaults and this feels redundant, these values may change to different defaults per service, and we need to know when that happens)
|
|
||
| func TestResourceManagerDefaults(t *testing.T) { | ||
| require := require.New(t) | ||
| require.Equal(expectedDefaultLimits, rcmgr.DefaultLimits, "===> OOF! go-libp2p-resource-manager changed DefaultLimits") |
There was a problem hiding this comment.
nit: each scope has the same field names, so it will be a bit tedious to figure out which specific field changed 🙈
Example: can you tell which StreamsInbound changed?
$ go test ./core/node/libp2p/rcmgr_defaults_test.go
--- FAIL: TestResourceManagerDefaults (0.00s)
rcmgr_defaults_test.go:13:
Error Trace: rcmgr_defaults_test.go:13
Error: Not equal:
expected: rcmgr.DefaultLimitConfig{SystemBaseLimit:rcmgr.BaseLimit{Streams:16384, StreamsInbound:4097, StreamsOutbound:16384, Conns:1024, ConnsInbound:256, ConnsOutbound:1024, FD:512}, SystemMemory:rcmgr.MemoryLimit{MemoryFraction:0.125, MinMemory:134217728, MaxMemory:1073741824}, TransientBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:128, StreamsOutbound:512, Conns:128, ConnsInbound:32, ConnsOutbound:128, FD:128}, TransientMemory:rcmgr.MemoryLimit{MemoryFraction:1, MinMemory:67108864, MaxMemory:67108864}, ServiceBaseLimit:rcmgr.BaseLimit{Streams:8192, StreamsInbound:2048, StreamsOutbound:8192, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ServiceMemory:rcmgr.MemoryLimit{MemoryFraction:0.03125, MinMemory:67108864, MaxMemory:268435456}, ServicePeerBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:256, StreamsOutbound:512, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ServicePeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:16777216, MaxMemory:67108864}, ProtocolBaseLimit:rcmgr.BaseLimit{Streams:4096, StreamsInbound:1024, StreamsOutbound:4096, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ProtocolMemory:rcmgr.MemoryLimit{MemoryFraction:0.015625, MinMemory:67108864, MaxMemory:134217728}, ProtocolPeerBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:128, StreamsOutbound:256, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ProtocolPeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:16777216, MaxMemory:67108864}, PeerBaseLimit:rcmgr.BaseLimit{Streams:1024, StreamsInbound:512, StreamsOutbound:1024, Conns:16, ConnsInbound:8, ConnsOutbound:16, FD:8}, PeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:67108864, MaxMemory:134217728}, ConnBaseLimit:rcmgr.BaseLimit{Streams:0, StreamsInbound:0, StreamsOutbound:0, Conns:1, ConnsInbound:1, ConnsOutbound:1, FD:1}, ConnMemory:1048576, StreamBaseLimit:rcmgr.BaseLimit{Streams:1, StreamsInbound:1, StreamsOutbound:1, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, StreamMemory:16777216}
actual : rcmgr.DefaultLimitConfig{SystemBaseLimit:rcmgr.BaseLimit{Streams:16384, StreamsInbound:4096, StreamsOutbound:16384, Conns:1024, ConnsInbound:256, ConnsOutbound:1024, FD:512}, SystemMemory:rcmgr.MemoryLimit{MemoryFraction:0.125, MinMemory:134217728, MaxMemory:1073741824}, TransientBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:128, StreamsOutbound:512, Conns:128, ConnsInbound:32, ConnsOutbound:128, FD:128}, TransientMemory:rcmgr.MemoryLimit{MemoryFraction:1, MinMemory:67108864, MaxMemory:67108864}, ServiceBaseLimit:rcmgr.BaseLimit{Streams:8192, StreamsInbound:2048, StreamsOutbound:8192, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ServiceMemory:rcmgr.MemoryLimit{MemoryFraction:0.03125, MinMemory:67108864, MaxMemory:268435456}, ServicePeerBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:256, StreamsOutbound:512, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ServicePeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:16777216, MaxMemory:67108864}, ProtocolBaseLimit:rcmgr.BaseLimit{Streams:4096, StreamsInbound:1024, StreamsOutbound:4096, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ProtocolMemory:rcmgr.MemoryLimit{MemoryFraction:0.015625, MinMemory:67108864, MaxMemory:134217728}, ProtocolPeerBaseLimit:rcmgr.BaseLimit{Streams:512, StreamsInbound:128, StreamsOutbound:256, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, ProtocolPeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:16777216, MaxMemory:67108864}, PeerBaseLimit:rcmgr.BaseLimit{Streams:1024, StreamsInbound:512, StreamsOutbound:1024, Conns:16, ConnsInbound:8, ConnsOutbound:16, FD:8}, PeerMemory:rcmgr.MemoryLimit{MemoryFraction:0.0078125, MinMemory:67108864, MaxMemory:134217728}, ConnBaseLimit:rcmgr.BaseLimit{Streams:0, StreamsInbound:0, StreamsOutbound:0, Conns:1, ConnsInbound:1, ConnsOutbound:1, FD:1}, ConnMemory:1048576, StreamBaseLimit:rcmgr.BaseLimit{Streams:1, StreamsInbound:1, StreamsOutbound:1, Conns:0, ConnsInbound:0, ConnsOutbound:0, FD:0}, StreamMemory:16777216}
Diff:
--- Expected
+++ Actual
@@ -3,3 +3,3 @@
Streams: (int) 16384,
- StreamsInbound: (int) 4097,
+ StreamsInbound: (int) 4096,
StreamsOutbound: (int) 16384,
Test: TestResourceManagerDefaults
Messages: ===> OOF! go-libp2p-resource-manager changed DefaultLimitsWe have ~7 of them, so it is not the end of the world, can do manual check, but perhaps we could add if assert.ObjectsAreEqual(expectedDefaultLimits, rcmgr.DefaultLimits) before and if when true print JSON diff, like we did in the old version?
(it indicated exact field with the parent scope – see #8857 (comment))
There was a problem hiding this comment.
The diff is giving you that information (@@ -3,3 +3,3 @@), it is the StreamsInbound from SystemBaseLimit.
I can change it back to JSON if you think is better, but I see some problems with that:
- we add a new dependency (jsondiff)
- more difficult to maintain (having a JSON string that you need to modify if something changes is more difficult in my opinion than changing a struct)
There was a problem hiding this comment.
I move back to the previous logic because is the easiest way to ignore variable values. I stop checking variable memory values.
lidel
changed the title
ajnavarro
force-pushed
the
fix/resource-manager-config-checking
branch
from
4e332df
to
719c8d9
Compare
And do not check variable values, only memory ones for now. Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
ajnavarro
force-pushed
the
fix/resource-manager-config-checking
branch
from
719c8d9
to
2dfbaa2
Compare
There was a problem hiding this comment.
I get what is trying to be achieved here and I won't question its motivation, but this approach is clearly getting a bit out of control.
Besides the whole "do programmatic checks for something we can control statically" argument, we are copying what we're trying to enforce in its entirety (and even much more, because we mimic value copying in the original Go code by actual copy-pasting in JSON). Then we're adding a bunch of JSON pre and postprocessing code because the comparison isn't even straightforward. After all the "detect changed config in JSON" logic, what if there's something that actually has changed upstream? Are we going to keep adding more changes in adjustedDefaultLimits?
If there's no elegant way around the original problem let's just copy the values here. (But I suspect we can convince our friends in libp2p world to dump the limits in a tagged JSON file we can lock to and consume here.)
|
2022-05-17 conversation: we like the idea of having a test to know that limits don't change rather than detecting this at runtime. That said, we're moving this to "Best Effort Track" because we have satisfied the goal for now of making sure we catch if a go-libp2p default limit changes on us before doing a new go-ipfs release. (This is because we have a sharness test that will fail.) |
|
Closing this as unnecessary right now. We can always tackle the problem in the future. |
Instead of checking it at runtime, I added a test to keep an eye on
config changes.
Having a look intorcmgr.NewStaticLimiter(rcmgr.DefaultLimits)I think checking it is redundant because all previously checked configurations are used specifically for each system.Using previous JSON diff logic but ignoring the specific memory values that can change.
It fixes #8949