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

WIP: Add limits.cpu.pin_strategy setting to disable CPU auto pinning by default #14171

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kadinsayani
Copy link
Contributor

@kadinsayani kadinsayani commented Sep 26, 2024

Fixes #14133.

This pull request introduces a configuration option for instances, limits.cpu.pin_strategy. This option allows users to specify the strategy for CPU auto pinning. The possible values are none (disables CPU auto pinning - also the new default) and auto (enables CPU auto pinning).

@github-actions github-actions bot added the Documentation Documentation needs updating label Sep 26, 2024
@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from 460df45 to ffe7a87 Compare September 26, 2024 23:33
Copy link

Heads up @mionaalex - the "Documentation" label was applied to this issue.

@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from ffe7a87 to bc4de3b Compare September 26, 2024 23:34
@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from ab52340 to 96fdbdf Compare September 27, 2024 18:41
@kadinsayani kadinsayani changed the title WIP: Add limits.cpu.pin_strategy setting to disable VM CPU auto pinning by default Add limits.cpu.pin_strategy setting to disable VM CPU auto pinning by default Sep 27, 2024
@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from 96fdbdf to a8e75ba Compare September 27, 2024 19:04
@kadinsayani kadinsayani marked this pull request as ready for review September 27, 2024 19:18
doc/reference/instance_options.md Outdated Show resolved Hide resolved
doc/reference/instance_options.md Outdated Show resolved Hide resolved
doc/reference/instance_options.md Outdated Show resolved Hide resolved
@kadinsayani
Copy link
Contributor Author

kadinsayani commented Sep 27, 2024

If we modify the current tests in lxd-ci to launch a VM with the limits.cpu.pin_strategy set, there will be a regression since earlier LXD versions will return Error: Failed instance creation: Failed creating instance record: Unknown configuration key: limits.cpu.pin_strategy, causing the cpu-vm tests to fail.

Currently, the checks in lxd-ci for cpu pinning only run if the feature is present:

if journalctl --quiet --no-hostname --no-pager --boot=0 --unit=snap.lxd.daemon.service | grep "Scheduler: virtual-machine"; then
...

However, due to my implementation of checking the config setting in deviceTaskBalance, the log message Scheduler: virtual-machine is printed regardless of whether auto cpu pinning is enabled or not. Hence, I have added a new log message which we can use to check if the feature is present.

if c.Type() == instancetype.VM {
	if conf["limits.cpu.pin_strategy"] == "none" {
		continue
	} else if conf["limits.cpu.pin_strategy"] == "auto" {
		logger.Debugf("CPU auto pinning enabled for %q", c.Name())
	}
}

Therefore, I think the only change required (for now) to ensure we don't introduce a regression to lxd-ci is to change the check to look for the new log message.

For reference: canonical/lxd-ci@972ef98.

Please let me know if you have any suggestions 😄

@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from a8e75ba to d83c50c Compare September 27, 2024 23:35
@tomponline
Copy link
Member

If we modify the current tests in lxd-ci to launch a VM with the limits.cpu.pin_strategy set, there will be a regression since earlier LXD versions will return Error: Failed instance creation: Failed creating instance record: Unknown configuration key: limits.cpu.pin_strategy, causing the cpu-vm tests to fail.

You will need to introduce a new API extension for the new config key and can test for support of that when testing to enable the functionality.

@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from d83c50c to 7d85357 Compare October 1, 2024 14:06
@github-actions github-actions bot added the API Changes to the REST API label Oct 1, 2024
@simondeziel
Copy link
Member

@kadinsayani I couldn't find any reference to back that but I think it's best for the commit adding the API extension to be the first in the PR. This way, when @tomponline does the backporting (if any, dunno for this one) he can quickly identify that a collection of commits need to be considered at once for the backport.

@kadinsayani
Copy link
Contributor Author

@kadinsayani I couldn't find any reference to back that but I think it's best for the commit adding the API extension to be the first in the PR. This way, when @tomponline does the backporting (if any, dunno for this one) he can quickly identify that a collection of commits need to be considered at once for the backport.

Sounds good, thanks for letting me know!

@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from 7d85357 to 0ca7deb Compare October 1, 2024 14:50
@kadinsayani
Copy link
Contributor Author

kadinsayani commented Oct 1, 2024

@tomponline will we be back porting the commit containing the new API extension to 6.1? Currently, the tests in lxd-ci are relying on LXD daemon logs for checking if the feature is present:

https://github.com/mihalicyn/lxd-ci/blob/972ef98d660c3c2efba3d8aa7408126a1b555ff9/tests/cpu-vm#L12

@tomponline
Copy link
Member

@tomponline will we be back porting the commit containing the new API extension to 6.1? Currently, the tests in lxd-ci are relying on LXD daemon logs for checking if the feature is present:

yes we can because it defaults to disabled

simondeziel
simondeziel previously approved these changes Oct 1, 2024
doc/reference/instance_options.md Outdated Show resolved Hide resolved
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@tomponline
Copy link
Member

@kadinsayani please can you ensure that the change to disable auto pin for containers is separate so i can choose to backport it or not.

Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Unless `limits.cpu.pin_strategy` is set to auto, VM CPU auto pinning is
disabled

Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@kadinsayani kadinsayani force-pushed the 14133-vm-cpu-auto-pinning-setting branch from 0ca7deb to 3c071ad Compare October 3, 2024 20:26
@kadinsayani kadinsayani changed the title Add limits.cpu.pin_strategy setting to disable VM CPU auto pinning by default Add limits.cpu.pin_strategy setting to disable CPU auto pinning by default Oct 3, 2024
@kadinsayani kadinsayani marked this pull request as draft October 3, 2024 20:33
@kadinsayani kadinsayani changed the title Add limits.cpu.pin_strategy setting to disable CPU auto pinning by default WIP: Add limits.cpu.pin_strategy setting to disable CPU auto pinning by default Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VM CPU auto pinning causes slowdowns and stealtime
3 participants