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.

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.

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).

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.

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

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.

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

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

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))

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.