Merge ~lihuiguo/charm-logrotated:bug/1864598 into charm-logrotated:master

[Linda Guo]

Clear up previous cron job file when cron job config is changed

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

It's dangerous for the logic when we want to modify it in the future.
The hidden logic here is.. we only put the cronjob file with one frequency ["hourly", "daily", "weekly", "monthly"]. That is the design limitation in charm-logrotate, not a limitation in linux cronjob. But I don't have good idea to make this design limitation explict in the cleanup_cronjob function right now.

I have other ideas to remove some duplicated code and avoid additional parameter in the cleanup_cronjob.

1. There could be a cronjob filepath generator to be used in install_cronjob and cleanup_cronjob
2. We can use frequency index to generate the path and it's easy to know which one should be reserved in the cleanup_cronjob. We also can change the parameter as an array in the future if need.

/* Pseudo Code */
def cleanup_cronjob(self, reserve=-1):
    '''
    if reserve = -1, delete cronjob file in all frequencies
    else: only reserve the cronjob file in corresponding folder.
    '''

    for i in range(len(self.cronjob_check_paths)):
        if reserve == -1 or i != reserve:
            path = cronjob_path_generator(i)
            os.remove(path)

[Eric Chen (eric-chen)]

Mark it as WIP until Linda provide the 2nd patch or continue the discussion

[Linda Guo (lihuiguo)]

Thanks Eric, I refactored the code to make it more easy for maintenance. Can you please take another look?

[Eric Chen (eric-chen)]

LGTM

review: Approve

[Eric Chen (eric-chen)]

review: Needs Information

[Eric Chen (eric-chen)]

I forget to request the log of unit&func test. Please prove the testing are pass. Thanks!

review: Needs Information

[Linda Guo (lihuiguo)]

unit tests: https://pastebin.canonical.com/p/3RzfjRBp2w/

[Linda Guo (lihuiguo)]

functional test is broken at MASTER branch, it's stuck at test_deploy[xenial] https://pastebin.canonical.com/p/Yj63xsbSvM/

Unit Workload Agent Machine Public address Ports Message
ubuntu-xenial/0* error idle 0 10.5.3.224 hook failed: "install"

juju log: https://pastebin.canonical.com/p/g7t3Svt9ZM/

[Eric Chen (eric-chen)]

> functional test is broken at MASTER branch, it's stuck at test_deploy[xenial]
> https://pastebin.canonical.com/p/Yj63xsbSvM/
>
> Unit Workload Agent Machine Public address Ports Message
> ubuntu-xenial/0* error idle 0 10.5.3.224 hook
> failed: "install"
>
> juju log: https://pastebin.canonical.com/p/g7t3Svt9ZM/

That is a known issue according to this MP.

https://code.launchpad.net/~txiao/charm-logrotated/+git/charm-logrotated/+merge/427993

But could you pass the func test on bionic, focal and jammy ? Thanks!

[Linda Guo (lihuiguo)]

yes. func test on bionic, focal and jammy passed. sorry, forgot to post the result https://pastebin.canonical.com/p/BkDTykKV2R/

[Eric Chen (eric-chen)]

review: Approve

[Eric Chen (eric-chen)]

Wait for one more engineer review it , then we can merge it.
I will let Garbriel to help it, but you can also call for help in MM.

[Gabriel Cocenza (gabrielcocenza)]

Hi Linda.

Thanks for this patch. I just have a small observation about the "cleanup_cronjob" that I don't think has a good readability.

review: Needs Information

[Eric Chen (eric-chen)]

Mark it as "WIP" until Linda have time to work it. Or please call for help from DOC. thanks

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

FAILED: Continuous integration, rev:ee2ae00d4a2dc0719778803ebc119980fdb35c3f
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/8/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/982/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1404/

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

review: Needs Fixing (continuous-integration)

Unmerged commits

ee2ae00... by Linda Guo on 2022-08-16

Clear up previous cron job file

Clear up previous cron job file when cron job config is changed.

diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index 5c17738..7114084 100644
--- a/src/lib/lib_cron.py
+++ b/src/lib/lib_cron.py
@@ -39,17 +39,12 @@ class CronHelper:
         If logrotate-cronjob config option is set to True install cronjob,
         otherwise cleanup.
         """
-        clean_up_file = self.cronjob_frequency if self.cronjob_enabled else -1
+        cronjob_freq = self.cronjob_frequency if self.cronjob_enabled else -1
 
         if self.cronjob_enabled is True:
             cronjob_path = os.path.realpath(__file__)
-            cron_file_path = (
-                self.cronjob_base_path
-                + self.cronjob_check_paths[clean_up_file]
-                + "/"
-                + self.cronjob_logrotate_cron_file
-            )
-
+            cron_file_path = self.cronjob_path_generator(
+                self.cronjob_check_paths[cronjob_freq])
             logrotate_unit = hookenv.local_unit()
             python_venv_path = os.getcwd().replace("charm", "") + ".venv/bin/python3"
             # upgrade to template if logic increases
@@ -63,22 +58,20 @@ class CronHelper:
             cron_file.close()
             os.chmod(cron_file_path, 700)
 
-        self.cleanup_cronjob(clean_up_file)
+        self.cleanup_cronjob(cronjob_freq)
 
     def cleanup_cronjob(self, frequency=-1):
-        """Cleanup previous config."""
-        if frequency == -1:
-            for check_path in self.cronjob_check_paths:
-                path = (
-                    self.cronjob_base_path
-                    + check_path
-                    + "/"
-                    + self.cronjob_logrotate_cron_file
-                )
-                if os.path.exists(path):
+        """Cleanup previous config.
+
+        If frequency=-1, clear all cron jobs
+        """
+        for check_path in self.cronjob_check_paths:
+            path = self.cronjob_path_generator(check_path)
+            if os.path.exists(path):
+                if frequency == -1 or self.cronjob_check_paths[frequency] != check_path:
                     os.remove(path)
-            if os.path.exists(self.cronjob_etc_config):
-                os.remove(self.cronjob_etc_config)
+        if frequency == -1 and os.path.exists(self.cronjob_etc_config):
+            os.remove(self.cronjob_etc_config)
 
     def update_logrotate_etc(self):
         """Run logrotate update config."""
@@ -86,6 +79,16 @@ class CronHelper:
         logrotate.read_config()
         logrotate.modify_configs()
 
+    def cronjob_path_generator(self, check_path):
+        """Generate the cron job file path."""
+        path = (
+                self.cronjob_base_path
+                + check_path
+                + "/"
+                + self.cronjob_logrotate_cron_file
+            )
+        return path
+
 
 def main():
     """Ran by cron."""