cryptoe commented on code in PR #18705:
URL: https://github.com/apache/druid/pull/18705#discussion_r2510631176
##########
processing/src/test/java/org/apache/druid/java/util/metrics/CgroupCpuSetMonitorTest.java:
##########
@@ -76,4 +81,32 @@ public void testMonitor()
emitter.verifyValue("cgroup/cpuset/mems_count", 4);
emitter.verifyValue("cgroup/cpuset/effective_mems_count", 1);
}
+
+ @Test
+ public void testCgroupsV2DetectionInConstructor() throws IOException
+ {
+ // Set up cgroups v2 structure
+ File cgroupV2Dir = temporaryFolder.newFolder();
+ File procV2Dir = temporaryFolder.newFolder();
+ TestUtils.setUpCgroupsV2(procV2Dir, cgroupV2Dir);
+
+ // Create v2 cpuset files in unified hierarchy
+ File cgroupRoot = new File(cgroupV2Dir, "unified");
+ FileUtils.mkdirp(cgroupRoot);
+ Files.write(Paths.get(cgroupRoot.getAbsolutePath(),
"cpuset.cpus.effective"), "0-3\n".getBytes(StandardCharsets.UTF_8));
+ Files.write(Paths.get(cgroupRoot.getAbsolutePath(),
"cpuset.mems.effective"), "0\n".getBytes(StandardCharsets.UTF_8));
+
+ CgroupDiscoverer v2Discoverer =
ProcSelfCgroupDiscoverer.autoCgroupDiscoverer(procV2Dir.toPath());
+ Assert.assertEquals(CgroupVersion.V2, v2Discoverer.getCgroupVersion());
+
+ // Constructor should detect v2 and log warning
+ CgroupCpuSetMonitor monitor = new CgroupCpuSetMonitor(v2Discoverer,
ImmutableMap.of(), "test-feed");
+
+ final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host");
+
+ // doMonitor should return true but skip actual monitoring
+ Assert.assertTrue(monitor.doMonitor(emitter));
+ Assert.assertEquals(4, emitter.getNumEmittedEvents());
Review Comment:
Added
##########
processing/src/test/java/org/apache/druid/java/util/metrics/cgroups/CpuV2Test.java:
##########
@@ -0,0 +1,422 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.java.util.metrics.cgroups;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+
+public class CpuV2Test
+{
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+ private File cgroupDir;
+ private File procDir;
+ private CgroupDiscoverer discoverer;
+
+ @Before
+ public void setUp() throws IOException
+ {
+ cgroupDir = temporaryFolder.newFolder();
+ procDir = temporaryFolder.newFolder();
+ TestUtils.setUpCgroupsV2(procDir, cgroupDir);
+ discoverer = new ProcCgroupV2Discoverer(procDir.toPath());
+ }
+
+ @Test
+ public void testCpuV2Snapshot() throws IOException
+ {
+ // Set up v2 files directly in cgroupDir (unified hierarchy root)
+ // Create cpu.stat with microsecond values
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec 123456789\nsystem_usec
987654321\ncore_sched.force_idle_usec 0\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ // Create cpu.weight (v2 weight)
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "200\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ // Create cpu.max (quota period)
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "150000 100000\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // Verify the conversion from v2 to v1 format
+ Assert.assertEquals("Weight should be converted to shares", 2048L,
metrics.getShares());
+ Assert.assertEquals("Quota should be preserved", 150000L,
metrics.getQuotaUs());
+ Assert.assertEquals("Period should be preserved", 100000L,
metrics.getPeriodUs());
+
+ // V2 should not provide jiffies, only microseconds
+ Assert.assertEquals("V2 should not provide user jiffies", -1L,
metrics.getUserJiffies());
+ Assert.assertEquals("V2 should not provide system jiffies", -1L,
metrics.getSystemJiffies());
+ Assert.assertEquals("V2 should not provide total jiffies", -1L,
metrics.getTotalJiffies());
+ }
+
+ @Test
+ public void testCpuV2SnapshotWithMaxQuota() throws IOException
+ {
+ // Set up v2 files with unlimited quota
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec 50000000\nsystem_usec
25000000\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "100\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ // cpu.max with "max" means no limit
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "max\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+ Assert.assertEquals("Default weight should convert to default shares",
1024L, metrics.getShares());
+ Assert.assertEquals("Max quota should be -1", -1L, metrics.getQuotaUs());
+ Assert.assertEquals("Max period should be -1", -1L, metrics.getPeriodUs());
+ Assert.assertEquals("V2 should not provide user jiffies", -1L,
metrics.getUserJiffies());
+ Assert.assertEquals("V2 should not provide system jiffies", -1L,
metrics.getSystemJiffies());
+ }
+
+ @Test
+ public void testCpuV2SnapshotWithMissingFiles() throws IOException
+ {
+ // Set up directory but don't create the files
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // Should return default/error values when files are missing
+ Assert.assertEquals("Missing weight should return default shares", -1L,
metrics.getShares());
+ Assert.assertEquals("Missing quota should be -1", -1L,
metrics.getQuotaUs());
+ Assert.assertEquals("Missing period should be -1", -1L,
metrics.getPeriodUs());
+ Assert.assertEquals("Missing user time should be -1", -1L,
metrics.getUserJiffies());
+ Assert.assertEquals("Missing system time should be -1", -1L,
metrics.getSystemJiffies());
+ Assert.assertEquals("Missing total should be -1", -1L,
metrics.getTotalJiffies());
+ }
+
+ @Test
+ public void testCpuV2SnapshotWithInvalidData() throws IOException
+ {
+ // Set up v2 files with invalid/malformed data
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec invalid_number\nsystem_usec
not_a_number\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "invalid\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "invalid format\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // Should handle invalid data gracefully
+ Assert.assertEquals("Invalid weight should return default", -1L,
metrics.getShares());
+ Assert.assertEquals("Invalid quota should be -1", -1L,
metrics.getQuotaUs());
+ Assert.assertEquals("Invalid period should be -1", -1L,
metrics.getPeriodUs());
+ Assert.assertEquals("Invalid user time should be -1", -1L,
metrics.getUserJiffies());
+ Assert.assertEquals("Invalid system time should be -1", -1L,
metrics.getSystemJiffies());
+ }
+
+ @Test
+ public void testWeightToSharesConversion()
+ {
+ // Test the weight to shares conversion logic via snapshot
+ try {
+
+ // Test various weight values
+ testWeightConversion(cgroupDir, 1, 10); // Minimum weight -> minimum
shares (clamped to 2)
+ testWeightConversion(cgroupDir, 100, 1024); // Default weight -> default
shares
+ testWeightConversion(cgroupDir, 200, 2048); // Double weight -> double
shares
+ testWeightConversion(cgroupDir, 1000, 10240); // 10x weight -> 10x shares
+ testWeightConversion(cgroupDir, 10000, 102400); // Maximum weight ->
maximum shares
+ }
+ catch (IOException e) {
+ Assert.fail("IOException during weight conversion test: " +
e.getMessage());
+ }
+ }
+
+ private void testWeightConversion(File cgroupDir, int weight, long
expectedShares) throws IOException
+ {
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ (weight + "\n").getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec 0\nsystem_usec 0\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "max\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ Assert.assertEquals(
+ "Weight " + weight + " should convert to shares " + expectedShares,
+ expectedShares, metrics.getShares()
+ );
+ }
+
+ @Test
+ public void testMicrosecondToJiffiesConversion() throws IOException
+ {
+
+ // Test microsecond to jiffies conversion (divide by 10000)
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec 100000000\nsystem_usec
50000000\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "100\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "max\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // V2 should not provide jiffies, only microseconds
+ Assert.assertEquals("V2 should not provide user jiffies", -1L,
metrics.getUserJiffies());
+ Assert.assertEquals("V2 should not provide system jiffies", -1L,
metrics.getSystemJiffies());
+ Assert.assertEquals("V2 should not provide total jiffies", -1L,
metrics.getTotalJiffies());
+ }
+
+ @Test
+ public void testCpuStatFileWithExtraFields() throws IOException
+ {
+ // Test parsing cpu.stat with additional fields that should be ignored
+
+
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ ("usage_usec 75000000\n" +
+ "user_usec 30000000\n" +
+ "system_usec 45000000\n" +
+ "core_sched.force_idle_usec 12345\n" +
+ "nr_periods 5000\n" +
+ "nr_throttled 100\n" +
+ "throttled_usec 1000000\n").getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "150\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "max\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // Should parse weight correctly and ignore extra cpu.stat fields
+ Assert.assertEquals("Weight should be converted", 1536L,
metrics.getShares()); // 150 * 1024 / 100
+ }
+
+ @Test
+ public void testCpuMaxFileWithOnlyQuota() throws IOException
+ {
+ // Test cpu.max with only quota value (no period)
+
+
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.stat"),
+ "user_usec 10000000\nsystem_usec
5000000\n".getBytes(StandardCharsets.UTF_8)
+ );
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.weight"),
+ "100\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ // Invalid format - single value instead of "quota period"
+ Files.write(
+ Paths.get(cgroupDir.getAbsolutePath(), "cpu.max"),
+ "75000\n".getBytes(StandardCharsets.UTF_8)
+ );
+
+ CpuV2 cpuV2 = new CpuV2(discoverer);
+ Cpu.CpuMetrics metrics = cpuV2.snapshot();
+
+ // Should handle malformed cpu.max gracefully
+ Assert.assertEquals("Invalid cpu.max should result in -1 quota", -1L,
metrics.getQuotaUs());
+ Assert.assertEquals("Invalid cpu.max should result in -1 period", -1L,
metrics.getPeriodUs());
+ }
+
+ @Test
+ public void testZeroMicrosecondValues() throws IOException
Review Comment:
Wanted to test if the values are initialized properly.
Added more asserts to check that.
##########
processing/src/main/java/org/apache/druid/java/util/metrics/CgroupV2CpuSetMonitor.java:
##########
Review Comment:
Added.
##########
processing/src/main/java/org/apache/druid/java/util/metrics/CgroupUtil.java:
##########
@@ -58,4 +59,57 @@ public static long readLongValue(CgroupDiscoverer
discoverer, String cgroup, Str
return defaultValue;
}
}
+
+ /**
+ * Detects if we're running on cgroups v2 by checking if the discoverer path
contains cgroup2 files.
+ * This is a simple heuristic to avoid running v1-specific monitors on v2
systems.
+ *
+ * @param cgroupDiscoverer the discoverer to check
+ * @return true if running on cgroups v2, false for v1 or unknown
+ */
+ public static boolean isRunningOnCgroupsV2(CgroupDiscoverer cgroupDiscoverer)
+ {
+ try {
+ // Try CPU cgroup first - check if cpu.max file exists (cgroups v2)
instead of cpu.cfs_quota_us (cgroups v1)
+ Path cpuCgroupPath = cgroupDiscoverer.discover("cpu");
+ if (cpuCgroupPath != null) {
+ Path cpuMaxFile = cpuCgroupPath.resolve("cpu.max");
+ Path cpuQuotaFile = cpuCgroupPath.resolve("cpu.cfs_quota_us");
+
+ if (Files.exists(cpuMaxFile) && !Files.exists(cpuQuotaFile)) {
+ return true;
+ }
+ }
+
+ // Fallback: Try cpuset cgroup - check if cpuset.cpus.effective exists
(v2) vs cpuset.effective_cpus (v1)
+ Path cpusetCgroupPath = cgroupDiscoverer.discover("cpuset");
+ if (cpusetCgroupPath != null) {
+ Path v2EffectiveCpusFile =
cpusetCgroupPath.resolve("cpuset.cpus.effective");
+ Path v1EffectiveCpusFile =
cpusetCgroupPath.resolve("cpuset.effective_cpus");
+
+ return Files.exists(v2EffectiveCpusFile) &&
!Files.exists(v1EffectiveCpusFile);
+ }
+ }
+ catch (Exception e) {
+ LOG.debug(e, "Could not determine cgroups version, assuming v1");
Review Comment:
This method is removed
##########
processing/src/main/java/org/apache/druid/java/util/metrics/CgroupUtil.java:
##########
@@ -58,4 +59,57 @@ public static long readLongValue(CgroupDiscoverer
discoverer, String cgroup, Str
return defaultValue;
}
}
+
+ /**
+ * Detects if we're running on cgroups v2 by checking if the discoverer path
contains cgroup2 files.
+ * This is a simple heuristic to avoid running v1-specific monitors on v2
systems.
+ *
+ * @param cgroupDiscoverer the discoverer to check
+ * @return true if running on cgroups v2, false for v1 or unknown
+ */
+ public static boolean isRunningOnCgroupsV2(CgroupDiscoverer cgroupDiscoverer)
Review Comment:
This method is no longer needed. Adjusted the code.
##########
processing/src/test/java/org/apache/druid/java/util/metrics/CgroupCpuMonitorTest.java:
##########
@@ -109,10 +111,53 @@ public void testMonitor() throws IOException,
InterruptedException
@Test
public void testQuotaCompute()
{
- Assert.assertEquals(-1, CgroupCpuMonitor.computeProcessorQuota(-1,
100000), 0);
- Assert.assertEquals(0, CgroupCpuMonitor.computeProcessorQuota(0, 100000),
0);
- Assert.assertEquals(-1, CgroupCpuMonitor.computeProcessorQuota(100000, 0),
0);
- Assert.assertEquals(2.0D, CgroupCpuMonitor.computeProcessorQuota(200000,
100000), 0);
- Assert.assertEquals(0.5D, CgroupCpuMonitor.computeProcessorQuota(50000,
100000), 0);
+ Assert.assertEquals(-1, CgroupUtil.computeProcessorQuota(-1, 100000), 0);
+ Assert.assertEquals(0, CgroupUtil.computeProcessorQuota(0, 100000), 0);
+ Assert.assertEquals(-1, CgroupUtil.computeProcessorQuota(100000, 0), 0);
+ Assert.assertEquals(2.0D, CgroupUtil.computeProcessorQuota(200000,
100000), 0);
+ Assert.assertEquals(0.5D, CgroupUtil.computeProcessorQuota(50000, 100000),
0);
+ }
+
+ @Test
+ public void testCgroupsV2Detection() throws IOException, URISyntaxException
+ {
+ // Set up cgroups v2 structure
+ File cgroupV2Dir = temporaryFolder.newFolder();
+ File procV2Dir = temporaryFolder.newFolder();
+ TestUtils.setUpCgroupsV2(procV2Dir, cgroupV2Dir);
+
+
+ CgroupDiscoverer v2Discoverer =
ProcSelfCgroupDiscoverer.autoCgroupDiscoverer(cgroupV2Dir.toPath());
+
+ // Constructor should detect v2 and log warning
+ CgroupCpuMonitor monitor = new CgroupCpuMonitor(v2Discoverer,
ImmutableMap.of(), "test-feed");
+
+ final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host");
+
+ // doMonitor should return true
+ Assert.assertTrue(monitor.doMonitor(emitter));
+
+ Assert.assertEquals(2, emitter.getEvents().size());
Review Comment:
Added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
