You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "phsm (via GitHub)" <gi...@apache.org> on 2025/01/20 16:48:46 UTC
[PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
phsm opened a new pull request, #10221:
URL: https://github.com/apache/cloudstack/pull/10221
### Description
This PR ensures that the cpu shares value is never < 2 to be compatible with Libvirt versions before 9.1.0.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
It turned out that the older libvirt versions, such as Ubuntu 22.04 libvirt 8.0.0 has the hardcoded range of allowed cpu shares values for both cgroupv1 and cgroupv2, which is 2-262144.
This range enforcement was removed in Libvirt 9.1.0, see: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
If a host has lots of cores, and the huge CPU overprovisioning factor is set, then the computed shares value can become 1.
In such case, the following exception is generated on the Cloudstack Agent during provisioning:
`org.libvirt.LibvirtException: unsupported configuration: Value of cputune 'shares' must be in range [2, 262144]`
We noticed it when tried to restart a Shared network with cleanup.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
Steps to reproduce:
1. Get a KVM hypervisor host with lots of cores.
2. Make sure that cgroup v2 is enabled on the hypervisor: `mount | grep -q cgroup2 && echo "yes, enabled"` should echo the output.
3. Set the overprovisioning ratio to some ridiculously high value, e.g. 1000
4. Try to restart any network with cleanup. Since the virtual routers have tiny CPU specs (1 core, 500Mhz by default), it should trigger the bug.
5. The `com.cloud.exception.InsufficientServerCapacityException: No destination found for a deployment for VM instance` will be generated on the management server.
6. On the agent, you will see the following message in the log: `org.libvirt.LibvirtException: unsupported configuration: Value of cputune 'shares' must be in range [2, 262144]`
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
- [ ] test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [x] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
Tested on Ubuntu 22.04 with Libvirt 8.0.0.
After the patch was applied, the error was gone, and the virtual router appeared after the restart with the cpushares value 2.
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
The only effective change that this change does, is excluding the return value "1". It is highly unlikely to break anything.
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926632745
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2796,16 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
+
+ /**
+ * Libvirt < 9.1.0 enforces the range [2, 262144] to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be < 2, then raise it to 2 to fit into the constraint.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (updatedCpuShares < 2) updatedCpuShares = 2;
Review Comment:
@phsm
can you define a constant ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2603062366
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/10221?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.02%. Comparing base [(`a163831`)](https://app.codecov.io/gh/apache/cloudstack/commit/a163831b7e884b11eb8b41b2aff592edc2d33061?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9e981b4`)](https://app.codecov.io/gh/apache/cloudstack/commit/9e981b43356e4e427fdaa2acb46c190d4b23a99c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :exclamation: There is a different number of reports uploaded between BASE (a163831) and HEAD (9e981b4). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (a163831) | HEAD (9e981b4) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #10221 +/- ##
=============================================
- Coverage 16.13% 4.02% -12.12%
=============================================
Files 5639 394 -5245
Lines 494264 32346 -461918
Branches 59899 5714 -54185
=============================================
- Hits 79760 1301 -78459
+ Misses 405684 30897 -374787
+ Partials 8820 148 -8672
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/10221/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/10221/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.02% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/10221/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/10221?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659622026
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12462
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2604124109
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12141
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659715536
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1927032066
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
thanks, lgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2609912334
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1922769110
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,7 +2796,15 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
+
+ /**
+ * Libvirt < 9.1.0 enforces the range [2, 262144] to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be < 2, then raise it to 2 to fit into the constraint.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (updatedCpuShares < 2) updatedCpuShares = 2;
+
+ logger.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
"consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
Review Comment:
Applied it, fixed the unmatched parentheses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2604144503
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2604138958
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926809604
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
@phsm
I checked libvirt code, it looks like the range for cgroup v1 and v2 are different
for v1
```
#define VIR_CGROUP_CPU_SHARES_MIN 2LL
#define VIR_CGROUP_CPU_SHARES_MAX 262144LL
```
and for v2
```
#define VIR_CGROUPV2_WEIGHT_MIN 1LL
#define VIR_CGROUPV2_WEIGHT_MAX 10000LL
```
it would be better to define the constants and add checks for each cgroup version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926778055
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2800,18 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
Review Comment:
Updated it:
Eliminated the premature return inside `if (hostCpuMaxCapacity > 0) {`, so the return value will be always checked against boundaries before return.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2665833593
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2665830326
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926963411
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
Well, not quite.
The root cause of the bug is that the older libvirt versions apply cgroupv1 limits checks to cgroupv2 systems. Libvirt still uses cgroupv2 for the VMs in this case.
So if we apply correctness checks "by the book", then the this will happen in the following situation:
1. The CPU overprovisioning factor is set to some very high value
2. The VM CPU service offering is set to something small.
3. Then Cloudstack will check that the target system uses cgroupv2, which is correct.
4. The CPU shares formula will produce the shares value 1. Which is also an acceptable value according to cgroupv2 allowed range.
5. Libvirt will erroneously check the value against the cgroupv1 range, which is 2-262144.
6. Libvirt will return an error.
So I guess the proper compromise could be this:
```
if cgroupv2 {
<adjust the value to range 2-10000>
} else {
<adjust the value to 2-262144>
}
```
Basically, we need to exclude the value 1 from cgroupv2 to always conform that buggy libvirt check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659713350
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659411292
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1922717244
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,7 +2796,15 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
+
+ /**
+ * Libvirt < 9.1.0 enforces the range [2, 262144] to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be < 2, then raise it to 2 to fit into the constraint.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (updatedCpuShares < 2) updatedCpuShares = 2;
+
+ logger.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
"consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
Review Comment:
```suggestion
LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
"consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926727174
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2800,18 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
Review Comment:
@phsm
I quickly checked the codebase
it looks like for cgroup V1, the hostCpuMaxCapacity is 0.
https://github.com/apache/cloudstack/blob/81e052cfebf5bcdb09e1922393a1753eb6707dae/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java#L3773-L3777
so this pr might not work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926741646
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2800,18 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
Review Comment:
You are correct: the Cloudstack agent determines that the system uses cgroup v2 correctly.
However, the older libvirt applies the cgroupv1 range as the restriction regardless to which cgroup version it uses.
For example, the libvirt code of version 8.0.0 which is used in Ubuntu 22.04: https://github.com/libvirt/libvirt/blob/v8.0.0/src/conf/domain_validate.c#L1536-L1538 . As you see there, the function just checks the value to be within range, without consideration which cgroup version is in action.
So technically this PR does not fix a bug in Cloudstack - it adjust Cloudstack to be compatible with the buggy behavior of older Libvirt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926754161
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2800,18 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
Review Comment:
thanks for the explanation, it makese sense.
I think more changes are needed.
the checks will not be performed for cgroup V1, because hostCpuMaxCapacity is 0, therefore line 2801
```
if (hostCpuMaxCapacity > 0) {
```
it looks like the checks need to be added in `else`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926963411
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
Well, not quite.
The root cause of the bug is that the older libvirt versions apply cgroupv1 limits checks to cgroupv2 systems. Libvirt still uses cgroupv2 for the VMs in this case.
So if we apply correctness checks "by the book", then the this will happen in the following situation:
1. The CPU overprovisioning factor is set to some very high value
2. The VM CPU service offering is set to something small.
3. Then Cloudstack will determine that the target system uses cgroupv2, which is correct.
4. The CPU shares formula will produce the shares value 1. Which is also an acceptable value according to cgroupv2 allowed range.
5. Libvirt will erroneously check the value against the cgroupv1 range, which is 2-262144.
6. Libvirt will return an error.
So I guess the proper compromise could be this:
```
if cgroupv2 {
<adjust the value to range 2-10000>
} else {
<adjust the value to 2-262144>
}
```
Basically, we need to exclude the value 1 from cgroupv2 to always conform that buggy libvirt check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1927025742
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
I believe I have implemented it as you suggested.
As for the "cgroup_v2" variable: I'm not quite sure how to properly place it, therefore I just relied on the existing condition `if (hostCpuMaxCapacity > 0)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659724682
<b>[SF] Trillian Build Failed (tid-12413)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926741646
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2800,18 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
Review Comment:
You are correct: the Cloudstack agent correctly determines that the system uses cgroup v2.
However, the older libvirt applies the cgroupv1 range as the restriction regardless to which cgroup version it uses.
For example, the libvirt code of version 8.0.0 which is used in Ubuntu 22.04: https://github.com/libvirt/libvirt/blob/v8.0.0/src/conf/domain_validate.c#L1536-L1538 . As you see there, the function just checks the value to be within range, without consideration which cgroup version is in action.
So technically this PR does not fix a bug in Cloudstack - it adjust Cloudstack to be compatible with the buggy behavior of older Libvirt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2606025977
<b>[SF] Trillian test result (tid-12144)</b>
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51870 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10221-t12144-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_11_isolated_network_with_dynamic_routed_mode | `Error` | 2.30 | test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode | `Error` | 3.41 | test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode | `Error` | 3.41 | test_ipv4_routing.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2609916781
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2667529234
<b>[SF] Trillian test result (tid-12462)</b>
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51811 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10221-t12462-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_11_isolated_network_with_dynamic_routed_mode | `Error` | 2.31 | test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode | `Error` | 3.38 | test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode | `Error` | 3.38 | test_ipv4_routing.py
test_06_purge_expunged_vm_background_task | `Failure` | 381.60 | test_purge_expunged_vms.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1922786484
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2796,16 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
+
+ /**
+ * Libvirt < 9.1.0 enforces the range [2, 262144] to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be < 2, then raise it to 2 to fit into the constraint.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (updatedCpuShares < 2) updatedCpuShares = 2;
+
+ LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
Review Comment:
sorry @phsm , meant this
```suggestion
LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
"consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2603910309
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2609070965
> [SF] Trillian test result (tid-12144) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 51870 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10221-t12144-kvm-ol8.zip Smoke tests completed. 140 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:
> Test Result Time (s) Test File
> test_11_isolated_network_with_dynamic_routed_mode `Error` 2.30 test_ipv4_routing.py
> test_12_vpc_and_tier_with_dynamic_routed_mode `Error` 3.41 test_ipv4_routing.py
> test_12_vpc_and_tier_with_dynamic_routed_mode `Error` 3.41 test_ipv4_routing.py
Looks like this test failure is not related to my changes. I checked the mgmt server logs by this link:
```
2025-01-21 12:52:45,288 DEBUG [c.c.a.ApiServlet] (qtp253011924-20:[ctx-fbcd068c]) (logid:0973a8ad) ===START=== 10.0.112.130 -- GET account=testD1-TestIpv4Routing-ITEMQ5&bgppeerids=d692cabe-00e8-4b00-ab7b-476deab31da9&cidr=172.31.8.0%2F22&displaytext=TestVPC-TZU9IM&domainid=74a01d7
c-09ba-4737-aac7-b5f3966100ba&name=TestVPC-AJ4H2C&start=False&vpcofferingid=446cdc68-4cc7-4822-ace9-e54bfded5bac&zoneid=d3b43424-16f5-4120-8cfa-9d08e7b367ac&command=createVPC&response=json&apiKey=LIN6rqXuaJwMPfGYFh13qDwYz5VNNz1J2J6qIOWcd3oLQOq0WtD4CwRundBL6rzXToa3lQOC_vKjI3nkHtiD8Q&
signature=l7vSIHJB7VOufpqhCaiu0YyrK9A%3D
.......
2025-01-21 12:52:45,368 ERROR [c.c.a.ApiServer] (qtp253011924-20:[ctx-fbcd068c, ctx-81d91df8, ctx-2e46861c]) (logid:0973a8ad) unhandled exception executing api command: [Ljava.lang.String;@7e035717 java.lang.NullPointerException
at java.base/java.util.Objects.requireNonNull(Objects.java:209)
at com.cloud.bgp.BGPServiceImpl.allocateASNumber(BGPServiceImpl.java:258)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926708523
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,8 +2796,16 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
+
+ /**
+ * Libvirt < 9.1.0 enforces the range [2, 262144] to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be < 2, then raise it to 2 to fit into the constraint.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (updatedCpuShares < 2) updatedCpuShares = 2;
Review Comment:
Updated.
In fact, I also introduced similar check against the upper value of the allowed range.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#discussion_r1926978836
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2796,12 +2800,23 @@ public int calculateCpuShares(VirtualMachineTO vmTO) {
if (hostCpuMaxCapacity > 0) {
int updatedCpuShares = (int) Math.ceil((requestedCpuShares * CGROUP_V2_UPPER_LIMIT) / (double) hostCpuMaxCapacity);
- LOGGER.debug(String.format("This host utilizes cgroupv2 (as the max shares value is [%s]), thus, the VM requested shares of [%s] will be converted to " +
- "consider the host limits; the new CPU shares value is [%s].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares));
- return updatedCpuShares;
- }
- LOGGER.debug(String.format("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [%s] will not be " +
- "converted.", requestedCpuShares));
+
+ LOGGER.debug("This host utilizes cgroupv2 (as the max shares value is [{}]), thus, the VM requested shares of [{}] will be converted to " +
+ "consider the host limits; the new CPU shares value is [{}].", hostCpuMaxCapacity, requestedCpuShares, updatedCpuShares);
+ requestedCpuShares = updatedCpuShares;
+ } else {
+ LOGGER.debug("This host does not have a maximum CPU shares set; therefore, this host utilizes cgroupv1 and the VM requested CPU shares [{}] will not be " +
+ "converted.", requestedCpuShares);
+ }
+
+ /**
+ * Libvirt < 9.1.0 enforces the same value range to both cgroupv1 and cgroupv2.
+ * Therefore, if the shares value is determined to be outside of boundaries,
+ * then bring it to the minimum or maximum allowed value.
+ * See: https://github.com/libvirt/libvirt/commit/38af6497610075e5fe386734b87186731d4c17ac
+ */
+ if (requestedCpuShares < LIBVIRT_CGROUPV1_CPU_SHARES_MIN) requestedCpuShares = LIBVIRT_CGROUPV1_CPU_SHARES_MIN;
Review Comment:
> So I guess the proper compromise could be this:
>
> ```
> if cgroupv2 {
> <adjust the value to range 2-10000>
> } else {
> <adjust the value to 2-262144>
> }
> ```
@phsm
this is exactly what I wanted to say.
currently hostCpuMaxCapacity is set if cgroup is v2.
so we can consider `if (hostCpuMaxCapacity > 0)` as `if (cgroupv2)`.
of couse it is better to add a variable like `cgroup_version` or `cgroup_v2`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2610077072
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12192
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2603906794
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 merged PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659876719
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659889036
<b>[SF] Trillian Build Failed (tid-12415)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "Pearl1594 (via GitHub)" <gi...@apache.org>.
Pearl1594 commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659871921
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: enforce the minimum cgroup cpu shares value to 2 [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10221:
URL: https://github.com/apache/cloudstack/pull/10221#issuecomment-2659415221
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org