Default machine CPUs to Cores/2 #1659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <acui@redhat.com>
|
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. |
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 22, 2023
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 <vrothberg@redhat.com>
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 22, 2023
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 <vrothberg@redhat.com>
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 22, 2023
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 <vrothberg@redhat.com>
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 25, 2023
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 <vrothberg@redhat.com>
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 25, 2023
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 <vrothberg@redhat.com>
vrothberg
added a commit
to vrothberg/libpod
that referenced
this pull request
Sep 25, 2023
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 <vrothberg@redhat.com>
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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