Code review comment for ~dashmage/charm-logrotated:bug/2017798-update-cron-daily-location

Revision history for this message
Ashley James (dashmage) wrote :

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

« Back to merge proposal