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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to