Repository: Azure/WALinuxAgent
Pull request #1677 (2019-10-22 22:10:44)

use alternate systemd detection
* Replacing run_get_output with run_command (errors were being ignored, since run_get_output does not throw)
* The new logic to detect systemd uses the same check as systemd itself (e.g. systemd-run).

<!-- 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/1677)
<!-- Reviewable:end -->


narrieta: DCR run OK
codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`develop@93ecdc4`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             develop   #1677   +/-   ##
=========================================
  Coverage           ?   67.3%           
=========================================
  Files              ?      80           
  Lines              ?   11297           
  Branches           ?    1579           
=========================================
  Hits               ?    7603           
  Misses             ?    3364           
  Partials           ?     330
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `79.41% <100%> (ø)` | |
| [azurelinuxagent/common/utils/shellutil.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi91dGlscy9zaGVsbHV0aWwucHk=) | `66.26% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677?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/1677?src=pr&el=footer). Last update [93ecdc4...ee51eb9](https://codecov.io/gh/Azure/WALinuxAgent/pull/1677?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 d497c68c..06bafb1b 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -88,19 +88,10 @@ class CGroupsApi(object):
     @staticmethod
     def _is_systemd():
         """
-        Determine if systemd is managing system services. If this process (presumed to be the agent) is in a CPU cgroup
-        that looks like one created by systemd, we can assume systemd is in use.
-
-        TODO: We need to re-evaluate whether this the right logic to determine if Systemd is managing cgroups.
-
-        :return: True if systemd is managing system services
-        :rtype: Bool
+        Determine if systemd is managing system services; the implementation follows the same strategy as, for example,
+        sd_booted() in libsystemd, or /usr/sbin/service
         """
-        controller_id = CGroupsApi._get_controller_id('cpu')
-        current_process_cgroup_path = CGroupsApi._get_current_process_cgroup_relative_path(controller_id)
-        is_systemd = current_process_cgroup_path == 'system.slice/walinuxagent.service'
-
-        return is_systemd
+        return os.path.exists('/run/systemd/system/')
 
     @staticmethod
     def _get_current_process_cgroup_relative_path(controller_id):
@@ -394,8 +385,8 @@ class SystemdCgroupsApi(CGroupsApi):
         try:
             unit_path = os.path.join(UNIT_FILES_FILE_SYSTEM_PATH, unit_filename)
             fileutil.write_file(unit_path, unit_contents)
-            shellutil.run_get_output("systemctl daemon-reload")
-            shellutil.run_get_output("systemctl start {0}".format(unit_filename))
+            shellutil.run_command(["systemctl", "daemon-reload"])
+            shellutil.run_command(["systemctl", "start", unit_filename])
         except Exception as e:
             raise CGroupsException("Failed to create and start {0}. Error: {1}".format(unit_filename, ustr(e)))
 
@@ -463,9 +454,9 @@ After=system-{1}.slice""".format(extension_name, EXTENSIONS_ROOT_CGROUP_NAME)
         unit_filename = self._get_extension_slice_name(extension_name)
         try:
             unit_path = os.path.join(UNIT_FILES_FILE_SYSTEM_PATH, unit_filename)
-            shellutil.run_get_output("systemctl stop {0}".format(unit_filename))
+            shellutil.run_command(["systemctl", "stop", unit_filename])
             fileutil.rm_files(unit_path)
-            shellutil.run_get_output("systemctl daemon-reload")
+            shellutil.run_command(["systemctl", "daemon-reload"])
         except Exception as e:
             raise CGroupsException("Failed to remove {0}. Error: {1}".format(unit_filename, ustr(e)))
 
diff --git a/azurelinuxagent/common/utils/shellutil.py b/azurelinuxagent/common/utils/shellutil.py
index 96897635..1260f9d9 100644
--- a/azurelinuxagent/common/utils/shellutil.py
+++ b/azurelinuxagent/common/utils/shellutil.py
@@ -89,13 +89,10 @@ def run_get_output(cmd, chk_err=True, log_cmd=True, expected_errors=[]):
         output = subprocess.check_output(cmd,
                                          stderr=subprocess.STDOUT,
                                          shell=True)
-        output = ustr(output,
-                      encoding='utf-8',
-                      errors="backslashreplace")
+        output = _encode_command_output(output)
     except subprocess.CalledProcessError as e:
-        output = ustr(e.output,
-                      encoding='utf-8',
-                      errors="backslashreplace")
+        output = _encode_command_output(e.output)
+
         if chk_err:
             msg = u"Command: [{0}], " \
                   u"return code: [{1}], " \


Test patch:
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index 324104b5..8d892e84 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -55,83 +55,41 @@ class CGroupsApiTestCase(AgentTestCase):
         self.assertTrue(type(api) == FileSystemCgroupsApi)
 
     def test_is_systemd_should_return_true_when_systemd_manages_current_process(self):
-        fileutil_read_file = fileutil.read_file
-
-        def mock_read_file(filepath, asbin=False, remove_bom=False, encoding='utf-8'):
-            if filepath == "/proc/cgroups":
-                return """
-#subsys_name	hierarchy	num_cgroups	enabled
-cpuset	11	1	1
-cpu	3	77	1
-cpuacct	3	77	1
-blkio	10	70	1
-memory	12	124	1
-devices	9	70	1
-freezer	4	1	1
-net_cls	2	1	1
-perf_event	7	1	1
-net_prio	2	1	1
-hugetlb	8	1	1
-pids	5	76	1
-rdma	6	1	1
-"""
-            if filepath == "/proc/self/cgroup":
-                return """
-12:memory:/system.slice/walinuxagent.service
-11:cpuset:/
-10:blkio:/system.slice/walinuxagent.service
-9:devices:/system.slice/walinuxagent.service
-8:hugetlb:/
-7:perf_event:/
-6:rdma:/
-5:pids:/system.slice/walinuxagent.service
-4:freezer:/
-3:cpu,cpuacct:/system.slice/walinuxagent.service
-2:net_cls,net_prio:/
-1:name=systemd:/system.slice/walinuxagent.service
-0::/system.slice/walinuxagent.service
-"""
-            return fileutil_read_file(filepath, asbin=asbin, remove_bom=remove_bom, encoding=encoding)
+        path_exists = os.path.exists
+
+        def mock_path_exists(path):
+            if path == "/run/systemd/system/":
+                mock_path_exists.path_tested = True
+                return True
+            return path_exists(path)
 
-        with patch("azurelinuxagent.common.cgroupapi.fileutil.read_file", mock_read_file):
+        mock_path_exists.path_tested = False
+
+        with patch("azurelinuxagent.common.cgroupapi.os.path.exists", mock_path_exists):
             is_systemd = CGroupsApi._is_systemd()
 
         self.assertTrue(is_systemd)
 
+        self.assertTrue(mock_path_exists.path_tested, 'The expected path was not tested; the implementation of CGroupsApi._is_systemd() may have changed.')
+
     def test_is_systemd_should_return_false_when_systemd_does_not_manage_current_process(self):
-        fileutil_read_file = fileutil.read_file
-
-        def mock_read_file(filepath, asbin=False, remove_bom=False, encoding='utf-8'):
-            if filepath == "/proc/cgroups":
-                return """
-#subsys_name	hierarchy	num_cgroups	enabled
-cpuset	11	1	1
-cpu	3	77	1
-cpuacct	3	77	1
-blkio	10	70	1
-memory	12	124	1
-devices	9	70	1
-freezer	4	1	1
-net_cls	2	1	1
-perf_event	7	1	1
-net_prio	2	1	1
-hugetlb	8	1	1
-pids	5	76	1
-rdma	6	1	1
-"""
-            if filepath == "/proc/self/cgroup":
-                return """
-3:name=systemd:/
-2:memory:/walinuxagent.service
-1:cpu,cpuacct:/walinuxagent.service
-"""
-            return fileutil_read_file(filepath, asbin=asbin, remove_bom=remove_bom, encoding=encoding)
-
-        with patch("azurelinuxagent.common.cgroupapi.fileutil.read_file", mock_read_file):
+        path_exists = os.path.exists
+
+        def mock_path_exists(path):
+            if path == "/run/systemd/system/":
+                mock_path_exists.path_tested = True
+                return False
+            return path_exists(path)
+
+        mock_path_exists.path_tested = False
+
+        with patch("azurelinuxagent.common.cgroupapi.os.path.exists", mock_path_exists):
             is_systemd = CGroupsApi._is_systemd()
 
         self.assertFalse(is_systemd)
 
+        self.assertTrue(mock_path_exists.path_tested, 'The expected path was not tested; the implementation of CGroupsApi._is_systemd() may have changed.')
+
     def test_foreach_controller_should_execute_operation_on_all_mounted_controllers(self):
         executed_controllers = []
 
diff --git a/tests/common/test_cgroupstelemetry.py b/tests/common/test_cgroupstelemetry.py
index 7fb6ba82..34abfb08 100644
--- a/tests/common/test_cgroupstelemetry.py
+++ b/tests/common/test_cgroupstelemetry.py
@@ -579,21 +579,28 @@ class TestCGroupsTelemetry(AgentTestCase):
     def test_telemetry_with_tracked_cgroup(self):
         self.assertTrue(i_am_root(), "Test does not run when non-root")
 
-        max_num_polls = 30
-        time_to_wait = 3
-        extn_name = "foobar-1.0.0"
-        num_summarization_values = 7
-
-        cgs = make_new_cgroup(extn_name)
-        self.assertEqual(len(cgs), 2)
-
-        ext_handler_properties = ExtHandlerProperties()
-        ext_handler_properties.version = "1.0.0"
-        self.ext_handler = ExtHandler(name='foobar')
-        self.ext_handler.properties = ext_handler_properties
-        self.ext_handler_instance = ExtHandlerInstance(ext_handler=self.ext_handler, protocol=None)
-
-        command = self.create_script("keep_cpu_busy_and_consume_memory_for_5_seconds", '''
+        # This test has some timing issues when systemd is managing cgroups, so we force the file system API
+        # by creating a new instance of the CGroupConfigurator
+        with patch("azurelinuxagent.common.cgroupapi.CGroupsApi._is_systemd", return_value=False):
+            cgroup_configurator_instance = CGroupConfigurator._instance
+            CGroupConfigurator._instance = None
+
+            try:
+                max_num_polls = 30
+                time_to_wait = 3
+                extn_name = "foobar-1.0.0"
+                num_summarization_values = 7
+
+                cgs = make_new_cgroup(extn_name)
+                self.assertEqual(len(cgs), 2)
+
+                ext_handler_properties = ExtHandlerProperties()
+                ext_handler_properties.version = "1.0.0"
+                self.ext_handler = ExtHandler(name='foobar')
+                self.ext_handler.properties = ext_handler_properties
+                self.ext_handler_instance = ExtHandlerInstance(ext_handler=self.ext_handler, protocol=None)
+
+                command = self.create_script("keep_cpu_busy_and_consume_memory_for_5_seconds", '''
 nohup python -c "import time
 
 for i in range(5):
@@ -603,48 +610,59 @@ for i in range(5):
     print('Test loop')" &
 '''.format(time_to_wait))
 
-        self.log_dir = os.path.join(self.tmp_dir, "log")
-
-        with patch("azurelinuxagent.ga.exthandlers.ExtHandlerInstance.get_base_dir", lambda *_: self.tmp_dir) as \
-                patch_get_base_dir:
-            with patch("azurelinuxagent.ga.exthandlers.ExtHandlerInstance.get_log_dir", lambda *_: self.log_dir) as \
-                    patch_get_log_dir:
-                self.ext_handler_instance.launch_command(command)
-
-        self.assertTrue(CGroupsTelemetry.is_tracked(os.path.join(
-            BASE_CGROUPS, "cpu", "walinuxagent.extensions", "foobar_1.0.0")))
-        self.assertTrue(CGroupsTelemetry.is_tracked(os.path.join(
-            BASE_CGROUPS, "memory", "walinuxagent.extensions", "foobar_1.0.0")))
-
-        for i in range(max_num_polls):
-            CGroupsTelemetry.poll_all_tracked()
-            time.sleep(0.5)
+                self.log_dir = os.path.join(self.tmp_dir, "log")
+
+                with patch("azurelinuxagent.ga.exthandlers.ExtHandlerInstance.get_base_dir", lambda *_: self.tmp_dir) as \
+                        patch_get_base_dir:
+                    with patch("azurelinuxagent.ga.exthandlers.ExtHandlerInstance.get_log_dir", lambda *_: self.log_dir) as \
+                            patch_get_log_dir:
+                        self.ext_handler_instance.launch_command(command)
+
+                #
+                # If the test is made to run using the systemd API, then the paths of the cgroups need to be checked differently:
+                #
+                #     self.assertEquals(len(CGroupsTelemetry._tracked), 2)
+                #     cpu = os.path.join(BASE_CGROUPS, "cpu", "system.slice", r"foobar_1.0.0_.*\.scope")
+                #     self.assertTrue(any(re.match(cpu, tracked.path) for tracked in CGroupsTelemetry._tracked))
+                #     memory = os.path.join(BASE_CGROUPS, "memory", "system.slice", r"foobar_1.0.0_.*\.scope")
+                #     self.assertTrue(any(re.match(memory, tracked.path) for tracked in CGroupsTelemetry._tracked))
+                #
+                self.assertTrue(CGroupsTelemetry.is_tracked(os.path.join(
+                    BASE_CGROUPS, "cpu", "walinuxagent.extensions", "foobar_1.0.0")))
+                self.assertTrue(CGroupsTelemetry.is_tracked(os.path.join(
+                    BASE_CGROUPS, "memory", "walinuxagent.extensions", "foobar_1.0.0")))
+
+                for i in range(max_num_polls):
+                    CGroupsTelemetry.poll_all_tracked()
+                    time.sleep(0.5)
 
-        collected_metrics = CGroupsTelemetry.report_all_tracked()
+                collected_metrics = CGroupsTelemetry.report_all_tracked()
 
-        self.assertIn("memory", collected_metrics[extn_name])
-        self.assertIn("cur_mem", collected_metrics[extn_name]["memory"])
-        self.assertIn("max_mem", collected_metrics[extn_name]["memory"])
-        self.assertEqual(len(collected_metrics[extn_name]["memory"]["cur_mem"]), num_summarization_values)
-        self.assertEqual(len(collected_metrics[extn_name]["memory"]["max_mem"]), num_summarization_values)
-
-        self.assertIsInstance(collected_metrics[extn_name]["memory"]["cur_mem"][5], str)
-        self.assertIsInstance(collected_metrics[extn_name]["memory"]["cur_mem"][6], str)
-        self.assertIsInstance(collected_metrics[extn_name]["memory"]["max_mem"][5], str)
-        self.assertIsInstance(collected_metrics[extn_name]["memory"]["max_mem"][6], str)
-
-        self.assertIn("cpu", collected_metrics[extn_name])
-        self.assertIn("cur_cpu", collected_metrics[extn_name]["cpu"])
-        self.assertEqual(len(collected_metrics[extn_name]["cpu"]["cur_cpu"]), num_summarization_values)
-
-        self.assertIsInstance(collected_metrics[extn_name]["cpu"]["cur_cpu"][5], str)
-        self.assertIsInstance(collected_metrics[extn_name]["cpu"]["cur_cpu"][6], str)
-
-        for i in range(5):
-            self.assertGreater(collected_metrics[extn_name]["memory"]["cur_mem"][i], 0)
-            self.assertGreater(collected_metrics[extn_name]["memory"]["max_mem"][i], 0)
-            self.assertGreaterEqual(collected_metrics[extn_name]["cpu"]["cur_cpu"][i], 0)
-            # Equal because CPU could be zero for minimum value.
+                self.assertIn("memory", collected_metrics[extn_name])
+                self.assertIn("cur_mem", collected_metrics[extn_name]["memory"])
+                self.assertIn("max_mem", collected_metrics[extn_name]["memory"])
+                self.assertEqual(len(collected_metrics[extn_name]["memory"]["cur_mem"]), num_summarization_values)
+                self.assertEqual(len(collected_metrics[extn_name]["memory"]["max_mem"]), num_summarization_values)
+
+                self.assertIsInstance(collected_metrics[extn_name]["memory"]["cur_mem"][5], str)
+                self.assertIsInstance(collected_metrics[extn_name]["memory"]["cur_mem"][6], str)
+                self.assertIsInstance(collected_metrics[extn_name]["memory"]["max_mem"][5], str)
+                self.assertIsInstance(collected_metrics[extn_name]["memory"]["max_mem"][6], str)
+
+                self.assertIn("cpu", collected_metrics[extn_name])
+                self.assertIn("cur_cpu", collected_metrics[extn_name]["cpu"])
+                self.assertEqual(len(collected_metrics[extn_name]["cpu"]["cur_cpu"]), num_summarization_values)
+
+                self.assertIsInstance(collected_metrics[extn_name]["cpu"]["cur_cpu"][5], str)
+                self.assertIsInstance(collected_metrics[extn_name]["cpu"]["cur_cpu"][6], str)
+
+                for i in range(5):
+                    self.assertGreater(collected_metrics[extn_name]["memory"]["cur_mem"][i], 0)
+                    self.assertGreater(collected_metrics[extn_name]["memory"]["max_mem"][i], 0)
+                    self.assertGreaterEqual(collected_metrics[extn_name]["cpu"]["cur_cpu"][i], 0)
+                    # Equal because CPU could be zero for minimum value.
+            finally:
+                CGroupConfigurator._instance = cgroup_configurator_instance
 
 
 class TestMetric(AgentTestCase):