Merge charm-hw-health:unknown-on-tool-error into charm-hw-health:master

[Facundo Ciccioli]

For megacli and perccli, report UNKNOWN when the tool's execution fails

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:2140e1484e99a83183915448af587c01a651a171
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/56/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/260/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/941/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/56//rebuild

review: Approve (continuous-integration)

[Stephan Pampel (stephanpampel)]

LGTM

review: Approve

[Xav Paice (xavpaice)]

The change makes sense, I agree UNKNOWN is a more appropriate state, and will alert where WARNING will not.

Let's not provide the location of the failed command output, unless we're able to show that output somehow without needing ssh to the unit. Nagios check output needs to be very brief and a single line, and the location of the file doesn't change according to the result of the test.

review: Needs Fixing

[Alvaro Uria (aluria)]

Marked WIP until Xav's concerns are addressed.

Unmerged commits

2140e14... by Facundo Ciccioli on 2022-02-04

For megacli and perccli, report UNKNOWN when the tool's execution fails

A warning is not enough. We want to know if the charm decided to install
the wrong tool or if the host's hardware ceased to be supported by the
tool.

diff --git a/src/files/megacli/check_megacli.py b/src/files/megacli/check_megacli.py
index be8c065..e5a83e5 100755
--- a/src/files/megacli/check_megacli.py
+++ b/src/files/megacli/check_megacli.py
@@ -7,7 +7,7 @@ import os
 import re
 import sys
 
-from nagios_plugin3 import CriticalError, WarnError, try_check
+from nagios_plugin3 import CriticalError, UnknownError, WarnError, try_check
 
 try:
     # shared lib will be under $NAGIOS_PLUGIN_DIR at runtime
@@ -62,8 +62,11 @@ def check_cron_return_code(rc_file=RC_FILE):
     with open(rc_file) as return_code_file:
         rc = return_code_file.read().strip()
         if rc != 0:
-            msg = "WARNING: megacli cron job failed, return code {}".format(rc)
-            raise WarnError(msg)
+            msg = (
+                "UNKNOWN: megacli cron job failed, return code {}. "
+                "Last job output on {}".format(rc, INPUT_FILE)
+            )
+            raise UnknownError(msg)
 
 
 def parse_output(policy=False):  # noqa:C901
@@ -151,7 +154,7 @@ def parse_args(argv=None):
 def main(argv, policy=False):
     """Define main subroutine."""
     parse_args(argv)
-    check_cron_return_code(RC_FILE)
+    try_check(check_cron_return_code, RC_FILE)
     try_check(parse_output, policy)
 
 
diff --git a/src/files/perccli/check_perccli.py b/src/files/perccli/check_perccli.py
index 7cc2c9c..5133830 100755
--- a/src/files/perccli/check_perccli.py
+++ b/src/files/perccli/check_perccli.py
@@ -6,7 +6,7 @@ import json
 import os
 import sys
 
-from nagios_plugin3 import CriticalError, WarnError, try_check
+from nagios_plugin3 import CriticalError, UnknownError, WarnError, try_check
 
 try:
     # shared lib will be under $NAGIOS_PLUGIN_DIR at runtime
@@ -21,6 +21,7 @@ except ImportError:
     from hw_health_lib import HWCheckArgumentParser
 
 INPUT_FILE = "/var/lib/nagios/perccli.out"
+RC_FILE = "/var/lib/nagios/perccli.retcode"
 ARGS = argparse.Namespace()
 
 
@@ -56,6 +57,17 @@ def handle_results(nlines, match, critical, errors, num_ldrive, num_pdrive, poli
         print(msg)
 
 
+def check_cron_return_code(rc_file=RC_FILE):
+    with open(rc_file) as return_code_file:
+        rc = return_code_file.read().strip()
+        if rc != 0:
+            msg = (
+                "UNKNOWN: perccli cron job failed, return code {}. "
+                "Last job output on {}".format(rc, INPUT_FILE)
+            )
+            raise UnknownError(msg)
+
+
 def parse_output(policy=False):  # noqa:C901
     num_pdrive = num_ldrive = failed_ld = wrg_policy_ld = 0
     nlines = 0
@@ -110,6 +122,7 @@ def parse_args(argv=None):
 def main(argv, policy=False):
     """Define main subroutine."""
     parse_args(argv)
+    try_check(check_cron_return_code, RC_FILE)
     try_check(parse_output, policy)
 
 
diff --git a/src/tests/unit/test_check_megacli.py b/src/tests/unit/test_check_megacli.py
index 1f77b9f..a6b7d22 100644
--- a/src/tests/unit/test_check_megacli.py
+++ b/src/tests/unit/test_check_megacli.py
@@ -48,8 +48,11 @@ class TestCheckMegaCLI(unittest.TestCase):
     def test_check_cron_return_code(self, mock_print):
         file_content = "127"
         mock_open = mock.mock_open(read_data=file_content)
-        expected = "WARNING: megacli cron job failed, return code 127"
+        expected = (
+            "UNKNOWN: megacli cron job failed, return code 127. "
+            "Last job output on {}".format(check_megacli.INPUT_FILE)
+        )
         with mock.patch("builtins.open", mock_open):
-            with self.assertRaises(nagios_plugin3.WarnError) as context:
+            with self.assertRaises(nagios_plugin3.UnknownError) as context:
                 check_megacli.check_cron_return_code("/path/to/return_code_file")
         self.assertEqual(expected, str(context.exception))
diff --git a/src/tests/unit/test_check_perccli.py b/src/tests/unit/test_check_perccli.py
index 9220d6b..35b4297 100644
--- a/src/tests/unit/test_check_perccli.py
+++ b/src/tests/unit/test_check_perccli.py
@@ -43,3 +43,16 @@ class TestCheckPercCli(unittest.TestCase):
         with self.assertRaises(nagios_plugin3.CriticalError) as context:
             check_perccli.parse_output()
         self.assertEqual(expected, str(context.exception))
+
+    @mock.patch("sys.stdout", new_callable=io.StringIO)
+    def test_check_cron_return_code(self, mock_print):
+        file_content = "127"
+        mock_open = mock.mock_open(read_data=file_content)
+        expected = (
+            "UNKNOWN: perccli cron job failed, return code 127. "
+            "Last job output on {}".format(check_perccli.INPUT_FILE)
+        )
+        with mock.patch("builtins.open", mock_open):
+            with self.assertRaises(nagios_plugin3.UnknownError) as context:
+                check_perccli.check_cron_return_code("/path/to/return_code_file")
+        self.assertEqual(expected, str(context.exception))