brumi1024 commented on code in PR #6780: URL: https://github.com/apache/hadoop/pull/6780#discussion_r1596818926
########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/CGroupsMixedResourceCalculator.java: ########## @@ -0,0 +1,98 @@ +/** + * 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.hadoop.yarn.server.nodemanager.containermanager.linux.resources; + +import java.util.Arrays; +import java.util.List; + +import org.apache.hadoop.yarn.exceptions.YarnException; +import org.apache.hadoop.yarn.util.ResourceCalculatorProcessTree; + +/** + * In case if {@link CGroupsV2ResourceCalculator} can provide the required info that will be used. + * Otherwise, it will fall back to the {@link CGroupsResourceCalculator} implementation. + * Similar like {@link CombinedResourceCalculator} works. + */ +public class CGroupsMixedResourceCalculator extends ResourceCalculatorProcessTree { Review Comment: Sorry, just to complicate things a bit more: there seems to be a CombinedResourceCalculator which takes cgroup and procfs based resourcecalculators into account. Probably it's CGroupsResourceCalculator cgroup should use this class instead. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/TestCGroupsResourceCalculator.java: ########## @@ -18,258 +18,138 @@ package org.apache.hadoop.yarn.server.nodemanager.containermanager.linux.resources; -import org.apache.commons.io.FileUtils; -import org.apache.hadoop.yarn.exceptions.YarnException; -import org.apache.hadoop.yarn.util.ControlledClock; -import org.apache.hadoop.yarn.util.ResourceCalculatorProcessTree; -import org.junit.Assert; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.stream.Collectors; + +import org.junit.After; +import org.junit.Before; import org.junit.Test; -import java.io.File; -import java.nio.charset.StandardCharsets; +import org.apache.commons.io.FileUtils; +import org.apache.hadoop.util.CpuTimeTracker; +import org.apache.hadoop.yarn.exceptions.YarnException; -import static org.mockito.Mockito.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** * Unit test for CGroupsResourceCalculator. */ public class TestCGroupsResourceCalculator { - private ControlledClock clock = new ControlledClock(); - private CGroupsHandler cGroupsHandler = mock(CGroupsHandler.class); - private String basePath = "/tmp/" + this.getClass().getName(); + private Path root; - public TestCGroupsResourceCalculator() { - when(cGroupsHandler.getRelativePathForCGroup("container_1")) - .thenReturn("/yarn/container_1"); - when(cGroupsHandler.getRelativePathForCGroup("")).thenReturn("/yarn/"); + @Before + public void before() throws IOException { + root = Files.createTempDirectory("TestCGroupsResourceCalculator"); + } + + @After + public void after() throws IOException { + FileUtils.deleteDirectory(root.toFile()); } - @Test(expected = YarnException.class) + @Test(expected = IOException.class) public void testPidNotFound() throws Exception { - CGroupsResourceCalculator calculator = - new CGroupsResourceCalculator( - "1234", ".", cGroupsHandler, clock, 10); + CGroupsResourceCalculator calculator = createCalculator(); calculator.setCGroupFilePaths(); - Assert.assertEquals("Expected exception", null, calculator); } - @Test(expected = YarnException.class) + @Test public void testNoMemoryCGgroupMount() throws Exception { - File procfs = new File(basePath + "/1234"); - Assert.assertTrue("Setup error", procfs.mkdirs()); + writeToFile("proc/41/cgroup", + "7:devices:/yarn/container_1", + "6:cpuacct,cpu:/yarn/container_1", + "5:pids:/yarn/container_1" + ); + + CGroupsResourceCalculator calculator = createCalculator(); try { - FileUtils.writeStringToFile( - new File(procfs, CGroupsResourceCalculator.CGROUP), - "7:devices:/yarn/container_1\n" + - "6:cpuacct,cpu:/yarn/container_1\n" + - "5:pids:/yarn/container_1\n", StandardCharsets.UTF_8); - CGroupsResourceCalculator calculator = - new CGroupsResourceCalculator( - "1234", basePath, - cGroupsHandler, clock, 10); calculator.setCGroupFilePaths(); - Assert.assertEquals("Expected exception", null, calculator); - } finally { - FileUtils.deleteDirectory(new File(basePath)); + fail("No exception was thrown"); + } catch (YarnException e) { + assertEquals("Controller MEMORY not found for 41 pid", e.getMessage()); Review Comment: Nit: it might be a bit more developer friendly to assert a contains(MEMORY) and contains(pid), because I wouldn't expect test failures for message wording changes. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/CGroupsV2ResourceCalculator.java: ########## @@ -0,0 +1,76 @@ +/** + * 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.hadoop.yarn.server.nodemanager.containermanager.linux.resources; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Stream; + +import org.apache.commons.lang3.StringUtils; + +/** + * A CGroupV2 file-system based Resource calculator without the process tree features. + * + * The feature only works if cluster runs in pure V2 version, because when we read the + * /proc/{pid}/cgroup file currently we can not handle multiple lines. + */ +public class CGroupsV2ResourceCalculator extends AbstractCGroupsResourceCalculator { + + /** + * Create resource calculator for the container that has the specified pid. + * @param pid A pid from the cgroup or null for all containers + */ + public CGroupsV2ResourceCalculator(String pid) { + super( + pid, + Collections.singletonList("cpu.stat#usage_usec"), + "memory.stat#anon", + "memory.stat#vmalloc" + ); Review Comment: Nit: just for the sake of symmetry: these could be extracted to static final consts, like the similar values in the v1 resourceCalculator. ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/AbstractCGroupsResourceCalculator.java: ########## @@ -0,0 +1,179 @@ +/** + * 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.hadoop.yarn.server.nodemanager.containermanager.linux.resources; + +import java.io.IOException; +import java.math.BigInteger; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.commons.io.FileUtils; +import org.apache.hadoop.classification.VisibleForTesting; +import org.apache.hadoop.util.CpuTimeTracker; +import org.apache.hadoop.util.SysInfoLinux; +import org.apache.hadoop.yarn.exceptions.YarnException; +import org.apache.hadoop.yarn.util.Clock; +import org.apache.hadoop.yarn.util.ResourceCalculatorProcessTree; +import org.apache.hadoop.yarn.util.SystemClock; + +/** + * Common code base for the CGroupsResourceCalculator implementations. + */ +public abstract class AbstractCGroupsResourceCalculator extends ResourceCalculatorProcessTree { + private static final Logger LOG = LoggerFactory.getLogger(AbstractCGroupsResourceCalculator.class); + protected final String pid; + protected final Clock clock = SystemClock.getInstance(); + protected final Map<String, String> stats = new ConcurrentHashMap<>(); + + @VisibleForTesting + long jiffyLengthMs = SysInfoLinux.JIFFY_LENGTH_IN_MILLIS; + @VisibleForTesting + CpuTimeTracker cpuTimeTracker; + @VisibleForTesting + CGroupsHandler cGroupsHandler; + @VisibleForTesting + String root = "/"; Review Comment: All the other implementations of the ResourceCalculatorProcessTree use a `private static final String PROCFS` variable, so that on the first glace it's visible that they operate on that file. This class does that too, however it's buried under a Path constructor. I think the previous solution, having a "/proc" variable (it even could be moved to the ResourceCalculatorProcessTree base class) and having a constructor that can override the procFS location is a bit more self explanatory. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
