Merge ~txiao/charm-logrotated:handle_unset into charm-logrotated:master

[Tianqi Xiao]

Handle "unset" value of update-cron-daily-schedule and fix workload status logic

Previously, validating "unset" value would result in silent error because:
1. the con_mapping in CronHelper.validate_cron_conf() doesn't contain "unset" key
which leads to an attemption of concatenating str with NoneType in later lines
2. wrongly checking index 0 of a str instead of list for self.cron_daily_schedule
in CronHelper.install_cronjob()
3. wrong logic for setting status in install and config_changed hook, which means
the workload always ends up in "active" even when exceptions are raised
4. didn't set workload status to blocked when executing cronjob.

This commit fixes the above mentioned issues and added related unit tests.
Additionally, more logs are added to provide extra information.

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

PASSED: Continuous integration, rev:1be70cc630c1f111d09f8a0ac5e59f6685fdb0ee
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/4/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/622/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/221/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/4//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

Good catch. It's shame that we didn't aware this in the previous code review
https://code.launchpad.net/~llama-charmers/charm-logrotated/+git/charm-logrotated/+merge/435334

Furthermore, we should not skip Diko's suggest in the MR too. From the source code, I do not find the code to reset the value. ( set -> unset, or random -> unset ). This can be another feature request. I will let Kamal review the code and confirm my idea is correct.

review: Needs Information

[Chi Wai CHAN (raychan96)]

I left some inline comments for some nitpicks (non-blocking)

[Kamal Bhaskar (kamalb)]

> Good catch. It's shame that we didn't aware this in the previous code review
> https://code.launchpad.net/~llama-charmers/charm-logrotated/+git/charm-
> logrotated/+merge/435334
>
> Furthermore, we should not skip Diko's suggest in the MR too. From the source
> code, I do not find the code to reset the value. ( set -> unset, or random ->
> unset ). This can be another feature request. I will let Kamal review the code
> and confirm my idea is correct.

Yes, your idea is absolutely correct and make sense. Resetting the config back to 'unset' from 'set' or 'random' is missing here and that can be taken as a feature request definitely.

[Kamal Bhaskar (kamalb)]

The code commit here LGTM and take care of the reported bug/problem.

review: Approve

[Eric Chen (eric-chen)]

Please check Ray's inline comment, he forgot to leave a status for "Needs Fixing"

review: Needs Fixing

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

PASSED: Continuous integration, rev:bff6b94d05de0283cedf121aa8f070426183edfc
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/5/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/668/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/224/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/5//rebuild

review: Approve (continuous-integration)

[Tianqi Xiao (txiao)]

Thanks @eric-chen @kamalb @raychan96 @rgildein for the reviews. The MP has been updated to include the suggested changes. For the resetting the cron value, since it's out of the current scope, I opened LP#2016659 to track it[1]

[1]: https://bugs.launchpad.net/charm-logrotated/+bug/2016659

[Robert Gildein (rgildein)]

LGTM, thanks

review: Approve

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

PASSED: Continuous integration, rev:8746e270196d0464b2c4e230f34cec756da15f11
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/6/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/669/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/225/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/6//rebuild

review: Approve (continuous-integration)

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

Change successfully merged at revision a137eb7f109dca499ec59fe7bcc7c47ee93d4596

diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index 1287524..0e1c9a9 100644
--- a/src/lib/lib_cron.py
+++ b/src/lib/lib_cron.py
@@ -76,7 +76,7 @@ class CronHelper:
             if (
                 self.validate_cron_conf()
                 and self.cronjob_frequency == 1
-                and self.cron_daily_schedule[0] != "unset"
+                and self.cron_daily_schedule.split(",")[0] != "unset"
             ):
                 self.update_cron_daily_schedule()
 
@@ -143,24 +143,33 @@ class CronHelper:
         try:
             conf = self.cron_daily_schedule
             conf = conf.split(",")
-            if conf[0] not in ("unset", "set", "random") or len(conf) > 3:
+            operation = conf[0]
+            if operation not in ("unset", "set", "random") or len(conf) > 3:
                 raise ValueError(
                     "Invalid value for update-cron-daily-schedule: {}".format(conf)
                 )
 
-            conf_mapping = {
-                "set": "_validate_set_schedule",
-                "random": "_validate_random_schedule",
-            }
+            result = True
+            # run additional validation functions
+            if operation == "set":
+                result = self._validate_set_schedule(conf)
+            elif operation == "random":
+                result = self._validate_random_schedule(conf)
 
-            conf_handler = conf_mapping.get(conf[0])
-            result = eval("self." + conf_handler)(conf)
             return result
 
         except ValueError as err:
+            hookenv.log(
+                "Cron config validation failed: {}".format(err),
+                level=hookenv.ERROR,
+            )
+            hookenv.status_set(
+                "blocked", "Cron config validation failed. Check log for more info."
+            )
             raise self.InvalidCronConfig(err)
 
     def _validate_set_schedule(self, conf):
+        """Validate update-cron-daily-schedule when the "set" keyword exists."""
         cron_daily_time = conf[1].split(":")
         if not self._valid_timestamp(cron_daily_time):
             raise ValueError(
@@ -173,6 +182,7 @@ class CronHelper:
             return True
 
     def _validate_random_schedule(self, conf):
+        """Validate update-cron-daily-schedule when the "random" keyword exists."""
         cron_daily_start_time = conf[1].split(":")
         cron_daily_end_time = conf[2].split(":")
         if not (
@@ -197,11 +207,13 @@ class CronHelper:
 
 def main():
     """Ran by cron."""
+    hookenv.log("Executing cron job.", level=hookenv.INFO)
     hookenv.status_set("maintenance", "Executing cron job.")
     cronhelper = CronHelper()
     cronhelper.read_config()
     cronhelper.update_logrotate_etc()
     cronhelper.install_cronjob()
+    hookenv.log("Cron job completed.", level=hookenv.INFO)
     hookenv.status_set("active", "Unit is ready.")
 
 
diff --git a/src/reactive/logrotate.py b/src/reactive/logrotate.py
index b699e8f..86620fa 100644
--- a/src/reactive/logrotate.py
+++ b/src/reactive/logrotate.py
@@ -23,7 +23,12 @@ def install_logrotate():
         logrotate.modify_configs()
         cron.install_cronjob()
     except Exception as ex:
-        hookenv.status_set("blocked", str(ex))
+        hookenv.log(
+            "Error running install hook: {}".format(str(ex)),
+            level=hookenv.ERROR,
+        )
+        hookenv.status_set("blocked", "Install hook failed. Check logs for more info.")
+        return
     hookenv.status_set("active", "Unit is ready.")
     set_flag("logrotate.installed")
 
@@ -39,7 +44,14 @@ def config_changed():
         logrotate.modify_configs()
         cron.install_cronjob()
     except Exception as ex:
-        hookenv.status_set("blocked", str(ex))
+        hookenv.log(
+            "Error running config-changed hook: {}".format(str(ex)),
+            level=hookenv.ERROR,
+        )
+        hookenv.status_set(
+            "blocked", "Config-changed hook failed. Check logs for more info."
+        )
+        return
     hookenv.status_set("active", "Unit is ready.")
 
 
diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py
index 4871183..c0c5cdb 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -142,6 +142,15 @@ class TestLogrotateHelper:
 class TestCronHelper:
     """Main cron test class."""
 
+    def test_cron_daily_schedule_unset(self, cron):
+        """Test the validation of unset update-cron-daily-schedule config value."""
+        cron_config = cron()
+        cron_config.cronjob_enabled = True
+        cron_config.cronjob_frequency = 1
+        cron_config.cron_daily_schedule = "unset"
+
+        assert cron_config.validate_cron_conf()
+
     @pytest.mark.parametrize(
         ("cron_schedule, exp_pattern"),
         [