cgroup: make paths available, simplify getting manager

[kolyshkin]

This is based on and currently includes #3215. used to be part of #3131.

High level overview:

• allow cgroup instantiation (NewManager) to return errors;
• initialize cgroup paths during instantiation, so paths are always available
  (and now we can use a new instance for e.g. Destroy or Exists,
  which was not possible before);
• simplifies cgroup manager instantiation, replacing the complicated
  logic of choosing a cgroup manager with a simple manager.New() call.
• fixes some panics in cgroup manager (with Resources==nil).

This helps further commits, and makes cgroup manager easier to use
from e.g. Kubernetes.

Fixes: #3177
Fixes: #3178

Proposed changelog entry

libcontainer API changes:
* New configs.Cgroup structure fields (#3177):
  * Systemd (whether to use systemd cgroup manager);
  * Rootless (whether to use rootless cgroups).
* New cgroups/manager package aiming to simplify cgroup manager instantiation. (#3177)
* All cgroup managers' instantiation methods now initialize cgroup paths and can return errors.
  This allows to use any cgroup manager method (e.g. Exists, Destroy, Set, GetStats) right after
  instantiation, which was not possible before (as paths were initialized in Apply only). (#3178)

[cyphar]

How many PRs until the cgroup handling is completely rewritten, it feels like this code is runc's Ship of Theseus. 😉 I'll review this once #3215 is done.

[kolyshkin]

I'm not 100% sure this is the right thing to do. Maybe we should not require it.

It is required because

1. cpu needs to set RT params, see

   runc/libcontainer/cgroups/fs/cpu.go

   Lines 26 to 29 in 79185bc

   	// We should set the real-Time group scheduling settings before moving
   	// in the process because if the process is already in SCHED_RR mode
   	// and no RT bandwidth is set, adding it will fail.
   	if err := s.SetRtSched(path, r); err != nil {

2. cpuset needs to set cpus and mems in the parent cgroup (yes this is super ugly), see

   runc/libcontainer/cgroups/fs/cpuset.go

   Lines 150 to 166 in 79185bc

   	// 'ensureParent' start with parent because we don't want to
   	// explicitly inherit from parent, it could conflict with
   	// 'cpuset.cpu_exclusive'.
   	if err := cpusetEnsureParent(filepath.Dir(dir)); err != nil {
   	return err
   	}
   	if err := os.Mkdir(dir, 0o755); err != nil && !os.IsExist(err) {
   	return err
   	}
   	// We didn't inherit cpuset configs from parent, but we have
   	// to ensure cpuset configs are set before moving task into the
   	// cgroup.
   	// The logic is, if user specified cpuset configs, use these
   	// specified configs, otherwise, inherit from parent. This makes
   	// cpuset configs work correctly with 'cpuset.cpu_exclusive', and
   	// keep backward compatibility.
   	if err := s.ensureCpusAndMems(dir, r); err != nil {

3. devices need to check SkipDevices flag, see

   runc/libcontainer/cgroups/fs/devices.go

   Line 24 in 79185bc

   if r.SkipDevices {

Now, we can modify all the three places to allow nil (and assume RT, cpus/mems, and SkipDevices are not set), and drop this requirement. After all, it is only for some specific cases, and not always required.

Honestly I don't know which way is better.

[mrunalp]

I think that creating a cgroup should be separated from the applying values, so my preference is to allow nil at those 3 places and remove this check.

[kolyshkin]

creating a cgroup should be separated from the applying values

Alas, with cgroup v1 it is not possible in some cases (outlined above), so as much as we want to separate create and set, we can not.

In this case, though, we can allow nil (the price to pay is to fail later on Apply in case either of CPU RT priority, cpus/mems, SkipDevices is set).

[kolyshkin]

OTOH it's easier to require non-nil Resources here, otherwise it's hard to explain later why cpu/cpuset/devices configuration has unexpectedly failed.

[kolyshkin]

Yes, let's leave it as is, unless there are other opinions.

[cyphar]

I think it's better to leave it as-is for the moment. The fact that we need to configure other cgroups for cpu makes me think that allowing nil Resources will make this even more ugly than it is today.

[mrunalp]

nit: Instead of "", could we use a predefined string like "unified"?

[kolyshkin]

I seriously considered doing it (about a year ago) and ultimately decided against it, mostly because changing it requires adding some backward-compatible hacks (this key and value is saved into state.json). I am not even taking external users such as kubernetes and cadvisor into account -- with those it will be a bloody mess.

Note that this is the convention that is universally used in many places in runc, I am not introducing it here, merely documenting the fact (which is also documented in some other places, for example

runc/libcontainer/cgroups/cgroups.go

Lines 38 to 46 in 147ad56

	// GetPaths returns cgroup path(s) to save in a state file in order to
	// restore later.
	//
	// For cgroup v1, a key is cgroup subsystem name, and the value is the
	// path to the cgroup for this subsystem.
	//
	// For cgroup v2 unified hierarchy, a key is "", and the value is the
	// unified path.
	GetPaths() map[string]string

)

[thaJeztah]

Looking for potential corner-cases: is an empty map[string]string to be considered different for this code path, or should it be treated the same? (if "the same", we'd have to check for len(paths))

[kolyshkin]

Forget to reply -- I thought about that and I guess the non-nil map (even when empty) should be handled as it is right now, IOW it means "paths are set, no need to guess any". Not sure if/when this can be useful but theoretically it kind of makes sense.

[kolyshkin]

Implemented some very minor changes (can't -> cannot plus the last commit) based on @thaJeztah review.

[thaJeztah]

LGTM, thanks!

[mrunalp]

This is a big improvement in cleaning up the code. Thanks!

[kolyshkin]

@AkihiroSuda PTAL (this is merged but if there's anything you feel is wrong here, we can address in a followup)

[cyphar]

FWIW, LGTM.