Fix docker CPU usage metric

5 files changed, 6 insertions, 51 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
index 6aecd0813b4..de4aacff09f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
@@ -37,7 +37,6 @@ import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import static com.yahoo.vespa.defaults.Defaults.getDefaults;
@@ -46,7 +45,6 @@ import static com.yahoo.vespa.defaults.Defaults.getDefaults;
  * @author freva
  */
 public class StorageMaintainer {
-    private static final Pattern TOTAL_MEMORY_PATTERN = Pattern.compile("^MemTotal:\\s*(?<totalMem>\\d+) kB$", Pattern.MULTILINE);
     private static final ContainerName NODE_ADMIN = new ContainerName("node-admin");
     private static final ObjectMapper objectMapper = new ObjectMapper();
     private static Optional<String> kernelVersion = Optional.empty();
@@ -56,7 +54,6 @@ public class StorageMaintainer {
     private final Docker docker;
     private final Environment environment;
     private final Clock clock;
-    private double hostTotalMemoryGb = 0;
 
     private Map<ContainerName, MaintenanceThrottler> maintenanceThrottlerByContainerName = new ConcurrentHashMap<>();
 
@@ -169,21 +166,6 @@ public class StorageMaintainer {
         }
     }
 
-    public double getHostTotalMemoryGb() {
-        if (hostTotalMemoryGb == 0) {
-            readMeminfo().ifPresent(memInfo -> {
-                Matcher matcher = TOTAL_MEMORY_PATTERN.matcher(memInfo);
-                if (matcher.find()) {
-                    hostTotalMemoryGb = Integer.valueOf(matcher.group("totalMem")) / 1024d / 1024;
-                } else {
-                    logger.log(LogLevel.WARNING, "Failed to parse total memory from meminfo: " + memInfo);
-                }
-            });
-        }
-
-        return hostTotalMemoryGb;
-    }
-
     /**
      * Deletes old log files for vespa, nginx, logstash, etc.
      */
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 6ee827d8db8..46990cf3630 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
@@ -520,6 +520,7 @@ public class NodeAgentImpl implements NodeAgent {
         Docker.ContainerStats stats = containerStats.get();
         final String APP = MetricReceiverWrapper.APPLICATION_NODE;
         final long bytesInGB = 1 << 30;
+        final int totalNumCpuCores = ((List<Number>) ((Map) stats.getCpuStats().get("cpu_usage")).get("percpu_usage")).size();
         final long cpuContainerTotalTime = ((Number) ((Map) stats.getCpuStats().get("cpu_usage")).get("total_usage")).longValue();
         final long cpuSystemTotalTime = ((Number) stats.getCpuStats().get("system_cpu_usage")).longValue();
         final long memoryTotalBytes = ((Number) stats.getMemoryStats().get("limit")).longValue();
@@ -532,22 +533,21 @@ public class NodeAgentImpl implements NodeAgent {
         // CPU usage by a container as percentage of total host CPU, cpuPercentageOfHost, is given by dividing used
         // CPU time by the container with CPU time used by the entire system.
         // CPU usage by a container as percentage of total CPU allocated to it is given by dividing the
-        // cpuPercentageOfHost with the ratio of container resources over total host resources. This calculation
-        // assumes that the ratio between container and host resources for disk, memory, and cpu is roughly equal
-        // and therefore only calculates the ratio of container memory against host memory.
+        // cpuPercentageOfHost with the ratio of container minCpuCores by total number of CPU cores.
         double cpuPercentageOfHost = lastCpuMetric.getCpuUsagePercentage(cpuContainerTotalTime, cpuSystemTotalTime);
-        double cpuPercentageOfAllocated = getInverseContainerShareOfHost(nodeSpec) * cpuPercentageOfHost;
+        Optional<Double> cpuPercentageOfAllocated = nodeSpec.minCpuCores.map(containerNumCpuCores ->
+                totalNumCpuCores * cpuPercentageOfHost / containerNumCpuCores);
         long memoryTotalBytesUsed = memoryTotalBytesUsage - memoryTotalBytesCache;
         double memoryPercentUsed = 100.0 * memoryTotalBytesUsed / memoryTotalBytes;
         Optional<Double> diskPercentUsed = diskTotalBytes.flatMap(total -> diskTotalBytesUsed.map(used -> 100.0 * used / total));
 
         List<DimensionMetrics> metrics = new ArrayList<>();
         DimensionMetrics.Builder systemMetricsBuilder = new DimensionMetrics.Builder(APP, dimensions)
-                .withMetric("cpu.util", cpuPercentageOfAllocated)
                 .withMetric("mem.limit", memoryTotalBytes)
                 .withMetric("mem.used", memoryTotalBytesUsed)
                 .withMetric("mem.util", memoryPercentUsed);
 
+        cpuPercentageOfAllocated.ifPresent(cpuUtil -> systemMetricsBuilder.withMetric("cpu.util", cpuUtil));
         diskTotalBytes.ifPresent(diskLimit -> systemMetricsBuilder.withMetric("disk.limit", diskLimit));
         diskTotalBytesUsed.ifPresent(diskUsed -> systemMetricsBuilder.withMetric("disk.used", diskUsed));
         diskPercentUsed.ifPresent(diskUtil -> systemMetricsBuilder.withMetric("disk.util", diskUtil));
@@ -610,14 +610,6 @@ public class NodeAgentImpl implements NodeAgent {
         return temp;
     }
 
-    private double getInverseContainerShareOfHost(ContainerNodeSpec nodeSpec) {
-        return nodeSpec.minMainMemoryAvailableGb
-                .map(memory -> {
-                        double hostMemory = storageMaintainer.map(StorageMaintainer::getHostTotalMemoryGb).orElse(0d);
-                        return hostMemory / memory;
-                }).orElse(0d);
-    }
-
     class CpuUsageReporter {
         private long totalContainerUsage = 0;
         private long totalSystemUsage = 0;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
index 9fa43752157..c4b0dfbe153 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
@@ -19,7 +19,6 @@ import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.time.Duration;
-import java.util.Optional;
 
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.any;
@@ -100,11 +99,6 @@ public class StorageMaintainerTest {
     public void testMaintenanceThrottlingAfterFailedMaintenance() {
         String hostname = "node-123.us-north-3.test.yahoo.com";
         ContainerName containerName = ContainerName.fromHostname(hostname);
-        ContainerNodeSpec nodeSpec = new ContainerNodeSpec.Builder()
-                .hostname(hostname)
-                .nodeState(Node.State.ready)
-                .nodeType("tenants")
-                .nodeFlavor("docker").build();
 
         when(docker.executeInContainerAsRoot(any(), anyVararg()))
                 .thenThrow(new RuntimeException("Something went wrong"))
@@ -120,18 +114,6 @@ public class StorageMaintainerTest {
         verify(docker, times(2)).executeInContainerAsRoot(any(), anyVararg());
     }
 
-    @Test
-    public void testGetTotalMemory() {
-        StorageMaintainer storageMaintainer = mock(StorageMaintainer.class);
-        when(storageMaintainer.getHostTotalMemoryGb()).thenCallRealMethod();
-
-        when(storageMaintainer.readMeminfo()).thenReturn(Optional.empty());
-        assertEquals(0d, storageMaintainer.getHostTotalMemoryGb(), 0);
-
-        when(storageMaintainer.readMeminfo()).thenReturn(Optional.of("MemTotal:   1572864 kB\nMemUsed:   1000000 kB\n"));
-        assertEquals(1.5d, storageMaintainer.getHostTotalMemoryGb(), 0);
-    }
-
     private static void writeNBytesToFile(File file, int nBytes) throws IOException {
         Files.write(file.toPath(), new byte[nBytes]);
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
index cc54961ee59..50e58bdaa6e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
@@ -525,7 +525,6 @@ public class NodeAgentImplTest {
 
         when(nodeRepository.getContainerNodeSpec(eq(hostName))).thenReturn(Optional.of(nodeSpec));
         when(storageMaintainer.getDiskUsageFor(eq(containerName))).thenReturn(Optional.of(42547019776L));
-        when(storageMaintainer.getHostTotalMemoryGb()).thenReturn(10d);
         when(dockerOperations.getContainerStats(eq(containerName)))
                 .thenReturn(Optional.of(stats1))
                 .thenReturn(Optional.of(stats2));
diff --git a/node-admin/src/test/resources/expected.container.system.metrics.txt b/node-admin/src/test/resources/expected.container.system.metrics.txt
index cfe01ae34f6..d3875113d33 100644
--- a/node-admin/src/test/resources/expected.container.system.metrics.txt
+++ b/node-admin/src/test/resources/expected.container.system.metrics.txt
@@ -12,7 +12,7 @@ s:
         "mem.used": 1073741824,
         "disk.used": 42547019776,
         "disk.util": 15.85,
-        "cpu.util": 6.75,
+        "cpu.util": 5.4,
         "mem.util": 25.0,
         "disk.limit": 268435456000
     },