Fix FsInfo device deduplication

docs/changelog/94744.yaml

@@ -0,0 +1,5 @@
+pr: 94744
+summary: Fix `FsInfo` device deduplication
+area: Stats
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

@@ -14,12 +14,12 @@
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.util.set.Sets;
-import org.elasticsearch.core.Nullable;
 import org.elasticsearch.xcontent.ToXContentFragment;
 import org.elasticsearch.xcontent.ToXContentObject;
 import org.elasticsearch.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.nio.file.FileStore;
 import java.util.Iterator;
 import java.util.Set;
 
@@ -28,18 +28,17 @@ public class FsInfo implements Iterable<FsInfo.Path>, Writeable, ToXContentFragm
     public static class Path implements Writeable, ToXContentObject {
 
         String path;
-        @Nullable
-        String mount;
-        /** File system type from {@code java.nio.file.FileStore type()}, if available. */
-        @Nullable
-        String type;
+        /** File system string from {@link FileStore#toString()}. The concrete subclasses of FileStore have meaningful toString methods. */
+        String mount; // e.g. "/app (/dev/mapper/lxc-data)", "/System/Volumes/Data (/dev/disk1s2)", "Local Disk (C:)", etc.
+        /** File system type from {@link FileStore#type()}. */
+        String type; // e.g. "xfs", "apfs", "NTFS", etc.
         long total = -1;
         long free = -1;
         long available = -1;
 
         public Path() {}
 
-        public Path(String path, @Nullable String mount, long total, long free, long available) {
+        public Path(String path, String mount, long total, long free, long available) {
             this.path = path;
             this.mount = mount;
             this.total = total;
@@ -51,7 +50,7 @@ public Path(String path, @Nullable String mount, long total, long free, long ava
          * Read from a stream.
          */
         public Path(StreamInput in) throws IOException {
-            path = in.readOptionalString();
+            path = in.readOptionalString(); // total aggregates do not have a path, mount, or type
             mount = in.readOptionalString();
             type = in.readOptionalString();
             total = in.readLong();
@@ -61,7 +60,7 @@ public Path(StreamInput in) throws IOException {
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
-            out.writeOptionalString(path); // total aggregates do not have a path
+            out.writeOptionalString(path); // total aggregates do not have a path, mount, or type
             out.writeOptionalString(mount);
             out.writeOptionalString(type);
             out.writeLong(total);
@@ -476,8 +475,8 @@ private Path total() {
         Path res = new Path();
         Set<String> seenDevices = Sets.newHashSetWithExpectedSize(paths.length);
         for (Path subPath : paths) {
-            if (subPath.path != null) {
-                if (seenDevices.add(subPath.path) == false) {
+            if (subPath.mount != null) {
+                if (seenDevices.add(subPath.mount) == false) {
                     continue; // already added numbers for this device;
                 }
             }

server/src/main/java/org/elasticsearch/monitor/fs/FsProbe.java

@@ -133,7 +133,7 @@ public static FsInfo.Path getFSInfo(DataPath dataPath) throws IOException {
         FsInfo.Path fsPath = new FsInfo.Path();
         fsPath.path = dataPath.path.toString();
 
-        // NOTE: we use already cached (on node startup) FileStore and spins
+        // NOTE: we use already cached (on node startup) FileStore
         // since recomputing these once per second (default) could be costly,
         // and they should not change:
         fsPath.total = getTotal(dataPath.fileStore);

server/src/test/java/org/elasticsearch/action/admin/cluster/stats/ClusterStatsNodesTests.java

@@ -216,13 +216,9 @@ public void testClusterFsStatsDeduplicator() {
             FsInfo.Path total = deduplicator.getTotal();
 
             // since it's the same device, they don't sum, we just see the one
-            // assertThat(total.getTotal().getBytes(), equalTo(3L));
-            // assertThat(total.getFree().getBytes(), equalTo(2L));
-            // assertThat(total.getAvailable().getBytes(), equalTo(1L));
-            // BUG 0: even though it's the same device, we're summing, bleh
-            assertThat(total.getTotal().getBytes(), equalTo(6L));
-            assertThat(total.getFree().getBytes(), equalTo(4L));
-            assertThat(total.getAvailable().getBytes(), equalTo(2L));
+            assertThat(total.getTotal().getBytes(), equalTo(3L));
+            assertThat(total.getFree().getBytes(), equalTo(2L));
+            assertThat(total.getAvailable().getBytes(), equalTo(1L));
         }
 
         {

server/src/test/java/org/elasticsearch/monitor/fs/FsInfoTests.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.monitor.fs;
+
+import org.elasticsearch.monitor.fs.FsInfo.Path;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.is;
+
+public class FsInfoTests extends ESTestCase {
+
+    public void testTotalSummation() {
+        Path p1 = new Path("J:\\data\\nodes\\0", "Disk10 (J:)", 4, 3, 2);
+        Path p2 = new Path("K:\\data\\nodes\\0", "Disk11 (K:)", 8, 6, 4);
+        FsInfo info = new FsInfo(-1, null, new Path[] { p1, p2 });
+
+        Path total = info.getTotal();
+        assertThat(total.getTotal().getBytes(), is(12L));
+        assertThat(total.getFree().getBytes(), is(9L));
+        assertThat(total.getAvailable().getBytes(), is(6L));
+    }
+
+    public void testTotalDeduplication() {
+        Path p1 = new Path("/app/data/es-1", "/app (/dev/sda1)", 8, 6, 4);
+        Path p2 = new Path("/app/data/es-2", "/app (/dev/sda1)", 8, 6, 4);
+        FsInfo info = new FsInfo(-1, null, new Path[] { p1, p2 });
+
+        Path total = info.getTotal();
+        assertThat(total.getTotal().getBytes(), is(8L));
+        assertThat(total.getFree().getBytes(), is(6L));
+        assertThat(total.getAvailable().getBytes(), is(4L));
+    }
+
+}