Repository: Azure/WALinuxAgent
Pull request #2127 (2021-01-06 00:44:07)

Disable resource enforcement when the agent's cgroup includes other processes
* Disable resource enforcement when the agent's cgroup includes other processes
* Move the reporting logic from the monitor thread to cgroup configurator, since this check will be done from other places, too
* Always create a periodic task for resource monitoring/enforcement, even if the feature is initially disabled (we will start enabling/disabling the feature dynamically)

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127?src=pr&el=h1) Report
> Merging [#2127](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127?src=pr&el=desc) (5071642) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/7fe94518e486245f477a5ee2b5667f1d68be5986?el=desc) (7fe9451) will **decrease** coverage by `0.04%`.
> The diff coverage is `86.04%`.

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

```diff
@@             Coverage Diff             @@
##           develop    #2127      +/-   ##
===========================================
- Coverage    70.70%   70.66%   -0.05%     
===========================================
  Files           95       95              
  Lines        12929    12927       -2     
  Branches      1835     1834       -1     
===========================================
- Hits          9142     9135       -7     
- Misses        3384     3387       +3     
- Partials       403      405       +2     
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/cgroupconfigurator.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9jZ3JvdXBjb25maWd1cmF0b3IucHk=) | `80.00% <84.61%> (-2.11%)` | :arrow_down: |
| [azurelinuxagent/ga/monitor.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL21vbml0b3IucHk=) | `78.06% <100.00%> (+0.22%)` | :arrow_up: |
| [azurelinuxagent/ga/collect\_telemetry\_events.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2127/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2dhL2NvbGxlY3RfdGVsZW1ldHJ5X2V2ZW50cy5weQ==) | `89.65% <0.00%> (-0.69%)` | :arrow_down: |

------

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


Patch:
diff --git a/azurelinuxagent/common/cgroupconfigurator.py b/azurelinuxagent/common/cgroupconfigurator.py
index cbaaf2aa..ea9b5f01 100644
--- a/azurelinuxagent/common/cgroupconfigurator.py
+++ b/azurelinuxagent/common/cgroupconfigurator.py
@@ -30,22 +30,11 @@ from azurelinuxagent.common.utils.extensionprocessutil import handle_process_com
 from azurelinuxagent.common.event import add_event, WALAEventOperation
 
 
-class UnexpectedProcessesInCGroupException(CGroupsException):
-    """
-    Raised by CGroupConfigurator.check_processes_in_agent_cgroup() when the agent's cgroup includes processes
-    that should not belong to it.
-    The 'unexpected' property is a list of the processes (strings) that should not belong to the agent's cgroup
-    """
-    def __init__(self, unexpected):
-        super(UnexpectedProcessesInCGroupException, self).__init__("Unexpected processes in agent's cgroup")
-        self.unexpected = unexpected
-
-
 class CGroupConfigurator(object):
     """
     This class implements the high-level operations on CGroups (e.g. initialization, creation, etc)
 
-    NOTE: with the exception of start_extension_command and check_processes_in_agent_cgroup, none of the methods in this class
+    NOTE: with the exception of start_extension_command, none of the methods in this class
     raise exceptions (cgroup operations should not block extensions)
     """
     class __Impl(object):
@@ -56,8 +45,8 @@ class CGroupConfigurator(object):
             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
+            self._check_processes_in_agent_cgroup_last_error = None
+            self._check_processes_in_agent_cgroup_error_count = 0
 
         def initialize(self):
             try:
@@ -248,37 +237,52 @@ class CGroupConfigurator(object):
             commands used to start extensions on their own cgroup).
             Other processes started by the agent (e.g. extensions) and processes not started by the agent (e.g. services installed by extensions) are reported
             as unexpected, since they should belong to their own cgroup.
-            The function raises an UnexpectedProcessesInCGroupException if the check fails.
             """
             if not self.enabled():
-                return
+                return True
 
-            daemon = os.getppid()
-            extension_handler = os.getpid()
-            agent_commands = set()
-            agent_commands.update(shellutil.get_running_commands())
-            systemd_run_commands = set()
-            systemd_run_commands.update(self._cgroups_api.get_systemd_run_commands())
-            agent_cgroup = CGroupsApi.get_processes_in_cgroup(self._agent_cpu_cgroup_path)
-            # get the running commands again in case new commands were started while we were fetching the processes in the cgroup;
-            agent_commands.update(shellutil.get_running_commands())
-            systemd_run_commands.update(self._cgroups_api.get_systemd_run_commands())
-
-            unexpected = []
-            for process in agent_cgroup:
-                # Note that the agent uses systemd-run to start extensions; systemd-run belongs to the agent cgroup, though the extensions don't
-                if process in (daemon, extension_handler) or process in systemd_run_commands:
-                    continue
-                # check if the process is a command started by the agent or a descendant of one of those commands
-                current = process
-                while current != 0 and current not in agent_commands:
-                    current = self._get_parent(current)
-                if current == 0:
-                    unexpected.append(process)
-                    if len(unexpected) >= 5:  # collect just a small sample
-                        break
-            if unexpected:
-                raise UnexpectedProcessesInCGroupException(unexpected=self._format_processes(unexpected))
+            def log_message(message):
+                # Report only a small sample of errors
+                if message != self._check_processes_in_agent_cgroup_last_error and self._check_processes_in_agent_cgroup_error_count < 5:
+                    self._check_processes_in_agent_cgroup_error_count += 1
+                    self._check_processes_in_agent_cgroup_last_error = message
+                    logger.info(message)
+                    add_event(op=WALAEventOperation.CGroupsDebug, message=message)
+
+            try:
+                daemon = os.getppid()
+                extension_handler = os.getpid()
+                agent_commands = set()
+                agent_commands.update(shellutil.get_running_commands())
+                systemd_run_commands = set()
+                systemd_run_commands.update(self._cgroups_api.get_systemd_run_commands())
+                agent_cgroup = CGroupsApi.get_processes_in_cgroup(self._agent_cpu_cgroup_path)
+                # get the running commands again in case new commands started or completed while we were fetching the processes in the cgroup;
+                agent_commands.update(shellutil.get_running_commands())
+                systemd_run_commands.update(self._cgroups_api.get_systemd_run_commands())
+
+                unexpected = []
+                for process in agent_cgroup:
+                    # Note that the agent uses systemd-run to start extensions; systemd-run belongs to the agent cgroup, though the extensions don't
+                    if process in (daemon, extension_handler) or process in systemd_run_commands:
+                        continue
+                    # check if the process is a command started by the agent or a descendant of one of those commands
+                    current = process
+                    while current != 0 and current not in agent_commands:
+                        current = self._get_parent(current)
+                    if current == 0:
+                        unexpected.append(process)
+                        if len(unexpected) >= 5:  # collect just a small sample
+                            break
+                if unexpected:
+                    unexpected = self._format_processes(unexpected)
+                    unexpected.sort()  # sort the PIDs so that the error message stays more consistent across different calls to this check
+                    log_message("The agent's cgroup includes unexpected processes; disabling CPU enforcement. Unexpected: {0}".format(unexpected))
+                    self.disable()
+                    return False
+            except Exception as exception:
+                log_message("Failed to check the processes in the agent's cgroup: {0}".format(ustr(exception)))
+            return True
 
         @staticmethod
         def _format_processes(pid_list):
diff --git a/azurelinuxagent/ga/monitor.py b/azurelinuxagent/ga/monitor.py
index 719b3f49..8b9edc5b 100644
--- a/azurelinuxagent/ga/monitor.py
+++ b/azurelinuxagent/ga/monitor.py
@@ -21,7 +21,7 @@ import uuid
 
 import azurelinuxagent.common.logger as logger
 import azurelinuxagent.common.utils.networkutil as networkutil
-from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator, UnexpectedProcessesInCGroupException
+from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.errorstate import ErrorState
 from azurelinuxagent.common.event import add_event, WALAEventOperation, report_metric
@@ -52,22 +52,10 @@ class PollResourceUsageOperation(PeriodicOperation):
             name="poll resource usage",
             operation=self._operation_impl,
             period=datetime.timedelta(minutes=5))
-        self._last_error = None
-        self._error_count = 0
 
-    def _operation_impl(self):
-        try:
-            CGroupConfigurator.get_instance().check_processes_in_agent_cgroup()
-        except UnexpectedProcessesInCGroupException as exception:
-            exception.unexpected.sort()
-            message = "The agent's cgroup includes unexpected processes: {0}".format(exception.unexpected)
-
-            # Report a small sample of errors
-            if message != self._last_error and self._error_count < 5:
-                self._error_count += 1
-                self._last_error = message
-                logger.info(message)
-                add_event(op=WALAEventOperation.CGroupsDebug, message=message)
+    @staticmethod
+    def _operation_impl():
+        CGroupConfigurator.get_instance().check_processes_in_agent_cgroup()
 
         for metric in CGroupsTelemetry.poll_all_tracked():
             report_metric(metric.category, metric.counter, metric.instance, metric.value)
@@ -164,9 +152,8 @@ class MonitorHandler(ThreadHandlerInterface):
             PeriodicOperation("send_host_plugin_heartbeat", self.send_host_plugin_heartbeat, self.HOST_PLUGIN_HEARTBEAT_PERIOD),
             PeriodicOperation("send_imds_heartbeat", self.send_imds_heartbeat, self.IMDS_HEARTBEAT_PERIOD),
             ReportNetworkConfigurationChangesOperation(),
+            PollResourceUsageOperation()
         ]
-        if CGroupConfigurator.get_instance().enabled():
-            self._periodic_operations.append(PollResourceUsageOperation())
 
         self.protocol = None
         self.protocol_util = None


Test patch:
diff --git a/tests/common/test_cgroupconfigurator.py b/tests/common/test_cgroupconfigurator.py
index adf04f32..547495e6 100644
--- a/tests/common/test_cgroupconfigurator.py
+++ b/tests/common/test_cgroupconfigurator.py
@@ -26,7 +26,7 @@ import time
 import threading
 
 from azurelinuxagent.common.cgroup import CGroup
-from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator, UnexpectedProcessesInCGroupException
+from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.utils import shellutil
@@ -34,6 +34,7 @@ from tests.common.mock_cgroup_commands import mock_cgroup_commands
 from tests.tools import AgentTestCase, patch, mock_sleep
 from tests.utils.miscellaneous_tools import format_processes, wait_for
 
+
 class CGroupConfiguratorSystemdTestCase(AgentTestCase):
     @classmethod
     def tearDownClass(cls):
@@ -295,7 +296,7 @@ cgroup on /sys/fs/cgroup/blkio type cgroup (rw,nosuid,nodev,noexec,relatime,blki
 
                 self.assertIn("A TEST EXCEPTION", str(context_manager.exception))
 
-    def test_check_processes_in_agent_cgroup_should_raise_when_there_are_unexpected_processes_in_the_agent_cgroup(self):
+    def test_check_processes_in_agent_cgroup_should_disable_cgroups_when_there_are_unexpected_processes_in_the_agent_cgroup(self):
         configurator = CGroupConfiguratorSystemdTestCase._get_new_cgroup_configurator_instance()
 
         # The test script recursively creates a given number of descendant processes, then it blocks until the
@@ -414,35 +415,37 @@ exit 0
                     completed = random.randint(1000, 10000)
                 return completed
 
+            def get_telemetry_event_messages():
+                return [kwargs["message"] for (_, kwargs) in add_event_patcher.call_args_list if "The agent's cgroup includes unexpected processes" in kwargs["message"]]
+
             agent_processes = [os.getppid(), os.getpid()] + agent_command_processes + [start_extension.systemd_run_pid]
             other_processes = [1, get_completed_process()] + extension_processes
 
             with patch("azurelinuxagent.common.cgroupconfigurator.CGroupsApi.get_processes_in_cgroup", return_value=agent_processes + other_processes):
-                with self.assertRaises(UnexpectedProcessesInCGroupException) as context_manager:
-                    CGroupConfigurator.get_instance().check_processes_in_agent_cgroup()
+                with patch("azurelinuxagent.common.cgroupconfigurator.add_event") as add_event_patcher:
+                    cgroup_configurator = CGroupConfigurator.get_instance()
 
-                reported = context_manager.exception.unexpected
+                    return_value = cgroup_configurator.check_processes_in_agent_cgroup()
 
-                self.assertEqual(
-                    len(other_processes), len(reported),
-                    "An incorrect number of processes was reported. Expected: {0} Got: {1}".format(format_processes(other_processes), reported))
-                for pid in other_processes:
-                    self.assertTrue(
-                        any(reported_process.startswith("[PID: {0}]".format(pid)) for reported_process in reported),
-                        "Process {0} was not reported. Got: {1}".format(format_processes([pid]), reported))
+                    self.assertFalse(return_value, "check_processes_in_agent_cgroup() should have failed")
+                    self.assertFalse(cgroup_configurator.enabled(), "Cgroups should have been disabled")
 
-            #
-            # And now verify that it does not raise when only the expected processes are in the cgroup
-            #
-            error = None
-            try:
-                with patch("azurelinuxagent.common.cgroupconfigurator.CGroupsApi.get_processes_in_cgroup", return_value=agent_processes):
-                    CGroupConfigurator.get_instance().check_processes_in_agent_cgroup()
-            except UnexpectedProcessesInCGroupException as exception:
-                error = exception
-            # we fail outside the except clause, otherwise the failure is reported as "During handling of the above exception, another exception occurred:..."
-            if error is not None:
-                self.fail("The check of the agent's cgroup should not have reported errors. Reported processes: {0}".format(error.unexpected))
+                    messages = get_telemetry_event_messages()
+
+                    self.assertEqual(1, len(messages), "Exactly 1 telemetry event should have been reported. Events: {0}".format(messages))
+
+                    # The list of processes in the message is an array of strings: "['foo', ..., 'bar']"
+                    search = re.search(r'\[(?P<processes>.+)\]', messages[0])
+                    self.assertIsNotNone(search, "The event message is not in the expected format: {0}".format(messages[0]))
+                    reported = search.group('processes').split(',')
+
+                    self.assertEqual(
+                        len(other_processes), len(reported),
+                        "An incorrect number of processes was reported. Expected: {0} Got: {1}".format(format_processes(other_processes), reported))
+                    for pid in other_processes:
+                        self.assertTrue(
+                            any("[PID: {0}]".format(pid) in reported_process for reported_process in reported),
+                            "Process {0} was not reported. Got: {1}".format(format_processes([pid]), reported))
 
         finally:
             # create the file that stops the test process and wait for them to complete
@@ -450,4 +453,3 @@ exit 0
             for thread in threads:
                 thread.join(timeout=5)
 
-
diff --git a/tests/ga/test_monitor.py b/tests/ga/test_monitor.py
index 510bc5b7..cf7218bb 100644
--- a/tests/ga/test_monitor.py
+++ b/tests/ga/test_monitor.py
@@ -19,12 +19,10 @@ import contextlib
 import datetime
 import os
 import random
-import re
 import string
 
 from azurelinuxagent.common import event, logger
 from azurelinuxagent.common.cgroup import CGroup, CpuCgroup, MemoryCgroup, MetricValue
-from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator, UnexpectedProcessesInCGroupException
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.event import EVENTS_DIRECTORY
 from azurelinuxagent.common.logger import Logger
@@ -32,7 +30,6 @@ from azurelinuxagent.common.protocol.util import ProtocolUtil
 from azurelinuxagent.common.protocol.wire import WireProtocol
 from azurelinuxagent.ga.monitor import get_monitor_handler, MonitorHandler, PeriodicOperation, \
     ResetPeriodicLogMessagesOperation, PollResourceUsageOperation
-from tests.common.mock_cgroup_commands import mock_cgroup_commands
 from tests.protocol.mocks import mock_wire_protocol, HttpRequestPredicates, MockHttpResponse
 from tests.protocol.mockwiredata import DATA_FILE
 from tests.tools import Mock, MagicMock, patch, AgentTestCase, clear_singleton_instances
@@ -277,36 +274,6 @@ class TestExtensionMetricsDataTelemetry(AgentTestCase):
                             CGroupsTelemetry.poll_all_tracked()
 
 
-class PollResourceUsageOperationTestCase(AgentTestCase):
-    @classmethod
-    def setUpClass(cls):
-        AgentTestCase.setUpClass()
-        # ensure cgroups are enabled by forcing a new instance
-        CGroupConfigurator._instance = None  # pylint: disable=protected-access
-        with mock_cgroup_commands():
-            CGroupConfigurator.get_instance().initialize()
-
-    @classmethod
-    def tearDownClass(cls):
-        CGroupConfigurator._instance = None  # pylint: disable=protected-access
-        AgentTestCase.tearDownClass()
-
-    def test_it_should_issue_a_telemetry_event_when_there_are_processes_that_do_not_belong_to_the_agent_cgroup(self):
-        with patch.object(CGroupConfigurator.get_instance(), "check_processes_in_agent_cgroup", side_effect=UnexpectedProcessesInCGroupException(["A-TEST-PROCESS"])):
-            with patch("azurelinuxagent.ga.monitor.add_event") as add_event_patcher:
-                PollResourceUsageOperation().run()
-
-                messages = [kwargs["message"] for (_, kwargs) in add_event_patcher.call_args_list if "The agent's cgroup includes unexpected processes" in kwargs["message"]]
-
-                self.assertEqual(1, len(messages), "Exactly 1 telemetry event should have been reported. Events: {0}".format(messages))
-
-                # The list of processes in the message is an array of strings: "['foo', ..., 'bar']"
-                search = re.search(r'\[(?P<processes>.+)\]', messages[0])
-                self.assertIsNotNone(search, "The event message is not in the expected format: {0}".format(messages[0]))
-                processes = search.group('processes')
-                self.assertIn("A-TEST-PROCESS", processes, 'Extra processes were reported as unexpected: {0}'.format(processes))
-
-
 @patch("azurelinuxagent.common.utils.restutil.http_post")
 @patch('azurelinuxagent.common.protocol.wire.WireClient.get_goal_state')
 @patch('azurelinuxagent.common.event.EventLogger.add_event')