Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

T6406: Container CPU limits #3530

Merged
merged 3 commits into from
May 28, 2024
Merged

T6406: Container CPU limits #3530

merged 3 commits into from
May 28, 2024

Conversation

nvollmar
Copy link
Contributor

@nvollmar nvollmar commented May 27, 2024

Change Summary

Add container config option for cpu limits

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T6406

Related PR(s)

Component(s) name

container

Proposed changes

Add option to enable cpu limits per container similar to memory l imits

How to test

set container name c1 image hello-world
set container name c1 allow-host-networks
set container name c1 cpus 1.5

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_container.py
test_basic (__main__.TestContainer.test_basic) ... ok
test_cpu_limit (__main__.TestContainer.test_cpu_limit) ... ok
test_dual_stack_network (__main__.TestContainer.test_dual_stack_network) ... 
IP address "192.0.2.1" can not be used for a container, reserved for the
container engine!

ok
test_ipv4_network (__main__.TestContainer.test_ipv4_network) ... 
IP address "192.0.2.1" can not be used for a container, reserved for the
container engine!

ok
test_ipv6_network (__main__.TestContainer.test_ipv6_network) ... 
IP address "2001:db8::1" can not be used for a container, reserved for
the container engine!

ok
test_uid_gid (__main__.TestContainer.test_uid_gid) ... 
Cannot set "gid" without "uid" for container

ok

----------------------------------------------------------------------
Ran 6 tests in 165.714s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/system/test_kernel_options.py
test_bond_interface (__main__.TestKernelModules.test_bond_interface) ... ok
test_bridge_interface (__main__.TestKernelModules.test_bridge_interface) ... ok
test_container_cgroup_support (__main__.TestKernelModules.test_container_cgroup_support) ... ok
test_container_cpu (__main__.TestKernelModules.test_container_cpu) ... ok
test_dropmon_enabled (__main__.TestKernelModules.test_dropmon_enabled) ... ok
test_ip_routing_support (__main__.TestKernelModules.test_ip_routing_support) ... ok
test_qemu_support (__main__.TestKernelModules.test_qemu_support) ... ok
test_synproxy_enabled (__main__.TestKernelModules.test_synproxy_enabled) ... ok
test_vfio (__main__.TestKernelModules.test_vfio) ... ok
test_vmware_support (__main__.TestKernelModules.test_vmware_support) ... ok

----------------------------------------------------------------------
Ran 10 tests in 0.068s

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@dmbaturin
Copy link
Member

Please add a PR description rather than just a task number to the title.

@sever-sever
Copy link
Member

sever-sever commented May 28, 2024

Does “cpus 1.5” means
that you want allocate core 1 and core 5 to the container? Or core 0 and core 4? Or is it CPU period and CPU quota?
It is not clear from the description

https://docs.podman.io/en/latest/markdown/podman-run.1.html#cpus-number

@nvollmar
Copy link
Contributor Author

It is a quota how much CPU resources a container can use.
1.5 CPUs means it can use 1.5 threads worth of the CPU time.

I try to reword the description to make that clearer.

interface-definitions/container.xml.in Outdated Show resolved Hide resolved
src/conf_mode/container.py Outdated Show resolved Hide resolved
@nvollmar nvollmar requested a review from c-po May 28, 2024 06:23
@sever-sever
Copy link
Member

Will you add --cpuset-cpus option? If yes, it will probably be more clear something like this

set container name c1 cpu 0-3
set container name c1 cpu-quota 1.25

@nvollmar
Copy link
Contributor Author

I didn't yet see the need for cpuset on a routing platform.
At least in my case all containers are pretty lightweight and I just want to make sure a bug in one of them can't eat all CPUs.

But renaming it to cpu-quota makes sense so we can easily add cpu pinning later if someone has a need for it.

@c-po
Copy link
Member

c-po commented May 28, 2024

@Mergifyio backport sagitta

Copy link
Contributor

mergify bot commented May 28, 2024

backport sagitta

✅ Backports have been created

@c-po c-po enabled auto-merge May 28, 2024 18:15
@c-po c-po merged commit f047760 into vyos:current May 28, 2024
8 checks passed
c-po added a commit that referenced this pull request May 28, 2024
@nvollmar nvollmar deleted the T6406 branch September 11, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants