Add CPU affinity to executed processes

[kolyshkin]

This allows to set initial and final CPU affinity for a process being run in a container, which is needed to solve the issue described in opencontainers/runc#3922.

This is an alternative to #1252.

[giuseppe]

LGTM

[kolyshkin]

@opencontainers/runtime-spec-maintainers PTAL (I'm going to merge this this week if there are no other comments)

[bartwensley]

LGTM

[kad]

shouldn't it be the opposite? I mean "if ommitted, cpu.cpus will be in effect"?
The initial seems to allow to set affinity to cpu cores that are outside of the set which is specified by cpu.cpus.
This also trigger the question of validation of the values between cpuAffinity and cpu.cpus.

[kolyshkin]

There are two questions here.

1. What is the default if final is not set. You are probably right that cpu.cpus makes more sense. UPDATED.

2. Whether to allow affinity that's outside of container's cpu.cpus. While it seems theoretically possible for the initial cpuset, I haven't checked that it works, and I don't want to add any specific constraints to the spec (or to runc/crun).

[kolyshkin]

PTAL @kad

[kolyshkin]

@cclerget PTAL

[utam0k]

LGTM. It's interesting.

[cclerget]

LGTM

[kad]

@kolyshkin please, give us some time, we would like to do few experiments with @klihub, as we also observed corner cases scenarios with DPDK-style apps in the past, but we need some time to replicate that environment and see behaviour in case of invalid arguments between requests via cgroups or setaffinity.

Regarding PR in general:

1. naming of the fields: initial and final: would it make sense to have more consistent names with other parts of the spec that corresponds to same lifecycle stages of the container? e.g. normalize names with hook names: createRuntime/createContainer (or startContainer)? This also helps spec consumers to understand at which stage this CPU affinity is supposed to be applied in container lifecycle.
2. initial seems to be mandatory argument right now, even were it is applied before entering container namespace. We seen in the past that bigger problem for execution inside container namespace, not before. I think it would make more sense if both initial/final will be optional. WDYT?
3. Overall error handling with setaffinity(): it might be good to add clause in the description that incorrect setting of those parameters would lead to container creation failure? because if spec requests some value in those fields, but corresponding function call fails (invalid arguments/permissions/etc), we can't silently ignore such errors.

[kolyshkin]

@kad thanks for reviewing this!

1. naming of the fields: initial and final: would it make sense to have more consistent names with other parts of the spec that corresponds to same lifecycle stages of the container? e.g. normalize names with hook names: createRuntime/createContainer (or startContainer)? This also helps spec consumers to understand at which stage this CPU affinity is supposed to be applied in container lifecycle.

I think you are misunderstanding. These two are for exec, not create/start/run. The problem we're trying to solve with these is laid out at opencontainers/runc#3922.

2. initial seems to be mandatory argument right now, even were it is applied before entering container namespace. We seen in the past that bigger problem for execution inside container namespace, not before. I think it would make more sense if both initial/final will be optional. WDYT?

See, they (initial and final) are both part of cpuAffinity object, which itself is optional. By saying that initial is mandatory we're just saying that if cpuAffinity is specified, it should not be empty.

3. Overall error handling with setaffinity(): it might be good to add clause in the description that incorrect setting of those parameters would lead to container creation failure? because if spec requests some value in those fields, but corresponding function call fails (invalid arguments/permissions/etc), we can't silently ignore such errors.

Again, this is about exec not create/start/run. Unfortunately, this distinction (between starting container's PID 1 and executing an arbitrary process in a running container) is not laid out very well in the runtime-spec.

I think it should be up to implementation what to do it case of errors. IOW the spec should not be way too rigid. Maybe some implementation could have a flag saying "ignore this" or "ignore error from that" or "if this won't work, fall back to that" etc. If the spec is too rigid, such hacks render an implementation incompatible, which I guess is not good.

[kolyshkin]

I guess I am merging this later today. @kad if you have something, please open a followup PR.

[kolyshkin]

We also need to add a flag into runtime features.

[kolyshkin]

We also need to add a flag into runtime features.

I couldn't find a good place to add such a flag, so maybe the upper-level runtime (cri-o, containerd) can check the features.ociVersionMax field instead? WDYT @haircommander

[haircommander]

if the feature piece isn't sanctified in the runtime spec that's fine, as long as runtimes that support it agree on the name of the feature

[kad]

sorry for delay, missed notification from previous reply :(

@kad thanks for reviewing this!

1. naming of the fields: initial and final: would it make sense to have more consistent names with other parts of the spec that corresponds to same lifecycle stages of the container? e.g. normalize names with hook names: createRuntime/createContainer (or startContainer)? This also helps spec consumers to understand at which stage this CPU affinity is supposed to be applied in container lifecycle.

I think you are misunderstanding. These two are for exec, not create/start/run. The problem we're trying to solve with these is laid out at opencontainers/runc#3922.

well, then it is even worse with naming, I'd say. Description says "specifies CPU affinity used to execute the process." but doesn't mention that it is not applicable for normal container entry point process.

Furthermore, the "exec" is not really part of the runtime spec since 2016 or so. Yes, runtimes are implementing it, and it can be used, but it is not covered by spec officially. So, any fields in the spec is originally referring to standard operations create/start/stop/status/...

if we want this field only be respected on exec, we have two choices: a) explicitly call it with exec prefix in name or something similar, b) having field still named more generically as now, but inside initial/final rename to something more closely named about exec.

2. initial seems to be mandatory argument right now, even were it is applied before entering container namespace. We seen in the past that bigger problem for execution inside container namespace, not before. I think it would make more sense if both initial/final will be optional. WDYT?

See, they (initial and final) are both part of cpuAffinity object, which itself is optional. By saying that initial is mandatory we're just saying that if cpuAffinity is specified, it should not be empty.

it is not really a problem to have empty struct, effect should be the same as it is not specified at all.

One thing which I'm asking about 'initial' be optional, is because sched_cpuaffinity is reset once process is attached to cgroup with cpuset. So, practically initial makes sense only if you want to execute something outside container, but once you're entering container namespace, the sched_cpuaffinity() needed to be called again.

So, my line of thinking is like this:

• if cpuAffinity struct not set at all, don't call sched_setaffinity at all.
• if set:
  • if initial is set -> set in createRuntime stage. fail on error.
  • if initial not set or empty -> don't call sched_setaffinity()
  • if final is set -> set in createContainer, fail on error. (this will validate disjoint sets with cpu.cpus).
  • if final not set or empty, default to cgroup cpu.cpus, but only if inital was set (probably not needed, as kernel will reset it anyway to match cgroup)

with both of those variants being optional, it would allow to do affinity only for "infrastructure" operations before entering into container, only after entering container, or both.

And if we do it linked with other container lifecycle points like createRuntime, createRuntime, etc... it will give more flexibility to set affinity for all scenarios.

3. Overall error handling with setaffinity(): it might be good to add clause in the description that incorrect setting of those parameters would lead to container creation failure? because if spec requests some value in those fields, but corresponding function call fails (invalid arguments/permissions/etc), we can't silently ignore such errors.

Again, this is about exec not create/start/run. Unfortunately, this distinction (between starting container's PID 1 and executing an arbitrary process in a running container) is not laid out very well in the runtime-spec.

well, exec was part of the spec at some point, but then it was removed from it. So, as I wrote above, fields present in spec are assumed to be used at all container lifecycle points, unless really explicitly called out.

I think it should be up to implementation what to do it case of errors. IOW the spec should not be way too rigid. Maybe some implementation could have a flag saying "ignore this" or "ignore error from that" or "if this won't work, fall back to that" etc. If the spec is too rigid, such hacks render an implementation incompatible, which I guess is not good.

Regarding implementation: if sched_cpuaffinity() is called with disjoint set of cores compared to cpuset.cpus, it would return EINVAL. for the rest, it is AND masked with cpuset.cpus.... So, in worst case scenarios the requested affinity might not be in place at all (if error is ignored) or failure (if error not ignored), to "best" case where affinity would be set to subset from what was actually requested....

[kolyshkin]

@kad thanks, changed like this:

diff --git a/config.md b/config.md
index eab6eec..b9b5573 100644
--- a/config.md
+++ b/config.md
@@ -340,9 +340,10 @@ For Linux-based systems, the `process` object supports the following process-spe
 
     * **`class`** (string, REQUIRED) specifies the I/O scheduling class. Possible values are `IOPRIO_CLASS_RT`, `IOPRIO_CLASS_BE`, and `IOPRIO_CLASS_IDLE`.
     * **`priority`** (int, REQUIRED) specifies the priority level within the class. The value should be an integer ranging from 0 (highest) to 7 (lowest).
-* **`cpuAffinity`** (object, OPTIONAL) specifies CPU affinity used to execute the process.
+* **`execCPUAffinity`** (object, OPTIONAL) specifies CPU affinity used to execute the process.
+    This setting is not applicable to the container's init process.
     The following properties are available:

-    * **`initial`** (string, REQUIRED) is a list of CPUs a runtime parent
+    * **`initial`** (string, OPTIONAL) is a list of CPUs a runtime parent
       process to be run on initially, before the transition to container's
       cgroup. This is a a comma-separated list, with dashes to represent
       ranges. For example, `0-3,7` represents CPUs 0,1,2,3, and 7.
@@ -427,7 +427,7 @@ _Note: symbolic name for uid and gid, such as uname and gname respectively, are
             "soft": 1024
         }
     ],
-    "cpuAffinity": {
+    "execCPUAffinity": {
         "initial": "7",
         "final": "0-3,7"
     }
diff --git a/schema/config-schema.json b/schema/config-schema.json
index 39cc71e..cb74342 100644
--- a/schema/config-schema.json
+++ b/schema/config-schema.json
@@ -221,11 +221,8 @@
                         }
                     }
                 },
-                "cpuAffinity": {
+                "execCPUAffinity": {
                     "type": "object",
-                    "required": [
-                        "initial"
-                    ],
                     "properties": {
                         "initial": {
                             "type": "string",
diff --git a/specs-go/config.go b/specs-go/config.go
index 00bf946..290b6dc 100644
--- a/specs-go/config.go
+++ b/specs-go/config.go
@@ -94,8 +94,8 @@ type Process struct {
        SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"`
        // IOPriority contains the I/O priority settings for the cgroup.
        IOPriority *LinuxIOPriority `json:"ioPriority,omitempty" platform:"linux"`
-       // CPUAffinity specifies CPU affinity for the process.
-       CPUAffinity *CPUAffinity `json:"cpuAffinity,omitempty" platform:"linux"`
+       // ExecCPUAffinity specifies CPU affinity for exec processes.
+       ExecCPUAffinity *CPUAffinity `json:"execCPUAffinity,omitempty" platform:"linux"`
 }
 
 // LinuxCapabilities specifies the list of allowed capabilities that are kept for a process.

[kad]

thanks for update. looks better now. I'm still not convinced about names initial and final, maybe something more descriptive can be done like execRuntime / execContainer for consistency with hooks?
But for that I'll let others to chime in.

[kad]

Based on the tests, if you don't call sched_setaffinity(), the kernel itself will reset it to cgroups cpuset.cpu, so, maybe here statement can be simplified, that if not specified, after transition into container namespace, default affinity depends on kernel implementation?

[kolyshkin]

I think it is nicer to have a reference to another way of setting cpu affinity here. If removed, it still says the same thing:

       on after the transition to container's cgroup. The format is the same as
-      for `initial`. If omitted or empty, the container's default CPU affinity,
-      as defined by [cpu.cpus property](./config.md#configLinuxCPUs)), is used.
+      for `initial`. If omitted or empty, the container's default CPU affinity
+      applies.
 

so I think it's better to keep it.

[kad]

well, ok for me.