Repository: Azure/WALinuxAgent
Pull request #1910 (2020-06-17 20:38:45)

Do not use --unit with systemd-cgls
systemd-cgls doesn't support --unit on ubuntu 16; using the cgroup path instead.

also, improved error handling and reporting.

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910?src=pr&el=h1) Report
> Merging [#1910](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/29f5b67318603eeb5a2566301f61a7479dc0377b&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `86.48%`.

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

```diff
@@             Coverage Diff             @@
##           develop    #1910      +/-   ##
===========================================
- Coverage    69.49%   69.48%   -0.02%     
===========================================
  Files           85       85              
  Lines        11865    11872       +7     
  Branches      1666     1667       +1     
===========================================
+ Hits          8246     8249       +3     
- Misses        3249     3252       +3     
- Partials       370      371       +1     
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupconfigurator.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBjb25maWd1cmF0b3IucHk=) | `73.20% <80.00%> (-0.80%)` | :arrow_down: |
| [azurelinuxagent/ga/monitor.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL21vbml0b3IucHk=) | `77.45% <84.21%> (-0.06%)` | :arrow_down: |
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `79.55% <100.00%> (-0.25%)` | :arrow_down: |
| [azurelinuxagent/common/utils/shellutil.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi91dGlscy9zaGVsbHV0aWwucHk=) | `67.44% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910?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/1910?src=pr&el=footer). Last update [29f5b67...f15562d](https://codecov.io/gh/Azure/WALinuxAgent/pull/1910?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 41d8e3fc..9b106525 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -628,19 +628,19 @@ After=system-{1}.slice""".format(extension_name, EXTENSIONS_ROOT_CGROUP_NAME)
         return self._agent_unit_name
 
     @staticmethod
-    def get_processes_in_cgroup(unit_name):
+    def get_processes_in_cgroup(cgroup_path):
         """
         Returns an array of tuples with the PID and command line of the processes that are currently
-        within the cgroup for the given unit.
+        within the cgroup for the given path (which must be within the cgroup filesystem).
         """
         #
         # The output of the command is similar to
         #
-        #     Unit walinuxagent.service (/system.slice/walinuxagent.service):
+        #     Directory /sys/fs/cgroup/cpu/system.slice/walinuxagent.service:
         #     ├─27519 /usr/bin/python3 -u /usr/sbin/waagent -daemon
         #     └─27547 python3 -u bin/WALinuxAgent-2.2.48.1-py2.7.egg -run-exthandlers
         #
-        output = shellutil.run_command(['systemd-cgls', '--unit', unit_name])
+        output = shellutil.run_command(['systemd-cgls', cgroup_path])
 
         processes = []
 
diff --git a/azurelinuxagent/common/cgroupconfigurator.py b/azurelinuxagent/common/cgroupconfigurator.py
index 42ba7f3c..ac45d3ce 100644
--- a/azurelinuxagent/common/cgroupconfigurator.py
+++ b/azurelinuxagent/common/cgroupconfigurator.py
@@ -41,6 +41,8 @@ class CGroupConfigurator(object):
             self._cgroups_supported = False
             self._cgroups_enabled = False
             self._cgroups_api = None
+            self._agent_cpu_cgroup_path = None
+            self._agent_memory_cgroup_path = None
             self._get_processes_in_agent_cgroup_last_error = None
             self._get_processes_in_agent_cgroup_error_count = 0
 
@@ -137,16 +139,16 @@ class CGroupConfigurator(object):
                 if cpu_controller_root is None or cpu_cgroup_relative_path is None:
                     logger.info("Will not track CPU for the agent's cgroup")
                 else:
-                    cpu_cgroup_path = os.path.join(cpu_controller_root, cpu_cgroup_relative_path)
-                    CGroupsTelemetry.track_cgroup(CpuCgroup(agent_unit_name, cpu_cgroup_path))
+                    self._agent_cpu_cgroup_path = os.path.join(cpu_controller_root, cpu_cgroup_relative_path)
+                    CGroupsTelemetry.track_cgroup(CpuCgroup(agent_unit_name, self._agent_cpu_cgroup_path))
 
                 if memory_controller_root is None or memory_cgroup_relative_path is None:
                     logger.info("Will not track memory for the agent's cgroup")
                 else:
-                    memory_cgroup_path = os.path.join(memory_controller_root, memory_cgroup_relative_path)
-                    CGroupsTelemetry.track_cgroup(MemoryCgroup(agent_unit_name, memory_cgroup_path))
+                    self._agent_memory_cgroup_path = os.path.join(memory_controller_root, memory_cgroup_relative_path)
+                    CGroupsTelemetry.track_cgroup(MemoryCgroup(agent_unit_name, self._agent_memory_cgroup_path))
 
-                log_cgroup_info("Agent cgroups: CPU: {0} -- MEMORY: {1}", cpu_cgroup_path, memory_cgroup_path)
+                log_cgroup_info("Agent cgroups: CPU: {0} -- MEMORY: {1}", self._agent_cpu_cgroup_path, self._agent_memory_cgroup_path)
 
             except Exception as e:
                 message = "Error initializing cgroups: {0}".format(ustr(e))
@@ -223,8 +225,9 @@ class CGroupConfigurator(object):
             The return value can be None if cgroups are not enabled or if an error occurs during the operation.
             """
             def __impl():
-                agent_unit = self._cgroups_api.get_agent_unit_name()
-                return self._cgroups_api.get_processes_in_cgroup(agent_unit)
+                if self._agent_cpu_cgroup_path is None:
+                    return []
+                return self._cgroups_api.get_processes_in_cgroup(self._agent_cpu_cgroup_path)
 
             def __on_error(exception):
                 #
@@ -302,7 +305,7 @@ class CGroupConfigurator(object):
             r".*waagent -daemon.*",
             r".*(WALinuxAgent-.+\.egg|waagent) -run-exthandlers",
             # The processes in the agent's cgroup are listed using systemd-cgls
-            r"^systemd-cgls --unit walinuxagent.service$",
+            r"^systemd-cgls.*walinuxagent.*$",
             # Extensions are started using systemd-run
             r"^systemd-run --unit=.+ --scope ",
             #
diff --git a/azurelinuxagent/common/utils/shellutil.py b/azurelinuxagent/common/utils/shellutil.py
index 9fc89b00..66a79396 100644
--- a/azurelinuxagent/common/utils/shellutil.py
+++ b/azurelinuxagent/common/utils/shellutil.py
@@ -120,14 +120,14 @@ class CommandError(Exception):
     Exception raised by run_command when the command returns an error
     """
     @staticmethod
-    def _get_message(command, returncode):
+    def _get_message(command, return_code, stderr):
         command_name = command[0] if isinstance(command, list) and len(command) > 0 else command
-        return "'{0}' failed: {1}".format(command_name, returncode)
+        return "'{0}' failed: {1} ({2})".format(command_name, return_code, stderr.rstrip())
 
-    def __init__(self, command, returncode, stdout, stderr):
-        super(Exception, self).__init__(CommandError._get_message(command, returncode))
+    def __init__(self, command, return_code, stdout, stderr):
+        super(Exception, self).__init__(CommandError._get_message(command, return_code, stderr))
         self.command = command
-        self.returncode = returncode
+        self.returncode = return_code
         self.stdout = stdout
         self.stderr = stderr
 
@@ -169,7 +169,7 @@ def run_command(command, log_error=False, cmd_input=None):
                 returncode,
                 encoded_stdout,
                 encoded_stderr)
-        raise CommandError(command=command, returncode=returncode, stdout=encoded_stdout, stderr=encoded_stderr)
+        raise CommandError(command=command, return_code=returncode, stdout=encoded_stdout, stderr=encoded_stderr)
 
     return _encode_command_output(stdout)
 
diff --git a/azurelinuxagent/ga/monitor.py b/azurelinuxagent/ga/monitor.py
index d4c31fa1..42e7c118 100644
--- a/azurelinuxagent/ga/monitor.py
+++ b/azurelinuxagent/ga/monitor.py
@@ -58,25 +58,29 @@ class PollResourceUsageOperation(PeriodicOperation):
         #
         # Check the processes in the agent cgroup
         #
-        processes = CGroupConfigurator.get_instance().get_processes_in_agent_cgroup()
-
-        if processes is not None:
-            unexpected_processes = []
-
-            for (_, command_line) in processes:
-                if not CGroupConfigurator.is_agent_process(command_line):
-                    unexpected_processes.append(command_line)
-
-            # Report a small sample of unexpected processes
-            if len(unexpected_processes) > 0 and self._error_count < 5:
-                unexpected_processes.sort()
-                error = ustr(unexpected_processes)
-                if error != self._last_error:
-                    self._error_count += 1
-                    self._last_error = error
-                    message = "The agent's cgroup includes unexpected processes: {0}".format(error)
-                    logger.info(message)
-                    add_event(op=WALAEventOperation.CGroupsDebug, message=message)
+        processes_check_error = None
+        try:
+            processes = CGroupConfigurator.get_instance().get_processes_in_agent_cgroup()
+
+            if processes is not None:
+                unexpected_processes = []
+
+                for (_, command_line) in processes:
+                    if not CGroupConfigurator.is_agent_process(command_line):
+                        unexpected_processes.append(command_line)
+
+                if len(unexpected_processes) > 0:
+                    unexpected_processes.sort()
+                    processes_check_error = "The agent's cgroup includes unexpected processes: {0}".format(ustr(unexpected_processes))
+        except Exception as e:
+            processes_check_error = "Failed to check the processes in the agent's cgroup: {0}".format(ustr(e))
+
+        # Report a small sample of errors
+        if processes_check_error != self._last_error and self._error_count < 5:
+            self._error_count += 1
+            self._last_error = processes_check_error
+            logger.info(processes_check_error)
+            add_event(op=WALAEventOperation.CGroupsDebug, message=processes_check_error)
 
         #
         # Report metrics


Test patch:
diff --git a/tests/common/mock_cgroup_commands.py b/tests/common/mock_cgroup_commands.py
index 223ab0f2..1267f91d 100644
--- a/tests/common/mock_cgroup_commands.py
+++ b/tests/common/mock_cgroup_commands.py
@@ -67,9 +67,9 @@ Running scope as unit: TEST_UNIT.scope
 Thu 28 May 2020 07:25:55 AM PDT
 '''),
 
-    (r"^systemd-cgls --unit walinuxagent.service$",
+    (r"^systemd-cgls.+/walinuxagent.service$",
 '''
-Unit walinuxagent.service (/system.slice/walinuxagent.service):
+Directory /sys/fs/cgroup/cpu/system.slice/walinuxagent.service:
 ├─27519 /usr/bin/python3 -u /usr/sbin/waagent -daemon
 └─27547 python3 -u bin/WALinuxAgent-2.2.48.1-py2.7.egg -run-exthandlers
 '''),
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index 00a8ad7f..7749d0d2 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -526,7 +526,7 @@ class SystemdCgroupsApiTestCase(AgentTestCase):
 
     def test_get_processes_in_cgroup_should_return_the_processes_within_the_cgroup(self):
         with mock_cgroup_commands():
-            processes = SystemdCgroupsApi.get_processes_in_cgroup("walinuxagent.service")
+            processes = SystemdCgroupsApi.get_processes_in_cgroup("/sys/fs/cgroup/cpu/system.slice/walinuxagent.service")
 
             self.assertTrue(len(processes) >= 2,
                 "The cgroup should contain at least 2 procceses (daemon and extension handler): [{0}]".format(processes))
diff --git a/tests/ga/test_monitor.py b/tests/ga/test_monitor.py
index e349e2f1..6482d61f 100644
--- a/tests/ga/test_monitor.py
+++ b/tests/ga/test_monitor.py
@@ -519,11 +519,12 @@ class PollResourceUsageOperationTestCase(AgentTestCase):
 
     def test_it_should_report_processes_that_do_not_belong_to_the_agent_cgroup(self):
         with mock_cgroup_commands() as mock_commands:
-            mock_commands.add_command(r'^systemd-cgls --unit walinuxagent.service$',
+            mock_commands.add_command(r'^systemd-cgls.+/walinuxagent.service$',
 '''
-Unit walinuxagent.service (/system.slice/walinuxagent.service):
+Directory /sys/fs/cgroup/cpu/system.slice/walinuxagent.service:
 ├─27519 /usr/bin/python3 -u /usr/sbin/waagent -daemon
 ├─27547 python3 -u bin/WALinuxAgent-2.2.48.1-py2.7.egg -run-exthandlers
+├─6200 systemd-cgls /sys/fs/cgroup/cpu,cpuacct/system.slice/walinuxagent.service
 ├─5821 pidof systemd-networkd
 ├─5822 iptables --version
 ├─5823 iptables -w -t security -D OUTPUT -d 168.63.129.16 -p tcp -m conntrack --ctstate INVALID,NEW -j ACCEPT
diff --git a/tests/utils/test_shell_util.py b/tests/utils/test_shell_util.py
index 27cd0933..3565a7ee 100644
--- a/tests/utils/test_shell_util.py
+++ b/tests/utils/test_shell_util.py
@@ -15,7 +15,6 @@
 #
 # Requires Python 2.6+ and Openssl 1.0+
 #
-
 from tests.tools import AgentTestCase, patch
 import unittest
 import azurelinuxagent.common.utils.shellutil as shellutil
@@ -140,7 +139,8 @@ class RunCommandTestCase(AgentTestCase):
             shellutil.run_command(command)
 
         exception = context_manager.exception
-        self.assertEquals(str(exception), "'ls' failed: 2")
+        self.assertIn("'ls' failed: 2", str(exception))
+        self.assertIn("No such file or directory", str(exception))
         self.assertEquals(exception.stdout, "/etc\n")
         self.assertIn("No such file or directory", exception.stderr)
         self.assertEquals(exception.returncode, 2)