Repository: Azure/WALinuxAgent
Pull request #1595 (2019-07-29 22:05:50)

Clean up old daemon cgroups
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description

Daemons from version 2.2.31 to 2.2.40 added their PID to a now-obsolete cgroup `WALinuxAgent/WALinuxAgent`. Add logic to move that PID to the new cgroup, `walinuxagent.service` for each controller to ensure we track both the daemon and extension handler process.

<!--
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/1595)
<!-- Reviewable:end -->



Patch:
diff --git a/azurelinuxagent/common/cgroupapi.py b/azurelinuxagent/common/cgroupapi.py
index 30136b66..f634fefa 100644
--- a/azurelinuxagent/common/cgroupapi.py
+++ b/azurelinuxagent/common/cgroupapi.py
@@ -16,12 +16,14 @@
 
 import errno
 import os
+import shutil
 import subprocess
 import time
 import uuid
 
 from azurelinuxagent.common import logger
 from azurelinuxagent.common.cgroup import CGroup
+from azurelinuxagent.common.conf import get_agent_pid_file_path
 from azurelinuxagent.common.event import add_event, WALAEventOperation
 from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.future import ustr
@@ -58,6 +60,9 @@ class CGroupsApi(object):
     def start_extension_command(self, extension_name, command, shell, cwd, env, stdout, stderr):
         raise NotImplementedError()
 
+    def cleanup_old_cgroups(self):
+        raise NotImplementedError()
+
     @staticmethod
     def _get_extension_cgroup_name(extension_name):
         # Since '-' is used as a separator in systemd unit names, we replace it with '_' to prevent side-effects.
@@ -183,6 +188,29 @@ class FileSystemCgroupsApi(CGroupsApi):
         fileutil.append_file(tasks_file, "{0}\n".format(pid))
         logger.info("Added PID {0} to cgroup {1}".format(pid, cgroup_path))
 
+    def cleanup_old_cgroups(self):
+        # Old daemon versions (2.2.31-2.2.40) wrote their PID to the WALinuxAgent/WALinuxAgent cgroup.
+        # Starting from version 2.2.41, we track the agent service in walinuxagent.service. This method
+        # cleans up the old behavior by moving the daemon's PID to the new cgroup and deleting the old cgroup.
+        daemon_pid_file = get_agent_pid_file_path()
+        daemon_pid = fileutil.read_file(daemon_pid_file)
+
+        def cleanup_old_controller(controller):
+            old_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, controller, "WALinuxAgent", "WALinuxAgent")
+            new_path = os.path.join(CGROUPS_FILE_SYSTEM_ROOT, controller, VM_AGENT_CGROUP_NAME)
+
+            if not os.path.isdir(old_path):
+                return
+
+            contents = fileutil.read_file(os.path.join(old_path, "cgroup.procs"))
+
+            if daemon_pid in contents:
+                fileutil.append_file(os.path.join(new_path, "cgroup.procs"), daemon_pid)
+                shutil.rmtree(old_path, ignore_errors=True)
+
+        self._foreach_controller(cleanup_old_controller, "Failed to update the tracking of the daemon; resource usage "
+                                                         "of the agent will not include the daemon process.")
+
     def create_agent_cgroups(self):
         """
         Creates a cgroup for the VM Agent in each of the controllers we are tracking; returns the created cgroups.
@@ -473,3 +501,7 @@ After=system-{1}.slice""".format(extension_name, EXTENSIONS_ROOT_CGROUP_NAME)
         self._foreach_controller(create_cgroup, 'Cannot create cgroup for extension {0}; resource usage will not be tracked.'.format(extension_name))
 
         return process, cgroups
+
+    def cleanup_old_cgroups(self):
+        # No cleanup needed from the old daemon in the systemd case.
+        pass
diff --git a/azurelinuxagent/common/cgroupconfigurator.py b/azurelinuxagent/common/cgroupconfigurator.py
index d954f745..8ab9e7a4 100644
--- a/azurelinuxagent/common/cgroupconfigurator.py
+++ b/azurelinuxagent/common/cgroupconfigurator.py
@@ -105,6 +105,13 @@ class CGroupConfigurator(object):
 
             self._invoke_cgroup_operation(__impl, "Failed to create a cgroup for the VM Agent; resource usage for the Agent will not be tracked")
 
+        def cleanup_old_cgroups(self):
+            def __impl():
+                self._cgroups_api.cleanup_old_cgroups()
+
+            self._invoke_cgroup_operation(__impl, "Failed to update the tracking of the daemon; resource usage "
+                                                  "of the agent will not include the daemon process.")
+
         def create_extension_cgroups_root(self):
             """
             Creates the container (directory/cgroup) that includes the cgroups for all extensions (/sys/fs/cgroup/*/walinuxagent.extensions)
diff --git a/azurelinuxagent/daemon/main.py b/azurelinuxagent/daemon/main.py
index a951e394..0f5296a4 100644
--- a/azurelinuxagent/daemon/main.py
+++ b/azurelinuxagent/daemon/main.py
@@ -69,6 +69,7 @@ class DaemonHandler(object):
         self.initialize_environment()
 
         CGroupConfigurator.get_instance().create_agent_cgroups(track_cgroups=False)
+        CGroupConfigurator.get_instance().cleanup_old_cgroups()
 
         # If FIPS is enabled, set the OpenSSL environment variable
         # Note:
diff --git a/azurelinuxagent/ga/update.py b/azurelinuxagent/ga/update.py
index 848ff0c2..7f4e19d2 100644
--- a/azurelinuxagent/ga/update.py
+++ b/azurelinuxagent/ga/update.py
@@ -452,6 +452,7 @@ class UpdateHandler(object):
     def _ensure_cgroups_initialized(self):
         configurator = CGroupConfigurator.get_instance()
         configurator.create_agent_cgroups(track_cgroups=True)
+        configurator.cleanup_old_cgroups()
         configurator.create_extension_cgroups_root()
 
     def _evaluate_agent_health(self, latest_agent):


Test patch:
diff --git a/tests/common/test_cgroupapi.py b/tests/common/test_cgroupapi.py
index c19b0a23..67111dbe 100644
--- a/tests/common/test_cgroupapi.py
+++ b/tests/common/test_cgroupapi.py
@@ -163,7 +163,6 @@ rdma	6	1	1
             self.assertIn('Error in cgroup controller "cpu": A test exception.', message)
 
 
-@skip_if_predicate_true(is_systemd_present, "FileSystem cgroups API doesn't manage cgroups on systems using systemd.")
 class FileSystemCgroupsApiTestCase(AgentTestCase):
 
     def setUp(self):
@@ -182,6 +181,46 @@ class FileSystemCgroupsApiTestCase(AgentTestCase):
 
         AgentTestCase.tearDown(self)
 
+    def test_cleanup_old_cgroups_should_move_daemon_pid_on_all_controllers(self):
+        # Set up the mock /var/run/waagent.pid file
+        daemon_pid = "42"
+        daemon_pid_file_tmp = os.path.join(self.tmp_dir, "waagent.pid")
+        with open(daemon_pid_file_tmp, "w") as f:
+            f.write(daemon_pid)
+
+        # Set up old controller cgroups and add the daemon PID to them
+        old_cpu_cgroup = os.path.join(self.cgroups_file_system_root, "cpu", "WALinuxAgent", "WALinuxAgent")
+        old_memory_cgroup = os.path.join(self.cgroups_file_system_root, "memory", "WALinuxAgent", "WALinuxAgent")
+
+        os.makedirs(old_cpu_cgroup)
+        os.makedirs(old_memory_cgroup)
+
+        fileutil.write_file(os.path.join(old_cpu_cgroup, "cgroup.procs"), daemon_pid + "\n")
+        fileutil.write_file(os.path.join(old_memory_cgroup, "cgroup.procs"), daemon_pid + "\n")
+
+        # Set up new controller cgroups and add another PID to them
+        new_cpu_cgroup = os.path.join(self.cgroups_file_system_root, "cpu", VM_AGENT_CGROUP_NAME)
+        new_memory_cgroup = os.path.join(self.cgroups_file_system_root, "memory", VM_AGENT_CGROUP_NAME)
+
+        os.makedirs(new_cpu_cgroup)
+        os.makedirs(new_memory_cgroup)
+
+        fileutil.write_file(os.path.join(new_cpu_cgroup, "cgroup.procs"), "999\n")
+        fileutil.write_file(os.path.join(new_memory_cgroup, "cgroup.procs"), "999\n")
+
+        with patch("azurelinuxagent.common.cgroupapi.get_agent_pid_file_path", return_value=daemon_pid_file_tmp):
+            FileSystemCgroupsApi().cleanup_old_cgroups()
+
+        # The method should have added the daemon PID to the new controllers and deleted the old ones
+        new_cpu_contents = fileutil.read_file(os.path.join(new_cpu_cgroup, "cgroup.procs"))
+        new_memory_contents = fileutil.read_file(os.path.join(new_memory_cgroup, "cgroup.procs"))
+
+        self.assertTrue(daemon_pid in new_cpu_contents)
+        self.assertTrue(daemon_pid in new_memory_contents)
+
+        self.assertFalse(os.path.exists(old_cpu_cgroup))
+        self.assertFalse(os.path.exists(old_memory_cgroup))
+
     def test_create_agent_cgroups_should_create_cgroups_on_all_controllers(self):
         agent_cgroups = FileSystemCgroupsApi().create_agent_cgroups()
 
diff --git a/tests/common/test_cgroupconfigurator.py b/tests/common/test_cgroupconfigurator.py
index f4b55c67..bc0596bb 100644
--- a/tests/common/test_cgroupconfigurator.py
+++ b/tests/common/test_cgroupconfigurator.py
@@ -106,6 +106,7 @@ class CGroupConfiguratorTestCase(AgentTestCase):
         # List of operations to test, and the functions to mock used in order to do verifications
         operations = [
             [lambda: configurator.create_agent_cgroups(track_cgroups=False), "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_agent_cgroups"],
+            [lambda: configurator.cleanup_old_cgroups(),                     "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.cleanup_old_cgroups"],
             [lambda: configurator.create_extension_cgroups_root(),           "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_extension_cgroups_root"],
             [lambda: configurator.create_extension_cgroups("A.B.C-1.0.0"),   "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_extension_cgroups"],
             [lambda: configurator.remove_extension_cgroups("A.B.C-1.0.0"),   "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.remove_extension_cgroups"]
@@ -123,6 +124,7 @@ class CGroupConfiguratorTestCase(AgentTestCase):
         # List of operations to test, and the functions to mock in order to raise exceptions
         operations = [
             [lambda: configurator.create_agent_cgroups(track_cgroups=False), "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_agent_cgroups"],
+            [lambda: configurator.cleanup_old_cgroups(),                     "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.cleanup_old_cgroups"],
             [lambda: configurator.create_extension_cgroups_root(),           "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_extension_cgroups_root"],
             [lambda: configurator.create_extension_cgroups("A.B.C-1.0.0"),   "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.create_extension_cgroups"],
             [lambda: configurator.remove_extension_cgroups("A.B.C-1.0.0"),   "azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.remove_extension_cgroups"]