Merge ~dashmage/charm-logrotated:bug/2017798-update-cron-daily-location into charm-logrotated:master

[Ashley James]

Change from sudo user to system crontab for cron daily

`update-cron-daily-schedule` currently updates the sudo user present in `/var/spool/cron/crontabs/root` instead of the system crontab file (`/etc/crontab`). This change was performed so that the script only runs from a single crontab file and to fix `hookenv.log` which wasn't correctly executed when running with sudo user cronjob (messages did not show up in juju debug-log).

Fixes LP#2017798

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

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

[Ashley James (dashmage)]

I see one potential problem now - in an upgrade scenario, even after implementing this fix, the sudo user crontab would still exist if cron-update-daily-schedule was configured. This would mean that the script would still run twice.

I propose that we add a function that checks whether the `/var/spool/cron/crontabs/root` crontab file exists and remove it in case it is present since we are only managing through `/etc/crontab`.

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

FAILED: Continuous integration, rev:6bb452dd5edde13992be6bd957ff4bd44fa31643
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/23/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/877/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/282/

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

review: Needs Fixing (continuous-integration)

[Ashley James (dashmage)]

> Is this really wisely to overwritte system crontab? Why not put new file to /etc/cron.d/<name>?

Even if we add this as a new file to `/etc/cron.d/<name>`, the script (placed in cron.daily) would be run twice - once for `/etc/crontab` and then for `/etc/cron.d/<name>`.

In case we're modifying the time for running cron.daily scripts and want to add it as a separate file in `/etc/cron.d`, we would need to remove the line from `/etc/crontab`. I prefer we make the change directly in `/etc/crontab` itself for changing the time.

Edit: Just to clarify, the `update-cron-daily-schedule` will only change the time for running the cron.daily scripts in the /etc/crontab file.

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

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

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:86032494a23a43b76c1ddb0dc5de155230b8f6c5
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/25/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/885/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/284/

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:86032494a23a43b76c1ddb0dc5de155230b8f6c5
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/27/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/887/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/286/

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

review: Approve (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:86032494a23a43b76c1ddb0dc5de155230b8f6c5
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/29/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/889/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/288/

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

review: Approve (continuous-integration)

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

FAILED: Continuous integration, rev:714a28da31c71fdc518e4b77e13d57c13c586113
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/30/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/890/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/289/

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:e4c83fa67de4aa2fde9b00fab579731dea6211e5
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/31/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/891/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/290/

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

review: Needs Fixing (continuous-integration)

[Robert Gildein (rgildein)]

LGTM, thanks

review: Approve

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

FAILED: Continuous integration, rev:e4c83fa67de4aa2fde9b00fab579731dea6211e5
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/32/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/892/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/291/

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:c73e48e05b51bac001963dc6e4e2deef37cace1e
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/33/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/893/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/292/

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

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

I am curious

1. who is the runner when this library being called. (Ubuntu? Juju?) why they have root permission.
2. what's the different between f.flush and f.seek.
3. The logic looks like a magic if we want to copy the temp file. Is there better way to avoid the magic flush() or seek() function? I prefer not to use some implictly ability of flush() or seek() here. I prefer to make it just like a human do here.

#1 create temp file
#2 write data and close
#3 copy file
#4 delete temp file

review: Needs Information

[Ashley James (dashmage)]

> I am curious
>
> 1. who is the runner when this library being called. (Ubuntu? Juju?) why they
> have root permission.

After looking into this, the script is called in cron.daily (or weekly/monthly depending on config) and it runs as root. Here is the relevant part in the code [1]. Which is why the copy command works without "sudo". I propose we can remove the sudo while copying from the tmp file.

```
$ cat /etc/cron.daily/charm-logrotated
#!/bin/bash
/usr/bin/sudo /usr/bin/juju-run logrotated/1 "/var/lib/juju/agents/unit-logrotated-1/.venv/bin/python3 /var/lib/juju/agents/unit-logrotated-1/charm/lib/lib_cron.py"
```

[1]: https://git.launchpad.net/charm-logrotated/tree/src/lib/lib_cron.py#n66

> 2. what's the different between f.flush and f.seek.

f.seek() would make the file pointer return to zero so that we can read contents from the start of the file. f.flush() is used to clear the internal buffer and put contents into the file explicitly before f.close() or reaching the end of the context manager (where the file is automatically closed). In this situation we could also use f.flush and push the contents to the tmp file before we read it, I just chose to pick f.seek here.

> 3. The logic looks like a magic if we want to copy the temp file. Is there
> better way to avoid the magic flush() or seek() function? I prefer not to use
> some implictly ability of flush() or seek() here. I prefer to make it just
> like a human do here.
>
> #1 create temp file
> #2 write data and close
> #3 copy file
> #4 delete temp file

In case you suggest to avoid using seek/flush, we can do it this way to make it more like how a human would do it:

```
with open(r"/tmp/crontab","w") as tmp_crontab:
    tmp_crontab.write(data)

try:
    cmd = ["cp", "/tmp/crontab", "/etc/crontab"]
    subprocess.run(cmd, check=True)

except subprocess.CalledProcessError as exception:
    hookenv.log(
        f"Writing to system crontab failed with error: {exception}",
        level=hookenv.ERROR,
    )
    raise
os.remove("/tmp/crontab")
```

[Eric Chen (eric-chen)]

Based on the description you provided in the document, f.flush() is more reasonable. There is no clue that f.seek() will flush the data into the disk. Anyway, I prefer we handle it like a human way unless there is any specific reason we must copy the file before we close the file handle.

review: Needs Fixing

[Eric Chen (eric-chen)]

I'm so surprise that we will execute /var/lib/juju/agents/unit-logrotated-1/charm/lib/lib_cron.py from the cronjob. It is very unreasonable to me. I suppose that every library should be used during

- event handler under src/reactive
- juju actions under src/actions

Could you check the reason why we need to design like this?

BTW, update_cron_daily_schedule is not only used in /etc/cron.daily/charm-logrotated, it may be triggered from

src/actions -> update_cronjob -> cron.install_cronjob -> update_cron_daily_schedule

The runner of these two behaviors may be different. Could you also confirm it?

1. from cron.daily
2. from juju actions

review: Needs Information

[Chi Wai CHAN (raychan96)]

LGTM

review: Approve

[Eric Chen (eric-chen)]

Change the status to "Needs Fixing"

review: Needs Fixing

[Ashley James (dashmage)]

> I'm so surprise that we will execute /var/lib/juju/agents/unit-
> logrotated-1/charm/lib/lib_cron.py from the cronjob. It is very unreasonable
> to me. I suppose that every library should be used during
>
> - event handler under src/reactive
> - juju actions under src/actions
>
> Could you check the reason why we need to design like this?

After going through the code, this is what I could infer ->

The cron_lib.py code should be executed by the cronjob because the charm is used to rotate the logrotate.d files at the regular interval that is determined by the time configured in the cronjob. The event handlers under src/reactive defines install/config-changed hook to run logrotate.modify_configs once during that event but to have it run regularly by the charm it would need to be present in the cronjob so that the script can be invoked at the hourly/daily/monthly times. Are you suggesting that instead of invoking the library script from the cronjob, there's another way to trigger the charm functionality at the different intervals?

In my view - the juju action allows to install the cronjob/ update logrotate files manually by the operator if they don't want to wait for the cronjob trigger time.

======

> BTW, update_cron_daily_schedule is not only used in /etc/cron.daily/charm-
> logrotated, it may be triggered from
>
> src/actions -> update_cronjob -> cron.install_cronjob ->
> update_cron_daily_schedule
>
> The runner of these two behaviors may be different. Could you also confirm it?
>
> 1. from cron.daily
> 2. from juju actions

So even if this functionality is triggered from src/actions (with update_cronjob) or during execution of the install hook event handler or config changed handler, they utilize the same `CronHelper.install_cronjob` method which then runs update_cron_daily_schedule`. Therefore, the runner in both cases will be root since sudo is used in the crontab entry to call the library script.

cron.daily is set with the `logrotate-cronjob-frequency` config and is used both by the action and by the other event handlers of the charm to run the same `update_cron_daily_schedule` function.

53248e0... by Ashley James on 2023-05-12

Fix flaky functional test

- Use tenacity.Retrying while checking for file in
`test_reconfigure_cronjob_frequency`

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

PASSED: Continuous integration, rev:7bc0ebbcff3972475002a3eaf7bf8217013dd885
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/34/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/902/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/301/

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

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:a213ad7957ba7d86ba8b426d5154dcb267fa6604
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/35/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/903/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/302/

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

review: Approve (continuous-integration)

[Tianqi Xiao (txiao)]

Have you considered using a mature library (e.g. python-crontab [1]) to interact with crontab? I'm a bit worried that there are some consequences we have not considered when directly overwriting the cron file.

[1]: https://pypi.org/project/python-crontab/

review: Needs Information

[Eric Chen (eric-chen)]

I cannot fully understand your answer above. I want to know why we need to execute cron_lib.py regularly. But I didn't find the real reason in your explanation.

My major concern is

1. In general case, the library in charm is used under the juju framework. The design in logrotate breaks the common situation and we should know the real reason. Maybe the solution is not to copy the file, we should just avoid this behavior.

2. Sudo is a dangerous command, we should only use it when needed. We should also need to know how the executor gets the permission?

3. subprocess is always my last choice to handle the program. because it implies many assumptions of the environment. So I agree with Tianqi's comment, we should consider the mature library. Even when we copy the file, we should think about how to use shutil.move if possible. (In the previous commit, you didn't use shutil.move when there is no "sudo").

4. "Retry" is also something I don't like a lot. Please write down the reason why we cannot avoid it. If there is a bug that only happen with 30% possibility, it will be concealed by the "Retry" mechanism.

I suggest that you can write a spec to clarify all the things and purpose the proposal again.

review: Needs Fixing

[Ashley James (dashmage)]

Problem
-------
Currently when the logrotate-cronjob-frequency config option is set to daily, then the configured value for update-cron-daily-schedule (eg: set,08:00) is used to update the job execution time for the logrotated cron.daily job. This is done using the root user’s crontab (with `sudo crontab -u root <file_with_new_crontab_contents>`). But this is in conflict with the job that already exists in the system crontab (/etc/crontab) which means that the cron.daily job would run twice. It also

Fixes LP#2017798 [1]

Proposal
--------
For daily cronjobs, create a new cron file in /etc/cron.d where the charm will be able to manage the execution time without messing with the system crontab (/etc/crontab) or the root user’s crontab. It also prevents the charm from running commands with sudo.

I’m proposing to add a new file to /etc/cron.d instead of modifying the cron.daily execution time directly in the system crontab (/etc/crontab) because this would affect the execution of other scripts in cron.daily.

Changes performed by MP
-----------------------
- Use tenacity.retry while checking whether a file exists in the functional tests. This prevents failure in cases of performance issues on the CI server.

- 2 new files are now managed by the charm --> `cronjob_daily_command_path` which has the juju-run command that executes the lib_cron.py script and is called from the cronjob. `cronjob_daily_file` is the file being added in /etc/cron.d.

- In the `update_cron_daily_schedule` method
    - Renames the function to something more appropriate (`create_cron_daily_job`) since we're creating a new cron job and not updating the cron.daily job.
    - Replaces updating root user crontab with creating a new cron file in /etc/cron.d for daily cronjob

- Replace dangling `open` function calls with a context manager in `install_cronjob`.

- `cleanup_cronjob_files` also cleans up the new daily cronjob files.

I'm still working on these:
- Fix existing unit tests in order to not write directly to the crontab of the system where it's being run. Mock the required files which are being accessed.
- Re-write unit test for `create_cron_daily_job` method.

Other Notes
------------
I couldn’t use the python-crontab [2] library in order to manage the crontab changes since it does not support the random execution time syntax (00~45 08~09). This feature is not supported by the cron package which is used by Ubuntu in the first place [3]. A separate bug report has been raised for this issue [4].

-------------

[1]: https://bugs.launchpad.net/charm-logrotated/+bug/2017798
[2]: https://pypi.org/project/python-crontab/
[3]: https://packages.ubuntu.com/jammy/cron
[4]: https://bugs.launchpad.net/charm-logrotated/+bug/2019990

[Mert Kirpici (mertkirpici)]

I am a little bit late to the discussion here, but I think I found the reason for the self-installing cronjob design decision in this charm, that Eric, Ray, Ashley and I were wondering at different times.

Taking from the original bug report:

LP #1832296: Create functionality to manage logrotate files
https://bugs.launchpad.net/charm-logrotated/+bug/1832296

"""
- As some packages might change the logrotate configurations - a cronjob option needs to be defined, where it will constantly update the configurations with regards to the retention period defined
"""

So it is for making sure, our retention settings are taking precedence if any other package/operator makes changes to the /etc/logrotate.d/ files. While I am not sure that it would be the right place to do this, but i suppose we could move this functionality to the "update-status" hook, since it is fired periodically.

Unmerged commits

78a0422... by Ashley James on 2023-05-09

Remove cron daily job from root user crontab.

Currently while modifying cron daily job execution time, it is
updated in the root user's crontab (/var/spool/cron/crontabs/root).
This leads to a duplicate execution along with the execution of
cron.daily scripts in /etc/crontab.

This commit aims to manage the execution of the charms' daily cron job
solely from the system crontab, /etc/crontab. It also fixes the existing
unit test `test_cron_daily_schedule` and mocks the crontab instead of
directly writing to it.

53248e0... by Ashley James on 2023-05-12

Fix flaky functional test

- Use tenacity.Retrying while checking for file in
`test_reconfigure_cronjob_frequency`

diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index 1f30ddc..a314a55 100644
--- a/src/lib/lib_cron.py
+++ b/src/lib/lib_cron.py
@@ -1,7 +1,6 @@
 """Cron helper module."""
 import os
 import re
-import subprocess
 
 from charmhelpers.core import hookenv
 
@@ -62,15 +61,14 @@ class CronHelper:
             logrotate_unit = hookenv.local_unit()
             python_venv_path = os.getcwd().replace("charm", "") + ".venv/bin/python3"
             # upgrade to template if logic increases
-            cron_file = open(cron_file_path, "w")
             cron_job = """#!/bin/bash
 /usr/bin/sudo /usr/bin/juju-run {} "{} {}"
 """.format(
                 logrotate_unit, python_venv_path, cronjob_path
             )
-            cron_file.write(cron_job)
-            cron_file.close()
-            os.chmod(cron_file_path, 700)
+            with open(cron_file_path, "w") as cron_file:
+                cron_file.write(cron_job)
+            os.chmod(cron_file_path, 0o744)
 
             # update cron.daily schedule if logrotate-cronjob-frequency set to "daily"
             if self.cronjob_frequency == 1 and self.validate_cron_conf():
@@ -123,7 +121,16 @@ class CronHelper:
         else:
             raise RuntimeError("Unknown daily schedule type: {}".format(schedule_type))
 
-        cron_daily_timestamp = cron_daily_minute + " " + cron_daily_hour + "\t"
+        cron_daily_timestamp = cron_daily_minute + " " + cron_daily_hour
+        self.write_to_crontab(cron_daily_timestamp)
+
+        return cron_daily_timestamp
+
+    def write_to_crontab(self, cron_daily_timestamp, target="/etc/crontab"):
+        """Write daily cronjob with provided timestamp to target crontab.
+
+        Default value for target is system crontab (/etc/crontab).
+        """
         cron_pattern = re.compile(r".*\/etc\/cron.daily.*")
         with open(r"/etc/crontab", "r") as crontab:
             data = crontab.read()
@@ -132,15 +139,13 @@ class CronHelper:
         updated_cron_daily = ""
         if cron_daily:
             updated_cron_daily = re.sub(
-                r"\d?\d(~\d\d)?\s\d?\d(~\d\d)?\t", cron_daily_timestamp, cron_daily[0]
+                r"\d?\d(~\d\d)?\s\d?\d(~\d\d)?\t",
+                cron_daily_timestamp + "\t",
+                cron_daily[0],
             )
-            data = data.replace(cron_daily[0], updated_cron_daily)
-            with open(r"/tmp/crontab", "w") as crontab:
-                crontab.write(data)
-
-            subprocess.check_output(["sudo", "crontab", "-u", "root", "/tmp/crontab"])
-
-        return updated_cron_daily
+            updated_data = data.replace(cron_daily[0], updated_cron_daily)
+            with open(target, "w") as crontab:
+                crontab.write(updated_data)
 
     def validate_cron_conf(self):
         """Block the unit and exit the hook if there is invalid configuration."""
diff --git a/src/tests/functional/requirements.txt b/src/tests/functional/requirements.txt
index 6c4de97..4128a5d 100644
--- a/src/tests/functional/requirements.txt
+++ b/src/tests/functional/requirements.txt
@@ -7,3 +7,4 @@ mock
 pytest
 pytest-asyncio
 requests
+tenacity
diff --git a/src/tests/functional/test_logrotate.py b/src/tests/functional/test_logrotate.py
index 0afbb82..7088371 100644
--- a/src/tests/functional/test_logrotate.py
+++ b/src/tests/functional/test_logrotate.py
@@ -9,6 +9,8 @@ import pytest
 
 import pytest_asyncio
 
+import tenacity
+
 pytestmark = pytest.mark.asyncio
 SERIES = ["bionic", "focal", "jammy"]
 
@@ -118,10 +120,17 @@ async def test_reconfigure_cronjob_frequency(model, deploy_app, unit, jujutools)
     await model.block_until(lambda: deploy_app.status == "active")
     config = await deploy_app.get_config()
 
-    result = await jujutools.run_command(
-        "test -f /etc/cron.weekly/charm-logrotate", unit
-    )
-    weekly_cronjob_exists = result["return-code"] == 0
+    # Retry because test fails sometimes when cron file isn't ready yet
+    for attempt in tenacity.Retrying(
+        stop=tenacity.stop_after_attempt(3),
+        wait=tenacity.wait_exponential(multiplier=1, min=2, max=10),
+    ):
+        with attempt:
+            result = await jujutools.run_command(
+                "test -f /etc/cron.weekly/charm-logrotate", unit
+            )
+            weekly_cronjob_exists = result["return-code"] == 0
+            assert weekly_cronjob_exists
 
     result = await jujutools.run_command(
         "test -f /etc/cron.daily/charm-logrotate", unit
@@ -130,7 +139,6 @@ async def test_reconfigure_cronjob_frequency(model, deploy_app, unit, jujutools)
 
     assert config["logrotate-cronjob-frequency"]["value"] == "weekly"
     assert not daily_cronjob_exists
-    assert weekly_cronjob_exists
 
 
 async def test_configure_override_01(model, deploy_app, jujutools, unit):
diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py
index 8d329e5..ee136cd 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -1,6 +1,7 @@
 """Main unit test module."""
 import json
 import os
+import re
 from textwrap import dedent
 from unittest import mock
 
@@ -245,18 +246,40 @@ class TestCronHelper:
             ("unset", "25 6"),
         ],
     )
-    def test_cron_daily_schedule(self, cron, cron_schedule, exp_pattern):
+    def test_cron_daily_schedule(self, cron, cron_schedule, exp_pattern, mocker):
         """Test the validate and update random cron.daily schedule."""
         cron_config = cron()
         cron_config.cronjob_enabled = True
         cron_config.cronjob_frequency = 1
         cron_config.cron_daily_schedule = cron_schedule
 
-        assert cron_config.validate_cron_conf()
+        mock_method = mocker.Mock()
+        mocker.patch.object(cron, "write_to_crontab", new=mock_method)
 
         updated_cron_daily = cron_config.update_cron_daily_schedule()
 
+        assert cron_config.validate_cron_conf()
         assert updated_cron_daily.split("\t")[0] == exp_pattern
+        mock_method.assert_called_once_with(exp_pattern)
+
+    @pytest.mark.parametrize("cron_daily_timestamp", ["00 08", "25 6", "00~50 06~07"])
+    def test_write_to_crontab(self, cron, cron_daily_timestamp, tmp_path):
+        """Test function that writes updated data to crontab file."""
+        cron_config = cron()
+        tmp_cronjob = tmp_path / "cron_file"
+
+        # write updated content into temp crontab file
+        cron_config.write_to_crontab(cron_daily_timestamp, target=tmp_cronjob)
+
+        with open(tmp_cronjob) as crontab:
+            data = crontab.read()
+
+        # get cron daily job from crontab data
+        cron_pattern = re.compile(r".*\/etc\/cron.daily.*")
+        cron_daily = cron_pattern.findall(data)
+
+        # assert if timestamps are equal
+        assert cron_daily[0].split("\t")[0] == cron_daily_timestamp
 
     @pytest.mark.parametrize(
         ("cron_schedule"),
@@ -292,9 +315,9 @@ class TestCronHelper:
             return_value=os.path.join(mock_charm_dir, "lib/lib_cron.py"),
         )
         mocker.patch("lib_cron.os.getcwd", return_value=mock_charm_dir)
-        mock_open = mocker.patch("lib_cron.open")
-        mock_handle = mock_open.return_value
-
+        mock_open = mocker.mock_open()
+        mocker.patch("lib_cron.open", mock_open)
+        mock_handle = mock_open.return_value.__enter__.return_value
         expected_files_to_be_removed = [
             "/etc/cron.hourly/charm-logrotate",
             "/etc/cron.daily/charm-logrotate",
@@ -322,8 +345,7 @@ class TestCronHelper:
                 """  # noqa
             )
         )
-        mock_handle.close.assert_called_once()
-        mock_chmod.assert_called_once_with("/etc/cron.weekly/charm-logrotate", 700)
+        mock_chmod.assert_called_once_with("/etc/cron.weekly/charm-logrotate", 0o744)
 
     def test_install_cronjob_removes_etc_config_when_cronjob_disabled(
         self, cron, mocker