fix: nerdctl stats on containers without a memory limit returns host memory limit #3369
Conversation
go.mod
Outdated
| github.com/containerd/stargz-snapshotter v0.15.2-0.20240709063920-1dac5ef89319 | ||
| github.com/containerd/stargz-snapshotter/estargz v0.15.2-0.20240709063920-1dac5ef89319 | ||
| github.com/containerd/stargz-snapshotter/ipfs v0.15.2-0.20240709063920-1dac5ef89319 | ||
| github.com/containerd/stargz-snapshotter v0.15.2-0.20240826180748-fbc3f6a1d4aa |
There was a problem hiding this comment.
avoid updating hash for unrelated things.
There was a problem hiding this comment.
These changes occurred after running go mod tidy, which I needed to run after adding the gopsutil package. Let me see how I can work around this
There was a problem hiding this comment.
Tried it, and go mod tidy did not up these dependencies.
Maybe you did go get -u somehow?
Anyhow, I don't think we should touch these at this time.
pkg/statsutil/stats_linux.go
Outdated
| @@ -73,6 +72,27 @@ func SetCgroup2StatsFields(previousStats *ContainerStats, metrics *v2.Metrics, l | |||
|
|
|||
| } | |||
|
|
|||
| func getMemLimit(metrics *v1.Metrics) float64 { | |||
pkg/statsutil/stats_linux.go
Outdated
| return memLimit | ||
| } | ||
|
|
||
| func getMemLimit2(metrics *v2.Metrics) float64 { |
pkg/statsutil/stats_linux.go
Outdated
| } | ||
|
|
||
| func getHostMemoryLimit() uint64 { | ||
| virtualMemLimit, _ := mem.VirtualMemory() |
There was a problem hiding this comment.
why virtual memory?
There was a problem hiding this comment.
Followed their documentation - https://github.com/shirou/gopsutil?tab=readme-ov-file#usage
I didn't catch any other relevant methods
There was a problem hiding this comment.
yeah seems its called virtualmemory
go.mod
Outdated
| @@ -59,6 +59,7 @@ require ( | |||
| github.com/pelletier/go-toml/v2 v2.2.3 | |||
| github.com/rootless-containers/bypass4netns v0.4.1 | |||
| github.com/rootless-containers/rootlesskit/v2 v2.3.1 | |||
| github.com/shirou/gopsutil/v3 v3.24.5 | |||
There was a problem hiding this comment.
hey @cezar-r - is there a reason to not use /v4?
There was a problem hiding this comment.
This was the recommended version that was prompted by Go and I was having some issues with v4 as well in go.sum but I can re-evaluate if needed @apostasie
There was a problem hiding this comment.
My concern is that v3 will drop out of support soon. Back in May: v3 will not be updated except for high level security issues.
There was a problem hiding this comment.
good catch, updated it to v4 in latest push
cezar-r
force-pushed
the
stats-bug
branch
6 times, most recently
from
6e1679a
to
9469eb3
Compare
go.mod
Outdated
| @@ -59,6 +59,7 @@ require ( | |||
| github.com/pelletier/go-toml/v2 v2.2.3 | |||
| github.com/rootless-containers/bypass4netns v0.4.1 | |||
| github.com/rootless-containers/rootlesskit/v2 v2.3.1 | |||
| github.com/shirou/gopsutil/v4 v4.24.7 | |||
There was a problem hiding this comment.
This seems to introduce a bunch of other deps that should not be necessary for fixing the issue
There was a problem hiding this comment.
Thanks, will explore other options
cezar-r
force-pushed
the
stats-bug
branch
2 times, most recently
from
e40e14f
to
87686d0
Compare
Apologies for the delay, the latest commit implements the fix without the need for other deps. I do want to note; it seems like there was a discussion upstream in the containerd cgroups package relating to this issue (containerd/cgroups#265) that never got resolved. Is there alignment for the one of 4 options from that discussion? Should this fix even be implemented in this package or should it be in containerd? Let me know your thoughts @AkihiroSuda |
… memory limit Signed-off-by: Cezar Rata <[email protected]> Signed-off-by: Cezar Rata <[email protected]> Signed-off-by: Cezar Rata <[email protected]> Signed-off-by: Cezar Rata <[email protected]>
This package |
Fix: runfinch/finch#45
nerdctl statswould show 16 exbibytes of memory limit when memory limit on container was not set.