Set up ulimits for rootless containers.

[rhatdan]

Currently we are setting the maximum limits for rootful podman containers,
no reason not to set them by default for rootless users as well

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

[mheon]

I don't think we can hard-error on these; I can see cases where this won't work, but the rest of Podman will function fine.

[rhatdan]

Switched to warning.

[giuseppe]

we have --ulimit host that tells Podman to not set any limit block in the OCI configuration file. When there are no limit sets the OCI runtime won't apply any limit to the container so the host settings are propagated to the container.

Do we have to make that the default (AFAICS it is not yet supported by Buildah)?

[rhatdan]

I would figure it was the default. Where does the 1024,1024 come from that we have now?
I also found it curious that

$ podman unshare grep open /proc/self/limits
Max open files 524288 524288 files

Why is the default to set the soft limit== harlimit in a user namespace?

[giuseppe]

I think we do that automatically to avoid the max number of open files error when binding too many ports: 9126b45.

The issue with rootless is that we cannot set higher limits that the user currently has, so we can't default to the values used by root as they could not be allowed in the current session.

At the same time, we risk of setting higher limits than a root container would have if defaulting to --ulimit host.

We'd probably have to check each default value and lower it when it is higher than what the user is allowed to use, but we can adress this part later

[rhatdan]

So should we merge this change or drop it?

[rh-atomic-bot]

☔ The latest upstream changes (presumably #6000) made this pull request unmergeable. Please resolve the merge conflicts.

[portante]

Perhaps consider a small DRY principle change?

if !nprocSet{
    var ( cur uint64 = kernelMax, max uint64 kernel Max )
    if isRootless {
        var rlimit unix.Rlimit
        if err := unit.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err != nil {
            return errors.Wrapf(err, "failed to return RLIMIT_NPROC ulimit")
        }
        cur = rlimit.Cur, max = rlimit.Max
    }
    g.AddProcessRlimits("RLIMIT_NPROC", cur, max)
}

[portante]

And this DRY would apply to number of files as well.

[portante]

Why is a hard-coded constant of 1048576 used for kernelMax for both number of files and number of processes? Is there a proper default that could be determined from the environment?

[rhatdan]

Don't know, I was trying to fix rootless case. I would actually think we should just use the current environment in both cases. But not sure. I am slightly worried about root case being able to override default MAX if the process has CAP_SYS_RESOURCE.

man capabilities
...
       CAP_SYS_RESOURCE
              * Use reserved space on ext2 filesystems;
              * make ioctl(2) calls controlling ext3 journaling;
              * override disk quota limits;
              * increase resource limits (see setrlimit(2));
              * override RLIMIT_NPROC resource limit;
              * override maximum number of consoles on console allocation;
              * override maximum number of keymaps;
              * allow more than 64hz interrupts from the real-time clock;
              * raise  msg_qbytes  limit  for  a  System  V  message  queue  above  the  limit  in
                /proc/sys/kernel/msgmnb (see msgop(2) and msgctl(2));
              * allow  the RLIMIT_NOFILE resource limit on the number of "in-flight" file descrip‐
                tors to be bypassed when passing file descriptors to another process  via  a  UNIX
                domain socket (see unix(7));
              * override  the /proc/sys/fs/pipe-size-max limit when setting the capacity of a pipe
                using the F_SETPIPE_SZ fcntl(2) command.
              * use F_SETPIPE_SZ to increase the capacity of a pipe above the limit  specified  by
                /proc/sys/fs/pipe-max-size;
              * override  /proc/sys/fs/mqueue/queues_max  limit when creating POSIX message queues
                (see mq_overview(7));
              * employ the prctl(2) PR_SET_MM operation;
              * set /proc/[pid]/oom_score_adj to a value lower  than  the  value  last  set  by  a
                process with CAP_SYS_RESOURCE.

@giuseppe @mheon Any idea where this kernelMax came from?

[giuseppe]

I think it is just the default used by Docker.

containers/buildah@a2b0184

Also, shouldn't these changes be done in pkg/specgen instead of pkg/spec?

[rhatdan]

It is being done in both places. pkg/spec is still used by varlink.

[portante]

Seems like the same DRY principle can be applied here.

[portante]

FWIW, looks good!

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: portante, 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 [rhatdan]

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

[rhatdan]

@TomSweeneyRedHat @baude @giuseppe @mheon @vrothberg @jwhonce @QiWang19 PTAL

[baude]

lgtm

[baude]

/lgtm