capistrant commented on code in PR #18705: URL: https://github.com/apache/druid/pull/18705#discussion_r2478130779
########## processing/src/test/java/org/apache/druid/java/util/metrics/cgroups/CpuSetV2Test.java: ########## @@ -0,0 +1,324 @@ +/* + * 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; Review Comment: should new test file be written with junit5? ########## 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 Review Comment: ```suggestion public void testV2doesNotConvertMicrosecondToJiffies() throws IOException ``` nit: existing name feels misleading since there is no conversion, jiffies just aren't provided ########## 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: is this actually testing anything that the jiffies conversion test above doesn't already cover? ########## 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: should we verify the version dimension is v2? ########## processing/src/test/java/org/apache/druid/java/util/metrics/cgroups/CpuSetV2Test.java: ########## @@ -0,0 +1,324 @@ +/* + * 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 CpuSetV2Test +{ + @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 testCpuSetV2Snapshot() throws IOException + { + // Set up v2 cpuset files directly in cgroupDir (unified hierarchy root) + File cgroupRoot = cgroupDir; + + // Create v2 cpuset files with different names than v1 + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-7\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "0-3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0-1\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Verify CPU set parsing + int[] expectedCpus = {0, 1, 2, 3, 4, 5, 6, 7}; + int[] expectedEffectiveCpus = {0, 1, 2, 3}; + int[] expectedMems = {0, 1}; + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("CPU set should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Effective CPU set should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Memory set should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Effective memory set should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testComplexCpuRangesParsing() throws IOException + { + File cgroupRoot = cgroupDir; + + // Test complex CPU ranges with mixed single CPUs and ranges + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-2,7,12-14\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "0,2,7\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0,2-3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // 0-2,7,12-14 should expand to [0,1,2,7,12,13,14] + int[] expectedCpus = {0, 1, 2, 7, 12, 13, 14}; + // 0,2,7 should expand to [0,2,7] + int[] expectedEffectiveCpus = {0, 2, 7}; + // 0,2-3 should expand to [0,2,3] + int[] expectedMems = {0, 2, 3}; + // 0 should be [0] + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("Complex CPU ranges should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Complex effective CPU ranges should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Complex memory ranges should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Single memory node should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testEmptyCpuSetFiles() throws IOException + { + File cgroupRoot = cgroupDir; + + // Create empty cpuset files + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Empty files should result in empty arrays + Assert.assertArrayEquals("Empty CPU file should result in empty array", new int[0], metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Empty effective CPU file should result in empty array", new int[0], metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Empty memory file should result in empty array", new int[0], metrics.getCpuSetMems()); + Assert.assertArrayEquals("Empty effective memory file should result in empty array", new int[0], metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testMissingCpuSetFiles() throws IOException + { + // Set up directory but don't create the files + File cgroupRoot = cgroupDir; Review Comment: I think just a comment is fine. The unused variable will just create scan warns, but doesn't provide any functional benefit as far as I can tell ########## 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; Review Comment: same as other new UT file, should we write it in junit5? ########## 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 looks like it will use cgroupv1 if both exist. in processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/ProcSelfCgroupDiscoverer.java `detectCgroupVersion` it looks like it prefers v2 even if both exist. Is this discrepancy intentional? ########## 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: should we verify the version dimension is set to v2? ########## 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: should this actually be a warn so operators can investigate and get things properly configured? ########## processing/src/test/java/org/apache/druid/java/util/metrics/cgroups/CpuSetV2Test.java: ########## @@ -0,0 +1,324 @@ +/* + * 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 CpuSetV2Test +{ + @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 testCpuSetV2Snapshot() throws IOException + { + // Set up v2 cpuset files directly in cgroupDir (unified hierarchy root) + File cgroupRoot = cgroupDir; + + // Create v2 cpuset files with different names than v1 + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-7\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "0-3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0-1\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Verify CPU set parsing + int[] expectedCpus = {0, 1, 2, 3, 4, 5, 6, 7}; + int[] expectedEffectiveCpus = {0, 1, 2, 3}; + int[] expectedMems = {0, 1}; + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("CPU set should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Effective CPU set should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Memory set should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Effective memory set should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testComplexCpuRangesParsing() throws IOException + { + File cgroupRoot = cgroupDir; + + // Test complex CPU ranges with mixed single CPUs and ranges + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-2,7,12-14\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "0,2,7\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0,2-3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // 0-2,7,12-14 should expand to [0,1,2,7,12,13,14] + int[] expectedCpus = {0, 1, 2, 7, 12, 13, 14}; + // 0,2,7 should expand to [0,2,7] + int[] expectedEffectiveCpus = {0, 2, 7}; + // 0,2-3 should expand to [0,2,3] + int[] expectedMems = {0, 2, 3}; + // 0 should be [0] + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("Complex CPU ranges should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Complex effective CPU ranges should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Complex memory ranges should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Single memory node should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testEmptyCpuSetFiles() throws IOException + { + File cgroupRoot = cgroupDir; + + // Create empty cpuset files + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Empty files should result in empty arrays + Assert.assertArrayEquals("Empty CPU file should result in empty array", new int[0], metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Empty effective CPU file should result in empty array", new int[0], metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Empty memory file should result in empty array", new int[0], metrics.getCpuSetMems()); + Assert.assertArrayEquals("Empty effective memory file should result in empty array", new int[0], metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testMissingCpuSetFiles() throws IOException + { + // Set up directory but don't create the files + File cgroupRoot = cgroupDir; + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Missing files should result in empty arrays + Assert.assertArrayEquals("Missing CPU file should result in empty array", new int[0], metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Missing effective CPU file should result in empty array", new int[0], metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Missing memory file should result in empty array", new int[0], metrics.getCpuSetMems()); + Assert.assertArrayEquals("Missing effective memory file should result in empty array", new int[0], metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testInvalidCpuSetData() throws IOException + { + File cgroupRoot = cgroupDir; + + // Create files with invalid data + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "invalid-range\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "not-a-number\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "1-abc\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "5-3\n".getBytes(StandardCharsets.UTF_8)); // Invalid range (high < low) + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Invalid data should be handled gracefully and result in empty arrays + Assert.assertArrayEquals("Invalid CPU data should result in empty array", new int[0], metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Invalid effective CPU data should result in empty array", new int[0], metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Invalid memory data should result in empty array", new int[0], metrics.getCpuSetMems()); + Assert.assertArrayEquals("Invalid effective memory data should result in empty array", new int[0], metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testMixedValidAndInvalidData() throws IOException + { + File cgroupRoot = cgroupDir; + + // Mix valid and invalid data in the same line + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-2,invalid,7,bad-range,10\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "0,not-valid,2\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + // Should parse only valid parts: 0-2,7,10 -> [0,1,2,7,10] + int[] expectedCpus = {0, 1, 2, 7, 10}; + // Should parse only valid parts: 0,2 -> [0,2] + int[] expectedEffectiveCpus = {0, 2}; + int[] expectedMems = {0}; + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("Should parse only valid CPU parts", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Should parse only valid effective CPU parts", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Valid memory data should parse correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Valid effective memory data should parse correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testSingleCpuValues() throws IOException + { + File cgroupRoot = cgroupDir; + + // Test single CPU values without ranges + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "5\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "1\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + Assert.assertArrayEquals("Single CPU should be parsed", new int[]{5}, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Single effective CPU should be parsed", new int[]{3}, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Single memory node should be parsed", new int[]{1}, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Single effective memory node should be parsed", new int[]{0}, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testLargeRanges() throws IOException + { + File cgroupRoot = cgroupDir; + + // Test larger ranges + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + "0-15\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "8-11\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + "0-3\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "1-2\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + int[] expectedCpus = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; + int[] expectedEffectiveCpus = {8, 9, 10, 11}; + int[] expectedMems = {0, 1, 2, 3}; + int[] expectedEffectiveMems = {1, 2}; + + Assert.assertArrayEquals("Large CPU range should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Large effective CPU range should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Large memory range should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Large effective memory range should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testWhitespaceInFiles() throws IOException + { + File cgroupRoot = cgroupDir; + + // Test files with extra whitespace + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus"), + " 0-2,7 \n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.cpus.effective"), + "\t0,2\t\n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems"), + " 0 \n".getBytes(StandardCharsets.UTF_8)); + Files.write(Paths.get(cgroupRoot.getAbsolutePath(), "cpuset.mems.effective"), + "0\n".getBytes(StandardCharsets.UTF_8)); + + CpuSetV2 cpuSetV2 = new CpuSetV2(discoverer); + CpuSet.CpuSetMetric metrics = cpuSetV2.snapshot(); + + int[] expectedCpus = {0, 1, 2, 7}; + int[] expectedEffectiveCpus = {0, 2}; + int[] expectedMems = {0}; + int[] expectedEffectiveMems = {0}; + + Assert.assertArrayEquals("CPU data with whitespace should be parsed correctly", expectedCpus, metrics.getCpuSetCpus()); + Assert.assertArrayEquals("Effective CPU data with whitespace should be parsed correctly", expectedEffectiveCpus, metrics.getEffectiveCpuSetCpus()); + Assert.assertArrayEquals("Memory data with whitespace should be parsed correctly", expectedMems, metrics.getCpuSetMems()); + Assert.assertArrayEquals("Effective memory data should be parsed correctly", expectedEffectiveMems, metrics.getEffectiveCpuSetMems()); + } + + @Test + public void testBackwardsCompatibilityWithV1Parsing() throws IOException Review Comment: could you point out how this format is different than all the tests above? I'm missing what is unique about it compared to the rest of the v2 tests ########## processing/src/main/java/org/apache/druid/java/util/metrics/CgroupV2CpuSetMonitor.java: ########## Review Comment: Does this require an entry in https://druid.apache.org/docs/latest/configuration/#metrics-monitors? Will cover the docs topic more in the top level review comment ########## processing/src/main/java/org/apache/druid/java/util/metrics/CgroupMemoryMonitor.java: ########## @@ -58,11 +76,28 @@ public CgroupMemoryMonitor() @Override public boolean doMonitor(ServiceEmitter emitter) + { + if (isRunningOnCgroupsV2) { + return cgroupV2MemoryMonitor.doMonitor(emitter); + } else { + return doMonitorInternal(emitter, cgroupDiscoverer, dimensions, MEMORY_USAGE_FILE, MEMORY_LIMIT_FILE, this); + } + } + + public static boolean doMonitorInternal( Review Comment: short javadoc might be helpful not that this is a static utility method used by v1 and v2 monitor. I don't fully understand the "internal" part of the name either ########## processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/CpuV2.java: ########## @@ -0,0 +1,136 @@ +/* + * 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 com.google.common.primitives.Longs; +import org.apache.druid.java.util.common.logger.Logger; +import org.apache.druid.java.util.metrics.CgroupUtil; + +import java.io.BufferedReader; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.regex.Pattern; + +/** + * Collect CPU weight, quota and usage information from cgroups v2 files. + * This class provides a bridge to return Cpu.CpuMetrics compatible data from cgroups v2. + */ +public class CpuV2 +{ + public static final String CGROUP = ""; // cgroups v2 uses unified hierarchy + private static final Logger LOG = new Logger(CpuV2.class); + private static final String CPU_STAT_FILE = "cpu.stat"; + private static final String CPU_WEIGHT_FILE = "cpu.weight"; + private static final String + CPU_MAX_FILE = "cpu.max"; Review Comment: ```suggestion private static final String CPU_MAX_FILE = "cpu.max"; ``` -- 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]
