Require an offering's overcommit ratio to be at least 1

The value means the same thing at every scope: how far a VM's declared
size is inflated relative to what it really holds. 1 is not overcommitted
at all, which is how an offering is exempted from its cluster's
overcommit.

Below 1 would mean the opposite, reserving more than the VM asked for.
That is a different feature and nobody asked for it, but it was reachable
by mistyping 1 as 0.1 - which would have charged the VM ten times its
size.

  - rejected when the offering is created
  - ignored with a warning at start, for a detail written directly

An offering may still declare a ratio higher than its cluster's. The
value is absolute rather than relative to the cluster, so there is no
contradiction in an offering being more oversubscribed than the cluster
default; it is logged so it is not a surprise.

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

PendingReleaseNotes

@@ -41,8 +41,15 @@ example.ver.1 > example.ver.2:
     size while a stripped down Linux should fit on a 2.88MB floppy.
 
   * A service offering may now set its own overcommit ratio, overriding its cluster's, using the
-    cpuOvercommitRatio and memoryOvercommitRatio service offering details. Absent, the cluster's
-    ratio applies as before, so nothing changes for existing offerings.
+    cpuOvercommitRatio and memoryOvercommitRatio service offering details - the same detail keys
+    already used at cluster and VM scope. Absent, the cluster's ratio applies as before, so nothing
+    changes for existing offerings.
+
+    The value means the same thing at every scope: how far a VM's declared size is inflated
+    relative to what it really holds. 1 means not overcommitted at all, which is how an offering is
+    exempted from its cluster's overcommit. Values below 1 would mean reserving more than the VM
+    asked for and are rejected. An offering may declare a ratio higher than its cluster's; this is
+    logged.
 
     An offering with a memory ratio of 1 on an overcommitted cluster is charged its full request
     and keeps all of it: free page reporting and balloon auto-deflate are turned off for its VMs,

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -1863,8 +1863,8 @@ private void updateOverCommitRatioForVmProfile(VirtualMachineProfile vmProfile,
         final float clusterCpuRatio = Float.parseFloat(clusterDetailCpu.getValue());
         final float clusterRamRatio = Float.parseFloat(clusterDetailRam.getValue());
 
-        final Float offeringCpuRatio = offeringOverCommitRatio(vmProfile, VmDetailConstants.CPU_OVER_COMMIT_RATIO);
-        final Float offeringRamRatio = offeringOverCommitRatio(vmProfile, VmDetailConstants.MEMORY_OVER_COMMIT_RATIO);
+        final Float offeringCpuRatio = offeringOverCommitRatio(vmProfile, VmDetailConstants.CPU_OVER_COMMIT_RATIO, clusterCpuRatio);
+        final Float offeringRamRatio = offeringOverCommitRatio(vmProfile, VmDetailConstants.MEMORY_OVER_COMMIT_RATIO, clusterRamRatio);
 
         final float cpuRatio = offeringCpuRatio != null ? offeringCpuRatio : clusterCpuRatio;
         final float ramRatio = offeringRamRatio != null ? offeringRamRatio : clusterRamRatio;
@@ -1884,24 +1884,31 @@ private void updateOverCommitRatioForVmProfile(VirtualMachineProfile vmProfile,
      * An offering setting a ratio of 1 on an overcommitted cluster is how infrastructure VMs are
      * kept off the overcommit: they are charged their full request and hold all of it.
      */
-    protected Float offeringOverCommitRatio(VirtualMachineProfile vmProfile, String key) {
+    protected Float offeringOverCommitRatio(VirtualMachineProfile vmProfile, String key, float clusterRatio) {
         String offeringRatio = _serviceOfferingDetailsDao.getDetail(vmProfile.getServiceOfferingId(), key);
         if (offeringRatio == null) {
             return null;
         }
+        float ratio;
         try {
-            float ratio = Float.parseFloat(offeringRatio);
-            if (ratio <= 0) {
-                logger.warn("Ignoring {} of {} on service offering {}: it must be greater than zero.",
-                        key, offeringRatio, vmProfile.getServiceOfferingId());
-                return null;
-            }
-            return ratio;
+            ratio = Float.parseFloat(offeringRatio);
         } catch (NumberFormatException e) {
-            logger.warn("Ignoring {} of {} on service offering {}: it is not a number.",
+            logger.warn("Ignoring {} of [{}] on service offering {}: it is not a number.",
+                    key, offeringRatio, vmProfile.getServiceOfferingId());
+            return null;
+        }
+        if (ratio < 1) {
+            // rejected when the offering is created; only reachable by writing the detail directly
+            logger.warn("Ignoring {} of {} on service offering {}: it must be at least 1.",
                     key, offeringRatio, vmProfile.getServiceOfferingId());
             return null;
         }
+        if (ratio > clusterRatio) {
+            logger.info("Service offering {} declares {} of {}, more than its cluster's {}, so its VMs are "
+                            + "oversubscribed further than the cluster default.",
+                    vmProfile.getServiceOfferingId(), key, ratio, clusterRatio);
+        }
+        return ratio;
     }
 
     /**

engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerOverCommitTest.java

@@ -75,25 +75,41 @@ private void vmDetailIs(String key, String value) {
     @Test
     public void testOfferingWithNoDetailInheritsTheCluster() {
         offeringSays(CPU, null);
-        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU));
+        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f));
     }
 
     @Test
     public void testOfferingRatioIsUsed() {
         offeringSays(CPU, "1");
-        assertEquals(1.0f, vmManager.offeringOverCommitRatio(profile(), CPU), 1e-6);
+        assertEquals(1.0f, vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f), 1e-6);
     }
 
     @Test
     public void testNonsenseOfferingRatioInheritsTheCluster() {
         offeringSays(CPU, "not a number");
-        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU));
+        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f));
     }
 
     @Test
     public void testNonPositiveOfferingRatioInheritsTheCluster() {
         offeringSays(CPU, "0");
-        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU));
+        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f));
+    }
+
+    @Test
+    public void testUndercommitIsRejected() {
+        // below 1 would reserve more than the VM asked for. That is a different feature, and this
+        // is how a mistyped "1" would otherwise land
+        offeringSays(CPU, "0.1");
+        assertNull(vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f));
+    }
+
+    @Test
+    public void testAnOfferingMayBeMoreOvercommittedThanItsCluster() {
+        // the value is absolute - "this offering is oversubscribed 20 times" - so it does not have
+        // to sit below whatever the cluster happens to be set to
+        offeringSays(CPU, "20");
+        assertEquals(20.0f, vmManager.offeringOverCommitRatio(profile(), CPU, 10.0f), 1e-6);
     }
 
     // --- what gets recorded on the VM ---

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -3743,8 +3743,13 @@ public static VMLeaseManager.ExpiryAction validateAndGetLeaseExpiryAction(Intege
     }
 
     /**
-     * A service offering may set its own overcommit ratio to override its cluster's, most usefully
-     * a ratio of 1 to keep infrastructure VMs off an overcommitted cluster's overcommit.
+     * A service offering may declare its own overcommit ratio, overriding whatever its cluster is
+     * set to. The value means the same thing at every scope: how far the declared size is inflated
+     * relative to what the VM really holds. 1 is not overcommitted at all, which is how a VM is
+     * exempted from a cluster's overcommit.
+     *
+     * Below 1 would mean the opposite - reserving more than the VM asked for. That is a different
+     * feature and is rejected rather than being reachable by mistyping 1 as 0.1.
      */
     protected void validateOverCommitRatioInServiceOfferingDetail(String key, String value) {
         float ratio;
@@ -3753,8 +3758,10 @@ protected void validateOverCommitRatioInServiceOfferingDetail(String key, String
         } catch (NumberFormatException | NullPointerException e) {
             throw new InvalidParameterValueException(String.format("Service offering detail %s must be a number, got [%s].", key, value));
         }
-        if (ratio <= 0) {
-            throw new InvalidParameterValueException(String.format("Service offering detail %s must be greater than zero, got [%s].", key, value));
+        if (ratio < 1) {
+            throw new InvalidParameterValueException(String.format(
+                    "Service offering detail %s must be at least 1, got [%s]. 1 means the offering is not "
+                            + "overcommitted; higher values mean it is oversubscribed by that factor.", key, value));
         }
     }
 

server/src/test/java/com/cloud/configuration/ConfigurationManagerOverCommitDetailTest.java

@@ -38,6 +38,16 @@ public void testRatiosAreAccepted() {
         validate("10.5");
     }
 
+    @Test(expected = InvalidParameterValueException.class)
+    public void testUndercommitIsRejected() {
+        validate("0.1");
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testJustBelowOneIsRejected() {
+        validate("0.99");
+    }
+
     @Test(expected = InvalidParameterValueException.class)
     public void testZeroIsRejected() {
         validate("0");