fix: nerdctl stats on containers without a memory limit returns host memory limit #3369

cezar-r · 2024-08-26T23:15:44Z

Fixed bug where nerdctl stats would show 16 exbibytes of memory limit when memory limit on container was not set.

Shubhranshu153 · 2024-08-26T23:19:54Z

go.mod

-	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


avoid updating hash for unrelated things.

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

probably is fine

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.

fixed in latest push

Shubhranshu153 · 2024-08-26T23:22:14Z

pkg/statsutil/stats_linux.go

@@ -73,6 +72,27 @@ func SetCgroup2StatsFields(previousStats *ContainerStats, metrics *v2.Metrics, l

 }

+func getMemLimit(metrics *v1.Metrics) float64 {


getCgroupMemLimit

Shubhranshu153 · 2024-08-26T23:22:35Z

pkg/statsutil/stats_linux.go

+	return memLimit
+}
+
+func getMemLimit2(metrics *v2.Metrics) float64 {


getCgroup2MemLimit

Shubhranshu153 · 2024-08-26T23:23:28Z

pkg/statsutil/stats_linux.go

+}
+
+func getHostMemoryLimit() uint64 {
+	virtualMemLimit, _ := mem.VirtualMemory()


why virtual memory?

Followed their documentation - https://github.com/shirou/gopsutil?tab=readme-ov-file#usage
I didn't catch any other relevant methods

yeah seems its called virtualmemory

apostasie · 2024-08-27T06:40:56Z

go.mod

@@ -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


hey @cezar-r - is there a reason to not use /v4?

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

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.

good catch, updated it to v4 in latest push

AkihiroSuda · 2024-08-29T02:54:30Z

go.mod

@@ -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


This seems to introduce a bunch of other deps that should not be necessary for fixing the issue

Thanks, will explore other options

AkihiroSuda

https://github.com/containerd/nerdctl/pull/3369/files#r1735502693

cezar-r · 2024-09-19T20:41:01Z

This seems to introduce a bunch of other deps that should not be necessary for fixing the issue

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

AkihiroSuda · 2024-09-20T00:23:03Z

Should this fix even be implemented in this package or should it be in containerd?

This package

Shubhranshu153 · 2024-10-07T07:21:13Z

@AkihiroSuda

Following this discussion:
is the recommendation to have config value (via os environment or config.json irrespective) that users can configure and return the value and if not then return not configured/nil (option 4)

If not configured:
should we return the system max value, as that what docker seems to return and people trying to integrate with systems which were based on docker might want this to return max system limit of the current system. We probably can do it in finch under dockercompat flags, but feels like the right place for it is nerdctl.

Thanks

AkihiroSuda · 2024-10-07T07:23:28Z

AFAICS I don't think this has to be configurable. It should just follow Docker's behavior.

Shubhranshu153 · 2024-10-07T07:30:55Z

Thanks @AkihiroSuda
In that case we can return the max system limit of the current system: (this is from a 15.26 GB system)

% grep MemTotal /proc/meminfo
MemTotal:       16004064 kB

CONTAINER ID   NAME            CPU %     MEM USAGE / LIMIT   MEM %     NET I/O     BLOCK I/O   PIDS
2b07ca5e9b88   goofy_khorana   0.00%     276KiB / 15.26GiB   0.00%     720B / 0B   0B / 0B     1

… 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]>

cezar-r · 2024-10-14T22:21:54Z

Reopened the PR as it seems the implementation is aligned with what we need

AkihiroSuda

Thanks

Shubhranshu153 reviewed Aug 26, 2024

View reviewed changes

apostasie reviewed Aug 27, 2024

View reviewed changes

cezar-r marked this pull request as ready for review August 27, 2024 17:14

cezar-r force-pushed the stats-bug branch 6 times, most recently from 6e1679a to 9469eb3 Compare August 28, 2024 18:42

AkihiroSuda reviewed Aug 29, 2024

View reviewed changes

AkihiroSuda requested changes Aug 29, 2024

View reviewed changes

cezar-r marked this pull request as draft September 19, 2024 20:25

cezar-r force-pushed the stats-bug branch 2 times, most recently from e40e14f to 87686d0 Compare September 19, 2024 20:31

cezar-r force-pushed the stats-bug branch from 87686d0 to ebc0fb9 Compare September 19, 2024 21:49

fix: nerdctl stats on a container without a memory limit returns host…

ad64f42

… 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]>

cezar-r force-pushed the stats-bug branch from ebc0fb9 to ad64f42 Compare October 14, 2024 21:48

cezar-r marked this pull request as ready for review October 14, 2024 22:21

AkihiroSuda added this to the v2.0.0 milestone Oct 15, 2024

AkihiroSuda approved these changes Oct 15, 2024

View reviewed changes

AkihiroSuda merged commit 380d703 into containerd:main Oct 15, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: nerdctl stats on containers without a memory limit returns host memory limit #3369

fix: nerdctl stats on containers without a memory limit returns host memory limit #3369

cezar-r commented Aug 26, 2024 •

edited

Loading

Shubhranshu153 Aug 26, 2024

cezar-r Aug 26, 2024

Shubhranshu153 Aug 26, 2024

apostasie Aug 27, 2024

cezar-r Aug 27, 2024

Shubhranshu153 Aug 26, 2024

Shubhranshu153 Aug 26, 2024

Shubhranshu153 Aug 26, 2024

cezar-r Aug 26, 2024

Shubhranshu153 Aug 26, 2024

apostasie Aug 27, 2024

cezar-r Aug 27, 2024

apostasie Aug 27, 2024

cezar-r Aug 27, 2024

AkihiroSuda Aug 29, 2024

cezar-r Aug 30, 2024

AkihiroSuda left a comment

cezar-r commented Sep 19, 2024 •

edited

Loading

AkihiroSuda commented Sep 20, 2024

Shubhranshu153 commented Oct 7, 2024

AkihiroSuda commented Oct 7, 2024

Shubhranshu153 commented Oct 7, 2024

cezar-r commented Oct 14, 2024

AkihiroSuda left a comment

		@@ -73,6 +72,27 @@ func SetCgroup2StatsFields(previousStats ContainerStats, metrics v2.Metrics, l

		}

		func getMemLimit(metrics *v1.Metrics) float64 {

fix: nerdctl stats on containers without a memory limit returns host memory limit #3369

fix: nerdctl stats on containers without a memory limit returns host memory limit #3369

Conversation

cezar-r commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

cezar-r commented Sep 19, 2024 • edited Loading

AkihiroSuda commented Sep 20, 2024

Shubhranshu153 commented Oct 7, 2024

AkihiroSuda commented Oct 7, 2024

Shubhranshu153 commented Oct 7, 2024

cezar-r commented Oct 14, 2024

AkihiroSuda left a comment

Choose a reason for hiding this comment

cezar-r commented Aug 26, 2024 •

edited

Loading

cezar-r commented Sep 19, 2024 •

edited

Loading