From da0c9847eaf3165d7b8b5a2631392c2e1abfb867 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 4 Aug 2023 14:26:42 -0400 Subject: [PATCH 1/7] maintenance: add get_random_minute() When we initially created background maintenance -- with its hourly, daily, and weekly schedules -- we considered the effects of all clients launching fetches to the server every hour on the hour. The worry of DDoSing server hosts was noted, but left as something we would consider for a future update. As background maintenance has gained more adoption over the past three years, our worries about DDoSing the big Git hosts has been unfounded. Those systems, especially those serving public repositories, are already resilient to thundering herds of much smaller scale. However, sometimes organizations spin up specific custom server infrastructure either in addition to or on top of their Git host. Some of these technologies are built for a different range of scale, and can hit concurrency limits sooner. Organizations with such custom infrastructures are more likely to recommend tools like `scalar` which furthers their adoption of background maintenance. To help solve for this, create get_random_minute() as a method to help Git select a random minute when creating schedules in the future. The integrations with this method do not yet exist, but will follow in future changes. To avoid multiple calls to srand() in the same process, create a new git_rand() method that focuses on the initialization of srand() and the call to rand(). To make it clear through this abstraction that it never returns a negative number, convert the result to uint32_t (this has implications in the typical case of taking the result modulo some positive integer). There is another use of srand() in lockfile.c, but this is being replaced with a cryptographic generator in an independent change. One thing that is important for testability is that we notice when we are under a test scenario and return a predictable result. The schedules themselves are not checked for this value, but at least one launchctl test checks that we do not unnecessarily reboot the schedule if it has not changed from a previous version. Signed-off-by: Derrick Stolee --- builtin/gc.c | 10 ++++++++++ git-compat-util.h | 11 +++++++++++ wrapper.c | 12 ++++++++++++ 3 files changed, 33 insertions(+) diff --git a/builtin/gc.c b/builtin/gc.c index f3942188a614c9..4b60f339b37fc6 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1708,6 +1708,16 @@ static int get_schedule_cmd(const char **cmd, int *is_available) return 1; } +MAYBE_UNUSED +static int get_random_minute(void) +{ + /* Use a static value when under tests. */ + if (!getenv("GIT_TEST_MAINTENANCE_SCHEDULER")) + return 13; + + return git_rand() % 60; +} + static int is_launchctl_available(void) { const char *cmd = "launchctl"; diff --git a/git-compat-util.h b/git-compat-util.h index 5b2b99c17c564c..62ce1fb5d64c0d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1673,4 +1673,15 @@ void sleep_millisec(int millisec); */ int csprng_bytes(void *buf, size_t len); +/* + * Returns a random int from rand() (0 to MAX_RAND inclusive). Converted + * to a uint32_t to emphasize that it will never be negative, though the + * number is from an isolated subset of the possible unsigned range. + * + * git_rand() centralizes the initializer of the pseudo-random number + * generator so we do not accidentally initialize it twice and reuse + * numbers from the sequence created after initialization. + */ +uint32_t git_rand(void); + #endif diff --git a/wrapper.c b/wrapper.c index 67f5f5dbe199f8..41aa7d64c58357 100644 --- a/wrapper.c +++ b/wrapper.c @@ -835,3 +835,15 @@ int csprng_bytes(void *buf, size_t len) return 0; #endif } + +uint32_t git_rand(void) +{ + static int random_initialized = 0; + + if (!random_initialized) { + srand((unsigned int)getpid()); + random_initialized = 1; + } + + return rand(); +} \ No newline at end of file From b9837f6b3f1fa01e4f57b4dbb96d37aabd854435 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 4 Aug 2023 14:27:08 -0400 Subject: [PATCH 2/7] maintenance: use random minute in launchctl scheduler The get_random_minute() method was created to allow maintenance schedules to be fixed to a random minute of the hour. This randomness is only intended to spread out the load from a number of clients, but each client should have an hour between each maintenance cycle. Use get_random_minute() when constructing the schedules for launchctl. The format already includes a 'Minute' key which is modified from 0 to the random minute. Signed-off-by: Derrick Stolee --- builtin/gc.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 4b60f339b37fc6..7979561a460c1e 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1708,7 +1708,6 @@ static int get_schedule_cmd(const char **cmd, int *is_available) return 1; } -MAYBE_UNUSED static int get_random_minute(void) { /* Use a static value when under tests. */ @@ -1830,6 +1829,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT; struct stat st; const char *cmd = "launchctl"; + int minute = get_random_minute(); get_schedule_cmd(&cmd, NULL); preamble = "\n" @@ -1855,29 +1855,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit case SCHEDULE_HOURLY: repeat = "\n" "Hour%d\n" - "Minute0\n" + "Minute%d\n" "\n"; for (i = 1; i <= 23; i++) - strbuf_addf(&plist, repeat, i); + strbuf_addf(&plist, repeat, i, minute); break; case SCHEDULE_DAILY: repeat = "\n" "Day%d\n" "Hour0\n" - "Minute0\n" + "Minute%d\n" "\n"; for (i = 1; i <= 6; i++) - strbuf_addf(&plist, repeat, i); + strbuf_addf(&plist, repeat, i, minute); break; case SCHEDULE_WEEKLY: - strbuf_addstr(&plist, - "\n" - "Day0\n" - "Hour0\n" - "Minute0\n" - "\n"); + strbuf_addf(&plist, + "\n" + "Day0\n" + "Hour0\n" + "Minute%d\n" + "\n", + minute); break; default: From a4199680d067b1c176de4dbe520143d1d82b9706 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 4 Aug 2023 14:27:38 -0400 Subject: [PATCH 3/7] maintenance: use random minute in Windows scheduler The get_random_minute() method was created to allow maintenance schedules to be fixed to a random minute of the hour. This randomness is only intended to spread out the load from a number of clients, but each client should have an hour between each maintenance cycle. Add this random minute to the Windows scheduler integration. We need only to modify the minute value for the 'StartBoundary' tag across the three schedules. Signed-off-by: Derrick Stolee --- builtin/gc.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 7979561a460c1e..e34f0b1a909e23 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1995,6 +1995,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority const char *frequency = get_frequency(schedule); char *name = schtasks_task_name(frequency); struct strbuf tfilename = STRBUF_INIT; + int minute = get_random_minute(); get_schedule_cmd(&cmd, NULL); @@ -2015,7 +2016,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority switch (schedule) { case SCHEDULE_HOURLY: fprintf(tfile->fp, - "2020-01-01T01:00:00\n" + "2020-01-01T01:%02d:00\n" "true\n" "\n" "1\n" @@ -2024,12 +2025,13 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority "PT1H\n" "PT23H\n" "false\n" - "\n"); + "\n", + minute); break; case SCHEDULE_DAILY: fprintf(tfile->fp, - "2020-01-01T00:00:00\n" + "2020-01-01T00:%02d:00\n" "true\n" "\n" "\n" @@ -2041,19 +2043,21 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority "\n" "\n" "1\n" - "\n"); + "\n", + minute); break; case SCHEDULE_WEEKLY: fprintf(tfile->fp, - "2020-01-01T00:00:00\n" + "2020-01-01T00:%02d:00\n" "true\n" "\n" "\n" "\n" "\n" "1\n" - "\n"); + "\n", + minute); break; default: From 6a33030f0243b4c3963a599857e87cfd3baa896f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 4 Aug 2023 14:27:54 -0400 Subject: [PATCH 4/7] maintenance: use random minute in cron scheduler The get_random_minute() method was created to allow maintenance schedules to be fixed to a random minute of the hour. This randomness is only intended to spread out the load from a number of clients, but each client should have an hour between each maintenance cycle. Add this random minute to the cron integration. The cron schedule specification starts with a minute indicator, which was previously inserted as the "0" string but now takes the given minute as an integer parameter. Signed-off-by: Derrick Stolee --- builtin/gc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index e34f0b1a909e23..1afbaf4f174bc6 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2174,6 +2174,7 @@ static int crontab_update_schedule(int run_maintenance, int fd) FILE *cron_list, *cron_in; struct strbuf line = STRBUF_INIT; struct tempfile *tmpedit = NULL; + int minute = get_random_minute(); get_schedule_cmd(&cmd, NULL); strvec_split(&crontab_list.args, cmd); @@ -2228,11 +2229,11 @@ static int crontab_update_schedule(int run_maintenance, int fd) "# replaced in the future by a Git command.\n\n"); strbuf_addf(&line_format, - "%%s %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n", + "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n", exec_path, exec_path); - fprintf(cron_in, line_format.buf, "0", "1-23", "*", "hourly"); - fprintf(cron_in, line_format.buf, "0", "0", "1-6", "daily"); - fprintf(cron_in, line_format.buf, "0", "0", "0", "weekly"); + fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly"); + fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily"); + fprintf(cron_in, line_format.buf, minute, "0", "0", "weekly"); strbuf_release(&line_format); fprintf(cron_in, "\n%s\n", END_LINE); From eb4aca745ac816ac19813ca8acd7db05eee61a19 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 7 Aug 2023 10:31:07 -0400 Subject: [PATCH 5/7] maintenance: swap method locations The systemd_timer_write_unit_templates() method writes a single template that is then used to start the hourly, daily, and weekly schedules with systemd. However, in order to schedule systemd maintenance on a given minute, these templates need to be replaced with specific schedules for each of these jobs. Before modifying the schedules, move the writing method above the systemd_timer_enable_unit() method, so we can write a specific schedule for each unit. The diff is computed smaller by showing systemd_timer_enable_unit() move instead of systemd_timer_write_unit_templates(). Signed-off-by: Derrick Stolee --- builtin/gc.c | 128 +++++++++++++++++++++++++-------------------------- 1 file changed, 64 insertions(+), 64 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 1afbaf4f174bc6..54536a1b3be3c8 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2292,70 +2292,6 @@ 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; @@ -2437,6 +2373,70 @@ static int systemd_timer_write_unit_templates(const char *exec_path) return -1; } +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_setup_units(void) { const char *exec_path = git_exec_path(); From 7be96b29b840d2ef4f681daf3d3f987e3edde7fc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 4 Aug 2023 16:49:59 -0400 Subject: [PATCH 6/7] maintenance: use random minute in systemd scheduler The get_random_minute() method was created to allow maintenance schedules to be fixed to a random minute of the hour. This randomness is only intended to spread out the load from a number of clients, but each client should have an hour between each maintenance cycle. Add this random minute to the systemd integration. This integration is more complicated than similar changes for other schedulers because of a neat trick that systemd allows: templating. The previous implementation generated two template files with names of the form 'git-maintenance@.(timer|service)'. The '.timer' or '.service' indicates that this is a template that is picked up when we later specify '...@.timer' or '...@.service'. The '' string is then used to insert into the template both the 'OnCalendar' schedule setting and the '--schedule' parameter of the 'git maintenance run' command. In order to set these schedules to a given minute, we can no longer use the 'hourly', 'daily', or 'weekly' strings for '' and instead need to abandon the template model. Modify the template with a custom schedule in the 'OnCalendar' setting. This schedule has some interesting differences from cron-like patterns, but is relatively easy to figure out from context. The one that might be confusing is that '*-*-*' is a date-based pattern, but this must be omitted when using 'Mon' to signal that we care about the day of the week. Monday is used since that matches the day used for the 'weekly' schedule used previously. Now that these are no longer templates, we might want to abandon the '@' symbol in the file names. However, this would cause users with existing schedules to get two competing schedules due to different names. The work to remove the old schedule name is one thing that we can avoid by keeping the '@' symbol in our unit names. The rest of the change involves making sure we are writing these .timer and .service files before initializing the schedule with 'systemctl' and deleting the files when we are done. Some changes are also made to share the random minute along with a single computation of the execution path of the current Git executable. Signed-off-by: Derrick Stolee --- builtin/gc.c | 89 ++++++++++++++++++++++++++++++++---------- t/t7900-maintenance.sh | 11 ++++-- 2 files changed, 76 insertions(+), 24 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 54536a1b3be3c8..6e1a17b2a48a5f 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2292,13 +2292,22 @@ static char *xdg_config_home_systemd(const char *filename) return xdg_config_home_for("systemd/user", filename); } -static int systemd_timer_write_unit_templates(const char *exec_path) +#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s" + +static int systemd_timer_write_unit_file(enum schedule_priority schedule, + const char *exec_path, + int minute) { 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"); + char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "service"); + + 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; @@ -2307,6 +2316,23 @@ static int systemd_timer_write_unit_templates(const char *exec_path) if (!file) goto error; + switch (schedule) { + case SCHEDULE_HOURLY: + schedule_pattern = xstrfmt("*-*-* *:%02d:00", minute); + break; + + case SCHEDULE_DAILY: + schedule_pattern = xstrfmt("*-*-* 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" @@ -2315,12 +2341,12 @@ static int systemd_timer_write_unit_templates(const char *exec_path) "Description=Optimize Git repositories data\n" "\n" "[Timer]\n" - "OnCalendar=%i\n" + "OnCalendar=%s\n" "Persistent=true\n" "\n" "[Install]\n" "WantedBy=timers.target\n"; - if (fputs(unit, file) == EOF) { + if (fprintf(file, unit, schedule_pattern) < 0) { error(_("failed to write to '%s'"), filename); fclose(file); goto error; @@ -2331,7 +2357,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path) } free(filename); - filename = xdg_config_home_systemd("git-maintenance@.service"); + filename = xdg_config_home_systemd(local_service_name); file = fopen_or_warn(filename, "w"); if (!file) goto error; @@ -2345,7 +2371,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path) "\n" "[Service]\n" "Type=oneshot\n" - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" + "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s\n" "LockPersonality=yes\n" "MemoryDenyWriteExecute=yes\n" "NoNewPrivileges=yes\n" @@ -2355,7 +2381,7 @@ static int systemd_timer_write_unit_templates(const char *exec_path) "RestrictSUIDSGID=yes\n" "SystemCallArchitectures=native\n" "SystemCallFilter=@system-service\n"; - if (fprintf(file, unit, exec_path, exec_path) < 0) { + if (fprintf(file, unit, exec_path, exec_path, frequency) < 0) { error(_("failed to write to '%s'"), filename); fclose(file); goto error; @@ -2368,13 +2394,16 @@ static int systemd_timer_write_unit_templates(const char *exec_path) return 0; error: + free(schedule_pattern); + free(local_timer_name); free(filename); - systemd_timer_delete_unit_templates(); return -1; } static int systemd_timer_enable_unit(int enable, - enum schedule_priority schedule) + enum schedule_priority schedule, + const char *exec_path, + int minute) { const char *cmd = "systemctl"; struct child_process child = CHILD_PROCESS_INIT; @@ -2391,12 +2420,14 @@ static int systemd_timer_enable_unit(int enable, */ if (!enable) child.no_stderr = 1; + else if (systemd_timer_write_unit_file(schedule, exec_path, 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, "git-maintenance@%s.timer", frequency); + strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer"); if (start_command(&child)) return error(_("failed to start systemctl")); @@ -2413,38 +2444,54 @@ static int systemd_timer_enable_unit(int enable, return 0; } -static int systemd_timer_delete_unit_templates(void) +static int systemd_timer_delete_unit_file(enum schedule_priority priority) { + const char *frequency = get_frequency(priority); + char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); + char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "service"); int ret = 0; - char *filename = xdg_config_home_systemd("git-maintenance@.timer"); + char *filename = xdg_config_home_systemd(local_timer_name); 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"); + filename = xdg_config_home_systemd(local_service_name); if (unlink(filename) && !is_missing_file_error(errno)) ret = error_errno(_("failed to delete '%s'"), filename); free(filename); + free(local_timer_name); + free(local_service_name); return ret; } +static int systemd_timer_delete_unit_files(void) +{ + /* Purposefully not short-circuited to make sure all are called. */ + return systemd_timer_delete_unit_file(SCHEDULE_HOURLY) | + systemd_timer_delete_unit_file(SCHEDULE_DAILY) | + systemd_timer_delete_unit_file(SCHEDULE_WEEKLY); +} + 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(); + int minute = get_random_minute(); + const char *exec_path = git_exec_path(); + /* Purposefully not short-circuited to make sure all are called. */ + return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, exec_path, minute) | + systemd_timer_enable_unit(0, SCHEDULE_DAILY, exec_path, minute) | + systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, exec_path, minute) | + systemd_timer_delete_unit_files(); } static int systemd_timer_setup_units(void) { + int minute = get_random_minute(); 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); + int ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY, exec_path, minute) || + systemd_timer_enable_unit(1, SCHEDULE_DAILY, exec_path, minute) || + systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, exec_path, minute); if (ret) systemd_timer_delete_units(); return ret; diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 487e326b3fac12..a327973990e069 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -744,7 +744,9 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' # start registers the repo git config --get --global --fixed-value maintenance.repo "$(pwd)" && - test_systemd_analyze_verify "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" && printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && test_cmp expect args && @@ -755,8 +757,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' ' # 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" && + for schedule in hourly daily weekly + do + test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" && + test_path_is_missing "systemd/user/git-maintenance@$schedule.service" || return 1 + done && printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && test_cmp expect args From 958f6440564f3c14e28a6d7f2f3ede9fb9d85bc9 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 8 Aug 2023 13:18:36 -0400 Subject: [PATCH 7/7] maintenance: fix systemd schedule overlaps The 'git maintenance run' command prevents concurrent runs in the same repository using a 'maintenance.lock' file. However, when using systemd the hourly maintenance runs the same time as the daily and weekly runs. (Similarly, daily maintenance runs at the same time as weekly maintenance.) These competing commands result in some maintenance not actually being run. This overlap was something we could not fix until we made the recent change to not use the builting 'hourly', 'daily', and 'weekly' schedules in systemd. We can adjust the schedules such that: 1. Hourly runs avoid the 0th hour. 2. Daily runs avoid Monday. This will keep maintenance runs from colliding when using systemd. Signed-off-by: Derrick Stolee --- builtin/gc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 6e1a17b2a48a5f..0a515ecb8e33f2 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2318,11 +2318,11 @@ static int systemd_timer_write_unit_file(enum schedule_priority schedule, switch (schedule) { case SCHEDULE_HOURLY: - schedule_pattern = xstrfmt("*-*-* *:%02d:00", minute); + schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute); break; case SCHEDULE_DAILY: - schedule_pattern = xstrfmt("*-*-* 0:%02d:00", minute); + schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute); break; case SCHEDULE_WEEKLY: