Merge ~mertkirpici/charm-nrpe:topic/cpu_governor_check into charm-nrpe:master

[Mert Kirpici]

Set default cpu_governor values for specific principle charms

The old behavior was to get this default value through the
nrpe-external-master relation. This new approach is introducing
inconsistent behavior for the default value however, this is saving us
from updating every single principle charm code that this is relevant.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Eric Chen (eric-chen)]

review: Approve

[Robert Gildein (rgildein)]

Can we add at least unit test for these changes?

[Robert Gildein (rgildein)]

Also commit title should not be longer than 50 characters.

[Mert Kirpici (mertkirpici)]

Fixed the commit message title and added unit tests in the second commit

[Mert Kirpici (mertkirpici)]

Current state of the unit tests:
https://pastebin.ubuntu.com/p/24rb7rWPF3/

Lint:
https://pastebin.ubuntu.com/p/D7zJwKPdnH/

Failing unit test was introduced by a recent change, not relevant to this change.
Failing lint rule has been there for some time..

[Robert Gildein (rgildein)]

Thanks Mert. LGTM

review: Approve

[Eric Chen (eric-chen)]

Mert, Can you rebase this PR can test the unit test & lint rule again to make sure it is pass? Thanks!

[Mert Kirpici (mertkirpici)]

rebased.

make lint:
https://pastebin.ubuntu.com/p/Bm8ydxHxcC/

make unittests:
https://pastebin.ubuntu.com/p/fxXw7ZZjNH/

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 07830068f89c297e6b60b65a532a9f98eff1dce9

[Facundo Ciccioli (fandanbango)]

My comments are non-blocking. This looks good to me, great job :)

review: Approve

diff --git a/config.yaml b/config.yaml
index ba882f5..b81e3f3 100644
--- a/config.yaml
+++ b/config.yaml
@@ -254,9 +254,15 @@ options:
     description: |
       CPU governor check. The string value here will be checked against all CPUs in
       /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor. The supported values are
-      'ondemand', 'performance', 'powersave'. Unset value means the check will be disabled.
-      There is a relation key called requested_cpu_governor='string', but the charm config value
-      will take precedence over the relation data.
+      'ondemand', 'performance', 'powersave'. Although this value is initially unset for
+      most principal charms, for the following principal charms a default check for
+      'performance' will be deployed to aid the operator since for most use cases these
+      applications will require all the available cpu power they need. If this value is set,
+      the set value will take precedence over the default.
+      - nova-compute
+      - rabbitmq-server
+      - kubernetes-worker
+      - percona-cluster
   cis_audit_enabled:
     default: False
     type: boolean
diff --git a/hooks/nrpe_helpers.py b/hooks/nrpe_helpers.py
index b980912..3aa9238 100644
--- a/hooks/nrpe_helpers.py
+++ b/hooks/nrpe_helpers.py
@@ -637,16 +637,33 @@ class SubordinateCheckDefinitions(dict):
                     checks.append(netlink_check)
 
         # Checking if CPU governor is supported by the system and add nrpe check
-        cpu_governor_paths = "/sys/devices/system/cpu/cpu*/cpufreq/scaling_governor"
-        cpu_governor_supported = glob.glob(cpu_governor_paths)
-        requested_cpu_governor = hookenv.relation_get("requested_cpu_governor")
-        cpu_governor_config = hookenv.config("cpu_governor")
-        wanted_cpu_governor = cpu_governor_config or requested_cpu_governor
-        if wanted_cpu_governor and cpu_governor_supported:
+        cpu_governor_supported = glob.glob(
+            "/sys/devices/system/cpu/cpu*/cpufreq/scaling_governor"
+        )
+        cpu_governor_setting = hookenv.config("cpu_governor")
+        if not cpu_governor_setting:
+            principal_unit = hookenv.principal_unit()
+            principal_charm_name = hookenv._metadata_unit(principal_unit).get("name")
+            if principal_charm_name in [
+                "nova-compute",
+                "kubernetes-worker",
+                "rabbitmq-server",
+                "percona-cluster",
+            ]:
+                hookenv.log(
+                    "Setting default cpu freq scaling governor to 'performance' \
+                     for unit:[{}] with charm name:[{}]".format(
+                        principal_unit, principal_charm_name
+                    ),
+                    level=hookenv.DEBUG,
+                )
+                cpu_governor_setting = "performance"
+
+        if cpu_governor_setting and cpu_governor_supported:
             description = "Check CPU governor scaler"
             cmd_name = "check_cpu_governor"
             cmd_exec = local_plugin_dir + "check_cpu_governor.py"
-            cmd_params = "--governor {}".format(wanted_cpu_governor)
+            cmd_params = "--governor {}".format(cpu_governor_setting)
             cpu_governor_check = {
                 "description": description,
                 "cmd_name": cmd_name,
diff --git a/tests/unit/test_nrpe_helpers.py b/tests/unit/test_nrpe_helpers.py
index fa5a54f..d9ea79a 100644
--- a/tests/unit/test_nrpe_helpers.py
+++ b/tests/unit/test_nrpe_helpers.py
@@ -1,4 +1,5 @@
 """Unit tests for hooks/nrpe_helpers.py module."""
+import os
 import unittest
 from unittest import mock
 
@@ -7,6 +8,8 @@ import netifaces
 import nrpe_helpers
 from nrpe_helpers import match_cidr_to_ifaces
 
+import yaml
+
 
 class TestMatchCidrToIfaces(unittest.TestCase):
     """Test match_cidr_to_ifaces helper function."""
@@ -289,3 +292,152 @@ class TestDiskSpaceCheck(unittest.TestCase):
         self.assertEqual("/" in result, True)
         self.assertEqual("/srv/instances" in result, True)
         self.assertEqual("/srv/jammy" in result, True)
+
+
+def load_default_config():
+    """Load the default config values from the charm config.yaml.
+
+    Returns a dict of {config:default}
+    """
+    with open(os.path.join(os.getcwd(), "config.yaml"), "r") as f:
+        config_raw = yaml.safe_load(f)
+    return {key: value["default"] for key, value in config_raw["options"].items()}
+
+
+class TestSubordinateCheckDefinitions(unittest.TestCase):
+    """Test SubordinateCheckDefinitions() related code."""
+
+    def glob_valid_cpufreq_path(self, arg):
+        """Return a valid list of cpufreq sysfs paths.
+
+        This function is meant to be used as a side_effect when
+        mocking glob.glob(), to provide a more controlled unit
+        testing environment.
+        """
+        ret = []
+        if arg == "/sys/devices/system/cpu/cpu*/cpufreq/scaling_governor":
+            ret = ["/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"]
+        return ret
+
+    @mock.patch("nrpe_helpers.glob.glob")
+    @mock.patch("nrpe_helpers.hookenv._metadata_unit")
+    @mock.patch("nrpe_helpers.hookenv.principal_unit")
+    @mock.patch("nrpe_helpers.hookenv.config")
+    def test_cpu_governor_default_enabled_for_principle_charms(
+        self, mock_config, mock_principal_unit, mock__metadata_unit, mock_glob
+    ):
+        """Test cpu_governor check is enabled by default for principle charms.
+
+        A cpu scaling governor value of 'performance' is expected
+        by default for the charms nova-compute, kubernetes-worker,
+        rabbitmq-server, percona-cluster even if the charm config
+        is unset
+        """
+        config = load_default_config()
+        mock_config.side_effect = lambda key: config[key]
+        mock_glob.side_effect = self.glob_valid_cpufreq_path
+
+        principal_charms = [
+            "nova-compute",
+            "rabbitmq-server",
+            "kubernetes-worker",
+            "percona-cluster",
+        ]
+        for charm in principal_charms:
+            mock_principal_unit.return_value = charm + "/0"
+            mock__metadata_unit.return_value = {"name": charm}
+
+            check = {
+                "description": "Check CPU governor scaler (sub)",
+                "cmd_name": "check_cpu_governor",
+                "cmd_exec": "/usr/local/lib/nagios/plugins/check_cpu_governor.py",
+                "cmd_params": "--governor performance",
+                "matching_files": [],
+            }
+            checks = nrpe_helpers.SubordinateCheckDefinitions()["checks"]
+            self.assertIn(check, checks)
+
+            mock_principal_unit.reset_mock(return_value=True)
+            mock__metadata_unit.reset_mock(return_value=True)
+
+    @mock.patch("nrpe_helpers.glob.glob")
+    @mock.patch("nrpe_helpers.hookenv._metadata_unit")
+    @mock.patch("nrpe_helpers.hookenv.principal_unit")
+    @mock.patch("nrpe_helpers.hookenv.config")
+    def test_cpu_governor_disabled_default_for_non_principle_charms(
+        self, mock_config, mock_principal_unit, mock__metadata_unit, mock_glob
+    ):
+        """Test cpu_governor check is disabled by default for non principle charms.
+
+        A cpu scaling governor value check should be disabled by default
+        by most charms.
+        """
+        config = load_default_config()
+        mock_config.side_effect = lambda key: config[key]
+        mock_glob.side_effect = self.glob_valid_cpufreq_path
+        mock_principal_unit.return_value = "mongodb/0"
+        mock__metadata_unit.return_value = {"name": "mongodb"}
+
+        checks = nrpe_helpers.SubordinateCheckDefinitions()["checks"]
+        check_in_list = False
+        for check in checks:
+            if check["cmd_name"].startswith("check_cpu_governor"):
+                check_in_list = True
+                break
+        self.assertFalse(check_in_list)
+
+    @mock.patch("nrpe_helpers.glob.glob")
+    @mock.patch("nrpe_helpers.hookenv._metadata_unit")
+    @mock.patch("nrpe_helpers.hookenv.principal_unit")
+    @mock.patch("nrpe_helpers.hookenv.config")
+    def test_cpu_governor_config_overrides_default(
+        self, mock_config, mock_principal_unit, mock__metadata_unit, mock_glob
+    ):
+        """Test cpu_governor default value overriden by config.
+
+        The value coming from the configuration should take precedence
+        in case it is set for principle charms.
+        """
+        config = load_default_config()
+        config["cpu_governor"] = "ondemand"
+        mock_config.side_effect = lambda key: config[key]
+        mock_glob.side_effect = self.glob_valid_cpufreq_path
+        mock_principal_unit.return_value = "nova-compute/0"
+        mock__metadata_unit.return_value = {"name": "nova-compute"}
+
+        check = {
+            "description": "Check CPU governor scaler (sub)",
+            "cmd_name": "check_cpu_governor",
+            "cmd_exec": "/usr/local/lib/nagios/plugins/check_cpu_governor.py",
+            "cmd_params": "--governor ondemand",
+            "matching_files": [],
+        }
+        checks = nrpe_helpers.SubordinateCheckDefinitions()["checks"]
+        self.assertIn(check, checks)
+
+    @mock.patch("nrpe_helpers.glob.glob")
+    @mock.patch("nrpe_helpers.hookenv._metadata_unit")
+    @mock.patch("nrpe_helpers.hookenv.principal_unit")
+    @mock.patch("nrpe_helpers.hookenv.config")
+    def test_cpu_governor_check_disabled_in_vm(
+        self, mock_config, mock_principal_unit, mock__metadata_unit, mock_glob
+    ):
+        """Test cpu_governor check is disabled for virtual environments.
+
+        When running in VM, this check should be disabled even if it is
+        set through the config.
+        """
+        config = load_default_config()
+        config["cpu_governor"] = "performance"
+        mock_config.side_effect = lambda key: config[key]
+        mock_glob.side_effect = lambda arg: []  # cpufreq paths no exist in VM
+        mock_principal_unit.return_value = "nova-compute/0"
+        mock__metadata_unit.return_value = {"name": "nova-compute"}
+
+        check_in_list = False
+        checks = nrpe_helpers.SubordinateCheckDefinitions()["checks"]
+        for check in checks:
+            if check["cmd_name"].startswith("check_cpu_governor"):
+                check_in_list = True
+                break
+        self.assertFalse(check_in_list)