Merge pull request #9565 from vespa-engine/freva/fix-vcpu-to-shares-ratio

Fix vcpu to shares ratio

3 files changed, 72 insertions, 23 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
index c3c4ca19555..70ba58cd9cf 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java
@@ -35,8 +35,8 @@ public class ContainerResources {
 
         if (cpus < 0)
             throw new IllegalArgumentException("CPUs must be a positive number or 0 for unlimited, was " + cpus);
-        if (cpuShares < 0)
-            throw new IllegalArgumentException("CPU shares must be a positive integer or 0 for unlimited, was " + cpuShares);
+        if (cpuShares != 0 && (cpuShares < 2 || cpuShares > 262_144))
+            throw new IllegalArgumentException("CPU shares must be a positive integer in [2, 262144] or 0 for unlimited, was " + cpuShares);
         if (memoryBytes < 0)
             throw new IllegalArgumentException("memoryBytes must be a positive integer or 0 for unlimited, was " + memoryBytes);
     }
@@ -54,7 +54,7 @@ public class ContainerResources {
      */
     public static ContainerResources from(double maxVcpu, double minVcpu, double memoryGb) {
         return new ContainerResources(maxVcpu,
-                                      (int) Math.round(10 * minVcpu),
+                                      (int) Math.round(32 * minVcpu),
                                       (long) ((1L << 30) * memoryGb));
     }
 
diff --git a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/ContainerResourcesTest.java b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/ContainerResourcesTest.java
new file mode 100644
index 00000000000..daf4639ad29
--- /dev/null
+++ b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/ContainerResourcesTest.java
@@ -0,0 +1,49 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.dockerapi;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * @author freva
+ */
+public class ContainerResourcesTest {
+
+    @Test
+    public void verify_unlimited() {
+        assertEquals(-1, ContainerResources.UNLIMITED.cpuQuota());
+        assertEquals(100_000, ContainerResources.UNLIMITED.cpuPeriod());
+        assertEquals(0, ContainerResources.UNLIMITED.cpuShares());
+    }
+
+    @Test
+    public void validate_shares() {
+        new ContainerResources(0, 0, 0);
+        new ContainerResources(0, 2, 0);
+        new ContainerResources(0, 2048, 0);
+        new ContainerResources(0, 262_144, 0);
+
+        assertThrows(IllegalArgumentException.class, () -> new ContainerResources(0, -1, 0)); // Negative shares not allowed
+        assertThrows(IllegalArgumentException.class, () -> new ContainerResources(0, 1, 0)); // 1 share not allowed
+        assertThrows(IllegalArgumentException.class, () -> new ContainerResources(0, 262_145, 0));
+    }
+
+    @Test
+    public void cpu_shares_scaling() {
+        ContainerResources resources = ContainerResources.from(5.3, 2.5, 0);
+        assertEquals(530_000, resources.cpuQuota());
+        assertEquals(100_000, resources.cpuPeriod());
+        assertEquals(80, resources.cpuShares());
+    }
+
+    private static void assertThrows(Class<? extends Throwable> clazz, Runnable runnable) {
+        try {
+            runnable.run();
+            fail("Expected " + clazz);
+        } catch (Throwable e) {
+            if (!clazz.isInstance(e)) throw e;
+        }
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
index f999ef0ffc6..8c762736b8a 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
@@ -209,7 +209,7 @@ public class NodeAgentImpl implements NodeAgent {
 
     private void startContainer(NodeAgentContext context) {
         ContainerData containerData = createContainerData(context);
-        dockerOperations.createContainer(context, containerData, getContainerResources(context.node()));
+        dockerOperations.createContainer(context, containerData, getContainerResources(context));
         dockerOperations.startContainer(context);
         lastCpuMetric = new CpuUsageReporter();
 
@@ -221,7 +221,7 @@ public class NodeAgentImpl implements NodeAgent {
     private Optional<Container> removeContainerIfNeededUpdateContainerState(
             NodeAgentContext context, Optional<Container> existingContainer) {
         if (existingContainer.isPresent()) {
-            Optional<String> reason = shouldRemoveContainer(context.node(), existingContainer.get());
+            Optional<String> reason = shouldRemoveContainer(context, existingContainer.get());
             if (reason.isPresent()) {
                 removeContainer(context, existingContainer.get(), reason.get(), false);
                 return Optional.empty();
@@ -291,28 +291,29 @@ public class NodeAgentImpl implements NodeAgent {
         }
     }
 
-    private Optional<String> shouldRemoveContainer(NodeSpec node, Container existingContainer) {
-        final NodeState nodeState = node.getState();
+    private Optional<String> shouldRemoveContainer(NodeAgentContext context, Container existingContainer) {
+        final NodeState nodeState = context.node().getState();
         if (nodeState == NodeState.dirty || nodeState == NodeState.provisioned) {
             return Optional.of("Node in state " + nodeState + ", container should no longer be running");
         }
-        if (node.getWantedDockerImage().isPresent() && !node.getWantedDockerImage().get().equals(existingContainer.image)) {
+        if (context.node().getWantedDockerImage().isPresent() &&
+                !context.node().getWantedDockerImage().get().equals(existingContainer.image)) {
             return Optional.of("The node is supposed to run a new Docker image: "
-                    + existingContainer.image.asString() + " -> " + node.getWantedDockerImage().get().asString());
+                    + existingContainer.image.asString() + " -> " + context.node().getWantedDockerImage().get().asString());
         }
         if (!existingContainer.state.isRunning()) {
             return Optional.of("Container no longer running");
         }
 
-        if (currentRebootGeneration < node.getWantedRebootGeneration()) {
+        if (currentRebootGeneration < context.node().getWantedRebootGeneration()) {
             return Optional.of(String.format("Container reboot wanted. Current: %d, Wanted: %d",
-                    currentRebootGeneration, node.getWantedRebootGeneration()));
+                    currentRebootGeneration, context.node().getWantedRebootGeneration()));
         }
 
         // Even though memory can be easily changed with docker update, we need to restart the container
         // for proton to pick up the change. If/when proton could detect available memory correctly (rather than reading
         // VESPA_TOTAL_MEMORY_MB env. variable set in DockerOperation), it would be enough with a services restart
-        ContainerResources wantedContainerResources = getContainerResources(node);
+        ContainerResources wantedContainerResources = getContainerResources(context);
         if (!wantedContainerResources.equalsMemory(existingContainer.resources)) {
             return Optional.of("Container should be running with different memory allocation, wanted: " +
                     wantedContainerResources.toStringMemory() + ", actual: " + existingContainer.resources.toStringMemory());
@@ -349,7 +350,7 @@ public class NodeAgentImpl implements NodeAgent {
 
 
     private void updateContainerIfNeeded(NodeAgentContext context, Container existingContainer) {
-        ContainerResources wantedContainerResources = getContainerResources(context.node());
+        ContainerResources wantedContainerResources = getContainerResources(context);
         if (wantedContainerResources.equalsCpu(existingContainer.resources)) return;
         context.log(logger, "Container should be running with different CPU allocation, wanted: %s, current: %s",
                 wantedContainerResources.toStringCpu(), existingContainer.resources.toStringCpu());
@@ -359,17 +360,16 @@ public class NodeAgentImpl implements NodeAgent {
         dockerOperations.updateContainer(context, wantedContainerResources);
     }
 
-    private ContainerResources getContainerResources(NodeSpec node) {
-        double cpuCap = node.getOwner()
-                .map(NodeOwner::asApplicationId)
-                .map(appId -> containerCpuCap.with(FetchVector.Dimension.APPLICATION_ID, appId.serializedForm()))
-                .orElse(containerCpuCap)
-                .value() * node.getMinCpuCores();
+    private ContainerResources getContainerResources(NodeAgentContext context) {
+        double cpuCap = context.zoneId().environment() == Environment.dev ?
+                0 :
+                context.node().getOwner()
+                        .map(NodeOwner::asApplicationId)
+                        .map(appId -> containerCpuCap.with(FetchVector.Dimension.APPLICATION_ID, appId.serializedForm()))
+                        .orElse(containerCpuCap)
+                        .value() * context.node().getMinCpuCores();
 
-        if ( contextSupplier.currentContext().zoneId().environment() == Environment.dev) // don't limit cpu
-            return ContainerResources.from(0, node.getMinCpuCores(), node.getMinMainMemoryAvailableGb());
-        else
-            return ContainerResources.from(cpuCap, node.getMinCpuCores(), node.getMinMainMemoryAvailableGb());
+        return ContainerResources.from(cpuCap, context.node().getMinCpuCores(), context.node().getMinMainMemoryAvailableGb());
     }