Deprecating static getFSInfo method

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 <lukas.vlcek@aiven.io>

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:

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<String> allTypes = new HashSet<>();
             Set<String> 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);

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<Path, CachedIndexInput> {
+
+    /**
+     * 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)}.
+     * <br>
+     * 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<Path, CachedIndexInput> theCache;
 

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());
     }

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,17 +109,16 @@ 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.
             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())));
@@ -439,9 +427,25 @@ List<String> 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 {