Repository: Azure/WALinuxAgent
Pull request #1572 (2019-07-01 21:30:02)

Make agent's cgroup name consistent
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
The current name picked up for the systemd case is the scope name rather than the agent name. Fixing it.

<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [x] The title of the PR is clear and informative.
- [x] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [x] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [x] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/azure/walinuxagent/1572)
<!-- Reviewable:end -->



Patch:
diff --git a/azurelinuxagent/common/cgroupapi.py b/azurelinuxagent/common/cgroupapi.py
index 6b6d06f5..30136b66 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -349,8 +349,8 @@ class SystemdCgroupsApi(CGroupsApi):
             cpu_cgroup_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, 'cpu', cgroup_unit)
             memory_cgroup_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, 'memory', cgroup_unit)
 
-            return [CGroup.create(cpu_cgroup_path, 'cpu', cgroup_unit),
-                    CGroup.create(memory_cgroup_path, 'memory', cgroup_unit)]
+            return [CGroup.create(cpu_cgroup_path, 'cpu', VM_AGENT_CGROUP_NAME),
+                    CGroup.create(memory_cgroup_path, 'memory', VM_AGENT_CGROUP_NAME)]
         except Exception as e:
             raise CGroupsException("Failed to get paths of agent's cgroups. Error: {0}".format(ustr(e)))
 
diff --git a/azurelinuxagent/common/osutil/default.py b/azurelinuxagent/common/osutil/default.py
index 7cc2be87..116ddd36 100644
--- a/azurelinuxagent/common/osutil/default.py
+++ b/azurelinuxagent/common/osutil/default.py
@@ -325,22 +325,34 @@ class DefaultOSUtil(object):
                            option="-t tmpfs",
                            chk_err=False)
             elif not os.path.isdir(self._cgroup_path()):
-                logger.error("Could not mount cgroups: ordinary file at {0}".format(path))
+                logger.error("Could not mount cgroups: ordinary file at {0}", path)
                 return
 
-            for metric_hierarchy in ['cpu,cpuacct', 'memory']:
-                target_path = self._cgroup_path(metric_hierarchy)
-                if not os.path.exists(target_path):
-                    fileutil.mkdir(target_path)
-                    self.mount(device=metric_hierarchy,
-                               mount_point=target_path,
-                               option="-t cgroup -o {0}".format(metric_hierarchy),
-                               chk_err=False)
-
-            for metric_hierarchy in ['cpu', 'cpuacct']:
-                target_path = self._cgroup_path(metric_hierarchy)
-                if not os.path.exists(target_path):
-                    os.symlink(self._cgroup_path('cpu,cpuacct'), target_path)
+            controllers_to_mount = ['cpu,cpuacct', 'memory']
+            errors = 0
+            cpu_mounted = False
+            for controller in controllers_to_mount:
+                try:
+                    target_path = self._cgroup_path(controller)
+                    if not os.path.exists(target_path):
+                        fileutil.mkdir(target_path)
+                        self.mount(device=controller,
+                                   mount_point=target_path,
+                                   option="-t cgroup -o {0}".format(controller),
+                                   chk_err=False)
+                        if controller == 'cpu,cpuacct':
+                            cpu_mounted = True
+                except Exception as exception:
+                    errors += 1
+                    if errors == len(controllers_to_mount):
+                        raise
+                    logger.warn("Could not mount cgroup controller {0}: {1}", controller, ustr(exception))
+
+            if cpu_mounted:
+                for controller in ['cpu', 'cpuacct']:
+                    target_path = self._cgroup_path(controller)
+                    if not os.path.exists(target_path):
+                        os.symlink(self._cgroup_path('cpu,cpuacct'), target_path)
 
         except OSError as oe:
             # log a warning for read-only file systems


Test patch:
diff --git a/tests/common/osutil/test_default_osutil.py b/tests/common/osutil/test_default_osutil.py
new file mode 100644
index 00000000..7c12f02e
--- /dev/null
+++ b/tests/common/osutil/test_default_osutil.py
@@ -0,0 +1,179 @@
+# Copyright 2018 Microsoft Corporation
+#
+# Licensed 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.
+#
+# Requires Python 2.4+ and Openssl 1.0+
+#
+
+from azurelinuxagent.common.osutil.default import DefaultOSUtil, shellutil
+from tests.tools import *
+
+
+class DefaultOsUtilTestCase(AgentTestCase):
+
+    def setUp(self):
+        AgentTestCase.setUp(self)
+        self.cgroups_file_system_root = os.path.join(self.tmp_dir, "cgroups")
+        self.mock_base_cgroups = patch("azurelinuxagent.common.osutil.default.BASE_CGROUPS", self.cgroups_file_system_root)
+        self.mock_base_cgroups.start()
+
+    def tearDown(self):
+        self.mock_base_cgroups.stop()
+
+    @staticmethod
+    def _get_mount_commands(mock):
+        mount_commands = ''
+        for call_args in mock.call_args_list:
+            args, kwargs = call_args
+            mount_commands += ';' + args[0]
+        return mount_commands
+
+    def test_mount_cgroups_should_mount_the_cpu_and_memory_controllers(self):
+        # the mount command requires root privileges; make it a no op and check only for file existence
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output) as patch_run_get_output:
+            DefaultOSUtil().mount_cgroups()
+
+            # the directories for the controllers should have been created
+            for controller in ['cpu', 'memory', 'cpuacct', 'cpu,cpuacct']:
+                directory = os.path.join(self.cgroups_file_system_root, controller)
+                self.assertTrue(os.path.exists(directory), "A directory for controller {0} was not created".format(controller))
+
+            # the cgroup filesystem and the cpu and memory controllers should have been mounted
+            mount_commands = DefaultOsUtilTestCase._get_mount_commands(patch_run_get_output)
+
+            self.assertRegex(mount_commands, ';mount.* cgroup_root ', 'The cgroups file system was not mounted')
+            self.assertRegex(mount_commands, ';mount.* cpu,cpuacct ', 'The cpu controller was not mounted')
+            self.assertRegex(mount_commands, ';mount.* memory ', 'The memory controller was not mounted')
+
+    def test_mount_cgroups_should_not_mount_the_cgroups_file_system_when_it_already_exists(self):
+        os.mkdir(self.cgroups_file_system_root)
+
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output) as patch_run_get_output:
+            DefaultOSUtil().mount_cgroups()
+
+            mount_commands = DefaultOsUtilTestCase._get_mount_commands(patch_run_get_output)
+
+            self.assertNotIn('cgroup_root', mount_commands, 'The cgroups file system should not have been mounted')
+            self.assertRegex(mount_commands, ';mount.* cpu,cpuacct ', 'The cpu controller was not mounted')
+            self.assertRegex(mount_commands, ';mount.* memory ', 'The memory controller was not mounted')
+
+    def test_mount_cgroups_should_not_mount_cgroup_controllers_when_they_already_exist(self):
+        os.mkdir(self.cgroups_file_system_root)
+        os.mkdir(os.path.join(self.cgroups_file_system_root, 'cpu,cpuacct'))
+        os.mkdir(os.path.join(self.cgroups_file_system_root, 'memory'))
+
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output) as patch_run_get_output:
+            DefaultOSUtil().mount_cgroups()
+
+            mount_commands = DefaultOsUtilTestCase._get_mount_commands(patch_run_get_output)
+
+            self.assertNotIn('cgroup_root', mount_commands, 'The cgroups file system should not have been mounted')
+            self.assertNotIn('cpu,cpuacct', mount_commands, 'The cpu controller should not have been mounted')
+            self.assertNotIn('memory', mount_commands, 'The memory controller should not have been mounted')
+
+    def test_mount_cgroups_should_handle_errors_when_mounting_an_individual_controller(self):
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                if 'memory' in cmd:
+                    raise Exception('A test exception mounting the memory controller')
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output) as patch_run_get_output:
+            with patch("azurelinuxagent.common.cgroupconfigurator.logger.warn") as mock_logger_warn:
+                DefaultOSUtil().mount_cgroups()
+
+                # the cgroup filesystem and the cpu controller should still have been mounted
+                mount_commands = DefaultOsUtilTestCase._get_mount_commands(patch_run_get_output)
+
+                self.assertRegex(mount_commands, ';mount.* cgroup_root ', 'The cgroups file system was not mounted')
+                self.assertRegex(mount_commands, ';mount.* cpu,cpuacct ', 'The cpu controller was not mounted')
+
+                # A warning should have been logged for the memory controller
+                args, kwargs = mock_logger_warn.call_args
+                self.assertIn('A test exception mounting the memory controller', args)
+
+    def test_mount_cgroups_should_raise_when_the_cgroups_filesystem_fails_to_mount(self):
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                if 'cgroup_root' in cmd:
+                    raise Exception('A test exception mounting the cgroups file system')
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output) as patch_run_get_output:
+            with self.assertRaises(Exception) as context_manager:
+                DefaultOSUtil().mount_cgroups()
+
+            self.assertRegex(str(context_manager.exception), 'A test exception mounting the cgroups file system')
+
+            mount_commands = DefaultOsUtilTestCase._get_mount_commands(patch_run_get_output)
+            self.assertNotIn('memory', mount_commands, 'The memory controller should not have been mounted')
+            self.assertNotIn('cpu', mount_commands, 'The cpu controller should not have been mounted')
+
+    def test_mount_cgroups_should_raise_when_all_controllers_fail_to_mount(self):
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                if 'memory' in cmd or 'cpu,cpuacct' in cmd:
+                    raise Exception('A test exception mounting a cgroup controller')
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output):
+            with self.assertRaises(Exception) as context_manager:
+                DefaultOSUtil().mount_cgroups()
+
+            self.assertRegex(str(context_manager.exception), 'A test exception mounting a cgroup controller')
+
+    def test_mount_cgroups_should_not_create_symbolic_links_when_the_cpu_controller_fails_to_mount(self):
+        original_run_get_output = shellutil.run_get_output
+
+        def mock_run_get_output(cmd, *args, **kwargs):
+            if cmd.startswith('mount '):
+                if 'cpu,cpuacct' in cmd:
+                    raise Exception('A test exception mounting the cpu controller')
+                return 0, None
+            return original_run_get_output(cmd, *args, **kwargs)
+
+        with patch("azurelinuxagent.common.osutil.default.shellutil.run_get_output", side_effect=mock_run_get_output):
+            with patch("azurelinuxagent.common.osutil.default.os.symlink") as patch_symlink:
+                DefaultOSUtil().mount_cgroups()
+
+                self.assertEquals(patch_symlink.call_count, 0, 'A symbolic link should not have been created')
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index 597ac6f2..eb87adca 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -19,7 +19,7 @@ from __future__ import print_function
 
 import subprocess
 
-from azurelinuxagent.common.cgroupapi import CGroupsApi, FileSystemCgroupsApi, SystemdCgroupsApi
+from azurelinuxagent.common.cgroupapi import CGroupsApi, FileSystemCgroupsApi, SystemdCgroupsApi, VM_AGENT_CGROUP_NAME
 from azurelinuxagent.common.utils import shellutil
 from azurelinuxagent.common.utils.processutil import read_output
 from tests.tools import *
@@ -166,6 +166,7 @@ rdma	6	1	1
             message = args[0]
             self.assertIn('Error in cgroup controller "cpu": A test exception.', message)
 
+
 class FileSystemCgroupsApiTestCase(AgentTestCase):
 
     def setUp(self):
@@ -188,8 +189,9 @@ class FileSystemCgroupsApiTestCase(AgentTestCase):
         agent_cgroups = FileSystemCgroupsApi().create_agent_cgroups()
 
         def assert_cgroup_created(controller):
-            cgroup_path = os.path.join(self.cgroups_file_system_root, controller, "walinuxagent.service")
+            cgroup_path = os.path.join(self.cgroups_file_system_root, controller, VM_AGENT_CGROUP_NAME)
             self.assertTrue(any(cgroups.path == cgroup_path for cgroups in agent_cgroups))
+            self.assertTrue(any(cgroups.name == VM_AGENT_CGROUP_NAME for cgroups in agent_cgroups))
             self.assertTrue(os.path.exists(cgroup_path))
             cgroup_task = int(fileutil.read_file(os.path.join(cgroup_path, "cgroup.procs")))
             current_process = os.getpid()
@@ -282,6 +284,16 @@ class FileSystemCgroupsApiTestCase(AgentTestCase):
 
 
 class SystemdCgroupsApiTestCase(AgentTestCase):
+    def setUp(self):
+        AgentTestCase.setUp(self)
+
+        self.cgroups_file_system_root = os.path.join(self.tmp_dir, "cgroup")
+        os.mkdir(self.cgroups_file_system_root)
+        os.mkdir(os.path.join(self.cgroups_file_system_root, "cpu"))
+        os.mkdir(os.path.join(self.cgroups_file_system_root, "memory"))
+
+    def tearDown(self):
+        AgentTestCase.tearDown(self)
 
     def test_it_should_return_extensions_slice_root_name(self):
         root_slice_name = SystemdCgroupsApi()._get_extensions_slice_root_name()
@@ -478,3 +490,35 @@ class SystemdCgroupsApiTestCase(AgentTestCase):
                         process.wait()
                         process_output = read_output(stdout, stderr)
                         self.assertEquals("[stdout]\nvery specific test message\n\n\n[stderr]\n", process_output)
+
+    @patch("azurelinuxagent.common.utils.fileutil.read_file")
+    def test_create_agent_cgroups_should_create_cgroups_on_all_controllers(self, patch_read_file):
+        mock__base_cgroups = patch("azurelinuxagent.common.cgroupapi.CGROUPS_FILE_SYSTEM_ROOT",
+                                   self.cgroups_file_system_root)
+        mock__base_cgroups.start()
+        mock_proc_self_cgroup = '''12:blkio:/system.slice/walinuxagent.service
+11:memory:/system.slice/walinuxagent.service
+10:perf_event:/
+9:hugetlb:/
+8:freezer:/
+7:net_cls,net_prio:/
+6:devices:/system.slice/walinuxagent.service
+5:cpuset:/
+4:cpu,cpuacct:/system.slice/walinuxagent.service
+3:pids:/system.slice/walinuxagent.service
+2:rdma:/
+1:name=systemd:/system.slice/walinuxagent.service
+0::/system.slice/walinuxagent.service
+'''
+        patch_read_file.return_value = mock_proc_self_cgroup
+        agent_cgroups = SystemdCgroupsApi().create_agent_cgroups()
+
+        def assert_cgroup_created(controller):
+            expected_cgroup_path = os.path.join(self.cgroups_file_system_root, controller, "system.slice", VM_AGENT_CGROUP_NAME)
+
+            self.assertTrue(any(cgroups.path == expected_cgroup_path for cgroups in agent_cgroups))
+            self.assertTrue(any(cgroups.name == VM_AGENT_CGROUP_NAME for cgroups in agent_cgroups))
+
+        assert_cgroup_created("cpu")
+        assert_cgroup_created("memory")
+        mock__base_cgroups.stop()