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

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 8, 2023, 17:06 UTC
Message-ID: <d247ce48-e2ba-4915-8d96-1fa2f693909d@github.com>
In-Reply-To: <3e254d61-b74a-f419-1a03-dbc557a9fe86@gmail.com>

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

Show 11 quoted lines

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

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

Show 7 quoted lines

>> @@ -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()

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

Show 11 quoted lines

>> +                         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"

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

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

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.

Show 10 quoted lines

>> [...]
>> @@ -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.

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.

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

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

I appreciate the careful review!

Thanks, -Stolee