Repository: Azure/WALinuxAgent
Pull request #1536 (2019-05-21 23:22:43)

Implement API for systemd-managed cgroups
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
Implement methods that take care of the following for environments using systemd to manage services and cgroups:
1) create a top-level extensions slice called `system-walinuxagent.extensions.slice`
2) create an extension-specific slice during extension handler's initialization called `system-walinuxagent.extensions-{extension_name}.slice`
3) run extension commands within their own scopes called `{extension_name}.scope`
4) clean up extension-specific slices during uninstallation

Also added unit tests for slice creation and updated the method of checking if systemd is managing cgroups or not.

<!--
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/1536)
<!-- Reviewable:end -->


codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=h1) Report
> Merging [#1536](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=desc) into [cgroups](https://codecov.io/gh/Azure/WALinuxAgent/commit/8e537f526872e03e796863f9591a12cac3ec7a80?src=pr&el=desc) will **decrease** coverage by `0.21%`.
> The diff coverage is `13.55%`.

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

```diff
@@            Coverage Diff             @@
##           cgroups   #1536      +/-   ##
==========================================
- Coverage    60.81%   60.6%   -0.22%     
==========================================
  Files           78      78              
  Lines        10992   11040      +48     
  Branches      1556    1557       +1     
==========================================
+ Hits          6685    6691       +6     
- Misses        3993    4035      +42     
  Partials       314     314
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `25.43% <12.96%> (-4.73%)` | :arrow_down: |
| [azurelinuxagent/common/cgroupconfigurator.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBjb25maWd1cmF0b3IucHk=) | `35.63% <20%> (-0.14%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?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/1536?src=pr&el=footer). Last update [8e537f5...a9442e7](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?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/1536?src=pr&el=h1) Report
> Merging [#1536](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=desc) into [cgroups](https://codecov.io/gh/Azure/WALinuxAgent/commit/8e537f526872e03e796863f9591a12cac3ec7a80?src=pr&el=desc) will **decrease** coverage by `0.21%`.
> The diff coverage is `13.55%`.

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

```diff
@@            Coverage Diff             @@
##           cgroups   #1536      +/-   ##
==========================================
- Coverage    60.81%   60.6%   -0.22%     
==========================================
  Files           78      78              
  Lines        10992   11040      +48     
  Branches      1556    1557       +1     
==========================================
+ Hits          6685    6691       +6     
- Misses        3993    4035      +42     
  Partials       314     314
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupapi.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBhcGkucHk=) | `25.43% <12.96%> (-4.73%)` | :arrow_down: |
| [azurelinuxagent/common/cgroupconfigurator.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBjb25maWd1cmF0b3IucHk=) | `35.63% <20%> (-0.14%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?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/1536?src=pr&el=footer). Last update [8e537f5...a9442e7](https://codecov.io/gh/Azure/WALinuxAgent/pull/1536?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

narrieta: @pgombar Thanks for this. Added a few comments.

Patch:
diff --git a/azurelinuxagent/common/cgroupapi.py b/azurelinuxagent/common/cgroupapi.py
index 9f27d2e1..4843ad6d 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -23,12 +23,14 @@ from azurelinuxagent.common import logger
 from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.osutil import get_osutil
-from azurelinuxagent.common.utils import fileutil
+from azurelinuxagent.common.utils import fileutil, shellutil
 
 CGROUPS_FILE_SYSTEM_ROOT = '/sys/fs/cgroup'
 CGROUP_CONTROLLERS = ["cpu", "memory"]
 VM_AGENT_CGROUP_NAME = "walinuxagent.service"
 EXTENSIONS_ROOT_CGROUP_NAME = "walinuxagent.extensions"
+UNIT_FILES_FILE_SYSTEM_PATH = "/etc/systemd/system"
+
 
 class CGroupsApi(object):
     """
@@ -46,8 +48,16 @@ class CGroupsApi(object):
     def remove_extension_cgroups(self, extension_name):
         raise NotImplementedError()
 
+    def get_extension_cgroups(self, extension_name):
+        raise NotImplementedError()
+
     def start_extension_command(self, extension_name, command, cwd, env, stdout, stderr):
-        pass
+        raise NotImplementedError()
+
+    @staticmethod
+    def _get_extension_scope_name(extension_name):
+        # Since '-' is used as a separator in systemd unit names, we replace it with '_' to prevent side-effects.
+        return extension_name.replace('-', '_')
 
 
 class FileSystemCgroupsApi(CGroupsApi):
@@ -102,8 +112,7 @@ class FileSystemCgroupsApi(CGroupsApi):
         if not os.path.exists(extensions_root):
             logger.warn("Root directory {0} does not exist.".format(extensions_root))
 
-        # '-' has a special meaning within systemd unit names; for consistency we also replace with '_' here
-        cgroup_name = extension_name.replace('-', '_')
+        cgroup_name = self._get_extension_scope_name(extension_name)
 
         return os.path.join(extensions_root, cgroup_name)
 
@@ -241,17 +250,104 @@ class SystemdCgroupsApi(CGroupsApi):
     """
     Cgroups interface via systemd
     """
+
+    @staticmethod
+    def create_and_start_unit(unit_filename, unit_contents):
+        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))
+        except Exception as e:
+            raise CGroupsException("Failed to create and start {0}. Error: {1}".format(unit_filename, ustr(e)))
+
+    @staticmethod
+    def _get_extensions_slice_root_name():
+        return "system-{0}.slice".format(EXTENSIONS_ROOT_CGROUP_NAME)
+
+    def _get_extension_slice_name(self, extension_name):
+        return "system-{0}-{1}.slice".format(EXTENSIONS_ROOT_CGROUP_NAME, self._get_extension_scope_name(extension_name))
+
     def create_agent_cgroups(self):
-        pass
+        try:
+            cgroup_unit = None
+            cgroup_paths = fileutil.read_file("/proc/self/cgroup")
+            for entry in cgroup_paths.splitlines():
+                fields = entry.split(':')
+                if fields[1] == "name=systemd":
+                    cgroup_unit = fields[2].lstrip(os.path.sep)
+
+            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 [cpu_cgroup_path, memory_cgroup_path]
+        except Exception as e:
+            raise CGroupsException("Failed to get paths of agent's cgroups. Error: {0}".format(ustr(e)))
 
     def create_extension_cgroups_root(self):
-        pass
+        unit_contents = """
+[Unit]
+Description=Slice for walinuxagent extensions
+DefaultDependencies=no
+Before=slices.target
+Requires=system.slice
+After=system.slice"""
+        unit_filename = self._get_extensions_slice_root_name()
+        self.create_and_start_unit(unit_filename, unit_contents)
+        logger.info("Created slice for walinuxagent extensions {0}".format(unit_filename))
 
     def create_extension_cgroups(self, extension_name):
-        pass
+        # TODO: revisit if we need this code now (not used until we interact with the D-bus API and run the
+        #  extension scopes within their designated slice)
+        unit_contents = """
+[Unit]
+Description=Slice for extension {0}
+DefaultDependencies=no
+Before=slices.target
+Requires=system-{1}.slice
+After=system-{1}.slice""".format(extension_name, EXTENSIONS_ROOT_CGROUP_NAME)
+        unit_filename = self._get_extension_slice_name(extension_name)
+        self.create_and_start_unit(unit_filename, unit_contents)
+        logger.info("Created slice for {0}".format(unit_filename))
+
+        # Reuse the same method for getting cgroup paths since we don't need to create them (systemd will)
+        return self.get_extension_cgroups(extension_name)
 
     def remove_extension_cgroups(self, extension_name):
-        pass
+        # For transient units, cgroups are released automatically when the unit stops, so it is sufficient
+        # to call stop on them. Persistent cgroups are released when the unit is disabled and its configuration
+        # file is deleted.
+        # The assumption is that this method is called after the extension has been uninstalled. For now, since
+        # we're running extensions within transient scopes which clean up after they finish running, no removal
+        # of units is needed. In the future, when the extension is running under its own slice,
+        # the following clean up is needed.
+        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))
+            fileutil.rm_files(unit_path)
+            shellutil.run_get_output("systemctl daemon-reload")
+        except Exception as e:
+            raise CGroupsException("Failed to remove {0}. Error: {1}".format(unit_filename, ustr(e)))
+
+    def get_extension_cgroups(self, extension_name):
+        scope_name = self._get_extension_scope_name(extension_name)
+        cpu_cgroup_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, 'cpu', 'system.slice', scope_name)
+        memory_cgroup_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, 'memory', 'system.slice', scope_name)
+
+        return [cpu_cgroup_path, memory_cgroup_path]
 
     def start_extension_command(self, extension_name, command, cwd, env, stdout, stderr):
-        pass
+        def pre_exec_function():
+            os.setsid()
+
+        process = subprocess.Popen(
+            "systemd-run --unit={0} --scope {1}".format(self._get_extension_scope_name(extension_name), command),
+            shell=True,
+            cwd=cwd,
+            stdout=stdout,
+            stderr=stderr,
+            env=env,
+            preexec_fn=pre_exec_function)
+
+        return process
diff --git a/azurelinuxagent/common/cgroupconfigurator.py b/azurelinuxagent/common/cgroupconfigurator.py
index 44f6d534..f5da59b9 100644
--- a/azurelinuxagent/common/cgroupconfigurator.py
+++ b/azurelinuxagent/common/cgroupconfigurator.py
@@ -24,7 +24,7 @@ from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.osutil import get_osutil
 from azurelinuxagent.common.osutil.default import BASE_CGROUPS
-from azurelinuxagent.common.utils import fileutil
+from azurelinuxagent.common.utils import fileutil, shellutil
 from azurelinuxagent.common.version import AGENT_NAME, CURRENT_VERSION
 from azurelinuxagent.common.event import add_event, WALAEventOperation
 
@@ -540,7 +540,7 @@ class CGroupConfigurator_tmp(object):
             :param cwd: The working directory for the command
             :param env:  The environment to pass to the command's process
             :param stdout: File object to redirect stdout to
-            :param stderr: File pbject to redirect stderr to
+            :param stderr: File object to redirect stderr to
             """
             if not CGroupConfigurator.enabled():
                 process = subprocess.Popen(


Test patch:
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index 6b903972..d3dff33d 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -17,11 +17,16 @@
 
 from __future__ import print_function
 
-from azurelinuxagent.common.cgroupapi import FileSystemCgroupsApi
-from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator
+from azurelinuxagent.common.cgroupapi import FileSystemCgroupsApi, SystemdCgroupsApi
+from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator, CGroupConfigurator_tmp
+from azurelinuxagent.common.utils import shellutil
 from tests.tools import *
 
 
+def i_am_root():
+    return os.geteuid() == 0
+
+
 @skip_if_predicate_false(CGroupConfigurator.enabled, "CGroups not supported in this environment")
 class TestCGroupConfigurator(AgentTestCase):
     #
@@ -31,3 +36,63 @@ class TestCGroupConfigurator(AgentTestCase):
         api = FileSystemCgroupsApi()
         api.create_agent_cgroups()
         self.assertTrue(True)
+
+    @patch('azurelinuxagent.common.cgroupapi.SystemdCgroupsApi.create_agent_cgroups')
+    @patch('azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_agent_cgroups')
+    @patch('azurelinuxagent.common.cgroupconfigurator.CGroupConfigurator.is_systemd')
+    def test_it_should_call_systemd_api(self, mock_is_systemd, mock_create_agent_cgroups_filesystem,
+                                        mock_create_agent_cgroups_systemd):
+        mock_is_systemd.return_value = True
+
+        configurator = CGroupConfigurator_tmp.get_instance()
+        configurator.create_agent_cgroups(track_cgroups=False)
+
+        self.assertEqual(mock_create_agent_cgroups_systemd.call_count, 1)
+        self.assertEqual(mock_create_agent_cgroups_filesystem.call_count, 0)
+
+
+@skip_if_predicate_false(CGroupConfigurator.enabled, "CGroups not supported in this environment")
+class TestSystemdCgroupsApi(AgentTestCase):
+
+    def test_it_should_return_extensions_slice_root_name(self):
+        root_slice_name = SystemdCgroupsApi()._get_extensions_slice_root_name()
+        self.assertEqual(root_slice_name, "system-walinuxagent.extensions.slice")
+
+    def test_it_should_return_extension_slice_name(self):
+        extension_name = "Microsoft.Azure.DummyExtension-1.0"
+        extension_slice_name = SystemdCgroupsApi()._get_extension_slice_name(extension_name)
+        self.assertEqual(extension_slice_name, "system-walinuxagent.extensions-Microsoft.Azure.DummyExtension_1.0.slice")
+
+    @skip_if_predicate_false(i_am_root, "Test does not run when normal user")
+    def test_if_extensions_root_slice_is_created(self):
+        SystemdCgroupsApi().create_extension_cgroups_root()
+
+        unit_name = SystemdCgroupsApi()._get_extensions_slice_root_name()
+        _, status = shellutil.run_get_output("systemctl status {0}".format(unit_name))
+        self.assertIn("Loaded: loaded", status)
+        self.assertIn("Active: active", status)
+
+        shellutil.run_get_output("systemctl stop {0}".format(unit_name))
+        shellutil.run_get_output("systemctl disable {0}".format(unit_name))
+        os.remove("/etc/systemd/system/{0}".format(unit_name))
+        shellutil.run_get_output("systemctl daemon-reload")
+
+    @skip_if_predicate_false(i_am_root, "Test does not run when normal user")
+    def test_it_should_create_extension_slice(self):
+        extension_name = "Microsoft.Azure.DummyExtension-1.0"
+        cgroups = SystemdCgroupsApi().create_extension_cgroups(extension_name)
+        cpu_cgroup, memory_cgroup = cgroups[0], cgroups[1]
+        self.assertEqual(cpu_cgroup, "/sys/fs/cgroup/cpu/system.slice/Microsoft.Azure.DummyExtension_1.0")
+        self.assertEqual(memory_cgroup, "/sys/fs/cgroup/memory/system.slice/Microsoft.Azure.DummyExtension_1.0")
+
+        unit_name = SystemdCgroupsApi._get_extension_slice_name(extension_name)
+        self.assertEqual("system-walinuxagent.extensions-Microsoft.Azure.DummyExtension_1.0.slice", unit_name)
+
+        _, status = shellutil.run_get_output("systemctl status {0}".format(unit_name))
+        self.assertIn("Loaded: loaded", status)
+        self.assertIn("Active: active", status)
+
+        shellutil.run_get_output("systemctl stop {0}".format(unit_name))
+        shellutil.run_get_output("systemctl disable {0}".format(unit_name))
+        os.remove("/etc/systemd/system/{0}".format(unit_name))
+        shellutil.run_get_output("systemctl daemon-reload")
diff --git a/tests/common/test_cgroups.py b/tests/common/test_cgroups.py
index d5e45b87..a2d14798 100644
--- a/tests/common/test_cgroups.py
+++ b/tests/common/test_cgroups.py
@@ -18,7 +18,8 @@
 from __future__ import print_function
 
 from azurelinuxagent.common.cgroup import CpuCgroup, MemoryCGroup
-from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator_tmp, CGroupConfigurator, CGroupsLimits, BASE_CGROUPS,  DEFAULT_MEM_LIMIT_MIN_MB
+from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator_tmp, CGroupConfigurator, CGroupsLimits, \
+    BASE_CGROUPS, DEFAULT_MEM_LIMIT_MIN_MB
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.version import AGENT_NAME
@@ -309,8 +310,8 @@ class TestCGroupsLimits(AgentTestCase):
     def test_no_limits_passed(self, patched_get_total_mem):
         cgroup_name = "test_cgroup"
         limits = CGroupsLimits(cgroup_name)
-        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name ))
-        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name ))
+        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name))
+        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name))
 
     @patch("azurelinuxagent.common.osutil.default.DefaultOSUtil.get_total_mem", return_value=1024)
     def test_with_limits_passed(self, patched_get_total_mem):