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