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

[Ashley James]

Manage charm's daily cronjob from /etc/crontab instead of root user's crontab.

`update-cron-daily-schedule` currently updates the charm's daily cronjob with the root user crontab present in `/var/spool/cron/crontabs/root`. This change was performed so that the cron.daily script only runs from a single crontab file (/etc/crotab) 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).

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.

- In the `update_cron_daily_schedule` method -- prevent writing to root user's crontab. Instead create a separate function (write_to_crontab) where /etc/crontab daily cron job time is modified.

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

- Fix existing unit tests in order mock the file and not write directly to the crontab of the system where it's being run.

- Add unit test for the new function `write_to_crontab`.

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.

[🤖 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.

[🤖 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:78a0422806db70feecddf7a19472ecb24258f2ee
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/39/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/966/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/311/

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

review: Needs Fixing (continuous-integration)

[Ashley James (dashmage)]

I've updated the MP with an important change - reverting from adding a file in /etc/cron.d and instead updating `/etc/crontab` itself. This is because the charm would also need to manage the `/etc/cron.daily/logrotate` file and this would involve changing the system crontab. Since we can't move the logrotate cronjob (provided by the apt package) out from cron.daily, we need to directly modify the time schedule in `/etc/crontab`.

Another important observation is that the `lib_cron.install_cronjob` and `lib_logrotate.modify_configs` are called in 3 situations - install/config changed hook, running an action and from the cronjob itself. In all these three places, the user running the code is root.

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

PASSED: Continuous integration, rev:01542d282785cd3febd4e859fa326dafccb8ebdc
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/41/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/969/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/314/

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

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

Please provide the final result we write into /etc/crontab for the verificiation

There are also inline comment. Not big change.

review: Needs Fixing

[JamesLin (jneo8)]

One inline comment.

review: Needs Fixing

[Ashley James (dashmage)]

> ditto, 0o755 or 0o644 ? , 0x744 seems weird

Yeah, I agree. I've made this `0o755` now.

> It seems we add function argument `target` is just because we need to test it.
But the value will never change in real case.
I will suggest to remove it and mock the `open` syntax.

I've now modified the unit test so that it mocks the open context manager instead of relying on the `target` argument which would never change. Good suggestion!

> Please provide the final result we write into /etc/crontab for the verificiation

As part of the updated unit tests, the final results are now specified and verified.

[Eric Chen (eric-chen)]

LGTM

review: Approve

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

PASSED: Continuous integration, rev:ce9dbcf7095d93c3c312f46f25ad6c1cc7bf83cd
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/43/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/971/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/316/

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

review: Approve (continuous-integration)

[Chi Wai CHAN (raychan96)]

Thanks for working on the fix.

After thinking this a bit, I am wondering why we are insist on using the default /etc/crontab and modify the cron.daily schedule, or copy the default /etc/crontab and modify the cron.daily schedule and put it under root's crontab. I think this is undesired because it will affect all cron jobs under /etc/cron.daily, not just the logrotate charm's cronjob.

One of the possible alternative is to just maintain our own cron job without using the default crontab.

For example, we can either 1. create a crontab for root user with only one entry, or 2. append an entry to default /etc/crontab without modifying original content

```
17 * * * * root path_to_charm_logrotate
```

This way, we can be sure that even if we add to root user's cronjob or /etc/crontab, it will not have big impacts. What do you think?

review: Needs Information

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

PASSED: Continuous integration, rev:13acd7b2a53e0663f8fe35ee458bb25be4b6035e
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/44/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/972/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/317/

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

review: Approve (continuous-integration)

[Ashley James (dashmage)]

> Thanks for working on the fix.
>
> After thinking this a bit, I am wondering why we are insist on using the
> default /etc/crontab and modify the cron.daily schedule, or copy the default
> /etc/crontab and modify the cron.daily schedule and put it under root's
> crontab. I think this is undesired because it will affect all cron jobs under
> /etc/cron.daily, not just the logrotate charm's cronjob.
>
> One of the possible alternative is to just maintain our own cron job without
> using the default crontab.
>
> For example, we can either 1. create a crontab for root user with only one
> entry, or 2. append an entry to default /etc/crontab without modifying
> original content
>
> ```
> 17 * * * * root path_to_charm_logrotate
> ```
>
> This way, we can be sure that even if we add to root user's cronjob or
> /etc/crontab, it will not have big impacts. What do you think?

Thank you for your comment!

So the requirement for using /etc/crontab is because the cron.daily job is used by the charm for managing the execution of `/etc/cron.daily/logrotate` along with charm's `/etc/cron.daily/charm-logrotate` file. This logrotate cronjob is installed by the apt package and therefore we can't modify its location (it will be overwritten). So when the charm modifies the daily cronjob time config, it also needs to modify it for the `logrotate` file. Which is why we would need to directly modify the existing cron.daily time.

[JamesLin (jneo8)]

Overall LGTM now.

review: Approve

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

Change successfully merged at revision 10ab2f6b55b60798c08a40c4a86014c542a7bac1

[Chi Wai CHAN (raychan96)]

> > Thanks for working on the fix.
> >
> > After thinking this a bit, I am wondering why we are insist on using the
> > default /etc/crontab and modify the cron.daily schedule, or copy the default
> > /etc/crontab and modify the cron.daily schedule and put it under root's
> > crontab. I think this is undesired because it will affect all cron jobs
> under
> > /etc/cron.daily, not just the logrotate charm's cronjob.
> >
> > One of the possible alternative is to just maintain our own cron job without
> > using the default crontab.
> >
> > For example, we can either 1. create a crontab for root user with only one
> > entry, or 2. append an entry to default /etc/crontab without modifying
> > original content
> >
> > ```
> > 17 * * * * root path_to_charm_logrotate
> > ```
> >
> > This way, we can be sure that even if we add to root user's cronjob or
> > /etc/crontab, it will not have big impacts. What do you think?
>
> Thank you for your comment!
>
> So the requirement for using /etc/crontab is because the cron.daily job is
> used by the charm for managing the execution of `/etc/cron.daily/logrotate`
> along with charm's `/etc/cron.daily/charm-logrotate` file. This logrotate
> cronjob is installed by the apt package and therefore we can't modify its
> location (it will be overwritten). So when the charm modifies the daily
> cronjob time config, it also needs to modify it for the `logrotate` file.
> Which is why we would need to directly modify the existing cron.daily time.

That's a good observation! and it's exactly the why I mentioned "it will affect all cron jobs
under /etc/cron.daily". The question is do we really want to change the `logrotate` script provided by the apt package? I guess is not because from the config.yaml, it appears to me that the "update-cron-daily-schedule" is designed for "logrotate-cronjob" and "logrotate-cronjob-frequency", and that's related to `charm-logrotate` script not the `logrotate` script.

And what's more is that the logic in "lib_cron.py" is weird / incorrect: when "logrotate-cronjob" is enabled, it creates a cronjob that runs "lib_cron.py", and "lib_cron.py" will create a cronjob that runs "lib_cron.py" and update logrotate config file. And the cycle keep going, so that's a logic loop.

That being said, the logic is already there before you create this MR, so it's not on you. If it's possible, we should raise a bug to refactor the code's logic first, before attempting a new features.

diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index 1f30ddc..f2383cc 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, 0o755)
 
             # update cron.daily schedule if logrotate-cronjob-frequency set to "daily"
             if self.cronjob_frequency == 1 and self.validate_cron_conf():
@@ -123,7 +121,13 @@ 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):
+        """Write daily cronjob with provided timestamp to /etc/crontab."""
         cron_pattern = re.compile(r".*\/etc\/cron.daily.*")
         with open(r"/etc/crontab", "r") as crontab:
             data = crontab.read()
@@ -132,15 +136,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(r"/etc/crontab", "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..3af2c13 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -245,18 +245,54 @@ 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, mocker):
+        """Test function that writes updated data to /etc/crontab."""
+        cron_config = cron()
+        mock_open = mocker.patch("lib_cron.open")
+        mock_handle = mock_open.return_value.__enter__.return_value
+        default_crontab_contents = dedent(
+            """
+            # some comment
+            17 *\t* * * root cd / && run-parts --report /etc/cron.hourly
+            25 6\t* * root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.daily )
+            47 6\t* * 7 root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.weekly )
+            52 6\t1 * * root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.monthly )
+            #
+            """  # noqa
+        )
+        updated_crontab_contents = dedent(
+            f"""
+            # some comment
+            17 *\t* * * root cd / && run-parts --report /etc/cron.hourly
+            {cron_daily_timestamp}\t* * root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.daily )
+            47 6\t* * 7 root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.weekly )
+            52 6\t1 * * root test -x /usr/sbin/anacron || ( cd / && run-parts --report /etc/cron.monthly )
+            #
+            """  # noqa
+        )
+        mock_handle.read.return_value = default_crontab_contents
+        cron_config.write_to_crontab(cron_daily_timestamp)
+
+        mock_open.assert_any_call("/etc/crontab", "r")
+        mock_open.assert_any_call("/etc/crontab", "w")
+        mock_handle.write.assert_called_with(updated_crontab_contents)
 
     @pytest.mark.parametrize(
         ("cron_schedule"),
@@ -293,8 +329,7 @@ class TestCronHelper:
         )
         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_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 +357,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", 0o755)
 
     def test_install_cronjob_removes_etc_config_when_cronjob_disabled(
         self, cron, mocker