From f608fce59ed32e5e63180ed0230a7bc6069e7a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Thu, 2 May 2024 17:16:08 +0200 Subject: [PATCH 1/9] Fix condition and parameter order for field cache utilization metric MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lukáš Vlček --- CHANGELOG-3.0.md | 1 + server/src/main/java/org/opensearch/monitor/fs/FsInfo.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG-3.0.md b/CHANGELOG-3.0.md index 06b761b1df8bd..dabecefb0f291 100644 --- a/CHANGELOG-3.0.md +++ b/CHANGELOG-3.0.md @@ -48,6 +48,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827)) - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944)) - Don't over-allocate in HeapBufferedAsyncEntityConsumer in order to consume the response ([#9993](https://github.com/opensearch-project/OpenSearch/pull/9993)) +- Fix condition and parameter order for field cache utilization metric ([#13232](https://github.com/opensearch-project/OpenSearch/pull/13232)) ### Security diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index ddff022112665..5dac71ccb6648 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -211,8 +211,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (fileCacheReserved != -1) { builder.humanReadableField(Fields.CACHE_RESERVED_IN_BYTES, Fields.CACHE_RESERVED, getFileCacheReserved()); } - if (fileCacheReserved != 0) { - builder.humanReadableField(Fields.CACHE_UTILIZED, Fields.CACHE_UTILIZED_IN_BYTES, getFileCacheUtilized()); + if (fileCacheUtilized != 0) { + builder.humanReadableField(Fields.CACHE_UTILIZED_IN_BYTES, Fields.CACHE_UTILIZED, getFileCacheUtilized()); } builder.endObject(); From ba195013c5adb3b6636063a97adc6f30490cf5cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Fri, 7 Jun 2024 17:10:55 +0200 Subject: [PATCH 2/9] Adding tests for file cache statistics response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addming missing tests for file caches in NodeStats FsInfo. This commit changes the test value for `cache_utilized` field. Going forward both file cache fields are present in the node stats REST response. Signed-off-by: Lukáš Vlček --- .../test/nodes.stats/70_file_cache.yml | 31 ++++++++++++++ .../decider/DiskThresholdDeciderIT.java | 2 +- .../allocation/decider/MockDiskUsagesIT.java | 2 +- .../org/opensearch/env/NodeEnvironment.java | 28 +++++++------ .../org/opensearch/monitor/fs/FsInfo.java | 15 ++++++- .../cluster/node/stats/NodeStatsTests.java | 40 +++++++++++++++---- .../opensearch/cluster/DiskUsageTests.java | 24 +++++------ .../opensearch/monitor/fs/FsProbeTests.java | 33 +++++++++++++-- 8 files changed, 136 insertions(+), 39 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml new file mode 100644 index 0000000000000..d65aa34c35cc4 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml @@ -0,0 +1,31 @@ +# As of now only nodes with the "search" role can utilize file caching. +# Moreover, until the remote store is used in the test and some searches executed (which will force +# downloading the files from remote store to node file cache) we should always expect 0b size. +# But at least the cache file fields must be present. +--- +"File Cache stats": + - skip: + version: " - 2.15.99" + reason: "file cache fields were added in 2.7 but #13232 was not back-ported yet" + features: [arbitrary_key] + + - do: + nodes.info: {} + - set: + nodes._arbitrary_key_: node_id + + - do: + nodes.stats: + metric: [ fs ] + + # In the future we shall test that the node has a "search" role + # otherwise the file cache will be always 0. + - is_true: nodes.$node_id.roles + + - is_true: nodes.$node_id.fs.total + - gte: { nodes.$node_id.fs.total.cache_reserved_in_bytes: 0 } + - gte: { nodes.$node_id.fs.total.cache_utilized_in_bytes: 0 } + + - is_true: nodes.$node_id.fs.data + - gte: { nodes.$node_id.fs.data.0.cache_reserved_in_bytes: 0 } + - gte: { nodes.$node_id.fs.data.0.cache_utilized_in_bytes: 0 } diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java index cc8747e5f5666..b3142e358b0d1 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/DiskThresholdDeciderIT.java @@ -563,7 +563,7 @@ private long createReasonableSizedShards(final String indexName) throws Interrup } private static FsInfo.Path setDiskUsage(FsInfo.Path original, long totalBytes, long freeBytes) { - return new FsInfo.Path(original.getPath(), original.getMount(), totalBytes, freeBytes, freeBytes); + return new FsInfo.Path(original.getPath(), original.getMount(), totalBytes, freeBytes, freeBytes, 0, 0); } private void refreshDiskUsage() { diff --git a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/MockDiskUsagesIT.java b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/MockDiskUsagesIT.java index a7755841c9ea9..718c06820bc9b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/MockDiskUsagesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/MockDiskUsagesIT.java @@ -89,7 +89,7 @@ public Settings indexSettings() { } private static FsInfo.Path setDiskUsage(FsInfo.Path original, long totalBytes, long freeBytes) { - return new FsInfo.Path(original.getPath(), original.getMount(), totalBytes, freeBytes, freeBytes); + return new FsInfo.Path(original.getPath(), original.getMount(), totalBytes, freeBytes, freeBytes, 0, 0); } public void testRerouteOccursOnDiskPassingHighWatermark() throws Exception { diff --git a/server/src/main/java/org/opensearch/env/NodeEnvironment.java b/server/src/main/java/org/opensearch/env/NodeEnvironment.java index 2748938d8b761..f847162e61ac5 100644 --- a/server/src/main/java/org/opensearch/env/NodeEnvironment.java +++ b/server/src/main/java/org/opensearch/env/NodeEnvironment.java @@ -126,6 +126,7 @@ public static class NodePath { public final Path indicesPath; /** Cached FileStore from path */ public final FileStore fileStore; + /* ${data.paths}/nodes/{node.id}/cache */ public final Path fileCachePath; /* Cache reserved size can default to a different value depending on configuration @@ -171,18 +172,21 @@ Path resolve(String uuid) { @Override public String toString() { - return "NodePath{" - + "path=" - + path - + ", indicesPath=" - + indicesPath - + ", fileStore=" - + fileStore - + ", majorDeviceNumber=" - + majorDeviceNumber - + ", minorDeviceNumber=" - + minorDeviceNumber - + '}'; + StringBuilder sb = new StringBuilder().append("NodePath{") + .append("path=") + .append(path) + .append(", indicesPath=") + .append(indicesPath) + .append(", fileStore=") + .append(fileStore) + .append(", fileCachePath=") + .append(fileCachePath) + .append(", majorDeviceNumber=") + .append(majorDeviceNumber) + .append(", minorDeviceNumber=") + .append(minorDeviceNumber) + .append("}"); + return sb.toString(); } } diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index 5dac71ccb6648..607c56591b5c8 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -79,12 +79,23 @@ public static class Path implements Writeable, ToXContentObject { public Path() {} - public Path(String path, @Nullable String mount, long total, long free, long available) { + // Used only in tests + public Path( + String path, + @Nullable String mount, + long total, + long free, + long available, + long fileCacheReserved, + long fileCacheUtilized + ) { this.path = path; this.mount = mount; this.total = total; this.free = free; this.available = available; + this.fileCacheReserved = fileCacheReserved; + this.fileCacheUtilized = fileCacheUtilized; } /** @@ -211,7 +222,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (fileCacheReserved != -1) { builder.humanReadableField(Fields.CACHE_RESERVED_IN_BYTES, Fields.CACHE_RESERVED, getFileCacheReserved()); } - if (fileCacheUtilized != 0) { + if (fileCacheUtilized != -1) { builder.humanReadableField(Fields.CACHE_UTILIZED_IN_BYTES, Fields.CACHE_UTILIZED, getFileCacheUtilized()); } diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java index f7bc96bdfe769..8ff169a6b6244 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -95,6 +95,8 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Spliterator; +import java.util.Spliterators; import java.util.TreeMap; import java.util.function.Function; import java.util.stream.Collectors; @@ -291,12 +293,23 @@ public void testSerialization() throws IOException { assertNull(deserializedFs); } else { assertEquals(fs.getTimestamp(), deserializedFs.getTimestamp()); - assertEquals(fs.getTotal().getAvailable(), deserializedFs.getTotal().getAvailable()); - assertEquals(fs.getTotal().getTotal(), deserializedFs.getTotal().getTotal()); - assertEquals(fs.getTotal().getFree(), deserializedFs.getTotal().getFree()); - assertEquals(fs.getTotal().getMount(), deserializedFs.getTotal().getMount()); - assertEquals(fs.getTotal().getPath(), deserializedFs.getTotal().getPath()); - assertEquals(fs.getTotal().getType(), deserializedFs.getTotal().getType()); + compareFsInfo(fs.getTotal(), deserializedFs.getTotal()); + + FsInfo.Path[] fsPaths = StreamSupport.stream( + Spliterators.spliteratorUnknownSize(fs.iterator(), Spliterator.ORDERED), + false + ).toArray(FsInfo.Path[]::new); + + FsInfo.Path[] deserializedFsPaths = StreamSupport.stream( + Spliterators.spliteratorUnknownSize(deserializedFs.iterator(), Spliterator.ORDERED), + false + ).toArray(FsInfo.Path[]::new); + + assertEquals(fsPaths.length, deserializedFsPaths.length); + for (int i = 0; i < fsPaths.length; i++) { + compareFsInfo(fsPaths[i], deserializedFsPaths[i]); + } + FsInfo.IoStats ioStats = fs.getIoStats(); FsInfo.IoStats deserializedIoStats = deserializedFs.getIoStats(); assertEquals(ioStats.getTotalOperations(), deserializedIoStats.getTotalOperations()); @@ -595,11 +608,22 @@ public void testSerialization() throws IOException { } } + private void compareFsInfo(FsInfo.Path path, FsInfo.Path otherPath) { + assertEquals(path.getAvailable(), otherPath.getAvailable()); + assertEquals(path.getTotal(), otherPath.getTotal()); + assertEquals(path.getFree(), otherPath.getFree()); + assertEquals(path.getFileCacheReserved(), otherPath.getFileCacheReserved()); + assertEquals(path.getFileCacheUtilized(), otherPath.getFileCacheUtilized()); + assertEquals(path.getMount(), otherPath.getMount()); + assertEquals(path.getPath(), otherPath.getPath()); + assertEquals(path.getType(), otherPath.getType()); + } + public static NodeStats createNodeStats() throws IOException { return createNodeStats(false); } - public static NodeStats createNodeStats(boolean remoteStoreStats) throws IOException { + private static NodeStats createNodeStats(boolean remoteStoreStats) throws IOException { DiscoveryNode node = new DiscoveryNode( "test_node", buildNewFakeTransportAddress(), @@ -764,6 +788,8 @@ public static NodeStats createNodeStats(boolean remoteStoreStats) throws IOExcep randomBoolean() ? randomAlphaOfLengthBetween(3, 10) : null, randomNonNegativeLong(), randomNonNegativeLong(), + randomNonNegativeLong(), + randomNonNegativeLong(), randomNonNegativeLong() ); } diff --git a/server/src/test/java/org/opensearch/cluster/DiskUsageTests.java b/server/src/test/java/org/opensearch/cluster/DiskUsageTests.java index 5539dd26dd52d..e58574acf3e22 100644 --- a/server/src/test/java/org/opensearch/cluster/DiskUsageTests.java +++ b/server/src/test/java/org/opensearch/cluster/DiskUsageTests.java @@ -157,14 +157,14 @@ public void testFillDiskUsage() { final Map newLeastAvaiableUsages = new HashMap<>(); final Map newMostAvaiableUsages = new HashMap<>(); FsInfo.Path[] node1FSInfo = new FsInfo.Path[] { - new FsInfo.Path("/middle", "/dev/sda", 100, 90, 80), - new FsInfo.Path("/least", "/dev/sdb", 200, 190, 70), - new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280), }; - FsInfo.Path[] node2FSInfo = new FsInfo.Path[] { new FsInfo.Path("/least_most", "/dev/sda", 100, 90, 80), }; + new FsInfo.Path("/middle", "/dev/sda", 100, 90, 80, 0, 0), + new FsInfo.Path("/least", "/dev/sdb", 200, 190, 70, 0, 0), + new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280, 0, 0), }; + FsInfo.Path[] node2FSInfo = new FsInfo.Path[] { new FsInfo.Path("/least_most", "/dev/sda", 100, 90, 80, 0, 0), }; FsInfo.Path[] node3FSInfo = new FsInfo.Path[] { - new FsInfo.Path("/least", "/dev/sda", 100, 90, 70), - new FsInfo.Path("/most", "/dev/sda", 100, 90, 80), }; + new FsInfo.Path("/least", "/dev/sda", 100, 90, 70, 0, 0), + new FsInfo.Path("/most", "/dev/sda", 100, 90, 80, 0, 0), }; List nodeStats = Arrays.asList( new NodeStats( new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), @@ -281,14 +281,14 @@ public void testFillDiskUsageSomeInvalidValues() { final Map newLeastAvailableUsages = new HashMap<>(); final Map newMostAvailableUsages = new HashMap<>(); FsInfo.Path[] node1FSInfo = new FsInfo.Path[] { - new FsInfo.Path("/middle", "/dev/sda", 100, 90, 80), - new FsInfo.Path("/least", "/dev/sdb", -1, -1, -1), - new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280), }; - FsInfo.Path[] node2FSInfo = new FsInfo.Path[] { new FsInfo.Path("/least_most", "/dev/sda", -1, -1, -1), }; + new FsInfo.Path("/middle", "/dev/sda", 100, 90, 80, 0, 0), + new FsInfo.Path("/least", "/dev/sdb", -1, -1, -1, 0, 0), + new FsInfo.Path("/most", "/dev/sdc", 300, 290, 280, 0, 0), }; + FsInfo.Path[] node2FSInfo = new FsInfo.Path[] { new FsInfo.Path("/least_most", "/dev/sda", -1, -1, -1, 0, 0), }; FsInfo.Path[] node3FSInfo = new FsInfo.Path[] { - new FsInfo.Path("/most", "/dev/sda", 100, 90, 70), - new FsInfo.Path("/least", "/dev/sda", 10, -1, 0), }; + new FsInfo.Path("/most", "/dev/sda", 100, 90, 70, 0, 0), + new FsInfo.Path("/least", "/dev/sda", 10, -1, 0, 0, 0), }; List nodeStats = Arrays.asList( new NodeStats( new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), diff --git a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java index e2e09d5ce63fe..658b04abcae58 100644 --- a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java +++ b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java @@ -204,24 +204,49 @@ public void testFsInfoWhenFileCacheOccupied() throws IOException { } public void testFsInfoOverflow() throws Exception { + final String path_r = "/foo/bar"; + final String path_z = "/foo/baz"; final FsInfo.Path pathStats = new FsInfo.Path( - "/foo/bar", + path_r, null, randomNonNegativeLong(), randomNonNegativeLong(), + randomNonNegativeLong(), + randomNonNegativeLong(), randomNonNegativeLong() ); - addUntilOverflow(pathStats, p -> p.total, "total", () -> new FsInfo.Path("/foo/baz", null, randomNonNegativeLong(), 0, 0)); + addUntilOverflow(pathStats, p -> p.total, "total", () -> new FsInfo.Path(path_z, null, randomNonNegativeLong(), 0, 0, 0, 0)); - addUntilOverflow(pathStats, p -> p.free, "free", () -> new FsInfo.Path("/foo/baz", null, 0, randomNonNegativeLong(), 0)); + addUntilOverflow(pathStats, p -> p.free, "free", () -> new FsInfo.Path(path_z, null, 0, randomNonNegativeLong(), 0, 0, 0)); + + addUntilOverflow( + pathStats, + p -> p.available, + "available", + () -> new FsInfo.Path(path_z, null, 0, 0, randomNonNegativeLong(), 0, 0) + ); - addUntilOverflow(pathStats, p -> p.available, "available", () -> new FsInfo.Path("/foo/baz", null, 0, 0, randomNonNegativeLong())); + addUntilOverflow( + pathStats, + p -> p.fileCacheReserved, + "fileCacheReserved", + () -> new FsInfo.Path(path_z, null, 0, 0, 0, randomNonNegativeLong(), 0) + ); + + addUntilOverflow( + pathStats, + p -> p.fileCacheUtilized, + "fileCacheUtilized", + () -> new FsInfo.Path(path_z, null, 0, 0, 0, 0, randomNonNegativeLong()) + ); // even after overflowing these should not be negative assertThat(pathStats.total, greaterThan(0L)); assertThat(pathStats.free, greaterThan(0L)); assertThat(pathStats.available, greaterThan(0L)); + assertThat(pathStats.fileCacheReserved, greaterThan(0L)); + assertThat(pathStats.fileCacheUtilized, greaterThan(0L)); } private void addUntilOverflow( From f2b7e195e27b0641154db5e098100b581bde7c8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 19 Jun 2024 16:42:22 +0200 Subject: [PATCH 3/9] The value of fileCacheUtilized should use standard init value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In FsInfo.Path object the value of fileCacheUtilized is now initialized like any other fields, that means it is -1, which means the value hasn't been populated yet. To make this possible it was necessary to fix FsProbeTest and use a real FileCache object to enable internal logic to handle initialization and safeguarding of the value properly. Signed-off-by: Lukáš Vlček --- .../org/opensearch/monitor/fs/FsInfo.java | 2 +- .../org/opensearch/monitor/fs/FsProbe.java | 5 ++++- .../opensearch/monitor/fs/FsProbeTests.java | 22 ++++++++++++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index 607c56591b5c8..07572c4c3018b 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -75,7 +75,7 @@ public static class Path implements Writeable, ToXContentObject { long free = -1; long available = -1; long fileCacheReserved = -1; - long fileCacheUtilized = 0; + long fileCacheUtilized = -1; public Path() {} diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java index db77ec7628e76..c715874c93689 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java @@ -78,7 +78,7 @@ public FsInfo stats(FsInfo previous) throws IOException { FsInfo.Path[] paths = new FsInfo.Path[dataLocations.length]; for (int i = 0; i < dataLocations.length; i++) { paths[i] = getFSInfo(dataLocations[i]); - if (fileCache != null && dataLocations[i].fileCacheReservedSize != ByteSizeValue.ZERO) { + if (fileCache != null && dataLocations[i].fileCacheReservedSize.compareTo(ByteSizeValue.ZERO) >= 0) { paths[i].fileCacheReserved = adjustForHugeFilesystems(dataLocations[i].fileCacheReservedSize.getBytes()); paths[i].fileCacheUtilized = adjustForHugeFilesystems(fileCache.usage().usage()); // fileCacheFree will be less than zero if the cache being over-subscribed @@ -214,6 +214,9 @@ public static FsInfo.Path getFSInfo(NodePath nodePath) throws IOException { fsPath.free = adjustForHugeFilesystems(nodePath.fileStore.getUnallocatedSpace()); fsPath.available = adjustForHugeFilesystems(nodePath.fileStore.getUsableSpace()); fsPath.fileCacheReserved = adjustForHugeFilesystems(nodePath.fileCacheReservedSize.getBytes()); + // fsPath.fileCacheUtilized = adjustForHugeFilesystems(...); + // We can not do this ^^ here because information about utilization of file cache is hold by FileCache + // which is not accessible here and since this method is static we can not assume relevant context. fsPath.type = nodePath.fileStore.type(); fsPath.mount = nodePath.fileStore.toString(); return fsPath; diff --git a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java index 658b04abcae58..ab9b864b5d93d 100644 --- a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java +++ b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java @@ -72,7 +72,17 @@ public class FsProbeTests extends OpenSearchTestCase { public void testFsInfo() throws IOException { try (NodeEnvironment env = newNodeEnvironment()) { - FsProbe probe = new FsProbe(env, null); + // Question: Shall we expose a public method in FileCacheTests to enable creation of FileCache + // so that it can be used by other testing classes? + int CONCURRENCY_LEVEL = 16; // not important + int CAPACITY = 1 * 1024; // not important + FileCache fileCache = FileCacheFactory.createConcurrentLRUFileCache( + CAPACITY, + CONCURRENCY_LEVEL, + new NoopCircuitBreaker(CircuitBreaker.REQUEST) + ); + // We need to pass a real FileCache object to FsProbe ctor to have it safeguard "path.fileCacheUtilized" values properly! + FsProbe probe = new FsProbe(env, fileCache); FsInfo stats = probe.stats(null); assertNotNull(stats); @@ -111,6 +121,16 @@ public void testFsInfo() throws IOException { assertThat(total.free, greaterThan(0L)); assertThat(total.available, greaterThan(0L)); + // The convention for "total" Path object is that some fields are not set + // which means they will not be included in output of toXContent method. + assertNull(total.path); + assertNull(total.mount); + assertNull(total.type); + + // Total file cache (sum over all "paths"): + assertEquals(total.getFileCacheReserved().getBytes(), 0); + assertEquals(total.getFileCacheUtilized().getBytes(), 0); + for (FsInfo.Path path : stats) { assertNotNull(path); assertThat(path.getPath(), is(not(emptyOrNullString()))); From b6020a1b2578f377f20d3ff9a882c304d846e1b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Thu, 20 Jun 2024 17:18:39 +0200 Subject: [PATCH 4/9] Addresing PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use String concatenation instead of StringBuilder. Signed-off-by: Lukáš Vlček --- .../org/opensearch/env/NodeEnvironment.java | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/opensearch/env/NodeEnvironment.java b/server/src/main/java/org/opensearch/env/NodeEnvironment.java index f847162e61ac5..e13e5e5f47cf9 100644 --- a/server/src/main/java/org/opensearch/env/NodeEnvironment.java +++ b/server/src/main/java/org/opensearch/env/NodeEnvironment.java @@ -172,21 +172,20 @@ Path resolve(String uuid) { @Override public String toString() { - StringBuilder sb = new StringBuilder().append("NodePath{") - .append("path=") - .append(path) - .append(", indicesPath=") - .append(indicesPath) - .append(", fileStore=") - .append(fileStore) - .append(", fileCachePath=") - .append(fileCachePath) - .append(", majorDeviceNumber=") - .append(majorDeviceNumber) - .append(", minorDeviceNumber=") - .append(minorDeviceNumber) - .append("}"); - return sb.toString(); + return "NodePath{" + + "path=" + + path + + ", indicesPath=" + + indicesPath + + ", fileStore=" + + fileStore + + ", fileCachePath=" + + fileCachePath + + ", majorDeviceNumber=" + + majorDeviceNumber + + ", minorDeviceNumber=" + + minorDeviceNumber + + '}'; } } From 0d12669a3594821e6b1afc9840315231bb3383a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Fri, 28 Jun 2024 17:44:45 +0200 Subject: [PATCH 5/9] Addresing PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a second public ctor for FsInfo.Path (to be used in tests only). Signed-off-by: Lukáš Vlček --- .../main/java/org/opensearch/monitor/fs/FsInfo.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index 07572c4c3018b..f6879109c337f 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -34,6 +34,7 @@ import org.opensearch.Version; import org.opensearch.common.Nullable; +import org.opensearch.common.annotation.InternalApi; import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; @@ -79,7 +80,17 @@ public static class Path implements Writeable, ToXContentObject { public Path() {} - // Used only in tests + /** + * Please notice that this constructor will use value of 0 + * for fileCacheReserved and fileCacheUtilized variables. + * + * See {@link #getFileCacheReserved()}, {@link #getFileCacheUtilized()} + */ + public Path(String path, @Nullable String mount, long total, long free, long available) { + new Path(path, mount, total, free, available, 0, 0); + } + + @InternalApi public Path( String path, @Nullable String mount, From 130832d90966af82055bb3c40bcfb5f3eab0fd2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Tue, 2 Jul 2024 16:31:06 +0200 Subject: [PATCH 6/9] Addressing PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add FsInfo.Path ctor asserts. Signed-off-by: Lukáš Vlček --- .../java/org/opensearch/monitor/fs/FsInfo.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java index f6879109c337f..67a2f6c43fc84 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsInfo.java @@ -81,8 +81,8 @@ public static class Path implements Writeable, ToXContentObject { public Path() {} /** - * Please notice that this constructor will use value of 0 - * for fileCacheReserved and fileCacheUtilized variables. + * Please notice that this constructor will set value of 0 + * to fileCacheReserved and fileCacheUtilized variables. * * See {@link #getFileCacheReserved()}, {@link #getFileCacheUtilized()} */ @@ -90,6 +90,11 @@ public Path(String path, @Nullable String mount, long total, long free, long ava new Path(path, mount, total, free, available, 0, 0); } + /** + * Do not assign negative values to variables total, free, available, + * fileCacheReserved and fileCacheUtilized. The only exception is value -1 + * which is interpreted as an uninitialized value. + */ @InternalApi public Path( String path, @@ -107,6 +112,12 @@ public Path( this.available = available; this.fileCacheReserved = fileCacheReserved; this.fileCacheUtilized = fileCacheUtilized; + + assert (this.total == -1 || this.total >= 0) : "Total value can not be negative"; + assert (this.free == -1 || this.free >= 0) : "Free value can not be negative"; + assert (this.available == -1 || this.available >= 0) : "Available value can not be negative"; + assert (this.fileCacheReserved == -1 || this.fileCacheReserved >= 0) : "File cache reserved value can not be negative"; + assert (this.fileCacheUtilized == -1 || this.fileCacheUtilized >= 0) : "File cache utilization value can not be negative"; } /** From 205d3863b9961c79b5a52ecfb6b7a9f058924c6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 3 Jul 2024 15:23:35 +0200 Subject: [PATCH 7/9] Update REST API test version skip condition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lukáš Vlček --- CHANGELOG-3.0.md | 2 +- .../rest-api-spec/test/nodes.stats/70_file_cache.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG-3.0.md b/CHANGELOG-3.0.md index dabecefb0f291..f4166cf3e6623 100644 --- a/CHANGELOG-3.0.md +++ b/CHANGELOG-3.0.md @@ -48,7 +48,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827)) - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944)) - Don't over-allocate in HeapBufferedAsyncEntityConsumer in order to consume the response ([#9993](https://github.com/opensearch-project/OpenSearch/pull/9993)) -- Fix condition and parameter order for field cache utilization metric ([#13232](https://github.com/opensearch-project/OpenSearch/pull/13232)) +- Display all file cache metrics in NodeStats and use the correct human-readable field name ([#13232](https://github.com/opensearch-project/OpenSearch/pull/13232)) ### Security diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml index d65aa34c35cc4..7284c5424c0b2 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml @@ -5,8 +5,8 @@ --- "File Cache stats": - skip: - version: " - 2.15.99" - reason: "file cache fields were added in 2.7 but #13232 was not back-ported yet" + version: " - 2.16.0" + reason: "file cache statistics fields were added in 2.7 (#6485) but the fix (#13232) was merged after 2.16.0" features: [arbitrary_key] - do: From 6dc9758456cc5d8650df872e0b13dc8519192b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 3 Jul 2024 17:28:18 +0200 Subject: [PATCH 8/9] Fix skip condition for REST API test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lukáš Vlček --- .../rest-api-spec/test/nodes.stats/70_file_cache.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml index 7284c5424c0b2..2282b27a7cded 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml @@ -5,16 +5,20 @@ --- "File Cache stats": - skip: - version: " - 2.16.0" + version: " - 2.16.99" reason: "file cache statistics fields were added in 2.7 (#6485) but the fix (#13232) was merged after 2.16.0" - features: [arbitrary_key] + features: [arbitrary_key, node_selector] - do: + node_selector: + version: "3.0.0 - " nodes.info: {} - set: nodes._arbitrary_key_: node_id - do: + node_selector: + version: "3.0.0 - " nodes.stats: metric: [ fs ] From 4466d346f5bb895f2369b62eefc13f10dcc42b96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Mon, 8 Jul 2024 22:54:02 +0200 Subject: [PATCH 9/9] Deprecating static getFSInfo method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deprecating `FsInfo.Path#getFSInfo(NodePath nodePath)` method because without FileCache instance it produces incomplete Path object that does not have initialized some file cache related variables. WIP: We still need better tests running multinode cluster with FileCache. Signed-off-by: Lukáš Vlček --- .../test/nodes.stats/70_file_cache.yml | 4 +- .../org/opensearch/env/NodeEnvironment.java | 5 ++- .../store/remote/filecache/FileCache.java | 25 +++++++++++ .../org/opensearch/monitor/fs/FsProbe.java | 32 ++++++++++++-- .../opensearch/monitor/fs/FsProbeTests.java | 42 ++++++++++--------- 5 files changed, 81 insertions(+), 27 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml index 2282b27a7cded..9ed45d7a4f396 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/70_file_cache.yml @@ -5,8 +5,8 @@ --- "File Cache stats": - skip: - version: " - 2.16.99" - reason: "file cache statistics fields were added in 2.7 (#6485) but the fix (#13232) was merged after 2.16.0" + version: " - 2.15.99" + reason: "file cache statistics fields were added in 2.7 (#6485) but the fix (#13232) was merged in 2.16.0" features: [arbitrary_key, node_selector] - do: diff --git a/server/src/main/java/org/opensearch/env/NodeEnvironment.java b/server/src/main/java/org/opensearch/env/NodeEnvironment.java index e13e5e5f47cf9..b127400ba4712 100644 --- a/server/src/main/java/org/opensearch/env/NodeEnvironment.java +++ b/server/src/main/java/org/opensearch/env/NodeEnvironment.java @@ -71,6 +71,7 @@ import org.opensearch.index.IndexSettings; import org.opensearch.index.shard.ShardPath; import org.opensearch.index.store.FsDirectoryFactory; +import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.monitor.fs.FsInfo; import org.opensearch.monitor.fs.FsProbe; import org.opensearch.monitor.jvm.JvmInfo; @@ -424,7 +425,7 @@ private void maybeLogPathDetails() throws IOException { for (NodePath nodePath : nodePaths) { sb.append('\n').append(" -> ").append(nodePath.path.toAbsolutePath()); - FsInfo.Path fsPath = FsProbe.getFSInfo(nodePath); + FsInfo.Path fsPath = FsProbe.getFSInfo(nodePath, FileCache.NOOP_FILE_CACHE); sb.append(", free_space [") .append(fsPath.getFree()) .append("], usable_space [") @@ -443,7 +444,7 @@ private void maybeLogPathDetails() throws IOException { Set allTypes = new HashSet<>(); Set allMounts = new HashSet<>(); for (NodePath nodePath : nodePaths) { - FsInfo.Path fsPath = FsProbe.getFSInfo(nodePath); + FsInfo.Path fsPath = FsProbe.getFSInfo(nodePath, FileCache.NOOP_FILE_CACHE); String mount = fsPath.getMount(); if (allMounts.contains(mount) == false) { allMounts.add(mount); diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCache.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCache.java index a8aa33a977cb8..2ca39e09c4518 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCache.java +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCache.java @@ -14,6 +14,8 @@ import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.common.breaker.CircuitBreaker; import org.opensearch.core.common.breaker.CircuitBreakingException; +import org.opensearch.core.common.breaker.NoopCircuitBreaker; +import org.opensearch.env.NodeEnvironment; import org.opensearch.index.store.remote.utils.cache.CacheUsage; import org.opensearch.index.store.remote.utils.cache.RefCountedCache; import org.opensearch.index.store.remote.utils.cache.SegmentedCache; @@ -49,6 +51,29 @@ */ @PublicApi(since = "2.7.0") public class FileCache implements RefCountedCache { + + /** + * A minimalistic instance of the File Cache (FC) which is not useful for any real caching task + * but is useful as a placeholder in API calls that require FC instance, for example + * {@link org.opensearch.monitor.fs.FsProbe#getFSInfo(NodeEnvironment.NodePath, FileCache)}. + *
+ * REMEMBER to use a valid FC instance if the output of such API call needs to reflect its state. + * + * @opensearch.internal + */ + public static FileCache NOOP_FILE_CACHE = null; + + // Perhaps we shall create a new class with static instance like NoopFileCache.INSTANCE ??? + static { + int CONCURRENCY_LEVEL = 1; + int CAPACITY = 0; + NOOP_FILE_CACHE = FileCacheFactory.createConcurrentLRUFileCache( + CAPACITY, + CONCURRENCY_LEVEL, + new NoopCircuitBreaker(CircuitBreaker.REQUEST) + ); + } + private static final Logger logger = LogManager.getLogger(FileCache.class); private final SegmentedCache theCache; diff --git a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java index c715874c93689..d17fa9fbec77e 100644 --- a/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java +++ b/server/src/main/java/org/opensearch/monitor/fs/FsProbe.java @@ -37,6 +37,7 @@ import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.lucene.util.Constants; import org.opensearch.common.SuppressForbidden; +import org.opensearch.common.annotation.DeprecatedApi; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.PathUtils; import org.opensearch.core.common.unit.ByteSizeValue; @@ -77,7 +78,7 @@ public FsInfo stats(FsInfo previous) throws IOException { NodePath[] dataLocations = nodeEnv.nodePaths(); FsInfo.Path[] paths = new FsInfo.Path[dataLocations.length]; for (int i = 0; i < dataLocations.length; i++) { - paths[i] = getFSInfo(dataLocations[i]); + paths[i] = getFSInfo(dataLocations[i], fileCache == null ? FileCache.NOOP_FILE_CACHE : fileCache); if (fileCache != null && dataLocations[i].fileCacheReservedSize.compareTo(ByteSizeValue.ZERO) >= 0) { paths[i].fileCacheReserved = adjustForHugeFilesystems(dataLocations[i].fileCacheReservedSize.getBytes()); paths[i].fileCacheUtilized = adjustForHugeFilesystems(fileCache.usage().usage()); @@ -203,6 +204,14 @@ static long adjustForHugeFilesystems(long bytes) { return bytes; } + /** + * Retrieves information about the path associated with the given node path. + * + * @deprecated This method has been deprecated since version 2.16.0 and will be removed in version 3.0.0, use {@link #getFSInfo(NodePath, FileCache)} instead. + * @see #getFSInfo(NodePath, FileCache) + */ + @Deprecated + @DeprecatedApi(since = "2.16.0", forRemoval = "3.0.0") public static FsInfo.Path getFSInfo(NodePath nodePath) throws IOException { FsInfo.Path fsPath = new FsInfo.Path(); fsPath.path = nodePath.path.toString(); @@ -214,14 +223,29 @@ public static FsInfo.Path getFSInfo(NodePath nodePath) throws IOException { fsPath.free = adjustForHugeFilesystems(nodePath.fileStore.getUnallocatedSpace()); fsPath.available = adjustForHugeFilesystems(nodePath.fileStore.getUsableSpace()); fsPath.fileCacheReserved = adjustForHugeFilesystems(nodePath.fileCacheReservedSize.getBytes()); - // fsPath.fileCacheUtilized = adjustForHugeFilesystems(...); - // We can not do this ^^ here because information about utilization of file cache is hold by FileCache - // which is not accessible here and since this method is static we can not assume relevant context. + // The fsPath.fileCacheUtilized cannot be calculated because the information about file cache utilization + // is hold by FileCache which is not available in this context. fsPath.type = nodePath.fileStore.type(); fsPath.mount = nodePath.fileStore.toString(); return fsPath; } + /** + * Retrieves information about the path associated with the given node path. + * + * @param nodePath The node path to retrieve the path information for. + * @param fileCache The FileCache object used for adjusting the file cache utilization. + * @return The path information for the given node path. + * @throws IOException If an I/O error occurs while retrieving the path information. + */ + public static FsInfo.Path getFSInfo(NodePath nodePath, FileCache fileCache) throws IOException { + assert fileCache != null : "FileCache cannot be null"; + FsInfo.Path fsPath = getFSInfo(nodePath); + fsPath.fileCacheUtilized = adjustForHugeFilesystems(fileCache.usage().usage()); + fsPath.available -= (fsPath.fileCacheReserved - fsPath.fileCacheUtilized); + return fsPath; + } + public static long getTotalSize(NodePath nodePath) throws IOException { return adjustForHugeFilesystems(nodePath.fileStore.getTotalSpace()); } diff --git a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java index ab9b864b5d93d..5f8691129a235 100644 --- a/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java +++ b/server/src/test/java/org/opensearch/monitor/fs/FsProbeTests.java @@ -72,18 +72,7 @@ public class FsProbeTests extends OpenSearchTestCase { public void testFsInfo() throws IOException { try (NodeEnvironment env = newNodeEnvironment()) { - // Question: Shall we expose a public method in FileCacheTests to enable creation of FileCache - // so that it can be used by other testing classes? - int CONCURRENCY_LEVEL = 16; // not important - int CAPACITY = 1 * 1024; // not important - FileCache fileCache = FileCacheFactory.createConcurrentLRUFileCache( - CAPACITY, - CONCURRENCY_LEVEL, - new NoopCircuitBreaker(CircuitBreaker.REQUEST) - ); - // We need to pass a real FileCache object to FsProbe ctor to have it safeguard "path.fileCacheUtilized" values properly! - FsProbe probe = new FsProbe(env, fileCache); - + FsProbe probe = new FsProbe(env, FileCache.NOOP_FILE_CACHE); FsInfo stats = probe.stats(null); assertNotNull(stats); assertThat(stats.getTimestamp(), greaterThan(0L)); @@ -120,6 +109,9 @@ public void testFsInfo() throws IOException { assertThat(total.total, greaterThan(0L)); assertThat(total.free, greaterThan(0L)); assertThat(total.available, greaterThan(0L)); + // Total file cache (sum over all "paths"): + assertThat(total.fileCacheReserved, equalTo(0L)); + assertThat(total.fileCacheUtilized, equalTo(0L)); // The convention for "total" Path object is that some fields are not set // which means they will not be included in output of toXContent method. @@ -127,10 +119,6 @@ public void testFsInfo() throws IOException { assertNull(total.mount); assertNull(total.type); - // Total file cache (sum over all "paths"): - assertEquals(total.getFileCacheReserved().getBytes(), 0); - assertEquals(total.getFileCacheUtilized().getBytes(), 0); - for (FsInfo.Path path : stats) { assertNotNull(path); assertThat(path.getPath(), is(not(emptyOrNullString()))); @@ -439,9 +427,25 @@ List readProcDiskStats() throws IOException { public void testAdjustForHugeFilesystems() throws Exception { NodePath np = new FakeNodePath(createTempDir()); - assertThat(FsProbe.getFSInfo(np).total, greaterThanOrEqualTo(0L)); - assertThat(FsProbe.getFSInfo(np).free, greaterThanOrEqualTo(0L)); - assertThat(FsProbe.getFSInfo(np).available, greaterThanOrEqualTo(0L)); + + FsInfo.Path path = FsProbe.getFSInfo(np, FileCache.NOOP_FILE_CACHE); + assertThat(path.total, greaterThanOrEqualTo(0L)); + assertThat(path.free, greaterThanOrEqualTo(0L)); + assertThat(path.available, greaterThanOrEqualTo(0L)); + assertThat(path.fileCacheReserved, greaterThanOrEqualTo(0L)); + assertThat(path.fileCacheUtilized, greaterThanOrEqualTo(0L)); + + /** The following test demonstrates that a call to {@link FsProbe#getFSInfo(NodePath)} + * leaves the file cache utilization value uninitialized which can have unexpected effects. + * Use of that method was deprecated and replaced by {@link FsProbe#getFSInfo(NodePath, FileCache)}. + * {@see https://github.com/opensearch-project/OpenSearch/pull/13232} + */ + path = FsProbe.getFSInfo(np); + assertThat(path.total, greaterThanOrEqualTo(0L)); + assertThat(path.free, greaterThanOrEqualTo(0L)); + assertThat(path.available, greaterThanOrEqualTo(0L)); + assertThat(path.fileCacheReserved, greaterThanOrEqualTo(0L)); + assertThat(path.fileCacheUtilized, greaterThanOrEqualTo(-1L)); // <-- !! } static class FakeNodePath extends NodeEnvironment.NodePath {