Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269

nagworld9 · 2024-12-03T19:22:59Z

Description

Current agents use drop-in file mechanism to set cgroup desired configuration. Creating drop-in files required daemon-reload run. This cmd can affect other services in the system like have seen for wlamart incident

The main changes in the PR:

Removes the drop-in files, deamon-reload and uses systemctl set-property --runtime for resource control settings.https://www.man7.org/linux/man-pages/man1/systemctl.1.html
New behavior: Raise the unexpected process if we see same process second time
CPU throttle metric calculated by default instead explicit enabling
In e2e test, added timeout for cmds/script runs over ssh

Added comments in the code and in the PR as well for other changes.

Issue #

PR information

Ensure development PR is based on the develop branch.
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.

nagworld9 · 2024-12-03T19:26:48Z

azurelinuxagent/ga/cgroupapi.py

@@ -650,8 +650,6 @@ def get_controllers(self, expected_relative_path=None):
                controller = MemoryControllerV1(self._cgroup_name, controller_path)

            if controller is not None:
-                msg = "{0} controller for cgroup: {1}".format(supported_controller_name, controller)


Logging here is a side-effect of calling get_controllers which being called everywhere. As a result, we are simply logging at random places where it's not needed. So, removing it here

nagworld9 · 2024-12-03T19:28:25Z

azurelinuxagent/ga/cgroupconfigurator.py


        def initialize(self):
            try:
                if self._initialized:
                    return
                # check whether cgroup monitoring is supported on the current distro
-                self._cgroups_supported = CGroupUtil.cgroups_supported()
+                self._cgroups_supported = self._check_cgroups_supported()


Moved all cgorups_supported checks to one place

nagworld9 · 2024-12-03T19:36:02Z

azurelinuxagent/ga/cgroupconfigurator.py

                #   running in system.slice.

-                self.__setup_azure_slice()
+                self._setup_azure_slice()


Not followed consistent naming in cgroupconfigurator. Renamed all of those to single underscore

nagworld9 · 2024-12-03T19:57:18Z

tests_e2e/tests/lib/shell.py

@@ -46,9 +46,17 @@ def run_command(command: Any, shell=False) -> str:
    NOTE: The command's stdout and stderr are read as text streams.
    """
    process = Popen(command, stdout=PIPE, stderr=PIPE, shell=shell, text=True)
+    timer = threading.Timer(15 * 60, process.kill)  # Kill process after timeout


Timeout for ssh runs

nagworld9 · 2024-12-03T19:57:43Z

azurelinuxagent/ga/cgroupstelemetry.py

    _rlock = threading.RLock()

-    @staticmethod
-    def set_track_throttled_time(value):


track throttled time by default as part of CPU metrics, no explicit enable

azurelinuxagent/common/osutil/systemd.py

azurelinuxagent/ga/cgroupcontroller.py

azurelinuxagent/ga/cgroupconfigurator.py

narrieta · 2024-12-06T18:32:46Z

tests_e2e/tests/lib/shell.py

@@ -46,9 +46,17 @@ def run_command(command: Any, shell=False) -> str:
    NOTE: The command's stdout and stderr are read as text streams.
    """
    process = Popen(command, stdout=PIPE, stderr=PIPE, shell=shell, text=True)
+    timer = threading.Timer(15 * 60, process.kill)  # Kill process after timeout


this is applied to all commands, not only ssh

also, for ssh, we should terminate the remote process, not the local process

yes, I would think we even don't want the commands to stuck.

is process.kill not terminate the child process which includes remote process? In my experiment processes in the vm also terminated.

ah, ok, as long as the remote process is killed too, we are ok

narrieta · 2024-12-06T18:34:54Z

tests_e2e/tests/scripts/get-agent-name

@@ -0,0 +1,28 @@
+#!/usr/bin/env bash


where is this script being invoked?

Initially thought of using it tests; seems at the end it was not used. If it's not needed in the future. I'll remove

tests_e2e/tests/scripts/get-agent-name

maddieford · 2024-12-07T00:31:47Z

azurelinuxagent/common/osutil/systemd.py

+
+def is_unit_loaded(unit_name):
+    """
+    Determine if a unit is loaded


What does it mean for a unit to be loaded

Unit being loaded means unit is parsed and available to the systemd process. We check this for extension services when we set quotas. If systemd is unaware of extension services and not loaded in the system yet, we get error while setting quotas. Hence, I added loaded check.

If systemd is unaware of extension services and not loaded in the system yet, we get error while setting quotas. Hence, I added loaded check.

can you add this as a comment where you call this function?

maddieford · 2024-12-07T00:53:11Z

azurelinuxagent/ga/exthandlers.py

@@ -1337,11 +1337,10 @@ def set_extension_resource_limits(self):
        # setup the resource limits for extension operations and it's services.
        man = self.load_manifest()
        resource_limits = man.get_resource_limits()
-        if not CGroupConfigurator.get_instance().is_extension_resource_limits_setup_completed(extension_name,


Why is this check getting removed? Will we set up resource limits on each operation for the ext?

Similar check was added in cgroupconfigurator setup_slice method. We don't touch the configuration again if limits already in place

azurelinuxagent/ga/cgroupconfigurator.py

maddieford · 2024-12-13T19:41:38Z

azurelinuxagent/ga/cgroupconfigurator.py

-            log_cgroup_info("Ensuring the agent's CPUQuota is {0}".format(quota_percentage))
-            if CGroupConfigurator._Impl.__try_set_cpu_quota(quota_percentage):
-                CGroupsTelemetry.set_track_throttled_time(True)
+            log_cgroup_info("Setting {0}'s CPUQuota is {1}".format(unit_name, quota_percentage))


Suggested change

log_cgroup_info("Setting {0}'s CPUQuota is {1}".format(unit_name, quota_percentage))

log_cgroup_info("Setting {0}'s CPUQuota to {1}".format(unit_name, quota_percentage))

azurelinuxagent/ga/cgroupconfigurator.py

maddieford · 2024-12-13T19:48:42Z

azurelinuxagent/ga/cgroupconfigurator.py

+                    if current == 0 and not (self._is_process_descendant_of_the_agent(process) or self._is_zombie_process(process)):
+                        current_unexpected[process] = self._format_process(process)
+                if report_immediately:
+                    report = [current_unexpected[process] for process in current_unexpected]


current_unexpected.values()?

narrieta · 2024-12-16T21:49:51Z

tests_e2e/tests/lib/shell.py


-    stdout, stderr = process.communicate()
+    try:
+        timer.start()


start() should probably be outside the try statement (if it throws, I think we do not want to call cancel())

narrieta

LGTM, approved with one minor comment on shell.py (e2e tests)

cgroup rewrite

2f8fbc7

nagworld9 requested review from narrieta, ZhidongPeng and maddieford as code owners December 3, 2024 19:23

nagworld9 commented Dec 3, 2024

View reviewed changes

nagworld9 added 2 commits December 3, 2024 13:42

new test

b863a09

mock cleanup

083a531

nagworld9 changed the title ~~Cgroup rewrite: uses systemctl for resource control settings instead drop-in files~~ Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files Dec 3, 2024

logging

3bd0904

narrieta reviewed Dec 6, 2024

View reviewed changes

maddieford reviewed Dec 7, 2024

View reviewed changes

maddieford reviewed Dec 9, 2024

View reviewed changes

nagworld9 added 3 commits December 11, 2024 14:23

address comments

cee9d2f

Merge remote-tracking branch 'origin/develop' into cgroup-v2

58709df

removed script

274def2

maddieford reviewed Dec 13, 2024

View reviewed changes

narrieta reviewed Dec 16, 2024

View reviewed changes

narrieta previously approved these changes Dec 16, 2024

View reviewed changes

new comment changes

39ec1ea

nagworld9 dismissed narrieta’s stale review via 39ec1ea December 17, 2024 00:36

nagworld9 and others added 2 commits December 16, 2024 16:42

pylint

8a31f42

Merge branch 'develop' into cgroup-v2

4533f3a

narrieta previously approved these changes Dec 17, 2024

View reviewed changes

e2e test changes

3b6d0d3

nagworld9 dismissed narrieta’s stale review via 3b6d0d3 December 17, 2024 02:51

maddieford approved these changes Dec 17, 2024

View reviewed changes

narrieta approved these changes Dec 17, 2024

View reviewed changes

nagworld9 merged commit d7fae2d into Azure:develop Dec 17, 2024
9 of 11 checks passed

nagworld9 deleted the cgroup-v2 branch December 17, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269

Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269

nagworld9 commented Dec 3, 2024 •

edited

Loading

nagworld9 Dec 3, 2024

nagworld9 Dec 3, 2024

nagworld9 Dec 3, 2024 •

edited

Loading

nagworld9 Dec 3, 2024

nagworld9 Dec 3, 2024

narrieta Dec 6, 2024

nagworld9 Dec 6, 2024 •

edited

Loading

narrieta Dec 16, 2024

narrieta Dec 6, 2024

nagworld9 Dec 6, 2024

maddieford Dec 7, 2024

nagworld9 Dec 9, 2024

narrieta Dec 11, 2024

maddieford Dec 7, 2024

nagworld9 Dec 9, 2024

maddieford Dec 13, 2024

maddieford Dec 13, 2024

narrieta Dec 16, 2024

narrieta left a comment

	log_cgroup_info("Setting {0}'s CPUQuota is {1}".format(unit_name, quota_percentage))
	log_cgroup_info("Setting {0}'s CPUQuota to {1}".format(unit_name, quota_percentage))

Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269

Cgroup rewrite: uses systemctl for expressing desired configuration instead drop-in files #3269

Conversation

nagworld9 commented Dec 3, 2024 • edited Loading

Description

PR information

Quality of Code and Contribution Guidelines

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagworld9 Dec 3, 2024 • edited Loading

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 Dec 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

narrieta left a comment

Choose a reason for hiding this comment

nagworld9 commented Dec 3, 2024 •

edited

Loading

nagworld9 Dec 3, 2024 •

edited

Loading

nagworld9 Dec 6, 2024 •

edited

Loading