From 75e04e49429fd8b7c1a384bd108efb11c558b10b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= <lukas.vlcek@aiven.io>
Date: Mon, 8 Jul 2024 22:54:02 +0200
Subject: [PATCH] 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 <lukas.vlcek@aiven.io>
---
.../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<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);
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<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;
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<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 {