hw-health-charm

Merge ~mertkirpici/charm-hw-health:lp/2036300 into charm-hw-health:master

  1. Git
  2. lp:~mertkirpici/charm-hw-health
  3. lp/2036300
  4. Merge into master
Proposed by Mert Kirpici
Status: Merged
Approved by: Mert Kirpici
Approved revision: 61b2bb7fa5b6246b576589ecad1a2308864c6a9b
Merged at revision: dc253db4e00dadb588b0c667eded65c57d59b768
Proposed branch: ~mertkirpici/charm-hw-health:lp/2036300
Merge into: charm-hw-health:master
Diff against target: 164 lines (+85/-13)
4 files modified
charmcraft.yaml (+1/-9)
src/files/ilorest/cron_ilorest.py (+14/-3)
src/tests/functional/test_hwhealth.py (+0/-1)
src/tests/unit/test_cron_ilorest.py (+70/-0)
Reviewer Review Type Date Requested Status
Robert Gildein Approve
🤖 prod-jenkaas-bootstack (community) continuous-integration Approve
Nikita Koltsov Approve
Paul Goins Approve
Review via email: mp+452276@code.launchpad.net

Commit message

LP #2036300

Description of the change

As discovered by Paul Goins not issuing an "ilorest login" before
calling "ilorest list" sometimes causes an "Error 32 occurred while
exchange chif packet" error when the script is run by cron in the
minimal environment that crond provides.

The exact root cause is still unknown however this patch tries to
mitigate this behavior in light of Paul's discovery.

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
Paul Goins (vultaire) wrote :

Review for what it's worth: LGTM

review: Approve
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:464242f6d257de3dce545a7b5117118491eddafa
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/9/
Executed test runs:
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/539/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:464242f6d257de3dce545a7b5117118491eddafa
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/10/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1877/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/542/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:464242f6d257de3dce545a7b5117118491eddafa
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/11/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1880/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/545/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

PASSED: Continuous integration, rev:f91568484c4461a1b17871872ff4bc68366ce2b3
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/12/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1885/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/549/

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

review: Approve (continuous-integration)
Revision history for this message
Robert Gildein (rgildein) :
review: Needs Fixing
Revision history for this message
Nikita Koltsov (nkoltsov) wrote :

Looks good to me, and passed testing on the env affected by the bug.

review: Approve
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

PASSED: Continuous integration, rev:41e823d613b4b809c660e842c185150ce6ce294a
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/13/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1891/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/555/

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

review: Approve (continuous-integration)
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

Pushed an update which hopefully addresses issues and we can land this change.

Revision history for this message
Robert Gildein (rgildein) :
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:61b2bb7fa5b6246b576589ecad1a2308864c6a9b
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/14/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1893/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/557/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:61b2bb7fa5b6246b576589ecad1a2308864c6a9b
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/15/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1894/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/558/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:61b2bb7fa5b6246b576589ecad1a2308864c6a9b
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/16/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1897/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/561/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

PASSED: Continuous integration, rev:61b2bb7fa5b6246b576589ecad1a2308864c6a9b
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/17/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1908/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/572/

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

review: Approve (continuous-integration)
Revision history for this message
Robert Gildein (rgildein) wrote :

LGTM

review: Approve
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision dc253db4e00dadb588b0c667eded65c57d59b768

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/charmcraft.yaml b/charmcraft.yaml
2index 3a47b4f..7187921 100644
3--- a/charmcraft.yaml
4+++ b/charmcraft.yaml
5@@ -3,11 +3,7 @@ parts:
6 charm:
7 source: src
8 plugin: reactive
9- # using charm/3.x/stable will cause the charm to fail installation on 18.04
10- # with the following error: "pip requires Python '>=3.7' but the running
11- # Python is 3.6.9". This is because charm/3.x/stable uses the system
12- # installation of python instead of a python snap.
13- build-snaps: [ charm/2.x/stable ]
14+ build-snaps: [charm]
15 bases:
16 - build-on:
17 - name: ubuntu
18@@ -22,7 +18,3 @@ bases:
19 channel: "20.04"
20 architectures:
21 - amd64
22- - name: ubuntu
23- channel: "18.04"
24- architectures:
25- - amd64
26diff --git a/src/files/ilorest/cron_ilorest.py b/src/files/ilorest/cron_ilorest.py
27index 98ef87b..8953734 100755
28--- a/src/files/ilorest/cron_ilorest.py
29+++ b/src/files/ilorest/cron_ilorest.py
30@@ -139,6 +139,14 @@ class CronILOrest:
31
32 return self._walk_selector(jsondata, [selector])
33
34+ def login(self):
35+ """Run the ilorest login command."""
36+ return self._run_ilorest("login")
37+
38+ def logout(self):
39+ """Run the ilorest logout command."""
40+ return self._run_ilorest("logout")
41+
42 def _run_ilorest(self, *args):
43 """Run ilorest executable and return stdout string.
44
45@@ -187,12 +195,12 @@ class CronILOrest:
46 errors = []
47
48 # if we start at a list, iterate through it, else make sure we've got a dict
49- if type(json_data) == list:
50+ if isinstance(json_data, list):
51 for index in range(len(json_data)):
52 errors.extend(
53 self._walk_selector(json_data[index], (crumb_trail + [str(index)]))
54 )
55- elif type(json_data) != dict:
56+ elif not isinstance(json_data, dict):
57 # we only dive through lists and dicts, so no errors below this point
58 return []
59 elif (
60@@ -224,10 +232,13 @@ def main(argv=None):
61
62 errors = []
63 try:
64+ cronilorest.login()
65 for selector in cronilorest.args.selectors:
66 errors.extend(cronilorest.check_selector(selector))
67- except RuntimeError as exc:
68+ except (RuntimeError, CalledProcessError) as exc:
69 errors.append(str(exc))
70+ finally:
71+ cronilorest.logout()
72
73 if len(errors) > 0:
74 msg = "CRIT {} error(s): {}".format(len(errors), " - ".join(errors))
75diff --git a/src/tests/functional/test_hwhealth.py b/src/tests/functional/test_hwhealth.py
76index 0d69814..b529d42 100644
77--- a/src/tests/functional/test_hwhealth.py
78+++ b/src/tests/functional/test_hwhealth.py
79@@ -19,7 +19,6 @@ pytestmark = pytest.mark.asyncio
80 SERIES = [
81 "jammy",
82 "focal",
83- "bionic",
84 ]
85 CHARM_DIR = dirname(dirname(dirname(abspath(__file__))))
86 CHARM_BUILD_DIR = dirname(CHARM_DIR)
87diff --git a/src/tests/unit/test_cron_ilorest.py b/src/tests/unit/test_cron_ilorest.py
88index d9f29bf..38200b8 100644
89--- a/src/tests/unit/test_cron_ilorest.py
90+++ b/src/tests/unit/test_cron_ilorest.py
91@@ -167,3 +167,73 @@ class TestCronILOrest(unittest.TestCase):
92 env={**os.environ, "TMP": "/var/lib/hw-health"},
93 universal_newlines=True,
94 )
95+
96+ @mock.patch.object(cron_ilorest, "check_output")
97+ def test_ilorest_login(self, check_output_mock):
98+ """Test the login method."""
99+ common_args = {
100+ "env": {**os.environ, "TMP": "/var/lib/hw-health"},
101+ "universal_newlines": True,
102+ }
103+ cron_ilorest.CronILOrest().login()
104+ check_output_mock.assert_called_once_with(
105+ ["/usr/sbin/ilorest", "login"], **common_args
106+ )
107+
108+ @mock.patch.object(cron_ilorest, "check_output")
109+ def test_ilorest_logout(self, check_output_mock):
110+ """Test the logout method."""
111+ common_args = {
112+ "env": {**os.environ, "TMP": "/var/lib/hw-health"},
113+ "universal_newlines": True,
114+ }
115+ cron_ilorest.CronILOrest().logout()
116+ check_output_mock.assert_called_once_with(
117+ ["/usr/sbin/ilorest", "logout"], **common_args
118+ )
119+
120+ @mock.patch.object(cron_ilorest, "open", new_callable=mock.mock_open)
121+ @mock.patch.object(cron_ilorest.sys, "exit")
122+ @mock.patch.object(cron_ilorest.CronILOrest, "check_selector")
123+ @mock.patch.object(cron_ilorest.CronILOrest, "logout")
124+ @mock.patch.object(cron_ilorest.CronILOrest, "login")
125+ def test_main_exits_ok_upon_no_errors(
126+ self, mock_login, mock_logout, mock_check_selector, mock_exit, mock_open
127+ ):
128+ """Test main function exits with OK upon no errors."""
129+ outfile = "test.out"
130+
131+ cron_ilorest.main(["cron_ilorest.py", "-w", outfile])
132+
133+ mock_login.assert_called_once()
134+ mock_check_selector.assert_called()
135+ mock_logout.assert_called_once()
136+ mock_open.assert_called_once_with(outfile, "w")
137+ mock_open().write.assert_called_once_with("OK No errors found")
138+ mock_exit.assert_called_with(0) # nrpe plugin OK
139+
140+ @mock.patch.object(cron_ilorest, "open", new_callable=mock.mock_open)
141+ @mock.patch.object(cron_ilorest.sys, "exit")
142+ @mock.patch.object(cron_ilorest.CronILOrest, "logout")
143+ @mock.patch.object(cron_ilorest.CronILOrest, "login")
144+ def test_main_exits_with_error_if_login_fails(
145+ self, mock_login, mock_logout, mock_exit, mock_open
146+ ):
147+ """Test main function exits with CRITICAL upon login errors."""
148+ outfile = "test.out"
149+ login_cmd = ["/usr/sbin/ilorest", "login"]
150+ login_cmd_ret = 1
151+ mock_login.side_effect = subprocess.CalledProcessError(
152+ returncode=login_cmd_ret, cmd=login_cmd
153+ )
154+
155+ cron_ilorest.main(["cron_ilorest.py", "-w", outfile])
156+
157+ mock_login.assert_called_once()
158+ mock_logout.assert_called_once()
159+ mock_open.assert_called_once_with(outfile, "w")
160+ mock_open().write.assert_called_once_with(
161+ f"CRIT 1 error(s): Command '{repr(login_cmd)}' "
162+ f"returned non-zero exit status {login_cmd_ret}."
163+ )
164+ mock_exit.assert_called_with(2) # nrpe plugin CRITICAL

Subscribers

People subscribed via source and target branches

to all changes: