containers / common

Location for shared common files in github.com/containers repos.
Apache License 2.0
191 stars 200 forks source link

[v0.51] cgroups: fix memory usage on cgroup v1 #2157

Closed giuseppe closed 1 month ago

giuseppe commented 2 months ago

calculate the memory usage on cgroup v1 using the same logic as cgroup v2.

Since there is no single "anon" field, calculate the memory usage by summing the two fields "total_active_anon" and "total_inactive_anon".

Closes: https://github.com/containers/common/issues/1642

Closes: https://issues.redhat.com/browse/RHEL-16376

[NO NEW TESTS NEEDED]

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com (cherry picked from commit 1a9d45ced26ca62aacc8d6ba4fa7e870acbf9eac)

openshift-ci[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/common/blob/v0.51/OWNERS)~~ [giuseppe] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
rhatdan commented 2 months ago

LGTM

TomSweeneyRedHat commented 2 months ago

I've put the merge hold on this as this branch is too far back. We need this fix in c/common v0.57 and Podman v4.9.-rhel. Then also please verify that it's in the Podman v5.2-rhel branch and c/common v0.60 @giuseppe

giuseppe commented 2 months ago

both versions already have this change

TomSweeneyRedHat commented 2 months ago

I've removed the do-not-merge/hold label. I've verified that the fix is in the v0.57 branch, which we need for RHEL 4.9 as @giuseppe pointed out. We do need this backported for this branch of c/common so it can be used by podman v4.4.1-rhel for https://issues.redhat.com/browse/ACCELFIX-285 and https://issues.redhat.com/browse/ACCELFIX-286.

The error that we're running into here is the same as #2154

TomSweeneyRedHat commented 2 months ago

@giuseppe #2154 has merged. I think if you rebase, your test error will go away. Once merged, I'll spin a new release and merge it into the Podman v4.4 branch, and then we can kill off a number of Jira cards at once.

TomSweeneyRedHat commented 2 months ago

Partially Fixes: https://issues.redhat.com/browse/RHEL-59127

rhatdan commented 1 month ago

/lgtm

TomSweeneyRedHat commented 1 month ago

LGTM

TomSweeneyRedHat commented 1 month ago

/lgtm

TomSweeneyRedHat commented 1 month ago

OK, I'm losing my mind a bit here. When I opened this up, it was all happy green test buttons. I added a LGTM and then a / lgtm, and then it showed a failure on a netavark network test that I've not seen before. @edsantiago do we have tests that run after a /lgtm? Anyway, I've rerun the failed tests in hopes that it was a flake.

edsantiago commented 1 month ago

Weird. I see tide in merge pool and also see failed f36 test. Let's see what tide does...

TomSweeneyRedHat commented 1 month ago

And after restarting, I hit a for real network flake, restarted again.

edsantiago commented 1 month ago

All green, but still in tide pool. I have not reviewed, but I'll trust y'all. Merged.

TomSweeneyRedHat commented 1 month ago

TY @edsantiago ! I'll create the release tomorrow, time to get off this contraption now.