fix stop tracking extension cgroups #2384
Conversation
nagworld9
requested review from
dhivyaganesan,
kevinclark19a,
larohra,
narrieta and
ZhidongPeng
as code owners
nagworld9
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #2384 +/- ##
===========================================
+ Coverage 71.14% 71.19% +0.04%
===========================================
Files 97 97
Lines 14387 14398 +11
Branches 2077 2078 +1
===========================================
+ Hits 10236 10250 +14
+ Misses 3689 3685 -4
- Partials 462 463 +1
Continue to review full report at Codecov.
|
| try: | ||
| extension_slice_name = SystemdCgroupsApi.get_extension_cgroup_name(extension_name) + ".slice" | ||
| cgroup_relative_path = os.path.join('azure.slice/azure-vmextensions.slice', | ||
| extension_slice_name + ".slice") |
There was a problem hiding this comment.
There seems to be a typo, an extra .slice here -
SystemdCgroupsApi.get_extension_cgroup_name(extension_name) + ".slice" + ".slice"
If this is used in multiple other places, maybe make this a function too (something like SystemdCgroupsApi.get_extension_slice_name(extension_name)) or something like that
There was a problem hiding this comment.
Good catch! Updated.
| tracked = CGroupsTelemetry._tracked | ||
|
|
||
| self.assertFalse( | ||
| any(cg for cg in tracked.values() if cg.name == 'Microsoft.CPlat.Extension' and 'cpu' in cg.path), |
There was a problem hiding this comment.
Can you also add another stricter check for making sure the tracked.values() actually contains what you expect (this would've caught the typo introduced above)
There was a problem hiding this comment.
Actually tracked.values() should not contain whatever I'm checking. The place where I mocked test data was wrong. Thanks for the catch. I fixed all those places.
nagworld9
force-pushed
the
cgroups-test
branch
from
0e0d5ad
to
b693bc5
Compare
|
@nagworld9 Please let's try to write a Description of the PR. Thanks. |
| TODO: Memory tracking | ||
| """ | ||
| try: | ||
| extension_slice_name = SystemdCgroupsApi.get_extension_slice_name(extension_name) + ".slice" |
There was a problem hiding this comment.
With the new name, get_extension_slice_name gives the impression that one should have to append ".slice" to it. Could you append it within the function or rename the function?
| @@ -678,6 +678,24 @@ def stop_tracking_unit_cgroups(self, unit_name): | |||
| except Exception as exception: | |||
| logger.info("Failed to stop tracking resource usage for the extension service: {0}", ustr(exception)) | |||
|
|
|||
| def stop_tracking_extension_cgroups(self, extension_name): | |||
| """ | |||
| TODO: Memory tracking | |||
There was a problem hiding this comment.
can we be more descriptive about what needs to be done?
| """ | ||
| try: | ||
| extension_slice_name = SystemdCgroupsApi.get_extension_slice_name(extension_name) + ".slice" | ||
| cgroup_relative_path = os.path.join('azure.slice/azure-vmextensions.slice', |
There was a problem hiding this comment.
We are using "azure.slice" and "azure-vmextensions.slice" as constants? seems to be the same case for the concatenation ("azure.slice/azure-vmextensions.slice")
| cpu_cgroup_mountpoint, _ = self._cgroups_api.get_cgroup_mount_points() | ||
| cpu_cgroup_path = os.path.join(cpu_cgroup_mountpoint, cgroup_relative_path) | ||
|
|
||
| if cpu_cgroup_path is not None and os.path.exists(cpu_cgroup_path): |
There was a problem hiding this comment.
why do we need to check that the path exists before removing the item from the tracked list?
There was a problem hiding this comment.
If not exist monitoring thread will remove when polling happens
There was a problem hiding this comment.
I think regardless we can remove here so that monitoring thread won't poll for metrics before it removes.
|
@narrieta Added description |
| @@ -39,6 +39,7 @@ | |||
| Before=slices.target | |||
| """ | |||
| _VMEXTENSIONS_SLICE = EXTENSION_SLICE_PREFIX + ".slice" | |||
| _AZURE_VMEXTENSIONS_SLICE = AZURE_SLICE + "/" + _VMEXTENSIONS_SLICE | |||
There was a problem hiding this comment.
NIT: Would os.path.join look a bit cleaner here?
Description
The tracking cgroups dict populated with key as cgroup path and value as Cgroup Instance.
Ex: Extension cgroup tracking
CGroupsTelemetry._tracked['/sys/fs/cgroup/cpu,cpuacct/azure.slice/azure-vmextensions.slice/azure-vmextensions-Microsoft.CPlat.Extension.slice'] = CpuCgroup('Microsoft.CPlat.Extension', '/sys/fs/cgroup/cpu,cpuacct/azure.slice/azure-vmextensions.slice/azure-vmextensions-Microsoft.CPlat.Extension.slice')Fixing the stop tracking extension groups from extension slice to build same cgroup /sys/fs path instead of just extension name which is used to find an entry in dictionary to remove.
Issue #
PR information
Quality of Code and Contribution Guidelines