Disable cgroups when daemon is setup incorrectly #1688

narrieta · 2019-10-31T18:45:20Z

Disable cgroups when, under systemd, the daemon's PID has been added to the file system explicitly.

Also disable when we cannot move the PID to walinuxagent.service (when not under systemd)

This change is

codecov · 2019-10-31T18:49:13Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@6211315). Click here to learn what that means.
The diff coverage is 93.22%.

@@            Coverage Diff             @@
##             develop    #1688   +/-   ##
==========================================
  Coverage           ?   67.61%           
==========================================
  Files              ?       80           
  Lines              ?    11477           
  Branches           ?     1608           
==========================================
  Hits               ?     7760           
  Misses             ?     3375           
  Partials           ?      342

Impacted Files	Coverage Δ
azurelinuxagent/daemon/main.py	`72.91% <ø> (ø)`
azurelinuxagent/common/cgroupstelemetry.py	`94.69% <ø> (ø)`
azurelinuxagent/common/exception.py	`94.56% <100%> (ø)`
azurelinuxagent/ga/update.py	`88.42% <100%> (ø)`
azurelinuxagent/common/cgroupconfigurator.py	`90.1% <88.88%> (ø)`
azurelinuxagent/common/cgroupapi.py	`84.7% <94.59%> (ø)`

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 6211315...50a0633. Read the comment docs.

narrieta · 2019-10-31T18:53:51Z

azurelinuxagent/common/cgroupapi.py

-                msg = 'Error in cgroup controller "{0}": {1}. {2}'.format(controller, ustr(e), message)
-                logger.warn(msg)
-                errors.append(msg)
-        return errors


now there is no need for this function to report errors to the caller

tests/common/test_cgroupapi.py

narrieta · 2019-10-31T19:01:06Z

tests/common/test_cgroupapi.py

 from tests.tools import *


-class CGroupsApiTestCase(AgentTestCase):
-    def setUp(self):
+class _MockedFileSystemTestCase(AgentTestCase):


refactored common mocks to this class

narrieta · 2019-10-31T19:01:52Z

tests/common/test_cgroupapi.py

        root_slice_name = SystemdCgroupsApi()._get_extensions_slice_root_name()
        self.assertEqual(root_slice_name, "system-walinuxagent.extensions.slice")

-    def test_it_should_return_extension_slice_name(self):
+    def test_get_extension_slice_name_should_return_the_slice_for_the_given_extension(self):


replaced 'it' with the actual test subject in these method names

pgombar

LGTM with one more general question: should we explicitly check if the daemon's (and extension handler's) PID is in the correct place, and, if not, also disable cgroups? Right now we're just checking if the PID is in the wrong place, not if it's in the right place, if that makes sense.

So, if the daemon and/or exthandler's PID is not in walinuxagent.service, then traverse the /proc/ filesystem to find where it is, log it, and disable cgroups. This way, we might identify more wrong places where our PIDs might have wandered off to.

pgombar · 2019-11-01T06:58:12Z

azurelinuxagent/common/cgroupapi.py

+                        operation(controller, daemon_pid)
+        finally:
+            for _, cgroup in legacy_cgroups:
+                logger.info('Removing {0}', cgroup)


nit: Suggestion to make the message Removing legacy cgroup {0} instead to make it even clearer what's getting removed when just looking at the logs.

the whole path is in the message. a few lines up in the log there would be a message identifying the path as a legacy group (logged at line 132)

pgombar · 2019-11-01T07:04:19Z

azurelinuxagent/common/cgroupconfigurator.py

@@ -79,7 +79,7 @@ def enable(self):
        def disable(self):
            self._enabled = False

-        def _invoke_cgroup_operation(self, operation, error_message):
+        def _invoke_cgroup_operation(self, operation, error_message, on_error=None):
            """
            Ensures the given operation is invoked only if cgroups are enabled and traps any errors on the operation.
            """


Can you update the comment explaining what on_error does and why it's there?

doesn't the name imply a callback to execute when an error occurs? :)

pgombar · 2019-11-01T07:12:01Z

azurelinuxagent/daemon/main.py

@@ -69,7 +69,6 @@ def run(self, child_args=None):
        self.initialize_environment()

        CGroupConfigurator.get_instance().create_agent_cgroups(track_cgroups=False)
-        CGroupConfigurator.get_instance().cleanup_old_cgroups()


Why not have the daemon clean up?

It is not needed. Or did you have an scenario in mind when it was added?

pgombar · 2019-11-01T07:13:02Z

tests/common/test_cgroupapi.py

 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.utils import shellutil
 from nose.plugins.attrib import attr
+from tests.utils.cgroups_tools import CGroupsTools
 from tests.tools import *


Refactor import *?

Looked into it when I started modifying this test but backed off. There is some funny stuff there that is going to require more time than I have in the current release.

Let's see who in the team gets there first.

pgombar · 2019-11-01T07:14:38Z

tests/common/test_cgroupapi.py

-        os.mkdir(os.path.join(self.cgroups_file_system_root, "cpu"))
-        os.mkdir(os.path.join(self.cgroups_file_system_root, "memory"))
+            (message_format, controller, error, message) = args
+            self.assertEquals(message_format, 'Error in cgroup controller "{0}": {1}. {2}')


So this will abstract away whatever's in {}? Cool, did not know this.

This was actually needed because I changed the statement that logs this message. Previously it was logging a string formatted with string.format, now it lets the logger do the formatting so the first argument is actually the formatting string.

pgombar · 2019-11-01T07:20:21Z

tests/common/test_cgroupconfigurator.py

 from azurelinuxagent.common.cgroupconfigurator import CGroupConfigurator
 from azurelinuxagent.common.cgroupstelemetry import CGroupsTelemetry
 from azurelinuxagent.common.exception import CGroupsException
 from azurelinuxagent.common.osutil.default import DefaultOSUtil
+from tests.utils.cgroups_tools import CGroupsTools
 from tests.tools import *


Another import *

pgombar · 2019-11-01T07:29:30Z

azurelinuxagent/common/cgroupapi.py

+            logger.info("Writing daemon's PID ({0}) to {1}", daemon_pid, new_path)
+            fileutil.append_file(os.path.join(new_path, "cgroup.procs"), daemon_pid)
+            msg = "Moved daemon's PID from legacy cgroup to {0}".format(new_path)
+            add_event(AGENT_NAME, version=CURRENT_VERSION, op=WALAEventOperation.CGroupsCleanUp, is_success=True, message=msg)


Can we explicitly say that the collection of resource usage data is still enabled? I'm looking at it from the perspective of querying for this event and parsing what the different event messages mean.

we can talk offline... not sure I understand how that would improve parsing

pgombar · 2019-11-01T07:30:26Z

azurelinuxagent/common/exception.py

@@ -55,7 +55,11 @@ class CGroupsException(AgentError):

    def __init__(self, msg, inner=None):
        super(AgentError, self).__init__(msg, inner)
+        # TODO: AgentError should set the message - investigate whether doing it there would break anything


So... would it? :)

Dunno; no time to check :)

Doing it in AgentError would change the formatting of all exceptions. Need to spend some time checking what would happen then.

narrieta · 2019-11-01T16:56:56Z

@pgombar

should we explicitly check if the daemon's (and extension handler's) PID is in the correct place, and, if not, also disable cgroups?

We could add more checks on a later release

vrdmr

Minor comments.

azurelinuxagent/common/cgroupapi.py

azurelinuxagent/ga/update.py

Disable cgroups when daemon is setup incorrectly

9fd59f0

narrieta requested review from vrdmr, pgombar and larohra October 31, 2019 18:45

narrieta commented Oct 31, 2019

View reviewed changes

tests/common/test_cgroupapi.py Outdated Show resolved Hide resolved

narrieta commented Oct 31, 2019

View reviewed changes

Remove unused parameter

37e6518

pgombar approved these changes Nov 1, 2019

View reviewed changes

Stop tracking after disabling cgroups

50a0633

vrdmr approved these changes Nov 1, 2019

View reviewed changes

azurelinuxagent/common/cgroupapi.py Show resolved Hide resolved

azurelinuxagent/ga/update.py Show resolved Hide resolved

narrieta merged commit 68176db into Azure:develop Nov 4, 2019

This was referenced Nov 6, 2019

Set message for cgroup exceptions #1700

Merged

Remove "import *" from cgroup tests #1703

Merged

narrieta deleted the systemd-check branch December 3, 2019 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable cgroups when daemon is setup incorrectly #1688

Disable cgroups when daemon is setup incorrectly #1688

narrieta commented Oct 31, 2019 •

edited by vrdmr

Loading

codecov bot commented Oct 31, 2019 •

edited

Loading

narrieta Oct 31, 2019

narrieta Oct 31, 2019

narrieta Oct 31, 2019

pgombar left a comment

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019 •

edited

Loading

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019

pgombar Nov 1, 2019

narrieta Nov 1, 2019

narrieta commented Nov 1, 2019

vrdmr left a comment

Disable cgroups when daemon is setup incorrectly #1688

Disable cgroups when daemon is setup incorrectly #1688

Conversation

narrieta commented Oct 31, 2019 • edited by vrdmr Loading

codecov bot commented Oct 31, 2019 • 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

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

narrieta Nov 1, 2019 • 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

narrieta commented Nov 1, 2019

vrdmr left a comment

Choose a reason for hiding this comment

narrieta commented Oct 31, 2019 •

edited by vrdmr

Loading

codecov bot commented Oct 31, 2019 •

edited

Loading

narrieta Nov 1, 2019 •

edited

Loading