Merge ~gabrielcocenza/charm-juju-lint:bug/1993334 into charm-juju-lint:master

[Gabriel Cocenza]

add timeout for running auto_lint.py

- hook enter into error state instead of holding machine lock
- change juju loop(deprecated) to jasyncio

Closes-Bug: #1993334

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

FAILED: Continuous integration, rev:3f55aa5d048305e7f9f286579e32c05915351977
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/19/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1124/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1543/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/19//rebuild

review: Needs Fixing (continuous-integration)

[🤖 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)]

FAILED: Continuous integration, rev:3f55aa5d048305e7f9f286579e32c05915351977
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/20/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1125/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1544/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/20//rebuild

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:8d4039ced375ef17d239f2ab14e0be59a938334c
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/21/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1126/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1545/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/21//rebuild

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:e13d02eef432b33d28602ee13701861d92273121
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/22/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1127/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1546/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/22//rebuild

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:9fa72929bf3d5eead2bf57b8d5296ea0ceb68d8b
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/23/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1128/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1547/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/23//rebuild

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:40c6b015e56aa08e615b4562c82a42123601787a
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/24/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1129/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1548/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-juju-lint-ci/24//rebuild

review: Approve (continuous-integration)

[Mert Kirpici (mertkirpici)]

Hi there Gabriel, thanks for the patch. It is looking good in my opinion.
I just left a question inline about the PID files.

[Erhan Sunar (esunar)]

LGTM

review: Approve

[Gabriel Cocenza (gabrielcocenza)]

Thanks for the review Mert and Erhan.

I left comments on your questions.

[Mert Kirpici (mertkirpici)]

Thanks Gabriel for the detailed explanation. I left a comment, this is a +1 from me.

review: Approve

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

Change successfully merged at revision 89392d23f89b265235937a6dc4236f6f1deb9e16

diff --git a/lib/lib_jujulint.py b/lib/lib_jujulint.py
index 0085aee..978aac7 100644
--- a/lib/lib_jujulint.py
+++ b/lib/lib_jujulint.py
@@ -33,6 +33,7 @@ class Paths:
     AUTO_LINT_SCRIPT_PATH = USR_LOCAL / "bin/auto_lint.py"
     AUTO_LINT_CRONTAB_PATH = pathlib.Path("/etc/cron.d/juju-lint")
     NAGIOS_PLUGINS_DIR = USR_LOCAL / "lib/nagios/plugins/"
+    PID_FILENAME = pathlib.Path("/tmp/auto_lint.pid")
 
 
 CHECK_SHORTNAME = "juju_lint_results"
@@ -170,17 +171,25 @@ class JujuLintHelper:
         """Run auto lint."""
         try:
             lint_script = str(Paths.AUTO_LINT_SCRIPT_PATH)
-            subprocess.check_output([lint_script], stderr=subprocess.STDOUT).decode(
-                "utf8"
-            )
+            subprocess.check_output(
+                [lint_script], stderr=subprocess.STDOUT, timeout=60
+            ).decode("utf8")
         except subprocess.CalledProcessError as error:
             # here we care about the output only if it fails
             logging.error(
-                "auto_lint.py failed with this output:\n{}".format(
-                    error.output.decode("utf8")
-                )
+                "auto_lint.py failed with this output:\n %s",
+                error.output.decode("utf8"),
+            )
+            raise error
+        except subprocess.TimeoutExpired as error:
+            logging.error(
+                "auto_lint.py timeout with this output:\n %s",
+                error.output.decode("utf8"),
             )
             raise error
+        finally:
+            if Paths.PID_FILENAME.exists():
+                Paths.PID_FILENAME.unlink()
 
     def update_nrpe_config(self, current_checks):
         """Run on relation config changed event."""
diff --git a/scripts/templates/auto_lint.py b/scripts/templates/auto_lint.py
index 5eb7b7b..65c76fc 100755
--- a/scripts/templates/auto_lint.py
+++ b/scripts/templates/auto_lint.py
@@ -31,7 +31,7 @@ import sys
 # The path below is templated in during charm install
 sys.path.append("REPLACE_CHARMDIR/venv")
 
-from juju import loop  # noqa E402
+from juju import jasyncio  # noqa E402
 from juju.model import Model  # noqa E402
 from juju.client.facade import TypeEncoder  # noqa E402
 
@@ -267,7 +267,7 @@ def main():
     try:
         auto_lint_config = read_json_file(LINT_CONFIG_PATH)
         verify_auto_lint_config(auto_lint_config)
-        juju_status = loop.run(get_juju_status(auto_lint_config))
+        juju_status = jasyncio.run(get_juju_status(auto_lint_config))
         write_file(
             JUJU_STATUS_PATH, json.dumps(juju_status, cls=TypeEncoder, sort_keys=True)
         )
diff --git a/tests/unit/test_auto_lint.py b/tests/unit/test_auto_lint.py
index 53d4fbf..72a663a 100644
--- a/tests/unit/test_auto_lint.py
+++ b/tests/unit/test_auto_lint.py
@@ -3,7 +3,7 @@ import copy
 import unittest
 
 import mock
-from juju import loop
+from juju import jasyncio
 from scripts.templates.auto_lint import (
     add_charm_config_to_juju_status,
     lint_juju,
@@ -82,7 +82,7 @@ class TestAutoLint(unittest.TestCase):
         It can be removed if this is implemented in juju, reference bug:
         https://bugs.launchpad.net/juju/+bug/1929911
         """
-        juju_status = loop.run(
+        juju_status = jasyncio.run(
             add_charm_config_to_juju_status(self.juju_status, fixtures.MockModel)
         )
         for app_name, app_dict in juju_status["applications"].items():
@@ -98,7 +98,7 @@ class TestAutoLint(unittest.TestCase):
 
     def test_add_config_to_status_unset(self):
         """Tests that the unset options are ignored."""
-        juju_status = loop.run(
+        juju_status = jasyncio.run(
             add_charm_config_to_juju_status(self.juju_status, fixtures.MockModelUnset)
         )
         for app_name, app_dict in juju_status["applications"].items():
@@ -109,7 +109,7 @@ class TestAutoLint(unittest.TestCase):
         """Tests handling the case where an option is missing a value."""
 
         def test_helper():
-            loop.run(
+            jasyncio.run(
                 add_charm_config_to_juju_status(
                     self.juju_status, fixtures.MockModelMissingValue
                 )
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index ee4e1e2..0786165 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -2,6 +2,7 @@
 import os
 import json
 import pathlib
+import subprocess
 import tempfile
 import unittest
 
@@ -158,7 +159,7 @@ class TestCharm(unittest.TestCase):
         """Test: change crontab configuration."""
         self.harness.begin()
         self.harness.charm.state.installed = True
-        self.harness.update_config({"lint-frequency": "100"})
+        self.harness.update_config({"lint-frequency": 100})
         self.assertEqual(self.get_notice_count("config_changed"), 0)
         with Paths.AUTO_LINT_CRONTAB_PATH.open() as f:
             self.assertRegex(f.read(), r"\*/100 \*")
@@ -263,6 +264,34 @@ class TestCharm(unittest.TestCase):
                 notice_count += 1
         return notice_count
 
+    @mock.patch("lib_jujulint.Paths.PID_FILENAME")
+    @mock.patch("lib_jujulint.subprocess.check_output")
+    def test_run_auto_lint_time_out(self, mock_check_output, mock_pid_file):
+        """Test time out exception."""
+        self.harness.begin()
+        mock_check_output.side_effect = subprocess.TimeoutExpired(
+            "/usr/local/bin/auto_lint.py",
+            60,
+            output=(
+                "Command '/usr/local/bin/auto_lint.py' timed out "
+                "after 60 seconds".encode()
+            ),
+        )
+        # with pid file
+        mock_pid_file.exists.return_value = True
+        with self.assertRaises(subprocess.TimeoutExpired):
+            self.harness.charm.helper.run_auto_lint()
+        mock_pid_file.assert_has_calls(
+            [unittest.mock.call.exists(), unittest.mock.call.unlink()], any_order=True
+        )
+        mock_pid_file.reset_mock()
+
+        # without pid file
+        mock_pid_file.exists.return_value = False
+        with self.assertRaises(subprocess.TimeoutExpired):
+            self.harness.charm.helper.run_auto_lint()
+        mock_pid_file.assert_has_calls([unittest.mock.call.exists()], any_order=True)
+
 
 if __name__ == "__main__":
     unittest.main()