Re: [PATCH v5 3/3] maintenance: add support for systemd timers on Linux
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 9, 2021, 15:01 UTC
- Message-ID
- <959c56a3-3290-b277-5d30-e16244ae44bf@gmail.com>
- In-Reply-To
- <20210608134000.663398-4-lenaic@lhuard.fr>
On 08/06/2021 14:40, Lénaïc Huard wrote:
Show 332 quoted lines
> The existing mechanism for scheduling background maintenance is done
> through cron. On Linux systems managed by systemd, systemd provides an
> alternative to schedule recurring tasks: systemd timers.
>
> The main motivations to implement systemd timers in addition to cron
> are:
> * cron is optional and Linux systems running systemd might not have it
> installed.
> * The execution of `crontab -l` can tell us if cron is installed but not
> if the daemon is actually running.
> * With systemd, each service is run in its own cgroup and its logs are
> tagged by the service inside journald. With cron, all scheduled tasks
> are running in the cron daemon cgroup and all the logs of the
> user-scheduled tasks are pretended to belong to the system cron
> service.
> Concretely, a user that doesn’t have access to the system logs won’t
> have access to the log of their own tasks scheduled by cron whereas
> they will have access to the log of their own tasks scheduled by
> systemd timer.
> Although `cron` attempts to send email, that email may go unseen by
> the user because these days, local mailboxes are not heavily used
> anymore.
>
> In order to schedule git maintenance, we need two unit template files:
> * ~/.config/systemd/user/git-maintenance@.service
> to define the command to be started by systemd and
> * ~/.config/systemd/user/git-maintenance@.timer
> to define the schedule at which the command should be run.
>
> Those units are templates that are parameterized by the frequency.
>
> Based on those templates, 3 timers are started:
> * git-maintenance@hourly.timer
> * git-maintenance@daily.timer
> * git-maintenance@weekly.timer
>
> The command launched by those three timers are the same as with the
> other scheduling methods:
>
> /path/to/git for-each-repo --exec-path=/path/to
> --config=maintenance.repo maintenance run --schedule=%i
>
> with the full path for git to ensure that the version of git launched
> for the scheduled maintenance is the same as the one used to run
> `maintenance start`.
>
> The timer unit contains `Persistent=true` so that, if the computer is
> powered down when a maintenance task should run, the task will be run
> when the computer is back powered on.
>
> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr>
> ---
> Documentation/git-maintenance.txt | 57 +++++++-
> builtin/gc.c | 228 ++++++++++++++++++++++++++++++
> t/t7900-maintenance.sh | 67 ++++++++-
> 3 files changed, 342 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 07065ed4f3..7b7dbbbca9 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -179,16 +179,19 @@ OPTIONS
> `maintenance.<task>.enabled` configured as `true` are considered.
> See the 'TASKS' section for the list of accepted `<task>` values.
>
> ---scheduler=auto|crontab|launchctl|schtasks::
> +--scheduler=auto|crontab|systemd-timer|launchctl|schtasks::
> When combined with the `start` subcommand, specify the scheduler
> to use to run the hourly, daily and weekly executions of
> `git maintenance run`.
> The possible values for `<scheduler>` depend on the system: `crontab`
> - is available on POSIX systems, `launchctl` is available on
> - MacOS and `schtasks` is available on Windows.
> + is available on POSIX systems, `systemd-timer` is available on Linux
> + systems, `launchctl` is available on MacOS and `schtasks` is available
> + on Windows.
> By default or when `auto` is specified, a suitable scheduler for
> the system is used. On MacOS, `launchctl` is used. On Windows,
> - `schtasks` is used. On all other systems, `crontab` is used.
> + `schtasks` is used. On Linux, `systemd-timer` is used if user systemd
> + timers are available, otherwise, `crontab` is used. On all other systems,
> + `crontab` is used.
>
>
> TROUBLESHOOTING
> @@ -288,6 +291,52 @@ schedule to ensure you are executing the correct binaries in your
> schedule.
>
>
> +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMS
> +-----------------------------------------------
> +
> +While Linux supports `cron`, depending on the distribution, `cron` may
> +be an optional package not necessarily installed. On modern Linux
> +distributions, systemd timers are superseding it.
> +
> +If user systemd timers are available, they will be used as a replacement
> +of `cron`.
> +
> +In this case, `git maintenance start` will create user systemd timer units
> +and start the timers. The current list of user-scheduled tasks can be found
> +by running `systemctl --user list-timers`. The timers written by `git
> +maintenance start` are similar to this:
> +
> +-----------------------------------------------------------------------
> +$ systemctl --user list-timers
> +NEXT LEFT LAST PASSED UNIT ACTIVATES
> +Thu 2021-04-29 19:00:00 CEST 42min left Thu 2021-04-29 18:00:11 CEST 17min ago git-maintenance@hourly.timer git-maintenance@hourly.service
> +Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago git-maintenance@daily.timer git-maintenance@daily.service
> +Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service
> +-----------------------------------------------------------------------
> +
> +One timer is registered for each `--schedule=<frequency>` option.
> +
> +The definition of the systemd units can be inspected in the following files:
> +
> +-----------------------------------------------------------------------
> +~/.config/systemd/user/git-maintenance@.timer
> +~/.config/systemd/user/git-maintenance@.service
> +~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
> +~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
> +~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
> +-----------------------------------------------------------------------
> +
> +`git maintenance start` will overwrite these files and start the timer
> +again with `systemctl --user`, so any customization should be done by
> +creating a drop-in file, i.e. a `.conf` suffixed file in the
> +`~/.config/systemd/user/git-maintenance@.service.d` directory.
> +
> +`git maintenance stop` will stop the user systemd timers and delete
> +the above mentioned files.
> +
> +For more details, see `systemd.timer(5)`.
> +
> +
> BACKGROUND MAINTENANCE ON MACOS SYSTEMS
> ---------------------------------------
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index f2a81ecb44..5fe416c903 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2040,10 +2040,221 @@ static int crontab_update_schedule(int run_maintenance, int fd)
> return result;
> }
>
> +#ifdef __linux__
> +
> +static int real_is_systemd_timer_available(void)
> +{
> + struct child_process child = CHILD_PROCESS_INIT;
> +
> + strvec_pushl(&child.args, "systemctl", "--user", "list-timers", NULL);
> + child.no_stdin = 1;
> + child.no_stdout = 1;
> + child.no_stderr = 1;
> + child.silent_exec_failure = 1;
> +
> + if (start_command(&child))
> + return 0;
> + if (finish_command(&child))
> + return 0;
> + return 1;
> +}
> +
> +#else
> +
> +static int real_is_systemd_timer_available(void)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> +static int is_systemd_timer_available(void)
> +{
> + const char *cmd = "systemctl";
> + int is_available;
> +
> + if (get_schedule_cmd(&cmd, &is_available))
> + return is_available;
> +
> + return real_is_systemd_timer_available();
> +}
> +
> +static char *xdg_config_home_systemd(const char *filename)
> +{
> + return xdg_config_home_for("systemd/user", filename);
> +}
> +
> +static int systemd_timer_enable_unit(int enable,
> + enum schedule_priority schedule)
> +{
> + const char *cmd = "systemctl";
> + struct child_process child = CHILD_PROCESS_INIT;
> + const char *frequency = get_frequency(schedule);
> +
> + /*
> + * Disabling the systemd unit while it is already disabled makes
> + * systemctl print an error.
> + * Let's ignore it since it means we already are in the expected state:
> + * the unit is disabled.
> + *
> + * On the other hand, enabling a systemd unit which is already enabled
> + * produces no error.
> + */
> + if (!enable)
> + child.no_stderr = 1;
> +
> + get_schedule_cmd(&cmd, NULL);
> + strvec_split(&child.args, cmd);
> + strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
> + "--now", NULL);
> + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
> +
> + if (start_command(&child))
> + return error(_("failed to start systemctl"));
> + if (finish_command(&child))
> + /*
> + * Disabling an already disabled systemd unit makes
> + * systemctl fail.
> + * Let's ignore this failure.
> + *
> + * Enabling an enabled systemd unit doesn't fail.
> + */
> + if (enable)
> + return error(_("failed to run systemctl"));
> + return 0;
> +}
> +
> +static int systemd_timer_delete_unit_templates(void)
> +{
> + int ret = 0;
> + char *filename = xdg_config_home_systemd("git-maintenance@.timer");
> + if(unlink(filename) && !is_missing_file_error(errno))
> + ret = error_errno(_("failed to delete '%s'"), filename);
> + FREE_AND_NULL(filename);
> +
> + filename = xdg_config_home_systemd("git-maintenance@.service");
> + if(unlink(filename) && !is_missing_file_error(errno))
> + ret = error_errno(_("failed to delete '%s'"), filename);
> +
> + free(filename);
> + return ret;
> +}
> +
> +static int systemd_timer_delete_units(void)
> +{
> + return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
> + systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
> + systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||
> + systemd_timer_delete_unit_templates();
> +}
> +
> +static int systemd_timer_write_unit_templates(const char *exec_path)
> +{
> + char *filename;
> + FILE *file;
> + const char *unit;
> +
> + filename = xdg_config_home_systemd("git-maintenance@.timer");
> + if (safe_create_leading_directories(filename)) {
> + error(_("failed to create directories for '%s'"), filename);
> + goto error;
> + }
> + file = fopen_or_warn(filename, "w");
> + if (file == NULL)
> + goto error;
> + FREE_AND_NULL(filename);
> +
> + unit = "# This file was created and is maintained by Git.\n"
> + "# Any edits made in this file might be replaced in the future\n"
> + "# by a Git command.\n"
> + "\n"
> + "[Unit]\n"
> + "Description=Optimize Git repositories data\n"
> + "\n"
> + "[Timer]\n"
> + "OnCalendar=%i\n"
> + "Persistent=true\n"
> + "\n"
> + "[Install]\n"
> + "WantedBy=timers.target\n";
> + if (fputs(unit, file) == EOF) {
> + error(_("failed to write to '%s'"), filename);
> + fclose(file);
> + goto error;
> + }
> + if (fclose(file) == EOF) {
> + error_errno(_("failed to flush '%s'"), filename);
> + goto error;
> + }
> +
> + filename = xdg_config_home_systemd("git-maintenance@.service");
> + file = fopen_or_warn(filename, "w");
> + if (file == NULL)
> + goto error;
> + FREE_AND_NULL(filename);
> +
> + unit = "# This file was created and is maintained by Git.\n"
> + "# Any edits made in this file might be replaced in the future\n"
> + "# by a Git command.\n"
> + "\n"
> + "[Unit]\n"
> + "Description=Optimize Git repositories data\n"
> + "\n"
> + "[Service]\n"
> + "Type=oneshot\n"
> + "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
> + "LockPersonality=yes\n"
> + "MemoryDenyWriteExecute=yes\n"
> + "NoNewPrivileges=yes\n"
> + "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n"
> + "RestrictNamespaces=yes\n"
> + "RestrictRealtime=yes\n"
> + "RestrictSUIDSGID=yes\n"
> + "SystemCallArchitectures=native\n"
> + "SystemCallFilter=@system-service\n";
> + if (fprintf(file, unit, exec_path, exec_path) < 0) {
> + error(_("failed to write to '%s'"), filename);
> + fclose(file);
> + goto error;
> + }
> + if (fclose(file) == EOF) {
> + error_errno(_("failed to flush '%s'"), filename);
> + goto error;
> + }
> + return 0;
> +
> +error:
> + free(filename);
> + systemd_timer_delete_unit_templates();One small comment just to show I have read the patches - I think systemd_timer_delete_unit_templates() ends up being called twice if we fail to write one of the template files as systemd_timer_setup_units() calls it if this function fails. I don't think this matters and there is no need to change it - from a future maintenance perspective it is probably safer to leave it as is.
Best Wishes
Phillip
Show 177 quoted lines
> + return -1;
> +}
> +
> +static int systemd_timer_setup_units(void)
> +{
> + const char *exec_path = git_exec_path();
> +
> + int ret = systemd_timer_write_unit_templates(exec_path) ||
> + systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
> + systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
> + systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);
> + if (ret)
> + systemd_timer_delete_units();
> + return ret;
> +}
> +
> +static int systemd_timer_update_schedule(int run_maintenance, int fd)
> +{
> + if (run_maintenance)
> + return systemd_timer_setup_units();
> + else
> + return systemd_timer_delete_units();
> +}
> +
> enum scheduler {
> SCHEDULER_INVALID = -1,
> SCHEDULER_AUTO,
> SCHEDULER_CRON,
> + SCHEDULER_SYSTEMD,
> SCHEDULER_LAUNCHCTL,
> SCHEDULER_SCHTASKS,
> };
> @@ -2058,6 +2269,11 @@ static const struct {
> .is_available = is_crontab_available,
> .update_schedule = crontab_update_schedule,
> },
> + [SCHEDULER_SYSTEMD] = {
> + .name = "systemctl",
> + .is_available = is_systemd_timer_available,
> + .update_schedule = systemd_timer_update_schedule,
> + },
> [SCHEDULER_LAUNCHCTL] = {
> .name = "launchctl",
> .is_available = is_launchctl_available,
> @@ -2078,6 +2294,9 @@ static enum scheduler parse_scheduler(const char *value)
> return SCHEDULER_AUTO;
> else if (!strcasecmp(value, "cron") || !strcasecmp(value, "crontab"))
> return SCHEDULER_CRON;
> + else if (!strcasecmp(value, "systemd") ||
> + !strcasecmp(value, "systemd-timer"))
> + return SCHEDULER_SYSTEMD;
> else if (!strcasecmp(value, "launchctl"))
> return SCHEDULER_LAUNCHCTL;
> else if (!strcasecmp(value, "schtasks"))
> @@ -2116,6 +2335,15 @@ static void resolve_auto_scheduler(enum scheduler *scheduler)
> *scheduler = SCHEDULER_SCHTASKS;
> return;
>
> +#elif defined(__linux__)
> + if (is_systemd_timer_available())
> + *scheduler = SCHEDULER_SYSTEMD;
> + else if (is_crontab_available())
> + *scheduler = SCHEDULER_CRON;
> + else
> + die(_("neither systemd timers nor crontab are available"));
> + return;
> +
> #else
> *scheduler = SCHEDULER_CRON;
> return;
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index b36b7f5fb0..b289cae6b9 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -20,6 +20,18 @@ test_xmllint () {
> fi
> }
>
> +test_lazy_prereq SYSTEMD_ANALYZE '
> + systemd-analyze --help >out &&
> + grep verify out
> +'
> +
> +test_systemd_analyze_verify () {
> + if test_have_prereq SYSTEMD_ANALYZE
> + then
> + systemd-analyze verify "$@"
> + fi
> +}
> +
> test_expect_success 'help text' '
> test_expect_code 129 git maintenance -h 2>err &&
> test_i18ngrep "usage: git maintenance <subcommand>" err &&
> @@ -634,15 +646,56 @@ test_expect_success 'start and stop Windows maintenance' '
> test_cmp expect args
> '
>
> +test_expect_success 'start and stop Linux/systemd maintenance' '
> + write_script print-args <<-\EOF &&
> + printf "%s\n" "$*" >>args
> + EOF
> +
> + XDG_CONFIG_HOME="$PWD" &&
> + export XDG_CONFIG_HOME &&
> + rm -f args &&
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&
> +
> + # start registers the repo
> + git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
> +
> + test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
> +
> + printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> + test_cmp expect args &&
> +
> + rm -f args &&
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop &&
> +
> + # stop does not unregister the repo
> + git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
> +
> + test_path_is_missing "systemd/user/git-maintenance@.timer" &&
> + test_path_is_missing "systemd/user/git-maintenance@.service" &&
> +
> + printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> + test_cmp expect args
> +'
> +
> test_expect_success 'start and stop when several schedulers are available' '
> write_script print-args <<-\EOF &&
> printf "%s\n" "$*" | sed "s:gui/[0-9][0-9]*:gui/[UID]:; s:\(schtasks /create .* /xml\).*:\1:;" >>args
> EOF
>
> rm -f args &&
> - GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl &&
> - printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=systemd-timer &&
> + printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> hourly daily weekly >expect &&
> + printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> + hourly daily weekly >>expect &&
> + printf -- "systemctl --user enable --now git-maintenance@%s.timer\n" hourly daily weekly >>expect &&
> + test_cmp expect args &&
> +
> + rm -f args &&
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl &&
> + printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> + printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> + hourly daily weekly >>expect &&
> for frequency in hourly daily weekly
> do
> PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> @@ -652,17 +705,19 @@ test_expect_success 'start and stop when several schedulers are available' '
> test_cmp expect args &&
>
> rm -f args &&
> - GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks &&
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks &&
> + printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> - hourly daily weekly >expect &&
> + hourly daily weekly >>expect &&
> printf "schtasks /create /tn Git Maintenance (%s) /f /xml\n" \
> hourly daily weekly >>expect &&
> test_cmp expect args &&
>
> rm -f args &&
> - GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop &&
> + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop &&
> + printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> - hourly daily weekly >expect &&
> + hourly daily weekly >>expect &&
> printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> hourly daily weekly >>expect &&
> test_cmp expect args
>