libcontainer/integration: fix cgroupv1 + systemd tests

[kolyshkin]

Commit f57bb2f set it to /sys/fs/cgroup which is somewhat confusing
as now tests create cgroups that look like this on the filesystem:

• /sys/fs/cgroup/memory/sys/fs/cgroup
• /sys/fs/cgroup/devices/sys/fs/cgroup
  ... etc.

but it worked!

It seems that some(?) tests were still not working properly with systemd because of invalid configuration.

Similar to what libcontainer/specconv does, add the functionality to set proper cgroup parameters for systemd.
This fixes all the tests.

Fixes: #2473

---

This PR also fixes the root cause of the breakage, which calls for
a separate story to be told. Gather around, kids, let me tell it to you.

Commit a1d5398 ("Respect container's cgroup path") added a
cgroupPath argument to FindCgroupMountpoint to make runc/libcontainer
work in a custom multitenant environment with multiple cgroup mount
points.

It also added passing c.Path as an argument to FindCgroupMountpoint
for systemd (v1) controller:

runc/libcontainer/cgroups/systemd/v1.go

Line 270 in 0a9af39

mountpoint, err := cgroups.FindCgroupMountpoint(c.Path, subsystem)

This change is wrong for two reasons:

1. systemd controller do not use c.Path at all (and c.Path is never set
   by specconv) -- instead, it uses Name and Parent fields.

2. c.Path, if set, is not absolute -- it is relative to /sys/fs/cgroup
   -- but it is used as an absolute path here.

Now, as c.Path is never set, the change did not result in any breakage,
and this code sit quietly for some time. The issue might not have been
discovered -- until we started running libcontainer/integration tests
in a CentOS 7 VM, which resulted in a following weird error:

FAIL: TestPidsSystemd: utils_test.go:55: exec_test.go:630: unexpected error: container_linux.go:353: starting container process caused: process_linux.go:326: applying cgroup configuration for process caused: mountpoint for devices not found

The error was "fixed" in commit f57bb2f by changing the tests'
cgroups Path to be "/sys/fs/cgroup/". This indeed fixed the issue, but
resulted in creation of cgroup directories like /sys/fs/cgroup/memory/sys/fs/cgroup,
/sys/fs/cgroup/devices/sys/fs/cgroup and so on.

So, revert the change to avoid confusion.

---

A bonus story for you.

Commit 27d3dd3 ("don't fail when subsystem not mounted") added
ignoring "not found" error to enableKmem, and as a result the function
now tries to call Mkdir with an empty path, which results in a weird
error message. For example, this is a failure from a
libcontainer/integration test:

=== RUN TestRunWithKernelMemorySystemd
exec_test.go:704: runContainer failed with kernel memory limit: container_linux.go:370: starting container process caused: process_linux.go:327: applying cgroup configuration for process caused: mkdir : no such file or directory

I am not entirely sure if it is a good idea to silently ignore set
limits, but at least let's fix the error handling.

[kolyshkin]

This is interesting and need to be looked at

=== RUN   TestPidsSystemd
time="2020-09-24T02:34:46Z" level=warning msg="signal: terminated"
    utils_test.go:55: exec_test.go:621: unexpected error: container_linux.go:370: starting container process caused: process_linux.go:327: applying cgroup configuration for process caused: mountpoint for devices not found
        
--- FAIL: TestPidsSystemd (0.30s)
=== RUN   TestRunWithKernelMemory
--- PASS: TestRunWithKernelMemory (0.28s)
=== RUN   TestRunWithKernelMemorySystemd
time="2020-09-24T02:34:46Z" level=warning msg="signal: killed"
    exec_test.go:704: runContainer failed with kernel memory limit: container_linux.go:370: starting container process caused: process_linux.go:327: applying cgroup configuration for process caused: mkdir : no such file or directory
--- FAIL: TestRunWithKernelMemorySystemd (0.06s)

(happens on CentOS 7)

[kolyshkin]

pushed a half-finished PR to test intermediate commits

[kolyshkin]

@AkihiroSuda @mrunalp PTAL

[thaJeztah]

This function will be a long, long list of ifs if more options are added; wondering, would it be an idea to use functional arguments for this? newTemplateConfig(withRootfs(foo), withSystemd())?

[kolyshkin]

Frankly speaking I just don't like that paradigm much. I find it harder to read as you have to jump back and forth too much to read the code. It might be useful for APIs with external users, to make the code using it less ugly, but in this case these are internal tests, so the only user is runc.

Actually, let me try rewriting it with functional args, and let's see which one looks better...

[kolyshkin]

OK, I tried, but I am not sure how to handle "else" cases. Such as this one:

        if p.userns {
                config.UidMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}}
                config.GidMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}}
                config.Namespaces = append(config.Namespaces, configs.Namespace{Type: configs.NEWUSER})
        } else {
                config.Mounts = append(config.Mounts, &configs.Mount{
                        Destination: "/sys/fs/cgroup",
                        Device:      "cgroup",
                        Flags:       defaultMountFlags | unix.MS_RDONLY,
                })
        }

Surely I can add this mount unconditionally, and make withUserNS remove it, but it does not look perfect :-\

So, I'd rather let it stay as is for now.

[kolyshkin]

Feel free to code the alternative though if you like @thaJeztah

[kolyshkin]

Added two more commit fixing the underlying code issues found by tests and hidden by #2501.

I find it quite interesting that bad tests revealed a couple of actual (albeit small) bugs.

[AkihiroSuda]

ping @cyphar @mrunalp