patch, 6 partsmaintenance: schedule maintenance on a 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.

This series attempts to help by spreading out background maintenance to a random minute of the hour. This minute is selected during git maintenance start, and the same minute applies to each of the three schedules.

This isn't a full solution to this problem, as the custom infrastructure needs to be resilient to bursts of activity, but at least this will help somewhat.

Each of the integrated schedulers needs a different way of integrating the random minute. The most problematic is systemd, since our integration had a clever use of templates to write one schedule that inserted the hourly, daily, and weekly schedules as a string into the template. This needs some refactoring before the custom minute could be inserted.

For the most part, each scheduler's integration is relatively simple. That is, until we get to the systemd integration. That integration made use of a clever templating technique that is no longer possible when making this adjustment. The last two patches involve systemd, though one is just a move of code (without edits) to make the second's diff look a lot simpler.

Thanks, -Stolee

Derrick Stolee (6):
  maintenance: add get_random_minute()
  maintenance: use random minute in launchctl scheduler
  maintenance: use random minute in Windows scheduler
  maintenance: use random minute in cron scheduler
  maintenance: swap method locations
  maintenance: use random minute in systemd scheduler

 builtin/gc.c           | 259 ++++++++++++++++++++++++++---------------
 t/t7900-maintenance.sh |   4 +-
 2 files changed, 166 insertions(+), 97 deletions(-)

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1567%2Fderrickstolee%2Fmaintenance-random-minute-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1567/derrickstolee/maintenance-random-minute-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1567

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

On Mon, Aug 07, 2023 at 06:51:36PM +0000, Derrick Stolee via GitGitGadget wrote:

All looks good. I was wondering if this is something that we'd want to allow users to opt out of, but I think that the built-in schedules are sufficiently vague that we can afford to have a fudge factor on the exact minute.

Thanks, Taylor

From: Derrick Stolee <derrickstolee@github.com>

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.

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 <derrickstolee@github.com>
---
 builtin/gc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

On Mon, Aug 07, 2023 at 06:51:35PM +0000, Derrick Stolee via GitGitGadget wrote:

I was wondering where else we call srand() within Git, and it looks like the only other spot is in `lock_file_timeout()`.

I doubt it, but is there a chance that that code depends on only calling srand() once? I think the answer is "no", since we only use rand() within that function to generate a random-ish backoff period, so I think the repeatability of it doesn't matter all that much.

So I think this is kind of outside the scope of your series, but I wonder if we should have a git_rand() that automatically initializes the PRNG with the value of getpid()? Then multiple callers can grab random values at will without reinitializing the PRNG.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

lock_file_timeout() should be updated to match git_mkstemps_mode(), which was taught to use the csprng_bytes() function with 47efda96 (wrapper: use a CSPRNG to generate random file names, 2022-01-17), and this new caller may want to do so as well, perhaps? I dunno, but the caller then does not have to worry about "initializing it just once".

Junio C Hamano <gitster@pobox.com> writes:

Of course, the obvious downside is that crypto-secure one may be, unlike for its use in mkstemps(), way overkill for lockfiles and cron dispersion purposes, as these codepaths are not on the target surface.

On Mon, Aug 07, 2023 at 05:22:49PM -0700, Junio C Hamano wrote:

I think that's an acceptable price to pay here, since we can drop the code to remember whether or not srand() has been called or not. Here's a patch that we could take in that direction:

--- 8< ---
Subject: [PATCH] lockfile.c: use a CSPRNG to generate backoff milliseconds

Upon failing to acquire the lockfile, `lock_file_timeout()` will try again with an exponential backoff. This backoff includes some noise as a multiplier over the default backoff behavior ranging from [0.75, 1.25].

It generates this noise via rand(3). Using a non-cryptographic source of randomness here is OK, since a more trivial attack vector (holding the file open via an external process for longer than the value of `timeout_ms`) is easier to exploit.

That all said, `lock_file_timeout()` initializes the PRNG with `srand()`. This has a couple of downsides:

  - lock_file_timeout() needs to remember whether or not the PRNG has
    or hasn't been seeded.

  - If a different function also calls `srand()`, the PRNG may start
    generating repeated values (if that caller also initialized the PRNG
    with `getpid()`).

Let's avoid both of these by using `csprng_bytes()`, in a similar spirit as 47efda967c (wrapper: use a CSPRNG to generate random file names, 2022-01-17).

Using a CSPRNG is definitely overkill for noising a backoff window, but it avoids the concerns about calling `srand()`, so let's use it here, too.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 lockfile.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Hmph, I didn't think about that one. I do not think it can be a viable attack vector to attack _this_ code, but the other function might be. But if the other function is worth attacking and attackable, it ought to be using crypto-secure one and not srand(), so this argument may not give us a good justification X-<.

Provided if code simplification is a good enough rationale, the patch looks sensible, but I find its use of u64 a bit questionable (though not wrong). I would have expected that the type of "rand" would be the same as backoff_ms and wait_ms, two variables involved in the same expression.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Ah, not so fast. The use of modulo means we need to be careful about about the fuzzing factor going negative, and use of unsigned type allows us to forget about it.

(fuzz % 250), when fuzz is of a signed random integral type, ranges between -250 and +250 and because we want the center of our distribution at 1000, so I think the following is equivalent.

 lockfile.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

On Tue, Aug 08, 2023 at 09:49:30AM -0700, Junio C Hamano wrote:

[...]

I was going to say that having rand be an unsigned type was the right behavior, but with this change it can (and should) be signed. Thanks for the simplification :-).

Thanks, Taylor

On 8/7/2023 5:20 PM, Taylor Blau wrote:

The main point would be to avoid the cost of spinning up the number generator, but there is potential for a bug if we run both initializers: the lockfile would re-use the same filenames after the generator is reset.

I see you're moving ahead with removing the srand() from the lockfile code, so I'll focus on creating a `git_rand()` that centralizes the use of srand(), but won't touch the code in the lockfile so your patch applies independently.

Thanks, -Stolee

On Tue, Aug 08, 2023 at 01:28:50PM -0400, Derrick Stolee wrote:

That thread may have progressed a little since you last looked at it.

Instead of using srand() and rand() (which would make sense to wrap with git_rand() as you propose), we can simplify our lives by using a CSPRNG, which only gets initialized once, as is already the case with csprng_bytes().

I think Junio is picking up a lightly modified version of my patch there, see [1].

Thanks, Taylor

[1]: https://lore.kernel.org/git/ZNKfKs1mLQhnybvF@nand.local/

On 8/8/2023 4:04 PM, Taylor Blau wrote:

I think this part of my summary is still correct.

So the idea is to use csprng_bytes() everywhere instead of srand()/rand().

I can adjust my local patch to still create git_rand(), but base it on csprng_bytes() and not collide with your patch. Mimicking rand()'s behavior is a simpler interface to consume.

Thanks, -Stolee

Derrick Stolee <derrickstolee@github.com> writes:

I am still ambivalent about wasting entropy for something that srand() would suffice, so git_rand() interface may be an welcome addition anyway, that serves an extra layer of indirection to insulate the callers from the implementation.

Thanks.

On Wed, Aug 09, 2023 at 11:50:38AM -0700, Junio C Hamano wrote:

Sounds good to me, I'm not particularly attached to one implementation over another. Thanks, both.

Thanks, Taylor

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 128 +++++++++++++++++++++++++--------------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

On Mon, Aug 07, 2023 at 06:51:39PM +0000, Derrick Stolee via GitGitGadget wrote:

This looks like a straightforward code movement, and diffing with `--color-moved` verifies it as such. LGTM.

Thanks, Taylor

From: Derrick Stolee <derrickstolee@github.com>

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 '...@<schedule>.timer' or '...@<schedule>.service'. The '<schedule>' 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 '<schedule>' 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.

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 <derrickstolee@github.com>
---
 builtin/gc.c           | 82 ++++++++++++++++++++++++++++++++----------
 t/t7900-maintenance.sh |  4 ++-
 2 files changed, 66 insertions(+), 20 deletions(-)

On Mon, Aug 07, 2023 at 06:51:40PM +0000, Derrick Stolee via GitGitGadget wrote:

Makes sense.

I think the launchd version (which uses "0" for the day of the week) runs on Sunday, if I remember correctly. I don't think that these two necessarily need to run on the same day of the week when configured to run weekly.

But I figured I'd raise the question in case you did mean for them to both run on either Sunday or Monday.

You should be able to drop the NULL initialization, since you assign this value unconditionally in the switch statement below (or BUG() on an unknown schedule type).

OK, this is the templating part that you were mentioning earlier. I was wondering what we were doing fputs()-ing a string with "%i" in it without a formatting value to fill it in with. But that "%i" pertains to systemd's instance value, IIUC.

The rest all looks good, thanks.

Thanks, Taylor

On 8/7/2023 5:31 PM, Taylor Blau wrote:

I don't intend to change the day that is run as part of this change.

I think all other schedulers run on Sunday, but systemd running on Monday is a particular detail of its "weekly" schedule.

Unfortunately, GCC complained about a possibly-unassigned value when I left this unset during development, so this actually is necessary for that compiler.

Thanks, -Stolee

On Tue, Aug 08, 2023 at 09:49:40AM -0400, Derrick Stolee wrote:

No problem -- I figured that you didn't intend on changing the days around here, just wanted to make sure it was known that the systemd scheduler picks a different day of the week for its weekly schedule than the others do.

Ah, I would have thought that GCC would be smart enough to figure out that schedule_pattern is unconditionally initialized via the switch statement, but I guess not. Makes sense.

Thanks, Taylor

Hi Stolee

On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:

This is not a new issue with this patch but we run the hourly job even when we want to run the daily job or the weekly job and we run the daily job when we want to run the weekly job. maintenance_run_tasks() contains

	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
		/*
		 * Another maintenance command is running.
		 *
		 * If --auto was provided, then it is likely due to a
		 * recursive process stack. Do not report an error in
		 * that case.
		 */
		if (!opts->auto_flag && !opts->quiet)
			warning(_("lock file '%s' exists, skipping maintenance"),
				lock_path);
		free(lock_path);
		return 0;
	}

So only one of these jobs will succeed. The cron entries are careful to only run one job at a time, I think it would be worth doing the same thing here. I think the using the following format strings would fix this.

Hourly: "Tue..Sun *-*-* 1..23:00:%02d"
Daily:  "Tue..Sun *-*-* 00:00:%02d"
Weekly: "Mon      *-*-* 00:00:%02d"

It looks like the launchctl schedule has the same issue.

One thing I've been wondering about which is related to maintenance but totally off-topic for this patch is that I think when auto maintenance is enabled we stop automatically running "gc" so how do the reflogs get expired?

Best Wishes

Phillip

On 8/8/2023 5:53 AM, Phillip Wood wrote:> Hi Stolee

This is an excellent point! Thanks for bringing it up.

I would modify this with dropping the "Tue..Sun" from the hourly schedule, as we still want 23 runs on Mondays.

I will take a look at this and consider some additional patches to correct these issues across both schedulers. Thank you for the attention to detail!

The default maintenance schedule does not include a 'gc' run as it does not intend to remove any data. Reflog expiration could be considered as a separate maintenance task that might be useful in the default maintenance schedule.

Thanks, -Stolee

On 8/8/2023 9:56 AM, Derrick Stolee wrote:

Taking a look, it seems that launchctl does not have this same problem.

The schedule is set via an <array> of <dict>s as follows:

	case SCHEDULE_HOURLY:
		repeat = "<dict>\n"
			 "<key>Hour</key><integer>%d</integer>\n"
			 "<key>Minute</key><integer>%d</integer>\n"
			 "</dict>\n";
		for (i = 1; i <= 23; i++)
			strbuf_addf(&plist, repeat, i, minute);
		break;

	case SCHEDULE_DAILY:
		repeat = "<dict>\n"
			 "<key>Day</key><integer>%d</integer>\n"
			 "<key>Hour</key><integer>0</integer>\n"
			 "<key>Minute</key><integer>%d</integer>\n"
			 "</dict>\n";
		for (i = 1; i <= 6; i++)
			strbuf_addf(&plist, repeat, i, minute);
		break;

This means that we create an hourly schedule for each hour 1..23 (no 0th hour means no collision with daily or weekly schedule) and a daily schedule for each day 1..6 (no 0th day means no collision with the weekly schedule).

Does this match your understanding?

The overlap _definitely_ exists in systemd, which I will fix.

Thanks, -Stolee

On 08/08/2023 18:24, Derrick Stolee wrote:

Oops, well spotted

Yes, having read it again - sorry I'd misunderstood it yesterday.

That's great

Best Wishes

Phillip

On 08/08/2023 10:53, Phillip Wood wrote:

Thinking about it some more, this only fixes the problem if the computer is actually on at midnight. If the computer is switched off overnight then we still try to start three maintenance jobs at the same time when the user turns their computer on on Tuesday morning. We could stop marking the hourly job as persistent on the assumption that it will be run soon anyway but that does not solve the problem of the daily and weekly jobs running concurrently on a Tuesday morning.

Best Wishes

Phillip

On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:

I think it makes sense to keep the random minute implementation the same across all the schedulers, but we could use RandomizedDelaySec (possibly combined with FixedRandomDelay) to randomize when the job is actually run.

I've left some comments about this below.

As we're not writing template files any more I think we should rename this to systemd_timer_write_unit_file()

The "@" in the name signifies that it is a template unit which it isn't anymore so I think we want to change this to "git-maintenance-%s.timer"

Same change to the name here. I wonder if we could still use a template service file but that would complicate the implementation as we'd need to write three timer files but only one service file.

This looks like a change in behavior as previously we'd remove any files if there was an error rather than leaving behind a timer file without a corresponding unit file.

Looking at maintenance_start() we call maintenance_register() which disables "gc --auto" before we get to this point so if we fail to write the files we'll end up disabling any form of gc in the repository.

Same suggestion as above to rename this to ..._unit_file()

I'm not sure it is worth it but we could perhaps

	#define SYSTEMD_UNIT_FORMAT "git-maintenance-%s.%s"

above and then these lines and the ones in systemd_timer_write_unit_file() would become

	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
	char *local_service = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "service");

Naming again.

Best Wishes

Phillip

On 8/8/2023 8:08 AM, Phillip Wood wrote:

That's an interesting suggestion, but I also think it is valuable to have consistent gaps between maintenance activities on the client, but RandomizedDelaySec would present the possibility of 60-3540 seconds between "hourly" maintenance runs (if I understand the option correctly).

Good point. I have adjusted all the names in my next version.

I'll also take your SYSTEMD_UNIT_FORMAT macro suggestion to simplify things.
 

Since we execute systemctl only passing the .timer filename, I think we'd need to keep the '@' symbol in the name if we wanted to use .schedule templates. Best to keep things simple and abandon templates completely.

The callers take care of deleting the unit files, but there was one place where a short-circuit could have avoided this deletion. I'll clean that up.

Adding this to the list of cleanups at the end. Thanks.

I appreciate the careful review!

Thanks, -Stolee

On 8/8/2023 1:06 PM, Derrick Stolee wrote:

As I was checking things, it turns out that we _should_ keep the '@' symbol if only to make sure that our new schedule overwrites the old schedule.

The alternative is that we manually try to delete the old schedule, but that feels like an inefficient way to do it, leaving some cruft around long-term.

For completeness, here is what I did to check:

$ systemctl --user list-timers NEXT LEFT LAST PASSED UNIT ACTIVATES Tue 2023-08-08 13:13:00 EDT 6s left n/a n/a git-maintenance-hourly.timer git-maintenance-hourly.service Tue 2023-08-08 13:50:00 EDT 37min left Tue 2023-08-08 12:50:10 EDT 22min ago git-maintenance@hourly.timer git-maintenance@hourly.service Wed 2023-08-09 00:13:00 EDT 11h left n/a n/a git-maintenance-daily.timer git-maintenance-daily.service Wed 2023-08-09 00:50:00 EDT 11h left Tue 2023-08-08 09:35:31 EDT 3h 37min ago git-maintenance@daily.timer git-maintenance@daily.service Mon 2023-08-14 00:13:00 EDT 5 days left n/a n/a git-maintenance-weekly.timer git-maintenance-weekly.service Mon 2023-08-14 00:50:00 EDT 5 days left Mon 2023-08-07 10:28:10 EDT 1 day 2h ago git-maintenance@weekly.timer git-maintenance@weekly.service

Do you have an alternative idea how to handle that?

Thanks, -Stolee

On 08/08/2023 18:14, Derrick Stolee wrote:

Oh, so if the user already has scheduled maintenance set up then running "git maintenance start" adds a new set of timers. I'd not though about that.

This patch still changes the names of the files we write. Currently we write

	$XDG_CONFIG_HOME/systemd/user/git-maintenance@.service
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@.timer

and this patch changes that to

	$XDG_CONFIG_HOME/systemd/user/git-maintenance@hourly.service
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@daily.service
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@weekly.service
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@hourly.timer
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@daily.timer
	$XDG_CONFIG_HOME/systemd/user/git-maintenance@weekly.timer

If the user has already enabled maintenance then

	$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@hourly.timer
	$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@daily.timer
	$XDG_CONFIG_HOME/systemd/user/timers.target.wants/git-maintenance@weekly.timer

will exist and are all symlinks pointing to

	$XDG_CONFIG_HOME/systemd/user/git-maintenance@.timer

After this patch if the user runs "git maintenance start" again then systemctl will update the symlinks tot point to the matching unit files rather than the old template file. That means the user will pick up the new schedule but we leave behind the original files that are unused.

I think we should stick with the names as you have them. It might be worth keeping the service file as a template so we only write the new timer files and have a reason to use the "@" naming scheme. We could update systemd_timer_setup_units() to delete git-maintenance@.timer if we successfully enable the new timer unit files.

Sorry for the confusion, I should have thought about the user running "git maintenance start" for a second time.

Best Wishes

Phillip

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.

This series attempts to help by spreading out background maintenance to a random minute of the hour. This minute is selected during git maintenance start, and the same minute applies to each of the three schedules.

This isn't a full solution to this problem, as the custom infrastructure needs to be resilient to bursts of activity, but at least this will help somewhat.

Each of the integrated schedulers needs a different way of integrating the random minute. The most problematic is systemd, since our integration had a clever use of templates to write one schedule that inserted the hourly, daily, and weekly schedules as a string into the template. This needs some refactoring before the custom minute could be inserted.

For the most part, each scheduler's integration is relatively simple. That is, until we get to the systemd integration. That integration made use of a clever templating technique that is no longer possible when making this adjustment.

Patches 5-7 involve systemd, though patch 5 is just a move of code (without edits) to make the diff in patch 6 somewhat simpler (it's still complicated due to templating changes). Patch 7 fixes an issue where the systemd schedules overlap.

Patch 8 fixes an issue where config changes persist even if the scheduler fails to initialize. Thanks for noticing, Philip!

Updates in version 2 ====================

 * get_random_minute() now uses a new helper, git_rand(), which is itself a
   wrapper around csprng_bytes() for easier use.
 * get_random_minute() also had an error in its use of getenv() which is now
   fixed.
 * Patch 6 has a lot of new changes, including:
   * Keeping the .service template.
   * Deleting the old .timer template when safe to do so.
 * Patch 7 fixes the schedule overlap in systemd.
 * Patch 8 fixes the issue where 'mainteancne.auto=false' would persist even
   if the scheduler failed to initialize.

Thanks, -Stolee

Derrick Stolee (8):
  maintenance: add get_random_minute()
  maintenance: use random minute in launchctl scheduler
  maintenance: use random minute in Windows scheduler
  maintenance: use random minute in cron scheduler
  maintenance: swap method locations
  maintenance: use random minute in systemd scheduler
  maintenance: fix systemd schedule overlaps
  maintenance: update schedule before config

 builtin/gc.c           | 291 +++++++++++++++++++++++++++++------------
 t/t7900-maintenance.sh |  28 +++-
 wrapper.c              |  10 ++
 wrapper.h              |   6 +
 4 files changed, 250 insertions(+), 85 deletions(-)

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1567%2Fderrickstolee%2Fmaintenance-random-minute-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1567/derrickstolee/maintenance-random-minute-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1567

Range-diff vs v1:

 1:  fefdaa94579 ! 1:  edc08023ed5 maintenance: add get_random_minute()
     @@ Commit message
          integrations with this method do not yet exist, but will follow in
          future changes.
      
     +    To avoid multiple sources of randomness in the Git codebase, create a
     +    new helper function, git_rand(), that returns a random uint32_t. This is
     +    similar to how rand() returns a random nonnegative value, except it is
     +    based on csprng_bytes() which is cryptographic and will return values
     +    larger than RAND_MAX.
     +
          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
     @@ builtin/gc.c: static int get_schedule_cmd(const char **cmd, int *is_available)
      +MAYBE_UNUSED
      +static int get_random_minute(void)
      +{
     -+	static int random_initialized = 0;
     -+
      +	/* Use a static value when under tests. */
     -+	if (!getenv("GIT_TEST_MAINTENANCE_SCHEDULER"))
     ++	if (getenv("GIT_TEST_MAINT_SCHEDULER"))
      +		return 13;
      +
     -+	if (!random_initialized) {
     -+		srand((unsigned int)getpid());
     -+		random_initialized = 1;
     -+	}
     -+
     -+	return rand() % 60;
     ++	return git_rand() % 60;
      +}
      +
       static int is_launchctl_available(void)
       {
       	const char *cmd = "launchctl";
     +
     + ## wrapper.c ##
     +@@ wrapper.c: int csprng_bytes(void *buf, size_t len)
     + 	return 0;
     + #endif
     + }
     ++
     ++uint32_t git_rand(void)
     ++{
     ++	uint32_t result;
     ++
     ++	if (csprng_bytes(&result, sizeof(result)) < 0)
     ++		die(_("unable to get random bytes"));
     ++
     ++	return result;
     ++}
     +
     + ## wrapper.h ##
     +@@ wrapper.h: void sleep_millisec(int millisec);
     +  */
     + int csprng_bytes(void *buf, size_t len);
     + 
     ++/*
     ++ * Returns a random uint32_t, uniformly distributed across all possible
     ++ * values.
     ++ */
     ++uint32_t git_rand(void);
     ++
     + #endif /* WRAPPER_H */
 2:  babf62ec6d5 ! 2:  72ec86f2f88 maintenance: use random minute in launchctl scheduler
     @@ builtin/gc.c: static int get_schedule_cmd(const char **cmd, int *is_available)
      -MAYBE_UNUSED
       static int get_random_minute(void)
       {
     - 	static int random_initialized = 0;
     + 	/* Use a static value when under tests. */
      @@ builtin/gc.c: static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
       	struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
       	struct stat st;
 3:  1048dff1d3a = 3:  f6d9c4f3b02 maintenance: use random minute in Windows scheduler
 4:  3ef3cf0861f = 4:  b291e6f7aec maintenance: use random minute in cron scheduler
 5:  37f96b6b619 ! 5:  88610437b4b maintenance: swap method locations
     @@ Commit message
          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().
     +    The diff is computed smaller by showing systemd_timer_enable_unit() and
     +    systemd_timer_delete_units()  move instead of
     +    systemd_timer_write_unit_templates() and
     +    systemd_timer_delete_unit_templates().
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     @@ builtin/gc.c: static char *xdg_config_home_systemd(const char *filename)
      -	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_unit_templates(void)
     + {
     + 	int ret = 0;
     +@@ builtin/gc.c: static int systemd_timer_delete_unit_templates(void)
     + 	return ret;
     + }
     + 
      -static int systemd_timer_delete_units(void)
      -{
      -	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
     @@ builtin/gc.c: error:
      +	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) ||
 6:  14e340b75fa ! 6:  e43778d3e40 maintenance: use random minute in systemd scheduler
     @@ Commit message
      
          In order to set these schedules to a given minute, we can no longer use
          the 'hourly', 'daily', or 'weekly' strings for '<schedule>' and instead
     -    need to abandon the template model.
     +    need to abandon the template model for the .timer files. We can still
     +    use templates for the .service files. For this reason, we split these
     +    writes into two methods.
      
          Modify the template with a custom schedule in the 'OnCalendar' setting.
          This schedule has some interesting differences from cron-like patterns,
     @@ Commit message
          week. Monday is used since that matches the day used for the 'weekly'
          schedule used previously.
      
     +    Now that the timer files are not 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. Since we are locked
     +    into this name, it makes sense that we keep the template model for the
     +    .service files.
     +
          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.
      
     +    In addition, older Git versions may have written a
     +    'git-maintenance@.timer' template file. Be sure to remove this when
     +    successfully enabling maintenance (or disabling maintenance).
     +
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
       ## builtin/gc.c ##
     @@ builtin/gc.c: static char *xdg_config_home_systemd(const char *filename)
       	return xdg_config_home_for("systemd/user", filename);
       }
       
     +-static int systemd_timer_delete_unit_templates(void)
     ++#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
     ++
     ++static int systemd_timer_delete_timer_file(enum schedule_priority priority)
     + {
     + 	int ret = 0;
     +-	char *filename = xdg_config_home_systemd("git-maintenance@.timer");
     ++	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);
     ++
     + 	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");
     ++	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);
     + 	if (unlink(filename) && !is_missing_file_error(errno))
     + 		ret = error_errno(_("failed to delete '%s'"), filename);
     + 
     + 	free(filename);
     ++	free(local_service_name);
     + 	return ret;
     + }
     + 
      -static int systemd_timer_write_unit_templates(const char *exec_path)
     -+static int systemd_timer_write_unit_template(enum schedule_priority schedule,
     -+					     const char *exec_path,
     -+					     int minute)
     ++/*
     ++ * 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)
       {
     ++	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("git-maintenance@%s.timer", frequency);
     -+	char *local_service_name = xstrfmt("git-maintenance@%s.service", frequency);
     ++	char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
      +
      +	filename = xdg_config_home_systemd(local_timer_name);
       
     @@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_pat
       		fclose(file);
       		goto error;
      @@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)
     + 		error_errno(_("failed to flush '%s'"), filename);
     + 		goto error;
       	}
     ++
     ++	res = 0;
     ++
     ++error:
     ++	free(schedule_pattern);
     ++	free(local_timer_name);
       	free(filename);
     ++	return res;
     ++}
       
      -	filename = xdg_config_home_systemd("git-maintenance@.service");
     ++/*
     ++ * 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;
     ++	}
       	file = fopen_or_warn(filename, "w");
       	if (!file)
       		goto error;
      @@ builtin/gc.c: 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"
     -@@ builtin/gc.c: 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);
     + 		error_errno(_("failed to flush '%s'"), filename);
       		goto error;
     -@@ builtin/gc.c: static int systemd_timer_write_unit_templates(const char *exec_path)
     - 	return 0;
     + 	}
     +-	free(filename);
     +-	return 0;
     ++
     ++	res = 0;
       
       error:
     -+	free(schedule_pattern);
     -+	free(local_timer_name);
     ++	free(local_service_name);
       	free(filename);
      -	systemd_timer_delete_unit_templates();
     - 	return -1;
     +-	return -1;
     ++	return res;
       }
       
       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";
     @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,
       	 */
       	if (!enable)
       		child.no_stderr = 1;
     -+	else if (systemd_timer_write_unit_template(schedule, exec_path, minute))
     ++	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, "git-maintenance@%s.timer", frequency);
     ++	strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");
     + 
     + 	if (start_command(&child))
     + 		return error(_("failed to start systemctl"));
      @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,
       	return 0;
       }
       
     --static int systemd_timer_delete_unit_templates(void)
     -+static int systemd_timer_delete_unit_template(enum schedule_priority priority)
     - {
     -+	const char *frequency = get_frequency(priority);
     -+	char *local_timer_name = xstrfmt("git-maintenance@%s.timer", frequency);
     -+	char *local_service_name = xstrfmt("git-maintenance@%s.service", frequency);
     - 	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_templates(void)
     ++/*
     ++ * 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_unit_template(SCHEDULE_HOURLY) |
     -+	       systemd_timer_delete_unit_template(SCHEDULE_DAILY) |
     -+	       systemd_timer_delete_unit_template(SCHEDULE_WEEKLY);
     ++	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)
     @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,
      -	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();
     -+	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_templates();
     ++	/* 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();
       }
       
       static int systemd_timer_setup_units(void)
     @@ builtin/gc.c: static int systemd_timer_enable_unit(int enable,
      -		  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);
     ++	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);
     ++
       	if (ret)
       		systemd_timer_delete_units();
     ++	else
     ++		systemd_timer_delete_stale_timer_templates();
     ++
       	return ret;
     + }
     + 
      
       ## t/t7900-maintenance.sh ##
      @@ t/t7900-maintenance.sh: test_expect_success 'start and stop Linux/systemd maintenance' '
     @@ t/t7900-maintenance.sh: test_expect_success 'start and stop Linux/systemd mainte
       	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
       
      -	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
     ++	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" &&
       
       	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
       	test_cmp expect args &&
     +@@ t/t7900-maintenance.sh: 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" &&
     ++	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@.service" &&
     + 
     + 	printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
 -:  ----------- > 7:  86c4255d63d maintenance: fix systemd schedule overlaps
 -:  ----------- > 8:  f0c0f6eff88 maintenance: update schedule before config

From: Derrick Stolee <derrickstolee@github.com>

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 sources of randomness in the Git codebase, create a new helper function, git_rand(), that returns a random uint32_t. This is similar to how rand() returns a random nonnegative value, except it is based on csprng_bytes() which is cryptographic and will return values larger than RAND_MAX.

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 <derrickstolee@github.com>
---
 builtin/gc.c | 10 ++++++++++
 wrapper.c    | 10 ++++++++++
 wrapper.h    |  6 ++++++
 3 files changed, 26 insertions(+)

On Thu, Aug 10, 2023 at 08:39:40PM +0000, Derrick Stolee via GitGitGadget wrote:

Very nice, this patch LGTM. Thanks for working on it!

Thanks, Taylor

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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() and systemd_timer_delete_units() move instead of systemd_timer_write_unit_templates() and systemd_timer_delete_unit_templates().

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c | 96 ++++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Simple and stupid is good. When I read the problem description, I wondered if the hour in which daily jobs are run will also be dispersed (so that the load of the server that helps daily tasks at the clients will not be concentrated in a single hour out of 24 hours of a day), but hopefully the load is still spread within that 60 minutes evenly, which probably is good enough.

Hi Stolee

On 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:

Thanks for fixing this, the patch looks good

Best Wishes

Phillip

From: Derrick Stolee <derrickstolee@github.com>

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 '...@<schedule>.timer' or '...@<schedule>.service'. The '<schedule>' 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 '<schedule>' and instead need to abandon the template model for the .timer files. We can still use templates for the .service files. For this reason, we split these writes into two methods.

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 the timer files are not 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. Since we are locked into this name, it makes sense that we keep the template model for the .service files.

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.

In addition, older Git versions may have written a 'git-maintenance@.timer' template file. Be sure to remove this when successfully enabling maintenance (or disabling maintenance).

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c           | 152 ++++++++++++++++++++++++++++++++++-------
 t/t7900-maintenance.sh |  15 +++-
 2 files changed, 142 insertions(+), 25 deletions(-)

Hi Stolee

This all looks good to me now, I've left a comment on the test but I think it is probably fine to go in as it is.

Thanks for adding comments to the functions that write the timer and service files, they really helpful especially for readers who are not that familiar with systemd.

Using "|" rather than "||" is a nice touch.

As we only write the template service file I'm not sure what we gain by these three checks but they don't seem to be doing any harm.

Best Wishes

Phillip

From: Derrick Stolee <derrickstolee@github.com>

When running 'git maintenance start', the current pattern is to configure global config settings to enable maintenance on the current repository and set 'maintenance.auto' to false and _then_ to set up the schedule with the system scheduler.

This has a problematic error condition: if the scheduler fails to initialize, the repository still will not use automatic maintenance due to the 'maintenance.auto' setting.

Fix this gap by swapping the order of operations. If Git fails to initialize maintenance, then the config changes should never happen.

Reported-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c           |  5 ++++-
 t/t7900-maintenance.sh | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Hi Stolee

On 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:

The commit message and patch look good to me

Thanks

Phillip