Enable Periodic Log Collection in systemd distros #2295

kevinclark19a · 2021-07-06T21:08:02Z

Description

Set the default configuration to enable periodic log collection.

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 · 2021-07-06T21:34:10Z

Codecov Report

Merging #2295 (44c8f47) into develop (f6699cd) will decrease coverage by 0.03%.
The diff coverage is 78.94%.

@@             Coverage Diff             @@
##           develop    #2295      +/-   ##
===========================================
- Coverage    70.71%   70.68%   -0.04%     
===========================================
  Files           96       96              
  Lines        13952    13940      -12     
  Branches      2001     1998       -3     
===========================================
- Hits          9866     9853      -13     
- Misses        3644     3647       +3     
+ Partials       442      440       -2

Impacted Files	Coverage Δ
azurelinuxagent/ga/collect_logs.py	`82.26% <62.50%> (-1.41%)`	⬇️
azurelinuxagent/common/cgroupconfigurator.py	`70.50% <87.50%> (+0.09%)`	⬆️
azurelinuxagent/agent.py	`57.43% <100.00%> (-0.77%)`	⬇️
azurelinuxagent/common/event.py	`85.22% <0.00%> (-0.69%)`	⬇️
azurelinuxagent/ga/collect_telemetry_events.py	`90.06% <0.00%> (+0.34%)`	⬆️

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 f6699cd...44c8f47. Read the comment docs.

…abled

…uxAgent into enable_log-collector

nagworld9 · 2021-07-08T22:35:24Z

azurelinuxagent/ga/collect_logs.py

@@ -169,18 +169,12 @@ def _collect_logs(self):

        systemd_cmd = [
            "systemd-run", "--unit={0}".format(logcollector.CGROUPS_UNIT),
-            "--slice={0}".format(cgroupconfigurator.AZURE_SLICE)
+            "--slice={0}".format(cgroupconfigurator.LOGCOLLECTOR_SLICE), "--scope"


Just to make sure, are we managing logcollector under azure.slice/azure-walinuxagent-periodic_logcollector.slice/collect-logs.scope If so, what I read is systemd-run creates the scope under the system slice by default as per our implementation for extension slice
https://github.com/Azure/WALinuxAgent/blob/develop/azurelinuxagent/common/cgroupapi.py#L258.
May be either defining slice name in contents or something like creating file under azure.slice should do the trick?

As per our offline sync, systemd-run is unfortunately a bit wild with how it generates the hierarchical slices: we're passing azure-walinuxagent-periodic_logcollector.slice as the slice name here, and systemd-run translates that to azure.slice/azure-walinuxagent.slice/azure-walinuxagent-periodic_logcollector.slice by using the -s in the original as delimiters.

nagworld9 · 2021-07-08T22:37:12Z

azurelinuxagent/agent.py

-            cpu_slice_matches, cpu_unit_matches = validate_cgroup_path(cpu_cgroup_path)
-            memory_slice_matches, memory_unit_matches = validate_cgroup_path(memory_cgroup_path)
+            cpu_slice_matches, cpu_unit_matches = validate_cgroup_path(cpu_cgroup_path,
+                cgroupconfigurator.LOGCOLLECTOR_SLICE, None)


Any reason why None here?

related to @nagworld9's question: why is there an asymmetry between the cpu & memory paths? (i.e. None vs CGROUPS_UNIT) thanks

@nagworld9, this is a reflection of systemd v245's behavior on my ubuntu 20.04 dev VM; the path within the process cgroup file is azure.slice/azure-walinuxagent.slice/azure-walinuxagent-periodic_logcollector.slice (note the missing unit name, collect-logs.scope). It isn't universal though, unfortunately, as I don't believe this asymmetry exists on ubuntu 18.04 (off the top of my head, systemd v237).

@nagworld9 & @narrieta: I need to do more research here as I'm not quite sure if this is just a configuration error, but barring that, we might need to abstract this check into osutil or something.

narrieta · 2021-07-08T23:50:39Z

azurelinuxagent/agent.py

                if path is None:
                    return False, False

-                regex_match = re.match(r'^(?P<slice>.*)/(?P<unit>.*)$', path)
+                # '(.*/)?': process slice may be nested in other, heirarchical slices.


small typo: "heirarchical"

azurelinuxagent/agent.py

narrieta · 2021-07-08T23:56:22Z

azurelinuxagent/agent.py

+            cpu_slice_matches, cpu_unit_matches = validate_cgroup_path(cpu_cgroup_path,
+                cgroupconfigurator.LOGCOLLECTOR_SLICE, None)
+            memory_slice_matches, memory_unit_matches = validate_cgroup_path(memory_cgroup_path,
+                cgroupconfigurator.LOGCOLLECTOR_SLICE, logcollector.CGROUPS_UNIT)


why using print? is this leftover debug code or should those messages go to the agent's log?

prints get routed, undecorated, to the log file when invoked by the periodic task and (obviously) printed out to stdout on manual invocation. (so the second)

narrieta · 2021-07-08T23:57:25Z

azurelinuxagent/common/cgroupconfigurator.py

@@ -290,6 +305,12 @@ def __setup_azure_slice():
            if not os.path.exists(vmextensions_slice):
                files_to_create.append((vmextensions_slice, _VMEXTENSIONS_SLICE_CONTENTS))

+            if not os.path.exists(logcollector_slice):
+                from azurelinuxagent.ga.collect_logs import CollectLogsHandler


can we move this import to the top of the file?

I was worried about a cyclical import (as we import cgroupconfigurator into ga.collect_logs); I haven't actually tried it yet, though, so I can do so and update here.

Update: this indeed does cause a cyclical import, so I'm inclined to leave it as is.

OK, but coming to think about it, the CGroupConfigurator should not have a dependency on the CollectLogsHandler.

Seems like you just need the value of a few constants? Maybe move those to this module, or move them to a module that both CGroupConfigurator and CollectLogsHandler depend on?

…og-collector

narrieta · 2021-07-16T16:48:34Z

azurelinuxagent/agent.py

+
+                # '(.*/)?': process slice may be nested in other, hierarchical slices.
+                # '[^\s./]*': process slice can't contain spaces, periods, or slashes.
+                slice_regex = r'(.*/)?(?P<slice>[^\s./]*.slice)'


I don't think we need such a generic regex for this check. Would it be enough to check that collector is within cgroupconfigurator.LOGCOLLECTOR_SLICE?

narrieta · 2021-07-16T16:49:25Z

azurelinuxagent/agent.py


                slice_group, unit_group = regex_match.group("slice", "unit")

-                slice_matches = (slice_group == cgroupconfigurator.AZURE_SLICE)
-                unit_matches = (unit_group == logcollector.CGROUPS_UNIT)
+                if unit_group != expected_unit:


If we just check against cgroupconfigurator.LOGCOLLECTOR_SLICE I think this check can also be omitted

Or maybe I am misunderstanding the reason for these 2 checks? The first check silently returns False, while this emits telemetry.

Note that the existing code for this function is more complicated because the collector was not on its own slice. Seems to me that most of this function can be replaced with a simple check for cgroupconfigurator.LOGCOLLECTOR_SLICE

If the process fails the first check, it should exit with INVALID_CGROUP_ERROR. The idea behind this check was to get telemetry for when we run into the abnormal /proc/self/cgroup content.

what is the first check doing?

never mind, misread the code... this is a helper function...

but i still think this is the case:

Note that the existing code for this function is more complicated because the collector was not on its own slice. Seems to me that most of this function can be replaced with a simple check for cgroupconfigurator.LOGCOLLECTOR_SLICE

…og-collector

kevinclark19a requested review from dhivyaganesan, larohra, nagworld9, narrieta and ZhidongPeng as code owners July 6, 2021 21:08

Flipped switch for ubuntu.

8fd744c

kevinclark19a force-pushed the enable_log-collector branch from a03b983 to 8fd744c Compare July 6, 2021 21:30

keclar and others added 7 commits July 6, 2021 14:36

Change log collection requirements from systemd present -> cgroups en…

f2e1c94

…abled

fixed some tests, updated confs

97470c2

Merge branch 'develop' into enable_log-collector

eb5a00d

enabling permanent slice for log collector

af4a195

Merge branch 'enable_log-collector' of github.com:kevinclark19a/WALin…

00419ad

…uxAgent into enable_log-collector

Fix broken test, remove now-unnecessary test

13c9571

removed unused import

1f17366

nagworld9 reviewed Jul 8, 2021

View reviewed changes

narrieta reviewed Jul 8, 2021

View reviewed changes

azurelinuxagent/agent.py Outdated Show resolved Hide resolved

narrieta reviewed Jul 8, 2021

View reviewed changes

keclar added 7 commits July 12, 2021 10:49

Merge branch 'develop' of github.com:Azure/WALinuxAgent into enable_l…

b86aa8d

…og-collector

addressed various comments, committing now to run DCR testing.

e9c8ee6

Merge branch 'develop' of github.com:Azure/WALinuxAgent into enable_l…

3b8464b

…og-collector

possible fix to track systemd-run command with cgroups

8093b23

Merge branch 'develop' of github.com:Azure/WALinuxAgent into enable_l…

d34ac02

…og-collector

fixed pylint error

d723dc6

fixed error logging

e6ba5d1

narrieta reviewed Jul 16, 2021

View reviewed changes

simplified cgroup validation logic

6e97fde

keclar added 3 commits July 19, 2021 13:54

removed unused import

a9258f7

Merge branch 'develop' of github.com:Azure/WALinuxAgent into enable_l…

f16b21a

…og-collector

moved log collector limits into the cgroupconfigurator module

a0b60bf

narrieta previously approved these changes Jul 19, 2021

View reviewed changes

removed import for debugging

44c8f47

kevinclark19a dismissed narrieta’s stale review via 44c8f47 July 19, 2021 22:25

narrieta approved these changes Jul 20, 2021

View reviewed changes

nagworld9 approved these changes Jul 20, 2021

View reviewed changes

kevinclark19a merged commit 6492ebd into Azure:develop Jul 20, 2021

kevinclark19a deleted the enable_log-collector branch December 13, 2021 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Periodic Log Collection in systemd distros #2295

Enable Periodic Log Collection in systemd distros #2295

kevinclark19a commented Jul 6, 2021

codecov bot commented Jul 6, 2021 •

edited

Loading

nagworld9 Jul 8, 2021 •

edited

Loading

kevinclark19a Jul 9, 2021

nagworld9 Jul 8, 2021

narrieta Jul 8, 2021 •

edited

Loading

kevinclark19a Jul 9, 2021

narrieta Jul 8, 2021

narrieta Jul 8, 2021

kevinclark19a Jul 9, 2021

narrieta Jul 8, 2021

kevinclark19a Jul 9, 2021

kevinclark19a Jul 15, 2021

narrieta Jul 16, 2021

narrieta Jul 16, 2021

narrieta Jul 16, 2021

narrieta Jul 16, 2021

narrieta Jul 16, 2021

kevinclark19a Jul 16, 2021

narrieta Jul 16, 2021

narrieta Jul 16, 2021

Enable Periodic Log Collection in systemd distros #2295

Enable Periodic Log Collection in systemd distros #2295

Conversation

kevinclark19a commented Jul 6, 2021

Description

PR information

Quality of Code and Contribution Guidelines

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

nagworld9 Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Jul 8, 2021 • 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

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

codecov bot commented Jul 6, 2021 •

edited

Loading

nagworld9 Jul 8, 2021 •

edited

Loading

narrieta Jul 8, 2021 •

edited

Loading