Commit daa787010c (maintenance: use random minute in systemd scheduler,
2023-08-10) hooked the systemd timers with the get_random_minute()
function, which is used to spread the load generated by clients, while
keeping one hour between each run (for the hourly run).
There is two problems with this approach:
1. The random minute is the same for all timers, which can lead to
overlapping execution, and in practice skipped execution for some of
the timers.
2. The timers are overwritten on each execution of `git maintenance
start`, with a new random minute, which can lead to missing a daily
or weekly timer.
Consider this (unlikely) sequence of events:
- weekly timer schedule is "Mon 0:52:00".
- user run `git maintenance start` Monday, at 0:30:00.
- the random minute happens to be 22, making the new weekly timer
schedule "Mon 0:22:00".
- the weekly timer does not fire until the next Monday.
Commit c97ec0378b (maintenance: fix systemd schedule overlaps,
2023-08-10) resolve problem 1 by skipping some executions of the timers
to avoid overlap, but not problem 2.
Revert both commits to instead use two native systemd timers properties:
- RandomizedDelaySec: as the name suggest, this adds a random offset
to each timer execution. Use 3540 seconds (59 minutes) to spread
execution as much as possible.
- FixedRandomDelay: this makes the offset from the previous setting
deterministic, depending on machine ID, timer name and user
identifier.
(For more details on these settings, see man 5 systemd.timer)
This give us timers which:
- don't overlap (schedule depend on timer name)
- are spread out by user (schedule depend on machine ID and user ID)
- happen at fixed intervals
This also restore the ability to use the systemd templating system, and
keep the systemd unit strings embedded in the C code simpler, with no
need for format strings.
Signed-off-by: Max Gautier <mg@xxxxxxxxxxxxxxxx>
---
builtin/gc.c | 154 +++++++----------------------------------
t/t7900-maintenance.sh | 15 +---
2 files changed, 27 insertions(+), 142 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..dac59414f0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2308,54 +2308,29 @@ static char *xdg_config_home_systemd(const char *filename)
return xdg_config_home_for("systemd/user", filename);
}
-#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
-
-static int systemd_timer_delete_timer_file(enum schedule_priority priority)
+static int systemd_timer_delete_unit_templates(void)
{
int ret = 0;
- const char *frequency = get_frequency(priority);
- char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
- char *filename = xdg_config_home_systemd(local_timer_name);
-
+ 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);
- free(filename);
- free(local_timer_name);
- return ret;
-}
-
-static int systemd_timer_delete_service_template(void)
-{
- int ret = 0;
- char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
- char *filename = xdg_config_home_systemd(local_service_name);
+ 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);
- free(local_service_name);
return ret;
}
-/*
- * Write the schedule information into a git-maintenance@<schedule>.timer
- * file using a custom minute. This timer file cannot use the templating
- * system, so we generate a specific file for each.
- */
-static int systemd_timer_write_timer_file(enum schedule_priority schedule,
- int minute)
+static int systemd_timer_write_unit_templates(const char *exec_path)
{
- int res = -1;
char *filename;
FILE *file;
const char *unit;
- char *schedule_pattern = NULL;
- const char *frequency = get_frequency(schedule);
- char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-
- filename = xdg_config_home_systemd(local_timer_name);
+ filename = xdg_config_home_systemd("git-maintenance@.timer");
if (safe_create_leading_directories(filename)) {
error(_("failed to create directories for '%s'"), filename);
goto error;
@@ -2364,23 +2339,6 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
if (!file)
goto error;
- switch (schedule) {
- case SCHEDULE_HOURLY:
- schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute);
- break;
-
- case SCHEDULE_DAILY:
- schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute);
- break;
-
- case SCHEDULE_WEEKLY:
- schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
- break;
-
- default:
- BUG("Unhandled schedule_priority");
- }
-
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"
@@ -2389,12 +2347,14 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
"Description=Optimize Git repositories data\n"
"\n"
"[Timer]\n"
- "OnCalendar=%s\n"
+ "OnCalendar=%i\n"
"Persistent=true\n"
+ "RandomizedDelaySecond=3540\n"
+ "FixedRandomDelay=true\n"
"\n"
"[Install]\n"
"WantedBy=timers.target\n";
- if (fprintf(file, unit, schedule_pattern) < 0) {
+ if (fputs(unit, file) == EOF) {
error(_("failed to write to '%s'"), filename);
fclose(file);
goto error;
@@ -2403,36 +2363,9 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
error_errno(_("failed to flush '%s'"), filename);
goto error;
}
-
- res = 0;
-
-error:
- free(schedule_pattern);
- free(local_timer_name);
free(filename);
- return res;
-}
-/*
- * No matter the schedule, we use the same service and can make use of the
- * templating system. When installing git-maintenance@<schedule>.timer,
- * systemd will notice that git-maintenance@.service exists as a template
- * and will use this file and insert the <schedule> into the template at
- * the position of "%i".
- */
-static int systemd_timer_write_service_template(const char *exec_path)
-{
- int res = -1;
- char *filename;
- FILE *file;
- const char *unit;
- char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-
- filename = xdg_config_home_systemd(local_service_name);
- if (safe_create_leading_directories(filename)) {
- error(_("failed to create directories for '%s'"), filename);
- goto error;
- }
+ filename = xdg_config_home_systemd("git-maintenance@.service");
file = fopen_or_warn(filename, "w");
if (!file)
goto error;
@@ -2465,18 +2398,17 @@ static int systemd_timer_write_service_template(const char *exec_path)
error_errno(_("failed to flush '%s'"), filename);
goto error;
}
-
- res = 0;
+ free(filename);
+ return 0;
error:
- free(local_service_name);
free(filename);
- return res;
+ systemd_timer_delete_unit_templates();
+ return -1;
}
static int systemd_timer_enable_unit(int enable,
- enum schedule_priority schedule,
- int minute)
+ enum schedule_priority schedule)
{
const char *cmd = "systemctl";
struct child_process child = CHILD_PROCESS_INIT;
@@ -2493,14 +2425,12 @@ static int systemd_timer_enable_unit(int enable,
*/
if (!enable)
child.no_stderr = 1;
- else if (systemd_timer_write_timer_file(schedule, minute))
- return -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, SYSTEMD_UNIT_FORMAT, frequency, "timer");
+ strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
if (start_command(&child))
return error(_("failed to start systemctl"));
@@ -2517,58 +2447,24 @@ static int systemd_timer_enable_unit(int enable,
return 0;
}
-/*
- * A previous version of Git wrote the timer units as template files.
- * Clean these up, if they exist.
- */
-static void systemd_timer_delete_stale_timer_templates(void)
-{
- char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer");
- char *filename = xdg_config_home_systemd(timer_template_name);
-
- if (unlink(filename) && !is_missing_file_error(errno))
- warning(_("failed to delete '%s'"), filename);
-
- free(filename);
- free(timer_template_name);
-}
-
-static int systemd_timer_delete_unit_files(void)
-{
- systemd_timer_delete_stale_timer_templates();
-
- /* Purposefully not short-circuited to make sure all are called. */
- return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
- systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
- systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
- systemd_timer_delete_service_template();
-}
-
static int systemd_timer_delete_units(void)
{
- int minute = get_random_minute();
- /* Purposefully not short-circuited to make sure all are called. */
- return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |
- systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |
- systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |
- systemd_timer_delete_unit_files();
+ 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_setup_units(void)
{
- int minute = get_random_minute();
const char *exec_path = git_exec_path();
- int ret = systemd_timer_write_service_template(exec_path) ||
- systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||
- systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||
- systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);
-
+ 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();
- else
- systemd_timer_delete_stale_timer_templates();
-
return ret;
}
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a..37aa408d26 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -790,15 +790,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
# start registers the repo
git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
- for schedule in hourly daily weekly
- do
- test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1
- done &&
- test_path_is_file "systemd/user/git-maintenance@.service" &&
-
- test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" &&
- test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
- test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&
+ 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 &&
@@ -809,10 +801,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
# stop does not unregister the repo
git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
- for schedule in hourly daily weekly
- do
- test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
- done &&
+ 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 &&
--
2.44.0
