check for unexpected process in agent cgroups before cgroups enabled #3103

nagworld9 · 2024-04-01T23:42:49Z

Description

This pr fixes the issue if unexpected process in the agent cgroup before enable cgroups, we don't continue with cgroups limit setup.

We have observed a scenario in production that
long running extension processes may be in agent cgroups if agent goes this cycle enabled(1)->disabled(2)->enabled(3).
1. Agent cgroups enabled in some version
2. Disabled agent cgroups due to check_cgroups regular check, now extension runs will be in agent cgroups.
3. When ext_hanlder restart and enable the cgroups again, already running processes from step 2 still be in agent cgroups. This may cause the extensions run with agent limit.

Issue #

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.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

codecov-commenter · 2024-04-01T23:49:43Z

Codecov Report

Attention: Patch coverage is 43.75000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 71.87%. Comparing base (3f49315) to head (a1dbe21).

❗ Current head a1dbe21 differs from pull request most recent head fd13729. Consider uploading reports for the commit fd13729 to get more accurate results

Files	Patch %	Lines
azurelinuxagent/ga/cgroupconfigurator.py	43.75%	17 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3103   +/-   ##
========================================
  Coverage    71.87%   71.87%           
========================================
  Files          110      110           
  Lines        16406    16424   +18     
  Branches      2344     2348    +4     
========================================
+ Hits         11791    11805   +14     
- Misses        4064     4067    +3     
- Partials       551      552    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nagworld9 · 2024-04-01T23:49:35Z

tests_e2e/tests/scripts/agent_cpu_quota-check_agent_cpu_quota.py

@@ -146,45 +146,18 @@ def wait_for_log_message(message, timeout=datetime.timedelta(minutes=5)):
    fail("The agent did not find [{0}] in its log within the allowed timeout".format(message))


-def verify_process_check_on_agent_cgroups():


removed here since checking as part of new test

nagworld9 · 2024-04-01T23:58:33Z

tests_e2e/tests/scripts/agent_cpu_quota-check_agent_cpu_quota.py

@@ -205,11 +178,20 @@ def verify_throttling_time_check_on_agent_cgroups():
        fail("The agent did not disable its CPUQuota: {0}".format(get_agent_cpu_quota()))


+def cleanup_test_setup():


Test has special setup, so cleaning up to avoid affecting other tests..

azurelinuxagent/ga/cgroupconfigurator.py

narrieta · 2024-04-02T15:11:05Z

azurelinuxagent/ga/cgroupconfigurator.py

+                self._check_processes_in_agent_cgroup()
+            except CGroupsException as exception:
+                _log_cgroup_warning(ustr(exception))
+                return True


It feels unintuitive that this function returns True when the check fails. Can we rename the function or switch the return value?

tests_e2e/tests/lib/cgroup_helpers.py

narrieta · 2024-04-02T16:39:58Z

tests_e2e/tests/scripts/agent_cgroups_process_check-unknown_process_check.py

+    Note: System may kick the added process out of the cgroups, keeps adding until agent detect that process
+    """
+    def unknown_process_found():
+        cgroup_procs_path = os.path.join(BASE_CGROUP, "cpu,cpuacct" + get_agent_cgroup_mount_path(), "cgroup.procs")


this path is specific to v1

narrieta · 2024-04-02T16:41:17Z

tests_e2e/tests/scripts/agent_cgroups_process_check-unknown_process_check.py

+    def unknown_process_found():
+        cgroup_procs_path = os.path.join(BASE_CGROUP, "cpu,cpuacct" + get_agent_cgroup_mount_path(), "cgroup.procs")
+        log.info("Adding dummy process %s to cgroup.procs file %s", pid, cgroup_procs_path)
+        process = subprocess.Popen(f"echo {pid} > {cgroup_procs_path}", shell=True)


need to check for errors writing to the file. you can use run_command or just python's open/write functions

narrieta · 2024-04-02T16:42:59Z

tests_e2e/tests/scripts/agent_cgroups_process_check-unknown_process_check.py

+
+def creating_dummy_process():
+    log.info("Creating dummy process to add to agent's cgroup")
+    dd_command = ["dd", "if=/dev/zero", "of=/dev/null"]


what is the intention of using dd? this will consume high CPU. sleep may be good enough

no intention, I remembered to use this command, decided to use. I can change it

tests_e2e/tests/scripts/agent_cgroups_process_check-unknown_process_check.py

maddieford · 2024-04-02T20:51:29Z

azurelinuxagent/ga/cgroupconfigurator.py

+
+            The issue we observed that long running extension processes may be in agent cgroups if agent goes this cycle enabled(1)->disabled(2)->enabled(3).
+            1. Agent cgroups enabled in some version
+            2. Disabled agent cgroups due to check_cgroups regular check, now extension runs will be in agent cgroups.


I don't understand #2.
If cgroups are disabled because some unexpected process is found in agent cgroup, then why would extension runs now be in agent cgroups?

Once agent cgroups disabled (disable for both agent and extensions), we don't run the extensions in the extension slice. That means it will be in agent cgroups.

could you please add those details to the comment

maddieford · 2024-04-02T20:52:12Z

azurelinuxagent/ga/cgroupconfigurator.py

+            if agent_slice not in AZURE_SLICE:
+                return False
+
+            if self._agent_cpu_cgroup_path is None:


Why is this check here?

we use this path for finding processes in cgroup.procs file. It could be none for v2

Why do we use agent_cpu_cgroup_path to check which processes in agent cgroup? Why not memory_cgroup_path? or should we be checking both cpu and memory paths?

it's possible for memory to be mounted/in use but cpu not

maddieford · 2024-04-02T20:57:27Z

tests/ga/test_cgroupconfigurator.py

@@ -905,6 +909,33 @@ def test_check_cgroups_should_disable_cgroups_when_a_check_fails(self):
                    for p in patchers:
                        p.stop()

+    @patch('azurelinuxagent.ga.cgroupconfigurator.CGroupConfigurator._Impl._check_processes_in_agent_cgroup', side_effect=CGroupsException("Test"))
+    @patch('azurelinuxagent.ga.cgroupconfigurator.add_event')
+    def test_agent_not_enable_cgroups_if_unexpected_process_already_in_agent_cgroups(self, add_event, _):


Suggested change

def test_agent_not_enable_cgroups_if_unexpected_process_already_in_agent_cgroups(self, add_event, _):

def test_agent_should_not_enable_cgroups_if_unexpected_process_already_in_agent_cgroups(self, add_event, _):

tests/ga/test_cgroupconfigurator.py

check for unexpected process in cgroup before enable

5558a78

nagworld9 requested review from narrieta, ZhidongPeng and maddieford as code owners April 1, 2024 23:42

agent restart

ae32cf3

nagworld9 commented Apr 1, 2024

View reviewed changes

nagworld9 added 2 commits April 1, 2024 20:47

move the process check

e837868

fix unit tests

a1dbe21

narrieta reviewed Apr 2, 2024

View reviewed changes

maddieford reviewed Apr 2, 2024

View reviewed changes

tests/ga/test_cgroupconfigurator.py Show resolved Hide resolved

nagworld9 added 2 commits April 3, 2024 18:02

address comments

fd13729

pylint

27941cc

narrieta approved these changes Apr 4, 2024

View reviewed changes

maddieford approved these changes Apr 4, 2024

View reviewed changes

nagworld9 merged commit 782a165 into Azure:develop Apr 4, 2024
11 checks passed

nagworld9 deleted the agentcgroup-check branch April 4, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for unexpected process in agent cgroups before cgroups enabled #3103

check for unexpected process in agent cgroups before cgroups enabled #3103

nagworld9 commented Apr 1, 2024

codecov-commenter commented Apr 1, 2024 •

edited

Loading

nagworld9 Apr 1, 2024

nagworld9 Apr 1, 2024

narrieta Apr 2, 2024

narrieta Apr 2, 2024

narrieta Apr 2, 2024

narrieta Apr 2, 2024

nagworld9 Apr 2, 2024

maddieford Apr 2, 2024

nagworld9 Apr 2, 2024

maddieford Apr 2, 2024

maddieford Apr 2, 2024

nagworld9 Apr 2, 2024 •

edited

Loading

maddieford Apr 2, 2024

maddieford Apr 2, 2024

		@@ -146,45 +146,18 @@ def wait_for_log_message(message, timeout=datetime.timedelta(minutes=5)):
		fail("The agent did not find [{0}] in its log within the allowed timeout".format(message))


		def verify_process_check_on_agent_cgroups():

		@@ -205,11 +178,20 @@ def verify_throttling_time_check_on_agent_cgroups():
		fail("The agent did not disable its CPUQuota: {0}".format(get_agent_cpu_quota()))


		def cleanup_test_setup():

	def test_agent_not_enable_cgroups_if_unexpected_process_already_in_agent_cgroups(self, add_event, _):
	def test_agent_should_not_enable_cgroups_if_unexpected_process_already_in_agent_cgroups(self, add_event, _):

check for unexpected process in agent cgroups before cgroups enabled #3103

check for unexpected process in agent cgroups before cgroups enabled #3103

Conversation

nagworld9 commented Apr 1, 2024

Description

PR information

Quality of Code and Contribution Guidelines

codecov-commenter commented Apr 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagworld9 Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 1, 2024 •

edited

Loading

nagworld9 Apr 2, 2024 •

edited

Loading