Cgroup support for monitoring agent and extensions #1199
Conversation
azurelinuxagent/common/cgroups.py
Outdated
| self.msg = msg | ||
| if CGroups.enabled(): | ||
| pid = os.getpid() | ||
| # logger.verbose("[{0}] Disabling cgroup support: {1}".format(pid, msg)) |
azurelinuxagent/common/cgroups.py
Outdated
| """ | ||
| results = None | ||
| try: | ||
| results = fileutil.read_file('/proc/stat') |
There was a problem hiding this comment.
I think my previous comments on this were lost. Why isn't this method and the callers of this method instead calling something in osutil?
There was a problem hiding this comment.
Sheer laziness, and "but it's only used on Linux and only for one thing and I'll generalize later". But you're right, the semantically meaningful operation should be exposed from osutil.
azurelinuxagent/common/cgroups.py
Outdated
|
|
||
| names_now_tracked = set(CGroupsTelemetry._tracked.keys()) | ||
| if CGroupsTelemetry.tracked_names != names_now_tracked: | ||
| now_tracking = " ".join("[{0}]".format(name) for name in names_now_tracked) |
There was a problem hiding this comment.
Please consider sorting the result to make comparison easier for humans.
azurelinuxagent/common/cgroups.py
Outdated
| :rtype: Bool | ||
| """ | ||
| if CGroups._use_systemd is None: | ||
| hierarchy = METRIC_HIERARCHIES[0] |
There was a problem hiding this comment.
I know that [0] is cpu, but it isn't a very constant.
There was a problem hiding this comment.
I don't care which hierarchy it is; it just has to be one of the hierarchies from which we're selecting metrics. It could be "memory" for all I care, and the code will work.
| return CGroups._use_systemd | ||
|
|
||
| @staticmethod | ||
| def _try_mkdir(path): |
There was a problem hiding this comment.
Lost comments. Seems useful on its own. Consider moving to osutil for a future PR.
There was a problem hiding this comment.
I'll consider it... later. :-)
| :param hierarchy_id: str | ||
| :return: str | ||
| """ | ||
| cgroup_paths = fileutil.read_file("/proc/self/cgroup") |
There was a problem hiding this comment.
Lost comments?
I believe this is an example of what you're parsing, correct? If so, please add in a comment.
3:cpu,cpuacct:/user.slice
Please add an example. Please consider encapsulating this parsing to return an object to make code read easier for a future PR.
There was a problem hiding this comment.
A person could grow old encapsulating one-off text file parsing into classes for potential future reuse... :-)
| """ | ||
| cgroup_states = fileutil.read_file("/proc/cgroups") | ||
| for entry in cgroup_states.splitlines(): | ||
| fields = entry.split('\t') |
There was a problem hiding this comment.
Thank you CGroups for using '\t' and ':' for separation.
There was a problem hiding this comment.
Please consider adding a comment on what you're parsing.
#subsys_name hierarchy num_cgroups enabled
cpuset 9 1 1
cpu 3 74 1
cpuacct 3 74 1
blkio 10 74 1
memory 12 111 1
devices 6 74 1
freezer 11 1 1
net_cls 2 1 1
perf_event 8 1 1
net_prio 2 1 1
hugetlb 5 1 1
pids 4 75 1
rdma 7 1 1
Please consider returning an object with each field called out to make it easier for readers in a future PR.
There was a problem hiding this comment.
Should've done this right the first time, I admit; if I believe the "we write code for humans to understand" maxim, then class encapsulation of parsed-from-text-file data is a best practice.
azurelinuxagent/common/cgroups.py
Outdated
|
|
||
| def get_file(self, hierarchy, file_name): | ||
| """ | ||
| Retrieve the value of a parameter from a hierarchy. |
There was a problem hiding this comment.
The method is called get_file, but that doesn't match the comment or code. Is read_file or get_file_contents better? Is this method only intended to be call by get_parameter?
There was a problem hiding this comment.
CGroups.get_file_contents() it is. Any holder of a CGroups object can legitimately call this; cgroups is so wildly varied that it's hard to know. I am pretty sure at least one DCR test for cgroups relies on this method being public.
azurelinuxagent/common/version.py
Outdated
| @@ -113,7 +113,7 @@ def get_distro(): | |||
|
|
|||
| AGENT_NAME = "WALinuxAgent" | |||
| AGENT_LONG_NAME = "Azure Linux Agent" | |||
| AGENT_VERSION = '2.2.25.1' | |||
| AGENT_VERSION = '2.2.26' | |||
There was a problem hiding this comment.
Lost comment? New version is 2.2.27.
There was a problem hiding this comment.
At the time, your comment said "make it 2.2.26", so I did. Gosh, you're so picky. :-)
Replace deprecated unittest assertions.
Passes hand smoke-tests on Ubuntu 14.04 (without systemd) and 16.04 (with systemd). Unit tests are disabled to keep Travis from trying to run them; mounting the cgroup device doesn't seem to work terribly well within containers, at least as Travis has them configured.