Re: [PATCH v2 1/1] maintenance: use systemd timers on Linux
flat view
From: Đoàn Trần Công Danh <hidden>
Date: 2021-05-10 01:20:34
On 2021-05-09 23:32:17+0200, Lénaïc Huard [off-list ref] wrote:
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.
+static int systemd_timer_enable_unit(int enable,
+ enum schedule_priority schedule,
+ const char *cmd)
+{
+ struct child_process child = CHILD_PROCESS_INIT;
+ const char *frequency = get_frequency(schedule);
+
+ 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))
+ die(_("failed to run systemctl"));
+ return finish_command(&child);
+}
+
+static int systemd_timer_delete_unit_templates(void)
+{
+ char *filename = xdg_config_home_systemd("git-maintenance@.timer");
+ unlink(filename);
+ free(filename);
+
+ filename = xdg_config_home_systemd("git-maintenance@.service");
+ unlink(filename);
+ free(filename);
+
+ return 0;
+}
+
+static int systemd_timer_delete_units(const char *cmd)
+{
+ return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, cmd) ||
+ systemd_timer_enable_unit(0, SCHEDULE_DAILY, cmd) ||
+ systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, cmd) ||
+ systemd_timer_delete_unit_templates();
+}
I'm not using any systemd-based distros. However, isn't this try to
enable all systemd's {hourly,daily,weekly} user's timer, then delete
the templates?^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W
Argh, we're disabling those systemd timer units first, by passing 0 as
first argument of systemd_timer_delete_units.
The fact that I read that twice, and still wrote down above reply
makes me think that above code is not self-explanatory enough.
May we switch to something else? Let's say using enum?
quoted hunk ↗ jump to hunk
+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)) + die(_("failed to create directories for '%s'"), filename);
This message is used by other codes, less works for translator, nice!
+ file = xfopen(filename, "w"); + free(filename);
I'm sure if we should use FREE_AND_NULL(filename) instead? Since, filename will be reused later.
quoted hunk ↗ jump to hunk
+ + 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"; + fputs(unit, file); + fclose(file); + + filename = xdg_config_home_systemd("git-maintenance@.service"); + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + file = xfopen(filename, "w"); + free(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=\"%1$s/git\" --exec-path=\"%1$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"; + fprintf(file, unit, exec_path);
I think others have strong opinion on not using "%1$s", and prefer simple "%s" and using "exec_path" twice instead.
quoted hunk ↗ jump to hunk
+ fclose(file); + + return 0; +} + +static int systemd_timer_setup_units(const char *cmd) +{ + const char *exec_path = git_exec_path(); + + return systemd_timer_write_unit_templates(exec_path) || + systemd_timer_enable_unit(1, SCHEDULE_HOURLY, cmd) || + systemd_timer_enable_unit(1, SCHEDULE_DAILY, cmd) || + systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, cmd); +} + +static int systemd_timer_update_schedule(int run_maintenance, int fd, + const char *cmd) +{ + if (run_maintenance) + return systemd_timer_setup_units(cmd); + else + return systemd_timer_delete_units(cmd); +} + +enum scheduler { + SCHEDULER_INVALID = -1, + SCHEDULER_AUTO = 0, + SCHEDULER_CRON = 1, + SCHEDULER_SYSTEMD = 2, + SCHEDULER_LAUNCHCTL = 3, + SCHEDULER_SCHTASKS = 4,
I think explicitly writing down values doesn't make things clearer, -1 would be nice, not a strong opinion, though. Anyway, would it be better to move those type declaration to top of file?
quoted hunk ↗ jump to hunk
+}; + +static const struct { + int (*is_available)(const char *cmd); + int (*update_schedule)(int run_maintenance, int fd, const char *cmd); + const char *cmd; +} scheduler_fn[] = { + [SCHEDULER_CRON] = { is_crontab_available, crontab_update_schedule, + "crontab" }, + [SCHEDULER_SYSTEMD] = { is_systemd_timer_available, + systemd_timer_update_schedule, "systemctl" }, + [SCHEDULER_LAUNCHCTL] = { is_launchctl_available, + launchctl_update_schedule, "launchctl" }, + [SCHEDULER_SCHTASKS] = { is_schtasks_available, + schtasks_update_schedule, "schtasks" }, +}; + +static enum scheduler parse_scheduler(const char *value) +{ + if (!value) + return SCHEDULER_INVALID; + else if (!strcasecmp(value, "auto")) + 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")) + return SCHEDULER_SCHTASKS; + else + return SCHEDULER_INVALID; +} + +static int maintenance_opt_scheduler(const struct option *opt, const char *arg, + int unset) +{ + enum scheduler *scheduler = opt->value; + + if (unset) + die(_("--no-scheduler is not allowed"));
I think it's better to use OPT_CALLBACK_F in the options list and we will write below code instead: BUG_ON_OPT_NEG(unset)
quoted hunk ↗ jump to hunk
+ + *scheduler = parse_scheduler(arg); + + if (*scheduler == SCHEDULER_INVALID) + die(_("unrecognized --scheduler argument '%s'"), arg);
Most of other callbacks do this instead:
return error(_("messsage.... '%s'"), arg);
quoted hunk ↗ jump to hunk
+ + return 0; +} + +struct maintenance_start_opts { + enum scheduler scheduler; +}; + +static void resolve_auto_scheduler(enum scheduler *scheduler) +{ + if (*scheduler != SCHEDULER_AUTO) + return; + #if defined(__APPLE__) -static const char platform_scheduler[] = "launchctl"; + *scheduler = SCHEDULER_LAUNCHCTL; + return; + #elif defined(GIT_WINDOWS_NATIVE) -static const char platform_scheduler[] = "schtasks"; + *scheduler = SCHEDULER_SCHTASKS; + return; + +#elif defined(__linux__) + if (is_systemd_timer_available("systemctl")) + *scheduler = SCHEDULER_SYSTEMD; + else if (is_crontab_available("crontab")) + *scheduler = SCHEDULER_CRON; + else + die(_("neither systemd timers nor crontab are available")); + return; + #else -static const char platform_scheduler[] = "crontab"; + *scheduler = SCHEDULER_CRON; + return; #endif +} -static int update_background_schedule(int enable) +static void validate_scheduler(enum scheduler scheduler) { - int result; - const char *scheduler = platform_scheduler; - const char *cmd = scheduler; + const char *cmd; + + if (scheduler == SCHEDULER_INVALID) + BUG("invalid scheduler"); + if (scheduler == SCHEDULER_AUTO) + BUG("resolve_auto_scheduler should have been called before"); + + cmd = scheduler_fn[scheduler].cmd; + if (!scheduler_fn[scheduler].is_available(cmd)) + die(_("%s scheduler is not available"), cmd); +} + +static int update_background_schedule(const struct maintenance_start_opts *opts, + int enable) +{ + unsigned int i; + int res, result = 0; + enum scheduler scheduler; + const char *cmd = NULL; char *testing; struct lock_file lk; char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path); + if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) + return error(_("another process is scheduling background maintenance")); + testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER")); if (testing) { char *sep = strchr(testing, ':'); if (!sep) die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing); *sep = '\0'; - scheduler = testing; + scheduler = parse_scheduler(testing); cmd = sep + 1; + result = scheduler_fn[scheduler].update_schedule( + enable, get_lock_file_fd(&lk), cmd); + goto done; } - if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) - return error(_("another process is scheduling background maintenance")); - - if (!strcmp(scheduler, "launchctl")) - result = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else if (!strcmp(scheduler, "schtasks")) - result = schtasks_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else if (!strcmp(scheduler, "crontab")) - result = crontab_update_schedule(enable, get_lock_file_fd(&lk), cmd); - else - die("unknown background scheduler: %s", scheduler); + for (i = 1; i < ARRAY_SIZE(scheduler_fn); i++) { + int enable_scheduler = enable && (opts->scheduler == i); + cmd = scheduler_fn[i].cmd; + if (!scheduler_fn[i].is_available(cmd)) + continue; + res = scheduler_fn[i].update_schedule( + enable_scheduler, get_lock_file_fd(&lk), cmd); + if (enable_scheduler) + result = res; + } +done: rollback_lock_file(&lk); free(testing); return result; } -static int maintenance_start(void) +static const char *const builtin_maintenance_start_usage[] = { + N_("git maintenance start [--scheduler=<scheduler>]"), NULL +}; + +static int maintenance_start(int argc, const char **argv, const char *prefix) { + struct maintenance_start_opts opts; + struct option builtin_maintenance_start_options[] = { + OPT_CALLBACK( + 0, "scheduler", &opts.scheduler, N_("scheduler"), + N_("scheduler to use to trigger git maintenance run"), + maintenance_opt_scheduler),
Following up my comment above, we're better to use:
OPT_CALLBACK_F(0, "scheduler", &opts.scheduler, N_("scheduler"),
N_("............"),
PARSE_OPT_NONEG, maintenance_opt_scheduler),
quoted hunk ↗ jump to hunk
+ OPT_END() + }; + memset(&opts, 0, sizeof(opts)); + + argc = parse_options(argc, argv, prefix, + builtin_maintenance_start_options, + builtin_maintenance_start_usage, 0); + + resolve_auto_scheduler(&opts.scheduler); + validate_scheduler(opts.scheduler); + + if (argc > 0) + usage_with_options(builtin_maintenance_start_usage, + builtin_maintenance_start_options); + if (maintenance_register()) warning(_("failed to add repo to global config")); - - return update_background_schedule(1); + return update_background_schedule(&opts, 1); } static int maintenance_stop(void) { - return update_background_schedule(0); + return update_background_schedule(NULL, 0); } static const char builtin_maintenance_usage[] = N_("git maintenance <subcommand> [<options>]");@@ -2027,7 +2354,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "run")) return maintenance_run(argc - 1, argv + 1, prefix); if (!strcmp(argv[1], "start")) - return maintenance_start(); + return maintenance_start(argc - 1, argv + 1, prefix); if (!strcmp(argv[1], "stop")) return maintenance_stop(); if (!strcmp(argv[1], "register"))diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 2412d8c5c0..6e6316cd90 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh@@ -20,6 +20,20 @@ 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 "$@" + else + true
The "else" leg is not necessary.
quoted hunk ↗ jump to hunk
+ fi +} + test_expect_success 'help text' ' test_expect_code 129 git maintenance -h 2>err && test_i18ngrep "usage: git maintenance <subcommand>" err &&@@ -615,6 +629,43 @@ 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 && + echo $* >>args
To avoid any possible incompatibility with zillion echo implementation out there. printf should be prefered over echo. Not a in this test case, however, it costs us nothing anyway. printf "%s\n" "$*"
quoted hunk ↗ jump to hunk
+ EOF + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="systemd:./print-args" git maintenance start && + + # start registers the repo + git config --get --global --fixed-value maintenance.repo "$(pwd)" && + + test_systemd_analyze_verify "$HOME/.config/systemd/user/git-maintenance@.service" && + + rm -f expect && + for frequency in hourly daily weekly + do + echo "--user enable --now git-maintenance@${frequency}.timer" >>expect || return 1 + done &&
And here, we can have a nicer syntax with printf: printf "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && With printf, we don't even need "rm -f expect" above.
quoted hunk ↗ jump to hunk
+ test_cmp expect args && + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="systemd:./print-args" git maintenance stop && + + # stop does not unregister the repo + git config --get --global --fixed-value maintenance.repo "$(pwd)" && + + test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.timer" && + test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.service" && + + rm -f expect && + for frequency in hourly daily weekly + do + echo "--user disable --now git-maintenance@${frequency}.timer" >>expect || return 1 + done &&
Ditto. All of this was written without testing, because I don't have any systemd based system near my hand, right now. So, please take it with a grain of salt. -- Danh