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

Revision history for this message
Chi Wai CHAN (raychan96) wrote :

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

« Back to merge proposal