Add support for CPU hot add/remove

[devimc]

For hot-add, we have to know what CPUs are available using query-hotpluggable-cpus. For hot-remove we only need to know the CPUs ids in order to avoid the use of 1 query 1 cpu, hotplugAddCPUs and hotRemoveCPUs receive the total amount of CPUs to hot add or hot remove.

depending of the container's state, pod's resources are updated. for example, if the container has a CPU constraint of 4 vCPU and it is stopped, then these 4 vCPUs are removed. another example, if the container is ready to be started and it has a CPU constraint of 3 vCPUs, 3 vCPUs are added and the constraint is sent to the agent to limit CPU usage of the container. With this patch the fractional part of the CPU constraint is also honoured, for example, if the container has a CPU constraint of 5.5, then 6 vCPUs are added to the POD and a constraint of 5.5 CPUs is applied to the container

fixes #511

Signed-off-by: Julio Montes julio.montes@intel.com

[devimc]

@mcastelino @jodh-intel please take a look

[devimc]

depends on https://github.com/containers/virtcontainers/pull/581

[jodh-intel]

@mcastelino - any final comments on this?

[devimc]

@sameo changes applied, thanks

[sameo]

@devimc Also, this is failing the unit tests.

[devimc]

depends-on https://github.com/containers/virtcontainers/pull/591 https://github.com/containers/virtcontainers/pull/589

[sboeuf]

@devimc CI is not passing here. #589 has been solved, and I don't think #591 is the reason why tests are failing, right ? Could you please make the tests passing on this PR ?

[devimc]

@sboeuf let me apply #591 and see if that fixes the CI

[devimc]

pssst hey @sboeuf take a look to the CI

[sboeuf]

@devimc thx ! I'll do the review !

[sboeuf]

@devimc is that ready for a new review ?

[devimc]

@sboeuf changes applied, thanks

[devimc]

@sboeuf changes applied, thanks

depends on https://github.com/clearcontainers/packaging/pull/279

[devimc]

@sboeuf I think your design is different than mine, you want to share vCPUs between containers while I don't. Correct me if I''m wrong, but in your design if we have 1 container with a constraint of 2 vCPUs, the VM should have 2 vCPUs, but if a new container is created with a constrain of 3 vCPUs, then only 1 vCPUs will be added, the idea is good but is not the correct solution. In order to don't have containers fighting for the CPU usage, each container should have assigned the number of vCPUs that it needs, we can not exceed the maximum number of actual physical cores because docker and kubernetes validates that before starting the container, if quota/period is greater than the actual number of physical cores the container won't start, also in qemu.go I have a validation to do not exceed the maximum number of vCPUs, in that way when the container is destroyed only its resources are hot removed (if it has any), that means we will not remove CPUs used by other containers neither the runtime will try to remove cold plugged vCPUs.

[devimc]

@sboeuf

Do you realize that we're always going to end up with number_of_CPUs_on_the_VM = coldplugged_CPUs + (container_i * number_of_CPUs_for_container_i) ?

Yes, when a CPU constrain is not specified for a container, then it will use the CPUs cold plugged.

How are we going to handle the constraint of the container process inside the VM so that we won't end up using only the first CPUs (0,1,2 for instance for a second container that needs 3 CPUs, but the first container needed 2 CPUs) ?

Before creating a container, the runtime check if a CPU constraint was specified, if so, then N cpus are hot added and the agent places the container inside a CPU cgroup, containers without CPU constraint have less priority than containers with CPUs constraint. For example, if default_vcpus is 1 and this pod configuration is used:

apiVersion: v1
kind: Pod
metadata:
  name: cpu-demo
  namespace: cpu
spec:
  containers:
  - name: container1
    image: vish/stress
    args:
    - -cpus
    - "4"
  - name: container2
    image: vish/stress
    resources:
      limits:
        cpu: "4"
    args:
    - -cpus
    - "4"

both containers container1 and container2 will try to consume 400% of CPU. When container1 is created the VM only have 1 vCPU, hence container1 uses 100% of CPU. When container2 is created, 4 vCPUs are hot added, container2 is placed inside a CPU cgroup, and it consumes 400% of CPU while container1 only 100%

[sboeuf]

@devimc Ok this is pretty clear, but I thought we had to specify which CPU (0, 1, 2, ..., N) we're gonna use for each container. If the kernel handles the workload on its own, then that's fine to only specify the number of CPU for each process and we don't have to bother with this. Is that right ?

Now, about the coldplugged CPU, I think we can do better here. In case all the containers running will have some CPU constraints, the number of CPUs coldplugged will never be used. We're wasting resources here. So I'd like that we take into account the number of coldplugged CPUs before we start hotplugging more CPUs. Also, I think that we should handle the non-constrained containers as if they had some CPUs to hotplug. The only way to do that is to define a default amount of CPUs that we expect acceptable to run a non-constrained container. Does that make sense ?

BTW, could you check why the CI is failing.

[devimc]

@sboeuf

I thought we had to specify which CPU (0, 1, 2, ..., N) we're gonna use for each container.

when I started with this, I thought the same, but that is not needed because the kernel automatically runs the workload in N vCPUs. Also we cannot decide what vCPUs the container have to use, the user can decide this with docker --cpuset-cpus(in another PR I'll add support for this option).

If the kernel handles the workload on its own, then that's fine to only specify the number of CPU for each process and we don't have to bother with this. Is that right ?

that's correct.

Now, about the coldplugged CPU, I think we can do better here. In case all the containers running will have some CPU constraints, the number of CPUs coldplugged will never be used. We're wasting resources here.

It depends, to deal with this limitation, in https://github.com/clearcontainers/runtime/pull/1041 I'm changing default_vcpus to 1, this vCPU can be used for other processes, remember that our containers are not running alone in the VM, we have other process running, like the kernel, systemd(journal, etc) and the agent. I agree with you, if the user changes default_vcpus to a number greater than 1, then we will be wasting resources, for that reason I'll document this in https://github.com/clearcontainers/runtime/pull/1041

So I'd like that we take into account the number of coldplugged CPUs before we start hotplugging more CPUs. Also,

and we do, these coldplugged vCPUs are used by containers without CPU constraints, it's up to the user decides the resources available for the containers without a CPU constraint.

I think that we should handle the non-constrained containers as if they had some CPUs to hotplug. The only way to do that is to define a default amount of CPUs that we expect acceptable to run a non-constrained container.

In runC, if containers don't have a CPU constraint, they will try to consume all the resources needed to complete theirs task, they will compete for the resources. Cold plugged vCPUs are used and shared between the containers without CPU constraint, if the user wants to give more priority to one container over these resources, He/She can use docker --cpu-shares (I'll add support for this option in other PR)

[sboeuf]

Following our discussion on IRC with @devimc, the change looks good and can now be merged as long as we document very well all the behavior we expect with unconstrained vs constrained containers. LGTM

[sboeuf]

@devimc please take a look why this is failing so that we can move forward and get this merged ;)