Improvements in setup of cgroups #1896

narrieta · 2020-05-28T03:00:52Z

Previously the cgroups code would make several assumptions as to how cgroups were setup.

These changes discover the current setup instead of making those assumptions. Also, it sends telemetry for important parts of that setup.

codecov · 2020-05-28T03:03:00Z

Codecov Report

Merging #1896 into develop will decrease coverage by 0.19%.
The diff coverage is 77.29%.

@@             Coverage Diff             @@
##           develop    #1896      +/-   ##
===========================================
- Coverage    69.62%   69.42%   -0.20%     
===========================================
  Files           85       85              
  Lines        11723    11788      +65     
  Branches      1631     1653      +22     
===========================================
+ Hits          8162     8184      +22     
- Misses        3214     3239      +25     
- Partials       347      365      +18

Impacted Files	Coverage Δ
azurelinuxagent/common/osutil/default.py	`57.89% <ø> (-1.24%)`	⬇️
azurelinuxagent/common/osutil/ubuntu.py	`68.49% <ø> (-0.34%)`	⬇️
azurelinuxagent/daemon/main.py	`72.16% <ø> (-0.57%)`	⬇️
azurelinuxagent/ga/exthandlers.py	`86.13% <ø> (-0.03%)`	⬇️
azurelinuxagent/common/cgroupconfigurator.py	`74.60% <64.38%> (-15.62%)`	⬇️
azurelinuxagent/common/cgroupapi.py	`81.42% <84.90%> (-3.74%)`	⬇️
azurelinuxagent/common/cgroupstelemetry.py	`86.81% <100.00%> (ø)`
azurelinuxagent/common/event.py	`85.93% <100.00%> (-0.41%)`	⬇️
azurelinuxagent/ga/update.py	`87.59% <100.00%> (-0.04%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca6a53d...9c2a202. Read the comment docs.

narrieta · 2020-05-28T03:04:31Z

azurelinuxagent/common/cgroupapi.py

+            distro_version = FlexibleVersion(distro_info[1])
+        except ValueError:
+            return False
+        return distro_name.lower() == 'ubuntu' and distro_version.major >= 16


I am focusing on Ubuntu 16, 18 & 20 so this is hardcoded for now

narrieta · 2020-05-28T03:04:55Z

azurelinuxagent/common/cgroupapi.py

@@ -202,6 +212,58 @@ def _add_process_to_cgroup(pid, cgroup_path):
        fileutil.append_file(tasks_file, "{0}\n".format(pid))
        logger.info("Added PID {0} to cgroup {1}".format(pid, cgroup_path))

+    @staticmethod


moved this from osutils (though it is not being used right now, I may need it when adding support for non-systemd distros)

narrieta · 2020-05-28T03:07:29Z

azurelinuxagent/common/cgroupapi.py

@@ -386,23 +538,6 @@ def _get_extensions_slice_root_name():
    def _get_extension_slice_name(self, extension_name):
        return "system-{0}-{1}.slice".format(EXTENSIONS_ROOT_CGROUP_NAME, self._get_extension_cgroup_name(extension_name))

-    def create_agent_cgroups(self):


systemd creates the cgroups for the agent so this function was misleading. now, CgroupConfigurator.initialize() discovers the cgroups for the agent and starts tracking for them

narrieta · 2020-05-28T03:12:25Z

azurelinuxagent/daemon/main.py

@@ -149,9 +148,6 @@ def daemon(self, child_args=None):
        # that require the goal state and IMDS
        self._initialize_telemetry()

-        # Initialize the agent cgroup


I am doing all the cgroups stuff in the extension handler. Systemd does all the setup and the handler simply discovers the setup and starts tracking the cgroups. When I get to non-systemd distros I will have to do some of the setup in the daemon, but for now it is not needed.

narrieta · 2020-05-28T03:13:38Z

azurelinuxagent/common/event.py

@@ -688,7 +689,7 @@ def initialize_event_logger_vminfo_common_parameters(protocol, reporter=__event_
    reporter.initialize_vminfo_common_parameters(protocol)


-def add_event(name, op=WALAEventOperation.Unknown, is_success=True, duration=0, version=str(CURRENT_VERSION),
+def add_event(name=AGENT_NAME, op=WALAEventOperation.Unknown, is_success=True, duration=0, version=str(CURRENT_VERSION),


I defaulted the 'name' parameter to the agent; makes the calls to add_event a little more readable

narrieta · 2020-05-28T03:15:17Z

tests/common/osutil/test_default_osutil.py



 class DefaultOsUtilTestCase(AgentTestCase):
-


Moved those to test_cgroupsapi.py/MountCgroupsTestCase

pgombar

LGTM with a couple of minor comments

pgombar · 2020-05-28T22:52:22Z

azurelinuxagent/common/cgroupconfigurator.py

+                #
+                self._cgroups_supported = CGroupsApi.cgroups_supported()
+                if not self._cgroups_supported:
+                    logger.info("Cgroup monitoring is not supported on {0}", get_distro())


Do we want a telemetry event for this?

The list of supported distros is hardcoded (ubuntu 16, 18, 20) so no need for telemetry

pgombar · 2020-05-28T22:55:11Z

azurelinuxagent/common/cgroupconfigurator.py

+                if memory_cgroup_relative_path is None:
+                    log_cgroup_warn("The agent's process is not within a memory cgroup")
+                else:
+                    cpu_accounting = self._cgroups_api.get_unit_property(agent_unit_name, "MemoryAccounting")


memory_accounting instead of cpu_accounting?

pgombar · 2020-05-28T23:27:58Z

tests/common/test_cgroupconfigurator.py

+        configurator = CGroupConfigurator.get_instance()
+        if initialize:
+            with patch('azurelinuxagent.common.cgroupapi.CGroupsApi.cgroups_supported', return_value=True):
+                with patch('azurelinuxagent.common.cgroupapi.CGroupsApi._is_systemd', return_value=True):


Before this PR, we were forcing FileSystemCgroupsApi here. Any reason for the change? I forget why filesystem was favoured before.

Yes, with these changes FileSystemCgroupsApi is dead code. I do not want to delete it yet since we will eventually get to distros without systemd but for now all the code is going with the systemd path. I updated these tests to follow the same

larohra · 2020-05-29T23:26:03Z

azurelinuxagent/common/cgroupconfigurator.py

-                message=status,
-                log_event=False)
+            except Exception as e:
+                message = "Error initializing cgroups: {0}".format(ustr(e))


Not sure if needed, but shouldn't we mark self._cgroups_enabled = False here?

initially I was setting enabled to True at the very end of the try clause, but then I realized cgroups should be ready to use if we reach line 133 so I moved the assignment there. If for any reason we cannot track the agent cgroups we should still track extensions; if for any reason we cannot track the agent's memory cgroup we should still track its cpu cgroup

larohra

1 small question, else everything LGTM

larohra · 2020-05-29T23:34:15Z

tests/common/mock_cgroup_commands.py

+    def mock_popen(command, *args, **kwargs):
+        if isinstance(command, list):
+            command_string = " ".join(command)
+            for c in _default_commands:


Curious to know why not just use a dict inplace of a list for _default_commands. Would definitely save you the trouble of walking the list everytime

initially it was a dict, but then i realized strict string comparison was not enough for I need to do and changed it to a sequential list of regular expressions so each time I do a sequential match.

now, most of those regular expressions are simple strings so at some point I considered having a dict search followed by a sequential regex match, but the tests actually run fast enough as it is, so I gave up the idea.

Improve setup of cgroups

d521d6c

narrieta requested review from larohra, pgombar and ZhidongPeng as code owners May 28, 2020 03:00

narrieta changed the title ~~Improvement in setup of cgroups~~ Improvements in setup of cgroups May 28, 2020

Merge branch 'develop' into cgroups

263fafd

narrieta commented May 28, 2020

View reviewed changes

narrieta added 2 commits May 28, 2020 09:04

enable unit test

9624413

Cleanup CGroupConfigurator

2188dbb

pgombar previously approved these changes May 28, 2020

View reviewed changes

Use correct variable name

37e7e8d

narrieta dismissed pgombar’s stale review via 37e7e8d May 29, 2020 00:09

Merge branch 'develop' into cgroups

1821e43

pgombar approved these changes May 29, 2020

View reviewed changes

larohra reviewed May 29, 2020

View reviewed changes

larohra approved these changes May 29, 2020

View reviewed changes

Merge branch 'develop' into cgroups

9c2a202

narrieta merged commit c881efb into Azure:develop May 30, 2020

narrieta deleted the cgroups branch May 30, 2020 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements in setup of cgroups #1896

Improvements in setup of cgroups #1896

narrieta commented May 28, 2020

codecov bot commented May 28, 2020 •

edited

narrieta May 28, 2020

narrieta May 28, 2020 •

edited

narrieta May 28, 2020 •

edited

narrieta May 28, 2020

narrieta May 28, 2020

narrieta May 28, 2020

pgombar left a comment

pgombar May 28, 2020

narrieta May 28, 2020

pgombar May 28, 2020

narrieta May 28, 2020

pgombar May 28, 2020

narrieta May 29, 2020

larohra May 29, 2020

narrieta May 30, 2020

larohra left a comment

larohra May 29, 2020

narrieta May 30, 2020 •

edited

Improvements in setup of cgroups #1896

Improvements in setup of cgroups #1896

Conversation

narrieta commented May 28, 2020

codecov bot commented May 28, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

narrieta May 28, 2020 • edited

Choose a reason for hiding this comment

narrieta May 28, 2020 • edited

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

pgombar left a 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

larohra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta May 30, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented May 28, 2020 •

edited

narrieta May 28, 2020 •

edited

narrieta May 28, 2020 •

edited

narrieta May 30, 2020 •

edited