Defined API for initialization of cgroups #1530

narrieta · 2019-05-17T20:17:55Z

Description

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-17T22:36:18Z

Codecov Report

Merging #1530 into cgroups will increase coverage by 0.17%.
The diff coverage is 44.71%.

@@             Coverage Diff             @@
##           cgroups    #1530      +/-   ##
===========================================
+ Coverage     60.9%   61.07%   +0.17%     
===========================================
  Files           77       78       +1     
  Lines        10893    10919      +26     
  Branches      1560     1554       -6     
===========================================
+ Hits          6634     6669      +35     
+ Misses        3944     3935       -9     
  Partials       315      315

Impacted Files	Coverage Δ
azurelinuxagent/common/cgroupstelemetry.py	`32.25% <ø> (+6.98%)`	⬆️
azurelinuxagent/ga/monitor.py	`60.55% <ø> (+2.27%)`	⬆️
azurelinuxagent/ga/update.py	`88.31% <100%> (+0.11%)`	⬆️
azurelinuxagent/daemon/main.py	`72.91% <100%> (ø)`	⬆️
azurelinuxagent/common/cgroupapi.py	`34.32% <34.32%> (ø)`
azurelinuxagent/common/cgroupconfigurator.py	`36.47% <50%> (+4.54%)`	⬆️

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 df97eec...0448d34. Read the comment docs.

codecov · 2019-05-17T22:36:18Z

Codecov Report

Merging #1530 into cgroups will increase coverage by 0.17%.
The diff coverage is 44.71%.

@@             Coverage Diff             @@
##           cgroups    #1530      +/-   ##
===========================================
+ Coverage     60.9%   61.07%   +0.17%     
===========================================
  Files           77       78       +1     
  Lines        10893    10919      +26     
  Branches      1560     1554       -6     
===========================================
+ Hits          6634     6669      +35     
+ Misses        3944     3935       -9     
  Partials       315      315

Impacted Files	Coverage Δ
azurelinuxagent/common/cgroupstelemetry.py	`32.25% <ø> (+6.98%)`	⬆️
azurelinuxagent/ga/monitor.py	`60.55% <ø> (+2.27%)`	⬆️
azurelinuxagent/ga/update.py	`88.31% <100%> (+0.11%)`	⬆️
azurelinuxagent/daemon/main.py	`72.91% <100%> (ø)`	⬆️
azurelinuxagent/common/cgroupapi.py	`34.32% <34.32%> (ø)`
azurelinuxagent/common/cgroupconfigurator.py	`36.47% <50%> (+4.54%)`	⬆️

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 df97eec...feee481. Read the comment docs.

pgombar

LGTM with a couple of comments and suggestions.

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

azurelinuxagent/common/cgroupapi.py

+                else:
+                    raise
+
+    def _foreach_controller(self, operation, message):


This method can be static.

pgombar · 2019-05-18T00:39:03Z

azurelinuxagent/common/cgroupconfigurator.py

+        Determine if systemd is managing system services. If this process (presumed to be the agent) is in a CPU cgroup
+        that looks like one created by systemd, we can assume systemd is in use.
+
+        NOTE: This function has the side effect of mounting the cgroups file system if it is not already mounted.


When does this happen? I don't see where the mounting happens here.

comment was for a previous iteration of the code... i'll remove it

pgombar · 2019-05-18T00:40:40Z

azurelinuxagent/common/cgroupconfigurator.py

+                message=status,
+                log_event=False)
+
+        def _invoke_cgroup_operation(self, operation, error_message):


This can be static.

thanks, fixed

azurelinuxagent/common/cgroupconfigurator.py

narrieta · 2019-05-20T16:58:52Z

@pgombar thanks for your comments. I updated the code, could you take a look at the last commit?

pgombar

LGTM

narrieta added 3 commits May 16, 2019 13:31

Created CGroupsApi

5fb0369

tmp

da416d2

Define API for initialization of cgroups

e5b02ed

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

Reenable cgroup telemetry

0448d34

pgombar approved these changes May 18, 2019

View reviewed changes

vrdmr reviewed May 20, 2019

View reviewed changes

azurelinuxagent/common/cgroupconfigurator.py Show resolved Hide resolved

Code review comments

feee481

pgombar approved these changes May 20, 2019

View reviewed changes

narrieta merged commit ab8f394 into Azure:cgroups May 20, 2019

narrieta deleted the cgroups_api branch May 20, 2019 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defined API for initialization of cgroups #1530

Defined API for initialization of cgroups #1530

narrieta commented May 17, 2019 •

edited by vrdmr

Loading

codecov bot commented May 17, 2019

codecov bot commented May 17, 2019 •

edited

Loading

pgombar left a comment

pgombar May 18, 2019

narrieta May 20, 2019

pgombar May 18, 2019

narrieta May 20, 2019

pgombar May 18, 2019

narrieta May 20, 2019

narrieta commented May 20, 2019

pgombar left a comment

Defined API for initialization of cgroups #1530

Defined API for initialization of cgroups #1530

Conversation

narrieta commented May 17, 2019 • edited by vrdmr Loading

Description

PR information

Quality of Code and Contribution Guidelines

codecov bot commented May 17, 2019

Codecov Report

codecov bot commented May 17, 2019 • edited Loading

Codecov Report

pgombar left a comment

Choose a reason for hiding this comment

pgombar May 18, 2019

Choose a reason for hiding this comment

narrieta May 20, 2019

Choose a reason for hiding this comment

pgombar May 18, 2019

Choose a reason for hiding this comment

narrieta May 20, 2019

Choose a reason for hiding this comment

pgombar May 18, 2019

Choose a reason for hiding this comment

narrieta May 20, 2019

Choose a reason for hiding this comment

narrieta commented May 20, 2019

pgombar left a comment

Choose a reason for hiding this comment

narrieta commented May 17, 2019 •

edited by vrdmr

Loading

codecov bot commented May 17, 2019 •

edited

Loading