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

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

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

« Back to merge proposal