fix(store): survive bad cgroup paths and honor a command-line memory budget

hugegraph-store/docs/deployment-guide.md

@@ -452,7 +452,9 @@ local file only when it has no local file or the file is no newer than PD's `S:T
 upload sets no timestamp and a missing one counts as 0, so changing `S:FS` alone changes nothing.
 The simplest upgrade is to set `HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE=auto` (or
 `-Drocksdb.total_memory_size=auto`) on every Store and restart it; the Store keeps this value
-above both its local file and PD's copy. To change the shared copy instead, set
+above both its local file and PD's copy, as it does a `--rocksdb.total_memory_size` argument to
+the Store jar. An environment variable such as `ROCKSDB_TOTAL_MEMORY_SIZE` doesn't set the
+budget, because Spring binds it to a different key. To change the shared copy instead, set
 `rocksdb.total_memory_size` to `0` or remove it in `S:FS`, then put the current time in
 milliseconds into `S:Timestamp` through PD's KV service (gRPC `KvService.put`). Use `auto` there
 only once every Store runs this release: an earlier release can't parse `auto` and fails to start,

hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/options/CgroupMemoryLimit.java

@@ -20,6 +20,7 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -81,43 +82,50 @@ static CgroupMemoryLimit read(Path procSelfCgroup, Path mountinfo, Path defaultR
             if (!v2 && !("," + parts[1] + ",").contains(",memory,")) {
                 continue;
             }
-            List<String[]> mounts = v2 ? v2Mounts : v1Mounts;
-            boolean matched = false;
-            boolean fullView = false;
             long lineLimit = -1L;
-            for (String[] mount : mounts) {
-                if (!contains(mount[0], parts[2])) {
-                    // A mount of another subtree says nothing about this cgroup
-                    continue;
-                }
-                matched = true;
-                Path point = Paths.get(mount[1]);
-                String relative = rebase(parts[2], mount[0]);
-                // A full-hierarchy mount only covers the ancestors if this process's own
-                // directory is reachable through it
-                fullView |= mount[0].equals("/") && visible(point, relative);
-                lineLimit = lowest(lineLimit, v2 ? v2Limit(point, relative) :
-                                              v1Limit(point, relative));
-            }
-            if (!matched && !mounts.isEmpty()) {
-                // In a cgroup namespace /proc/self/cgroup says "/" while a mount made outside
-                // it has a root such as "/..", so the process's own cgroup can't be located.
-                // The limits at the mount roots still bound it
-                incomplete = true;
+            try {
+                List<String[]> mounts = v2 ? v2Mounts : v1Mounts;
+                boolean matched = false;
+                boolean fullView = false;
                 for (String[] mount : mounts) {
+                    if (!contains(mount[0], parts[2])) {
+                        // A mount of another subtree says nothing about this cgroup
+                        continue;
+                    }
+                    matched = true;
                     Path point = Paths.get(mount[1]);
-                    lineLimit = lowest(lineLimit, v2 ? readLimit(point.resolve("memory.max")) :
-                                                  v1Limit(point, ""));
+                    String relative = rebase(parts[2], mount[0]);
+                    // A full-hierarchy mount only covers the ancestors if this process's own
+                    // directory is reachable through it
+                    fullView |= mount[0].equals("/") && visible(point, relative);
+                    lineLimit = lowest(lineLimit, v2 ? v2Limit(point, relative) :
+                                                  v1Limit(point, relative));
                 }
-            } else if (!matched) {
-                String relative = rebase(parts[2], "/");
-                lineLimit = v2 ? v2Limit(defaultRoot, relative) :
-                            v1Limit(defaultRoot.resolve("memory"), relative);
-            } else if (v2 && (!fullView || parts[2].equals("/"))) {
-                // Only subtree mounts such as /kubepods, or the process sits at the root it
-                // can see, which inside a private cgroup namespace is the namespace root: a
-                // memory.max above is hidden. v1's hierarchical limit has no such gap
+                if (!matched && !mounts.isEmpty()) {
+                    // In a cgroup namespace /proc/self/cgroup says "/" while a mount made outside
+                    // it has a root such as "/..", so the process's own cgroup can't be located.
+                    // The limits at the mount roots still bound it
+                    incomplete = true;
+                    for (String[] mount : mounts) {
+                        Path point = Paths.get(mount[1]);
+                        lineLimit = lowest(lineLimit, v2 ? readLimit(point.resolve("memory.max")) :
+                                                      v1Limit(point, ""));
+                    }
+                } else if (!matched) {
+                    String relative = rebase(parts[2], "/");
+                    lineLimit = v2 ? v2Limit(defaultRoot, relative) :
+                                v1Limit(defaultRoot.resolve("memory"), relative);
+                } else if (v2 && (!fullView || parts[2].equals("/"))) {
+                    // Only subtree mounts such as /kubepods, or the process sits at the root it
+                    // can see, which inside a private cgroup namespace is the namespace root: a
+                    // memory.max above is hidden. v1's hierarchical limit has no such gap
+                    incomplete = true;
+                }
+            } catch (InvalidPathException e) {
+                // A path the JVM can't represent, such as a NUL from a \000 escape, hides
+                // this line's limits; say so rather than fail the Store's start
                 incomplete = true;
+                continue;
             }
             if (lineLimit > 0 && (limit <= 0 || lineLimit < limit)) {
                 limit = lineLimit;

hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/listener/PdConfigureListener.java

@@ -57,10 +57,12 @@ public class PdConfigureListener implements
     private static final String TIMESTAMP_KEY = "S:Timestamp";
     private static final String PD_CONFIG_FILE_NAME = "application-pd.yml";
     static final String MEMORY_KEY = "rocksdb.total_memory_size";
-    // Where an operator sets one Store's RocksDB budget: the Docker entrypoint's
-    // SPRING_APPLICATION_JSON (HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE) or
-    // -Drocksdb.total_memory_size
-    private static final String[] MEMORY_OVERRIDE_SOURCES = {"spring.application.json",
+    // Where an operator sets one Store's RocksDB budget: --rocksdb.total_memory_size, the
+    // Docker entrypoint's SPRING_APPLICATION_JSON (HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE) or
+    // -Drocksdb.total_memory_size. Not systemEnvironment: ROCKSDB_TOTAL_MEMORY_SIZE binds to
+    // rocksdb's total.memory.size key, so it never sets the budget and must not drop PD's
+    private static final String[] MEMORY_OVERRIDE_SOURCES = {"commandLineArgs",
+                                                             "spring.application.json",
                                                              "systemProperties"};
     private final String workDir = System.getProperty("user.dir");
     private final String fileSeparator = System.getProperty("file.separator");

hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/node/listener/PdConfigureListenerMemoryOverrideTest.java

@@ -30,13 +30,15 @@
 import org.springframework.core.env.MapPropertySource;
 import org.springframework.core.env.MutablePropertySources;
 import org.springframework.core.env.PropertiesPropertySource;
+import org.springframework.core.env.SimpleCommandLinePropertySource;
 import org.springframework.core.env.StandardEnvironment;
+import org.springframework.core.env.SystemEnvironmentPropertySource;
 
 /**
  * PdConfigureListener puts the PD-managed application-pd.yml in front of every other
- * property source. An explicit per-Store rocksdb.total_memory_size, from the Docker
- * entrypoint's SPRING_APPLICATION_JSON or -D, must still win when RocksdbConfig binds,
- * while the rest of the PD configuration keeps its precedence.
+ * property source. An explicit per-Store rocksdb.total_memory_size, from a command-line
+ * argument, the Docker entrypoint's SPRING_APPLICATION_JSON or -D, must still win when
+ * RocksdbConfig binds, while the rest of the PD configuration keeps its precedence.
  */
 public class PdConfigureListenerMemoryOverrideTest {
 
@@ -65,6 +67,37 @@ public void testSystemPropertyOverrideWinsOverPdConfig() {
                 .get("total_memory_size"));
     }
 
+    @Test
+    public void testCommandLineOverrideWinsOverPdConfig() {
+        assertEquals("1073741824",
+                     bind(withCommandLine(environment(null, null))).get("total_memory_size"));
+        // Spring puts command-line arguments ahead of SPRING_APPLICATION_JSON
+        assertEquals("1073741824", bind(withCommandLine(environment(json("268435456"), null)))
+                .get("total_memory_size"));
+    }
+
+    private static StandardEnvironment withCommandLine(StandardEnvironment env) {
+        env.getPropertySources().addFirst(new SimpleCommandLinePropertySource(
+                "--" + PdConfigureListener.MEMORY_KEY + "=1073741824"));
+        return env;
+    }
+
+    @Test
+    public void testEnvironmentVariableKeepsPdConfig() {
+        StandardEnvironment env = environment(null, null);
+        Map<String, Object> variables = new HashMap<>();
+        variables.put("ROCKSDB_TOTAL_MEMORY_SIZE", "268435456");
+        env.getPropertySources().addBefore("applicationConfig: [application-pd.yml]",
+                                           new SystemEnvironmentPropertySource(
+                                                   StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME,
+                                                   variables));
+        // The variable binds to the total.memory.size key, not the budget, so it must not
+        // drop PD's value and leave the Store on its local file
+        Map<String, String> rocksdb = bind(env);
+        assertEquals(PD_BUDGET, rocksdb.get("total_memory_size"));
+        assertEquals("268435456", rocksdb.get("total.memory.size"));
+    }
+
     @Test
     public void testWithoutOverridePdConfigStillApplies() {
         Map<String, String> rocksdb = bind(environment(null, null));

hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/options/CgroupMemoryLimitTest.java

@@ -196,6 +196,28 @@ public void testCgroupV1SubtreeMountAndEscapedMountPoint() throws IOException {
         }
     }
 
+    @Test
+    public void testUnrepresentableMountPointIsFlaggedNotFatal() throws IOException {
+        Path root = Files.createTempDirectory("cgroup-bad-path");
+        try {
+            Path v1 = root.resolve("memory");
+            write(v1, "memory.limit_in_bytes", String.valueOf(4 * GIB));
+            Path proc = write(root, "proc-self-cgroup", "0::/kubepods/pod1\n4:memory:/\n");
+            // The kernel never writes \000, but a NUL in the v2 mount point must only hide
+            // that line's limits, not stop the Store or the v1 line's lookup
+            Path mountinfo = write(root, "mountinfo",
+                                   "30 25 0:26 / " + root.resolve("v2") +
+                                   "\\000x rw - cgroup2 cgroup2 rw\n" +
+                                   "40 25 0:30 / " + v1 + " rw - cgroup cgroup rw,memory\n");
+            CgroupMemoryLimit limit = CgroupMemoryLimit.read(proc, mountinfo, root);
+            assertTrue(limit.incomplete);
+            assertEquals(4 * GIB, limit.bytes);
+            assertEquals(1, limit.version);
+        } finally {
+            FileUtils.deleteQuietly(root.toFile());
+        }
+    }
+
     @Test
     public void testCgroupV2MountOrderDoesNotHideAncestorLimit() throws IOException {
         Path root = Files.createTempDirectory("cgroup-v2-mounts");