API for extension cgroups #1533

narrieta · 2019-05-20T22:43:08Z

Description

Completed API for managing extension cgroups

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.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

This change is

codecov · 2019-05-20T22:49:32Z

Codecov Report

Merging #1533 into cgroups will decrease coverage by 0.2%.
The diff coverage is 39.13%.

@@             Coverage Diff             @@
##           cgroups    #1533      +/-   ##
===========================================
- Coverage    61.07%   60.87%   -0.21%     
===========================================
  Files           78       78              
  Lines        10919    10982      +63     
  Branches      1554     1555       +1     
===========================================
+ Hits          6669     6685      +16     
- Misses        3935     3984      +49     
+ Partials       315      313       -2

Impacted Files	Coverage Δ
azurelinuxagent/common/cgroupapi.py	`30.97% <21.81%> (-3.36%)`	⬇️
azurelinuxagent/common/cgroupconfigurator.py	`36.46% <54.16%> (-0.01%)`	⬇️
azurelinuxagent/ga/exthandlers.py	`81.54% <84.61%> (+0.11%)`	⬆️
azurelinuxagent/common/cgroupstelemetry.py	`30.64% <0%> (-1.62%)`	⬇️

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 ab8f394...a99463a. Read the comment docs.

pgombar

LGTM

pgombar · 2019-05-21T00:12:25Z

azurelinuxagent/common/cgroupapi.py

@@ -81,45 +88,121 @@ def _foreach_controller(operation, message):
            else:
                operation(controller)

+    def _get_extension_cgroups_root_path(self, controller):


This can be static.

pgombar · 2019-05-21T00:12:42Z

azurelinuxagent/common/cgroupapi.py

+
+        return os.path.join(extensions_root, cgroup_name)
+
+    def _add_process_to_cgroup(self, pid, cgroup_path):


This can be static.

pgombar · 2019-05-21T00:18:24Z

azurelinuxagent/common/cgroupconfigurator.py

+                def add_to_cgroup():
+                    self.cgroups_api.add_process_to_extension_cgroup(extension_name, os.getpid())
+
+                self._invoke_cgroup_operation(add_to_cgroup, "Failed add extension '{0}' to its cgroup; resource usage will not be tracked".format(extension_name))


nit: "Failed add extension" -> "Failed to add extension"

pgombar · 2019-05-21T00:22:55Z

azurelinuxagent/ga/exthandlers.py

@@ -822,7 +823,7 @@ def download(self):
        self.report_event(message="Download succeeded", duration=duration)

    def initialize(self):
-        self.logger.info("Initialize extension directory")
+        self.logger.info("Initialize extension")


Since you're updating this, can we add the extension name here? The message seems ambiguous without it.

pgombar · 2019-05-21T01:03:48Z

azurelinuxagent/common/cgroupapi.py

@@ -38,6 +39,12 @@ def create_agent_cgroups(self):
    def create_extension_cgroups_root(self):
        raise NotImplementedError()

+    def create_extension_cgroups(self):


You're missing the parameter extension_name in the interface signature. The implementing methods use it.

thank you, fixed

narrieta · 2019-05-21T15:24:19Z

@pgombar thanks for the review, could you check the updates in the last commit? thanks

API for extension cgroups

36254b1

narrieta requested review from vrdmr, pgombar and larohra May 20, 2019 22:43

pgombar approved these changes May 21, 2019

View reviewed changes

pgombar reviewed May 21, 2019

View reviewed changes

Code review feedback

47d2772

Remove reference to self from static method

a99463a

narrieta merged commit 8d02446 into Azure:cgroups May 21, 2019

narrieta deleted the extension_cgroups branch May 21, 2019 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API for extension cgroups #1533

API for extension cgroups #1533

narrieta commented May 20, 2019 •

edited by vrdmr

Loading

codecov bot commented May 20, 2019 •

edited

Loading

pgombar left a comment

pgombar May 21, 2019

narrieta May 21, 2019

pgombar May 21, 2019

narrieta May 21, 2019

pgombar May 21, 2019

narrieta May 21, 2019

pgombar May 21, 2019

narrieta May 21, 2019

pgombar May 21, 2019

narrieta May 21, 2019

narrieta commented May 21, 2019


		return os.path.join(extensions_root, cgroup_name)

		def _add_process_to_cgroup(self, pid, cgroup_path):

API for extension cgroups #1533

API for extension cgroups #1533

Conversation

narrieta commented May 20, 2019 • edited by vrdmr Loading

Description

PR information

Quality of Code and Contribution Guidelines

codecov bot commented May 20, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta commented May 21, 2019

narrieta commented May 20, 2019 •

edited by vrdmr

Loading

codecov bot commented May 20, 2019 •

edited

Loading