Use controller mount point for extension cgroup path #1899
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1899 +/- ##
===========================================
- Coverage 69.42% 69.37% -0.06%
===========================================
Files 85 85
Lines 11789 11811 +22
Branches 1653 1658 +5
===========================================
+ Hits 8185 8194 +9
- Misses 3239 3249 +10
- Partials 365 368 +3
Continue to review full report at Codecov.
|
| if cpu_cgroup_mountpoint is None: | ||
| logger.info("The CPU controller is not mounted; will not track resource usage") | ||
| else: | ||
| cpu_cgroup_path = os.path.join(cpu_cgroup_mountpoint, cgroup_relative_path) |
There was a problem hiding this comment.
Just curious, would this path and the path from get_process_cgroup_paths be same?
There was a problem hiding this comment.
Yes. I was planning on using that function, but then I realize I do not have the process of the extension (I do have the process id of systemd-run, which the agent calls, but not of the extension itself.)
The documentation of systemd-run specifies that the cgroup is created under the system slice (unless changed by the --slice argument) so this should be safe.
There was a problem hiding this comment.
Ahh ok makes sense.
I have another question (possibly outside the scope of this PR), why dont we create a new slice for each extension when invoking them? Using the --slice argument that you mentioned? Or were we actually doing that at some point but reverted to the system slice for some unknown issues?
There was a problem hiding this comment.
Yes, we already have the code that creates the slices and starts extensions in them, but it is not enabled because some versions of systemd simply ignore the --slice argument. As I start working on extensions I will add code to use those features when they are available.
| stdout=stdout, | ||
| stderr=stderr, | ||
| error_code=error_code) | ||
| process_output = self._cgroups_api.start_extension_command(extension_name, |
There was a problem hiding this comment.
You should change the return signature of azurelinuxagent.common.cgroupapi.FileSystemCgroupsApi.start_extension_command too since it still returns extension_cgroups, process_output.
There was a problem hiding this comment.
FileSystemCgroupsApi is at this point dead code. I am keeping it there because I will use some of that code when I get to distros without systemd
| @@ -64,13 +64,14 @@ | |||
| ''' | |||
| Running scope as unit: TEST_UNIT.scope | |||
| Thu 28 May 2020 07:25:55 AM PDT | |||
| ''') | |||
| '''), | |||
There was a problem hiding this comment.
yes, makes adding more items a little simple
* Use controller mount point for extension cgroup path * Fixed typos Co-authored-by: narrieta <narrieta>
A small change in how we create the path for extensions cgroups. Instead of a hardcoded path now we use the mount point for the controllers.
Also, some test updates.