Pids-limit should only be set if the user set it #6842

rhatdan · 2020-07-02T10:32:49Z

Currently we are sending over pids-limits from the user even if they
never modified the defaults. The pids limit should be set at the server
side unless modified by the user.

This issue has led to failures on systems that were running with cgroups V1.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2020-07-02T10:32:59Z

@giuseppe @mheon PTAL

giuseppe

LGTM

mheon · 2020-07-02T13:21:08Z

I don't like this because it disables the default PID limit entirely, even for root containers. We should preserve the default limit where possible (root and cgroups v2 rootless)

rhatdan · 2020-07-02T13:30:46Z

@mheon The default limit should be handled on the server side, if the user does not specify a root limit then pkg/specgen should use the default.

mheon · 2020-07-02T13:34:27Z

I don't see code for that anywhere. pkg/specgen takes the resource limits as they were given and uses them unmodified.

rhatdan · 2020-07-02T14:06:16Z

@mheon Added the defaults.

mheon · 2020-07-02T14:14:16Z

pkg/specgen/generate/container.go

@@ -169,6 +170,19 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 		}
 	}

+	// If caller did not specify Pids Limits load default
+	if s.ResourceLimits.Pids == nil {
+		if s.CgroupsMode != "disabled" {


We also need to make sure we are not rootless + cgroups v1

The rtc.PidsLimit() takes care of this.

rtc.PidsLimit() is returning 4096 on my cgroups v1 machine, rootless, even though the default does not show in run --help.

mheon · 2020-07-02T14:14:49Z

pkg/specgen/generate/container.go

@@ -169,6 +170,19 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 		}
 	}

+	// If caller did not specify Pids Limits load default
+	if s.ResourceLimits.Pids == nil {


This could null-pointer, I think - s.ResourceLimits may be null.

mheon · 2020-07-02T14:26:32Z

pkg/specgen/generate/container.go

@@ -169,6 +170,19 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 		}
 	}

+	// If caller did not specify Pids Limits load default
+	if s.ResourceLimits != nil && s.ResourceLimits.Pids == nil {


Do we want to make ResourceLimits if it was null and Pids is not empty?

(I think this will be the case for rootless v2, for example)

goochjj · 2020-07-02T17:30:08Z

Does not work.

(focal)mrwizard@FocalCG1Dev:~/src/podman
$ bin/podman --runtime /usr/local/bin/crun-debug run --rm -it alpine sh
Error: cannot set limits without cgroups: OCI runtime error
jq .linux.resources /run/user/1000/crun/config.json
{
  "pids": {
    "limit": 4096
  }
}

mheon · 2020-07-02T17:37:19Z

Confirmed here as well.

mheon · 2020-07-02T17:40:18Z

Ah, I think I know what's going on here. The code in containers/common for determining default limit is broken.

func (c *Config) PidsLimit() int64 {
        if unshare.IsRootless() {
                if c.Engine.CgroupManager == SystemdCgroupsManager {
                        cgroup2, _ := cgroupv2.Enabled()
                        if cgroup2 {
                                return c.Containers.PidsLimit
                        }
                        return 0
                }
        }
        return sysinfo.GetDefaultPidsLimit()
}

Notice that if cgroup manager is set to cgroupfs, we will drop through the conditionals and return sysinfo.GetDefaultPidsLimit - and v1 rootless defaults to cgroupfs.

goochjj · 2020-07-02T17:54:57Z

Ah, I think I know what's going on here. The code in containers/common for determining default limit is broken.
func (c *Config) PidsLimit() int64 {
        if unshare.IsRootless() {
                if c.Engine.CgroupManager == SystemdCgroupsManager {
                        cgroup2, _ := cgroupv2.Enabled()
                        if cgroup2 {
                                return c.Containers.PidsLimit
                        }
                        return 0
                }
        }
        return sysinfo.GetDefaultPidsLimit()
}
Notice that if cgroup manager is set to cgroupfs, we will drop through the conditionals and return sysinfo.GetDefaultPidsLimit - and v1 rootless defaults to cgroupfs.

if it's cgroupfs, we return 0 - that's what I see

goochjj · 2020-07-02T17:55:19Z

Ah, I think I know what's going on here. The code in containers/common for determining default limit is broken.
func (c *Config) PidsLimit() int64 {
        if unshare.IsRootless() {
                if c.Engine.CgroupManager == SystemdCgroupsManager {
                        cgroup2, _ := cgroupv2.Enabled()
                        if cgroup2 {
                                return c.Containers.PidsLimit
                        }
                        return 0
                }
        }
        return sysinfo.GetDefaultPidsLimit()
}
Notice that if cgroup manager is set to cgroupfs, we will drop through the conditionals and return sysinfo.GetDefaultPidsLimit - and v1 rootless defaults to cgroupfs.
if it's cgroupfs, we return 0 - that's what I see

NVM you're right.

goochjj · 2020-07-02T17:56:43Z

Ah, I think I know what's going on here. The code in containers/common for determining default limit is broken.
func (c *Config) PidsLimit() int64 {
        if unshare.IsRootless() {
                if c.Engine.CgroupManager == SystemdCgroupsManager {
                        cgroup2, _ := cgroupv2.Enabled()
                        if cgroup2 {
                                return c.Containers.PidsLimit
                        }
                        return 0
                }
        }
        return sysinfo.GetDefaultPidsLimit()
}
Notice that if cgroup manager is set to cgroupfs, we will drop through the conditionals and return sysinfo.GetDefaultPidsLimit - and v1 rootless defaults to cgroupfs.
if it's cgroupfs, we return 0 - that's what I see
NVM you're right.

The return 0 needs to move out one level probably.

goochjj · 2020-07-02T18:14:35Z

Confirmed this fixes it:

--- a/vendor/github.com/containers/common/pkg/config/default.go
+++ b/vendor/github.com/containers/common/pkg/config/default.go
@@ -489,8 +489,8 @@ func (c *Config) PidsLimit() int64 {
                        if cgroup2 {
                                return c.Containers.PidsLimit
                        }
-                       return 0
                }
+               return 0
        }
        return sysinfo.GetDefaultPidsLimit()
 }

goochjj · 2020-07-02T18:15:31Z

Though I'm not sure why cgroups v2 can't set a pids limit in cgroupfs mode - if the pids controller is in our cgroup, and we have write access to it, why can't we?

rhatdan · 2020-07-02T18:37:11Z

@goochjj Ask @giuseppe I don't remember why.
Anyways lets get this in.

goochjj · 2020-07-05T13:43:54Z

This merges a bunch of vendor dependencies which breaks the build.

Could you remove the runc bump from 1.0.0-rc90 to rc91? It breaks things in buildah due to changes in libcontainer...

IMHO that bump should be a separate PR, allowing for fixes to that issue to be tracked under their own issue/PR.

I've verified removing the runc bump fixes the compile.

goochjj · 2020-07-05T13:50:20Z

rhatdan#6

goochjj · 2020-07-07T14:55:24Z

@rhatdan https://github.com/goochjj/libpod/tree/rhatdan-pids-limit

Rebased, with runc rc90 and common 0.15.2

goochjj · 2020-07-08T01:13:31Z

My branch updated and ready if #6897 is merged.

openshift-ci-robot · 2020-07-10T07:31:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

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~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Currently we are sending over pids-limits from the user even if they never modified the defaults. The pids limit should be set at the server side unless modified by the user. This issue has led to failures on systems that were running with cgroups V1. Signed-off-by: Daniel J Walsh <[email protected]>

mheon · 2020-07-12T14:35:25Z

I'm going to give this a final test on Debian to verify it fixes v1, but LGTM based on what I see now.

mheon · 2020-07-13T18:50:31Z

Confirmed working on Debian with v1
/lgtm

openshift-ci-robot requested review from jwhonce and mheon July 2, 2020 10:32

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2020

giuseppe approved these changes Jul 2, 2020

View reviewed changes

rhatdan mentioned this pull request Jul 2, 2020

In rootless + cgroupsv1, resource limits are an error #6837

Closed

rhatdan added the Backport to v2.0 label Jul 2, 2020

rhatdan mentioned this pull request Jul 2, 2020

podman run failure on cgroupsv1 rootless related to --pids-limit #6834

Closed

rhatdan force-pushed the pids-limit branch from 96b4452 to b157e75 Compare July 2, 2020 14:05

mheon reviewed Jul 2, 2020

View reviewed changes

rhatdan force-pushed the pids-limit branch from b157e75 to 3866943 Compare July 2, 2020 14:20

mheon reviewed Jul 2, 2020

View reviewed changes

rhatdan force-pushed the pids-limit branch 2 times, most recently from dba4cad to 9b4e26a Compare July 2, 2020 16:56

rhatdan force-pushed the pids-limit branch 2 times, most recently from 7e42ddc to 4828943 Compare July 3, 2020 09:02

rhatdan mentioned this pull request Jul 4, 2020

Running a pod as rootless user with systemd: "setting rlimit type 7: operation not permitted" #6668

Closed

rhatdan changed the title ~~Pids-limit should only be set if the user set it~~ [WIP] Pids-limit should only be set if the user set it Jul 6, 2020

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 6, 2020

goochjj mentioned this pull request Jul 7, 2020

Revert runc bump to rc91 #6882

Closed

jdoss mentioned this pull request Jul 8, 2020

sending podman 2.x major version bump to next stream first coreos/fedora-coreos-tracker#560

Closed

rhatdan force-pushed the pids-limit branch 3 times, most recently from a7b2fbe to 1c2cdf2 Compare July 10, 2020 07:31

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2020

rhatdan removed Backport to v2.0 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 10, 2020

rhatdan changed the title ~~[WIP] Pids-limit should only be set if the user set it~~ Pids-limit should only be set if the user set it Jul 10, 2020

rhatdan force-pushed the pids-limit branch from 1c2cdf2 to 677ad10 Compare July 10, 2020 16:46

openshift-ci-robot assigned mheon Jul 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2020

openshift-merge-robot merged commit d86acf2 into containers:master Jul 13, 2020

dustymabe mentioned this pull request Jul 17, 2020

tracker for podman 2.x progress coreos/fedora-coreos-tracker#575

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pids-limit should only be set if the user set it #6842

Pids-limit should only be set if the user set it #6842

rhatdan commented Jul 2, 2020

rhatdan commented Jul 2, 2020

giuseppe left a comment

mheon commented Jul 2, 2020

rhatdan commented Jul 2, 2020

mheon commented Jul 2, 2020

rhatdan commented Jul 2, 2020

mheon Jul 2, 2020

rhatdan Jul 2, 2020

goochjj Jul 2, 2020

mheon Jul 2, 2020

rhatdan Jul 2, 2020

mheon Jul 2, 2020

mheon Jul 2, 2020

goochjj commented Jul 2, 2020

mheon commented Jul 2, 2020

mheon commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

rhatdan commented Jul 2, 2020

goochjj commented Jul 5, 2020

goochjj commented Jul 5, 2020

goochjj commented Jul 7, 2020

goochjj commented Jul 8, 2020

openshift-ci-robot commented Jul 10, 2020

mheon commented Jul 12, 2020

mheon commented Jul 13, 2020

Pids-limit should only be set if the user set it #6842

Pids-limit should only be set if the user set it #6842

Conversation

rhatdan commented Jul 2, 2020

rhatdan commented Jul 2, 2020

giuseppe left a comment

Choose a reason for hiding this comment

mheon commented Jul 2, 2020

rhatdan commented Jul 2, 2020

mheon commented Jul 2, 2020

rhatdan commented Jul 2, 2020

mheon Jul 2, 2020

Choose a reason for hiding this comment

rhatdan Jul 2, 2020

Choose a reason for hiding this comment

goochjj Jul 2, 2020

Choose a reason for hiding this comment

mheon Jul 2, 2020

Choose a reason for hiding this comment

rhatdan Jul 2, 2020

Choose a reason for hiding this comment

mheon Jul 2, 2020

Choose a reason for hiding this comment

mheon Jul 2, 2020

Choose a reason for hiding this comment

goochjj commented Jul 2, 2020

mheon commented Jul 2, 2020

mheon commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

rhatdan commented Jul 2, 2020

goochjj commented Jul 5, 2020

goochjj commented Jul 5, 2020

goochjj commented Jul 7, 2020

goochjj commented Jul 8, 2020

openshift-ci-robot commented Jul 10, 2020

mheon commented Jul 12, 2020

mheon commented Jul 13, 2020