-
Notifications
You must be signed in to change notification settings - Fork 261
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
Handle many vCPUs #293
Handle many vCPUs #293
Conversation
...broker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/LibvirtVmXmlBuilder.java
Outdated
Show resolved
Hide resolved
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
great, LGTM |
Seems to be working (as for value in the domain.xml). NVDIMM calculation wasn't tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's good that we avoid the changes in most of the common configurations for which it is not needed and that we try to minimize the amount of resources when it is applied, yet - it sounds like kind of experience so better to make it a bit more configurable
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
...vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/builder/vminfo/VmInfoBuildUtils.java
Outdated
Show resolved
Hide resolved
Additionally the hard max vCPU limit is not strictly enforced again, it can be lifted up to LEGACY_MAX_SOCKETS based value again unless the newly introduced limit ManyVmCpus is exceeded. Not yet tested and tests fail (due to missing mockings). I don't know how to put the PR back to draft status. |
Tests fixed. |
With the lifted limit on the maximum number of CPU sockets implemented in commit a4c0858, the maximum possible number of vCPUs increased significantly. But it is not without its price. The increased maximum number of vCPUs reduces memory available to the guest OS and may cause some problems, especially with UEFI firmware. There is already MaxNumOfCpusCoefficient config value limiting the maximum number of vCPUs. But it is not enforced strictly, the maximum number of vCPUs can still be significantly higher, to be consistent with the previous behavior. Let’s be more cautious and clearer and apply MaxNumOfCpusCoefficient strictly, unless the maximum number of vCPUs is not very large, in the sense of the newly introduced ManyVmCpus config value. Bug-Url: https://bugzilla.redhat.com/2075486
Large VMs using UEFI may not start if the TSEG size is not big enough. There is no clear rule when the TSEG size must be increased. We increase it in the following cases: - If the maximum number of VM vCPUs is at least 128. - If the maximum memory is at least 16 GB, i.e. the memory taken from the guest by changing the TSEG size is negligible. We increase the TSEG size by wild-guess values based on various comments from Bugzilla, see the comment in the code for more details. As the TSEG size is taken from the memory available to the guest, we do not want to make it unnecessarily large by default. On the other hand, it’s better to be on the safe side and allocate a bit more than perhaps needed rather than causing the VM not to start. We introduce config values for both the limits mentioned above, in case the selected values don’t work well enough. Bug-Url: https://bugzilla.redhat.com/2075486
Verified: Quite many combinations to check but I believe it produces expected domain XMLs. |
/ost |
LGTM |
See the commit messages for more details.
Bug-Url: https://bugzilla.redhat.com/2075486