Default machine CPUs to Cores/2 #1659
Conversation
1 CPU core typically is not enough for most use cases, so we default to available cores/2 for new machines. Signed-off-by: Ashley Cui <[email protected]>
|
LGTM |
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashley-cui, baude The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@ashley-cui regarding dan's question, should it be used for the default config or do you think it is good enough as is ? |
|
The question then is your code used at all? |
|
Yes, the CPUS/2 is passed as the default. The value can also be set from inside containers.conf |
|
/lgtm |
|
Reminder to only merge c/common PRs when an accompanying Podman PR is green. Not a big deal but it's the 2nd time in 24 hours that some other PRs broke my work in containers/podman#20088. |
The processing and setting of the static and volume directories was scattered across the code base (including c/common) leading to subtle errors that surfaced in containers#19938. There were multiple issues that I try to summarize below: - c/common loaded the graphroot from c/storage to set the defaults for static and volume dir. That ignored Podman's --root flag and surfaced in containers#19938 and other bugs. c/common does not set the defaults anymore which gives Podman the ability to detect when the user/admin configured a custom directory (not empty value). - When parsing the CLI, Podman (ab)uses containers.conf structures to set the defaults but also to override them in case the user specified a flag. The --root flag overrode the static dir which is wrong and broke a couple of use cases. Now there is a dedicated field for in the "PodmanConfig" which also includes a containers.conf struct. - The defaults for static and volume dir and now being set correctly and adhere to --root. - The CONTAINERS_CONF_OVERRIDE env variable has not been passed to the cleanup process. I believe that _all_ env variables should be passed to conmon to avoid such subtle bugs. Overall I find that the code and logic is scattered and hard to understand and follow. I refrained from larger refactorings as I really just want to get containers#19938 fixed and then go back to other priorities. containers/common#1659 broke three pkg/machine tests. Those have been commented out until getting fixed. Fixes: containers#19938 Signed-off-by: Valentin Rothberg <[email protected]>
1 CPU core typically is not enough for most use cases, so we default to available cores/2 for new machines.
Fixes: containers/podman#17066