Repository: Azure/WALinuxAgent
Pull request #1394 (2018-11-13 00:01:22)

Fix the threshold telemetry issue
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
Earlier the thresholds were being assigned during the set_cgroups_limits (and being maintained in the Cgroup class, and member of _tracked in CGroupTelemetry). The monitor thread wasn't picking up the data and we were seeing `2018/11/07 21:18:42.654547 WARNING ExtHandler Monitor: failed to collect Cgroups performance metrics: 'NoneType' object has no attribute '__getitem__'`.

This change now explicitly updates the `limits[cgroup_name]` dictionary, which is returned by the `collect_all_tracked()`.

Issue #1395, also does half work for #1363 <!-- if any -->
<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [x] The title of the PR is clear and informative.
- [X] 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](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [x] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [x] If applicable, the PR references the bug/issue that it fixes in the description.
- [x] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [x] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).


Patch:
diff --git a/azurelinuxagent/common/cgroups.py b/azurelinuxagent/common/cgroups.py
index fdd7bc15..fad8629e 100644
--- a/azurelinuxagent/common/cgroups.py
+++ b/azurelinuxagent/common/cgroups.py
@@ -285,6 +285,7 @@ class CGroupsTelemetry(object):
             cgroup_name = cgroup_name if cgroup_name else WRAPPER_CGROUP_NAME
             results[cgroup_name] = collector.collect()
             limits[cgroup_name] = collector.cgroup.threshold
+
         return results, limits
 
     @staticmethod
@@ -370,12 +371,12 @@ class CGroups(object):
         return os.path.join(BASE_CGROUPS, hierarchy, 'system.slice', cgroup_name).rstrip(os.path.sep)
 
     @staticmethod
-    def for_extension(name):
-        return CGroups(name, CGroups._construct_custom_path_for_hierarchy)
+    def for_extension(name, limits=None):
+        return CGroups(name, CGroups._construct_custom_path_for_hierarchy, limits)
 
     @staticmethod
-    def for_systemd_service(name):
-        return CGroups(name.lower(), CGroups._construct_systemd_path_for_hierarchy)
+    def for_systemd_service(name, limits=None):
+        return CGroups(name.lower(), CGroups._construct_systemd_path_for_hierarchy, limits)
 
     @staticmethod
     def enabled():
@@ -389,14 +390,14 @@ class CGroups(object):
     def enable():
         CGroups._enabled = True
 
-    def __init__(self, name, path_maker):
+    def __init__(self, name, path_maker, limits=None):
         """
         Construct CGroups object. Create appropriately-named directory for each hierarchy of interest.
 
         :param str name: Name for the cgroup (usually the full name of the extension)
         :param path_maker: Function which constructs the root path for a given hierarchy where this cgroup lives
         """
-        if name == "":
+        if not name or name == "":
             self.name = "Agents+Extensions"
             self.is_wrapper_cgroup = True
         else:
@@ -404,7 +405,8 @@ class CGroups(object):
             self.is_wrapper_cgroup = False
 
         self.cgroups = {}
-        self.threshold = None
+
+        self.threshold = CGroupsLimits(self.name, limits)
 
         if not self.enabled():
             return
@@ -481,22 +483,6 @@ class CGroups(object):
             tasks_file = self._get_cgroup_file(hierarchy, 'cgroup.procs')
             fileutil.append_file(tasks_file, "{0}\n".format(pid))
 
-    def get_cpu_limits(self):
-        # default values
-        cpu_limit = DEFAULT_CPU_LIMIT_AGENT if AGENT_NAME.lower() in self.name.lower() else DEFAULT_CPU_LIMIT_EXT
-
-        return cpu_limit
-
-    def get_memory_limits(self):
-        # default values
-        mem_limit = max(DEFAULT_MEM_LIMIT_MIN_MB, round(self._osutil.get_total_mem() * DEFAULT_MEM_LIMIT_PCT / 100, 0))
-
-        # agent values
-        if AGENT_NAME.lower() in self.name.lower():
-            mem_limit = min(DEFAULT_MEM_LIMIT_MAX_MB, mem_limit)
-
-        return mem_limit
-
     def set_limits(self):
         """
         Set per-hierarchy limits based on the cgroup name (agent or particular extension)
@@ -513,9 +499,8 @@ class CGroups(object):
                 logger.info('No cgroups limits for {0}'.format(self.name))
                 return
 
-        # default values
-        cpu_limit = self.get_cpu_limits()
-        mem_limit = self.get_memory_limits()
+        cpu_limit = self.threshold.cpu_limit
+        mem_limit = self.threshold.memory_limit
 
         msg = '{0}: {1}% {2}mb'.format(self.name, cpu_limit, mem_limit)
         logger.info("Setting cgroups limits for {0}".format(msg))
@@ -538,9 +523,6 @@ class CGroups(object):
                 message=msg,
                 log_event=False)
 
-        # Returning the limits -
-        self.threshold = {"cpu": cpu_limit, "memory": mem_limit}
-
     @staticmethod
     def _apply_wrapper_limits(path, hierarchy):
         """
@@ -796,3 +778,35 @@ class CGroups(object):
             fileutil.write_file(memory_limit_file, '{0}\n'.format(value))
         else:
             raise CGroupsException("Memory hierarchy not available in this cgroup")
+
+
+class CGroupsLimits(object):
+    @staticmethod
+    def _get_value_or_default(name, threshold, limit, compute_default):
+        return threshold[limit] if threshold and limit in threshold else compute_default(name)
+
+    def __init__(self, cgroup_name, threshold=None):
+        if not cgroup_name or cgroup_name == "":
+            cgroup_name = "Agents+Extensions"
+
+        self.cpu_limit = self._get_value_or_default(cgroup_name, threshold, "cpu", CGroupsLimits.get_default_cpu_limits)
+        self.memory_limit = self._get_value_or_default(cgroup_name, threshold, "memory",
+                                                       CGroupsLimits.get_default_memory_limits)
+
+    @staticmethod
+    def get_default_cpu_limits(cgroup_name):
+        # default values
+        cpu_limit = DEFAULT_CPU_LIMIT_AGENT if AGENT_NAME.lower() in cgroup_name.lower() else DEFAULT_CPU_LIMIT_EXT
+        return cpu_limit
+
+    @staticmethod
+    def get_default_memory_limits(cgroup_name):
+        os_util = get_osutil()
+
+        # default values
+        mem_limit = max(DEFAULT_MEM_LIMIT_MIN_MB, round(os_util.get_total_mem() * DEFAULT_MEM_LIMIT_PCT / 100, 0))
+
+        # agent values
+        if AGENT_NAME.lower() in cgroup_name.lower():
+            mem_limit = min(DEFAULT_MEM_LIMIT_MAX_MB, mem_limit)
+        return mem_limit
diff --git a/azurelinuxagent/ga/monitor.py b/azurelinuxagent/ga/monitor.py
index f3967d35..f59aa724 100644
--- a/azurelinuxagent/ga/monitor.py
+++ b/azurelinuxagent/ga/monitor.py
@@ -427,9 +427,11 @@ class MonitorHandler(object):
                             report_metric(metric_group, metric_name, cgroup_name, value)
 
                         if metric_group == "Memory":
-                            if value >= thresholds["memory"]:
-                                msg = "CGroup {0}: Crossed the Memory Threshold. Current Value:{1}, Threshold:{2}.".format(
-                                    cgroup_name, value, thresholds["memory"])
+                            # Memory is collected in bytes, and limit is set in megabytes.
+                            if value >= CGroups._format_memory_value('megabytes', thresholds.memory_limit):
+                                msg = "CGroup {0}: Crossed the Memory Threshold. " \
+                                      "Current Value:{1} bytes, Threshold:{2} megabytes.".format(cgroup_name, value,
+                                                                                 thresholds.memory_limit)
                                 add_event(name=AGENT_NAME,
                                           version=CURRENT_VERSION,
                                           op=WALAEventOperation.CGroupsLimitsCrossed,
@@ -438,9 +440,9 @@ class MonitorHandler(object):
                                           log_event=True)
 
                         if metric_group == "Process":
-                            if value >= thresholds["cpu"]:
+                            if value >= thresholds.cpu_limit:
                                 msg = "CGroup {0}: Crossed the Processor Threshold. Current Value:{1}, Threshold:{2}.".format(
-                                    cgroup_name, value, thresholds["cpu"])
+                                    cgroup_name, value, thresholds.cpu_limit)
                                 add_event(name=AGENT_NAME,
                                           version=CURRENT_VERSION,
                                           op=WALAEventOperation.CGroupsLimitsCrossed,


Test patch:
diff --git a/tests/common/test_cgroups.py b/tests/common/test_cgroups.py
index 56d5a1df..6838275d 100644
--- a/tests/common/test_cgroups.py
+++ b/tests/common/test_cgroups.py
@@ -17,8 +17,8 @@
 
 from __future__ import print_function
 
-from azurelinuxagent.common.cgroups import CGroupsTelemetry, CGroups, CGroupsException, BASE_CGROUPS, Cpu, Memory, \
-    DEFAULT_MEM_LIMIT_MIN_MB
+from azurelinuxagent.common.cgroups import CGroupsTelemetry, CGroups, CGroupsLimits, \
+    CGroupsException, CGroupsLimits, BASE_CGROUPS, Cpu, Memory, DEFAULT_MEM_LIMIT_MIN_MB
 from azurelinuxagent.common.version import AGENT_NAME
 from tests.tools import *
 
@@ -144,6 +144,11 @@ class TestCGroups(AgentTestCase):
             else:
                 self.fail("Unknown metric {0}/{1} value {2}".format(metric_family, metric_name, metric_value))
 
+        my_limits = limits[test_extension_name]
+        self.assertIsInstance(my_limits, CGroupsLimits, msg="is not the correct instance")
+        self.assertGreater(my_limits.cpu_limit, 0.0)
+        self.assertGreater(my_limits.memory_limit, 0.0)
+
     @skip_if_predicate_false(i_am_root, "Test does not run when non-root")
     def test_telemetry_instantiation_as_superuser(self):
         """
@@ -286,3 +291,43 @@ class TestCGroups(AgentTestCase):
         self.assert_limits(ext_name="normal_extension", expected_cpu_limit=-1, limits_enforced=False)
         self.assert_limits(ext_name=AGENT_NAME, expected_cpu_limit=-1, limits_enforced=False)
         self.assert_limits(ext_name="normal_extension", expected_cpu_limit=40, exception_raised=True)
+
+
+class TestCGroupsLimits(AgentTestCase):
+    @patch("azurelinuxagent.common.osutil.default.DefaultOSUtil.get_total_mem", return_value=1024)
+    def test_no_limits_passed(self, patched_get_total_mem):
+        cgroup_name = "test_cgroup"
+        limits = CGroupsLimits(cgroup_name)
+        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name ))
+        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name ))
+
+        limits = CGroupsLimits(None)
+        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name))
+        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name))
+
+    @patch("azurelinuxagent.common.osutil.default.DefaultOSUtil.get_total_mem", return_value=1024)
+    def test_with_limits_passed(self, patched_get_total_mem):
+        cpu_limit = 50
+        memory_limit = 300
+        cgroup_name = "test_cgroup"
+
+        threshold = {"cpu": cpu_limit}
+        limits = CGroupsLimits(cgroup_name, threshold=threshold)
+        self.assertEqual(limits.cpu_limit, cpu_limit)
+        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name))
+
+        threshold = {"memory": memory_limit}
+        limits = CGroupsLimits(cgroup_name, threshold=threshold)
+        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name))
+        self.assertEqual(limits.memory_limit, memory_limit)
+
+        threshold = {"cpu": cpu_limit, "memory": memory_limit}
+        limits = CGroupsLimits(cgroup_name, threshold=threshold)
+        self.assertEqual(limits.cpu_limit, cpu_limit)
+        self.assertEqual(limits.memory_limit, memory_limit)
+
+        # Incorrect key
+        threshold = {"cpux": cpu_limit}
+        limits = CGroupsLimits(cgroup_name, threshold=threshold)
+        self.assertEqual(limits.cpu_limit, CGroupsLimits.get_default_cpu_limits(cgroup_name))
+        self.assertEqual(limits.memory_limit, CGroupsLimits.get_default_memory_limits(cgroup_name))