Repository: Azure/WALinuxAgent
Pull request #1570 (2019-06-28 18:43:14)

Handle errors in individual controllers
Added try/except to _for_each_controller
<!--
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
- [ ] The title of the PR is clear and informative.
- [ ] 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).
- [ ] 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
- [ ] 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/1570)
<!-- Reviewable:end -->


codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=h1) Report
> Merging [#1570](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/b581fdc2b71a24fc03ff5fd869973084336c6f42?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `81.25%`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570/graphs/tree.svg?width=650&token=ae90E2KYCw&height=150&src=pr)](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           develop    #1570   +/-   ##
========================================
  Coverage    63.37%   63.37%           
========================================
  Files           78       78           
  Lines        11058    11058           
  Branches      1559     1559           
========================================
  Hits          7008     7008           
  Misses        3711     3711           
  Partials       339      339
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `62.5% <81.25%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=footer). Last update [b581fdc...d766842](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=h1) Report
> Merging [#1570](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/b581fdc2b71a24fc03ff5fd869973084336c6f42?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `81.25%`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570/graphs/tree.svg?width=650&token=ae90E2KYCw&height=150&src=pr)](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           develop    #1570   +/-   ##
========================================
  Coverage    63.37%   63.37%           
========================================
  Files           78       78           
  Lines        11058    11058           
  Branches      1559     1559           
========================================
  Hits          7008     7008           
  Misses        3711     3711           
  Partials       339      339
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `62.5% <81.25%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=footer). Last update [b581fdc...d766842](https://codecov.io/gh/Azure/WALinuxAgent/pull/1570?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/azurelinuxagent/common/cgroupapi.py b/azurelinuxagent/common/cgroupapi.py
index da1baa4e..6b6d06f5 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -117,15 +117,19 @@ class CGroupsApi(object):
     @staticmethod
     def _foreach_controller(operation, message):
         """
-        Executes the given operation on all controllers that need to be tracked; outputs 'message' if the controller is not mounted.
+        Executes the given operation on all controllers that need to be tracked; outputs 'message' if the controller
+        is not mounted or if an error occurs in the operation
         """
         mounted_controllers = os.listdir(CGROUPS_FILE_SYSTEM_ROOT)
 
         for controller in CGROUP_CONTROLLERS:
-            if controller not in mounted_controllers:
-                logger.warn('Controller "{0}" is not mounted. {1}'.format(controller, message))
-            else:
-                operation(controller)
+            try:
+                if controller not in mounted_controllers:
+                    logger.warn('Cgroup controller "{0}" is not mounted. {1}'.format(controller, message))
+                else:
+                    operation(controller)
+            except Exception as e:
+                logger.warn('Error in cgroup controller "{0}": {1}. {2}'.format(controller, ustr(e), message))
 
 
 class FileSystemCgroupsApi(CGroupsApi):
@@ -188,21 +192,17 @@ class FileSystemCgroupsApi(CGroupsApi):
         pid = int(os.getpid())
 
         def create_cgroup(controller):
-            try:
-                path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, controller, VM_AGENT_CGROUP_NAME)
+            path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, controller, VM_AGENT_CGROUP_NAME)
 
-                if not os.path.isdir(path):
-                    FileSystemCgroupsApi._try_mkdir(path)
-                    logger.info("Created cgroup {0}".format(path))
-
-                self._add_process_to_cgroup(pid, path)
+            if not os.path.isdir(path):
+                FileSystemCgroupsApi._try_mkdir(path)
+                logger.info("Created cgroup {0}".format(path))
 
-                cgroups.append(CGroup.create(path, controller, VM_AGENT_CGROUP_NAME))
+            self._add_process_to_cgroup(pid, path)
 
-            except Exception as e:
-                logger.warn('Cannot create "{0}" cgroup for the agent. Error: {1}'.format(controller, ustr(e)))
+            cgroups.append(CGroup.create(path, controller, VM_AGENT_CGROUP_NAME))
 
-        self._foreach_controller(create_cgroup, 'Will not create a cgroup for the VM Agent')
+        self._foreach_controller(create_cgroup, 'Failed to create a cgroup for the VM Agent; resource usage will not be tracked')
 
         if len(cgroups) == 0:
             raise CGroupsException("Failed to create any cgroup for the VM Agent")
@@ -220,7 +220,7 @@ class FileSystemCgroupsApi(CGroupsApi):
                 FileSystemCgroupsApi._try_mkdir(path)
                 logger.info("Created {0}".format(path))
 
-        self._foreach_controller(create_cgroup, 'Will not create a root cgroup for extensions')
+        self._foreach_controller(create_cgroup, 'Failed to create a root cgroup for extensions')
 
     def create_extension_cgroups(self, extension_name):
         """
@@ -237,7 +237,7 @@ class FileSystemCgroupsApi(CGroupsApi):
 
             cgroups.append(cgroup)
 
-        self._foreach_controller(create_cgroup, 'Will not create a cgroup for extension {0}'.format(extension_name))
+        self._foreach_controller(create_cgroup, 'Failed to create a cgroup for extension {0}'.format(extension_name))
 
         return cgroups
 
@@ -470,8 +470,6 @@ After=system-{1}.slice""".format(extension_name, EXTENSIONS_ROOT_CGROUP_NAME)
             cgroup_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, controller, 'system.slice', scope_name + ".scope")
             cgroups.append(CGroup.create(cgroup_path, controller, extension_name))
 
-        self._foreach_controller(create_cgroup,
-                                 'Cannot create cgroup for extension {0}; resource usage will not be tracked.'.format(
-                                     extension_name))
+        self._foreach_controller(create_cgroup, 'Cannot create cgroup for extension {0}; resource usage will not be tracked.'.format(extension_name))
 
         return process, cgroups


Test patch:
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index 817add93..597ac6f2 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -30,6 +30,19 @@ def i_am_root():
 
 
 class CGroupsApiTestCase(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"))
+
+        self.mock__base_cgroups = patch("azurelinuxagent.common.cgroupapi.CGROUPS_FILE_SYSTEM_ROOT", self.cgroups_file_system_root)
+        self.mock__base_cgroups.start()
+
+    def tearDown(self):
+        self.mock__base_cgroups.stop()
 
     def test_create_should_return_a_SystemdCgroupsApi_on_systemd_platforms(self):
         with patch("azurelinuxagent.common.cgroupapi.CGroupsApi._is_systemd", return_value=True):
@@ -121,6 +134,37 @@ rdma	6	1	1
 
         self.assertFalse(is_systemd)
 
+    def test_foreach_controller_should_execute_operation_on_all_mounted_controllers(self):
+        executed_controllers = []
+
+        def controller_operation(controller):
+            executed_controllers.append(controller)
+
+        CGroupsApi._foreach_controller(controller_operation, 'A dummy message')
+
+        # The setUp method mocks azurelinuxagent.common.cgroupapi.CGROUPS_FILE_SYSTEM_ROOT to have the cpu and memory controllers mounted
+        self.assertIn('cpu', executed_controllers, 'The operation was not executed on the cpu controller')
+        self.assertIn('memory', executed_controllers, 'The operation was not executed on the memory controller')
+        self.assertEqual(len(executed_controllers), 2, 'The operation was not executed on unexpected controllers: {0}'.format(executed_controllers))
+
+    def test_foreach_controller_should_handle_errors_in_individual_controllers(self):
+        successful_controllers = []
+
+        def controller_operation(controller):
+            if controller == 'cpu':
+                raise Exception('A test exception')
+
+            successful_controllers.append(controller)
+
+        with patch("azurelinuxagent.common.cgroupapi.logger.warn") as mock_logger_warn:
+            CGroupsApi._foreach_controller(controller_operation, 'A dummy message')
+
+            self.assertIn('memory', successful_controllers, 'The operation was not executed on the memory controller')
+            self.assertEqual(len(successful_controllers), 1, 'The operation was not executed on unexpected controllers: {0}'.format(successful_controllers))
+
+            args, kwargs = mock_logger_warn.call_args
+            message = args[0]
+            self.assertIn('Error in cgroup controller "cpu": A test exception.', message)
 
 class FileSystemCgroupsApiTestCase(AgentTestCase):