Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler

[Phillip Wood <phillip.wood123@gmail.com>]

On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:
> 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.

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.

> 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.

I've left some comments about this below.


> @@ -2299,13 +2299,20 @@ 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)
> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,

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

> +					     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("git-maintenance@%s.timer", frequency);

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"

> +	char *local_service_name = xstrfmt("git-maintenance@%s.service", frequency);

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.

> [...]
> @@ -2375,13 +2399,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();

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.

> [...]
> -static int systemd_timer_delete_unit_templates(void)
> +static int systemd_timer_delete_unit_template(enum schedule_priority priority)

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

>   {
> +	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);

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");

> [...]
> +static int systemd_timer_delete_unit_templates(void)

Naming again.

Best Wishes

Phillip