Re: [PATCH v5 3/3] maintenance: add support for systemd timers on Linux

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

On 08/06/2021 14:40, Lénaïc Huard wrote:
> The existing mechanism for scheduling background maintenance is done
> through cron. On Linux systems managed by systemd, systemd provides an
> alternative to schedule recurring tasks: systemd timers.
> 
> The main motivations to implement systemd timers in addition to cron
> are:
> * cron is optional and Linux systems running systemd might not have it
>    installed.
> * The execution of `crontab -l` can tell us if cron is installed but not
>    if the daemon is actually running.
> * With systemd, each service is run in its own cgroup and its logs are
>    tagged by the service inside journald. With cron, all scheduled tasks
>    are running in the cron daemon cgroup and all the logs of the
>    user-scheduled tasks are pretended to belong to the system cron
>    service.
>    Concretely, a user that doesn’t have access to the system logs won’t
>    have access to the log of their own tasks scheduled by cron whereas
>    they will have access to the log of their own tasks scheduled by
>    systemd timer.
>    Although `cron` attempts to send email, that email may go unseen by
>    the user because these days, local mailboxes are not heavily used
>    anymore.
> 
> In order to schedule git maintenance, we need two unit template files:
> * ~/.config/systemd/user/git-maintenance@.service
>    to define the command to be started by systemd and
> * ~/.config/systemd/user/git-maintenance@.timer
>    to define the schedule at which the command should be run.
> 
> Those units are templates that are parameterized by the frequency.
> 
> Based on those templates, 3 timers are started:
> * git-maintenance@hourly.timer
> * git-maintenance@daily.timer
> * git-maintenance@weekly.timer
> 
> The command launched by those three timers are the same as with the
> other scheduling methods:
> 
> /path/to/git for-each-repo --exec-path=/path/to
> --config=maintenance.repo maintenance run --schedule=%i
> 
> with the full path for git to ensure that the version of git launched
> for the scheduled maintenance is the same as the one used to run
> `maintenance start`.
> 
> The timer unit contains `Persistent=true` so that, if the computer is
> powered down when a maintenance task should run, the task will be run
> when the computer is back powered on.
> 
> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr>
> ---
>   Documentation/git-maintenance.txt |  57 +++++++-
>   builtin/gc.c                      | 228 ++++++++++++++++++++++++++++++
>   t/t7900-maintenance.sh            |  67 ++++++++-
>   3 files changed, 342 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 07065ed4f3..7b7dbbbca9 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -179,16 +179,19 @@ OPTIONS
>   	`maintenance.<task>.enabled` configured as `true` are considered.
>   	See the 'TASKS' section for the list of accepted `<task>` values.
>   
> ---scheduler=auto|crontab|launchctl|schtasks::
> +--scheduler=auto|crontab|systemd-timer|launchctl|schtasks::
>   	When combined with the `start` subcommand, specify the scheduler
>   	to use to run the hourly, daily and weekly executions of
>   	`git maintenance run`.
>   	The possible values for `<scheduler>` depend on the system: `crontab`
> -	is available on POSIX systems, `launchctl` is available on
> -	MacOS and `schtasks` is available on Windows.
> +	is available on POSIX systems, `systemd-timer` is available on Linux
> +	systems, `launchctl` is available on MacOS and `schtasks` is available
> +	on Windows.
>   	By default or when `auto` is specified, a suitable scheduler for
>   	the system is used. On MacOS, `launchctl` is used. On Windows,
> -	`schtasks` is used. On all other systems, `crontab` is used.
> +	`schtasks` is used. On Linux, `systemd-timer` is used if user systemd
> +	timers are available, otherwise, `crontab` is used. On all other systems,
> +	`crontab` is used.
>   
>   
>   TROUBLESHOOTING
> @@ -288,6 +291,52 @@ schedule to ensure you are executing the correct binaries in your
>   schedule.
>   
>   
> +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMS
> +-----------------------------------------------
> +
> +While Linux supports `cron`, depending on the distribution, `cron` may
> +be an optional package not necessarily installed. On modern Linux
> +distributions, systemd timers are superseding it.
> +
> +If user systemd timers are available, they will be used as a replacement
> +of `cron`.
> +
> +In this case, `git maintenance start` will create user systemd timer units
> +and start the timers. The current list of user-scheduled tasks can be found
> +by running `systemctl --user list-timers`. The timers written by `git
> +maintenance start` are similar to this:
> +
> +-----------------------------------------------------------------------
> +$ systemctl --user list-timers
> +NEXT                         LEFT          LAST                         PASSED     UNIT                         ACTIVATES
> +Thu 2021-04-29 19:00:00 CEST 42min left    Thu 2021-04-29 18:00:11 CEST 17min ago  git-maintenance@hourly.timer git-maintenance@hourly.service
> +Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago    git-maintenance@daily.timer  git-maintenance@daily.service
> +Mon 2021-05-03 00:00:00 CEST 3 days left   Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service
> +-----------------------------------------------------------------------
> +
> +One timer is registered for each `--schedule=<frequency>` option.
> +
> +The definition of the systemd units can be inspected in the following files:
> +
> +-----------------------------------------------------------------------
> +~/.config/systemd/user/git-maintenance@.timer
> +~/.config/systemd/user/git-maintenance@.service
> +~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer
> +~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer
> +~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer
> +-----------------------------------------------------------------------
> +
> +`git maintenance start` will overwrite these files and start the timer
> +again with `systemctl --user`, so any customization should be done by
> +creating a drop-in file, i.e. a `.conf` suffixed file in the
> +`~/.config/systemd/user/git-maintenance@.service.d` directory.
> +
> +`git maintenance stop` will stop the user systemd timers and delete
> +the above mentioned files.
> +
> +For more details, see `systemd.timer(5)`.
> +
> +
>   BACKGROUND MAINTENANCE ON MACOS SYSTEMS
>   ---------------------------------------
>   
> diff --git a/builtin/gc.c b/builtin/gc.c
> index f2a81ecb44..5fe416c903 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2040,10 +2040,221 @@ static int crontab_update_schedule(int run_maintenance, int fd)
>   	return result;
>   }
>   
> +#ifdef __linux__
> +
> +static int real_is_systemd_timer_available(void)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	strvec_pushl(&child.args, "systemctl", "--user", "list-timers", NULL);
> +	child.no_stdin = 1;
> +	child.no_stdout = 1;
> +	child.no_stderr = 1;
> +	child.silent_exec_failure = 1;
> +
> +	if (start_command(&child))
> +		return 0;
> +	if (finish_command(&child))
> +		return 0;
> +	return 1;
> +}
> +
> +#else
> +
> +static int real_is_systemd_timer_available(void)
> +{
> +	return 0;
> +}
> +
> +#endif
> +
> +static int is_systemd_timer_available(void)
> +{
> +	const char *cmd = "systemctl";
> +	int is_available;
> +
> +	if (get_schedule_cmd(&cmd, &is_available))
> +		return is_available;
> +
> +	return real_is_systemd_timer_available();
> +}
> +
> +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;
> +	FILE *file;
> +	const char *unit;
> +
> +	filename = xdg_config_home_systemd("git-maintenance@.timer");
> +	if (safe_create_leading_directories(filename)) {
> +		error(_("failed to create directories for '%s'"), filename);
> +		goto error;
> +	}
> +	file = fopen_or_warn(filename, "w");
> +	if (file == NULL)
> +		goto error;
> +	FREE_AND_NULL(filename);
> +
> +	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"
> +	       "\n"
> +	       "[Unit]\n"
> +	       "Description=Optimize Git repositories data\n"
> +	       "\n"
> +	       "[Timer]\n"
> +	       "OnCalendar=%i\n"
> +	       "Persistent=true\n"
> +	       "\n"
> +	       "[Install]\n"
> +	       "WantedBy=timers.target\n";
> +	if (fputs(unit, file) == EOF) {
> +		error(_("failed to write to '%s'"), filename);
> +		fclose(file);
> +		goto error;
> +	}
> +	if (fclose(file) == EOF) {
> +		error_errno(_("failed to flush '%s'"), filename);
> +		goto error;
> +	}
> +
> +	filename = xdg_config_home_systemd("git-maintenance@.service");
> +	file = fopen_or_warn(filename, "w");
> +	if (file == NULL)
> +		goto error;
> +	FREE_AND_NULL(filename);
> +
> +	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"
> +	       "\n"
> +	       "[Unit]\n"
> +	       "Description=Optimize Git repositories data\n"
> +	       "\n"
> +	       "[Service]\n"
> +	       "Type=oneshot\n"
> +	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
> +	       "LockPersonality=yes\n"
> +	       "MemoryDenyWriteExecute=yes\n"
> +	       "NoNewPrivileges=yes\n"
> +	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n"
> +	       "RestrictNamespaces=yes\n"
> +	       "RestrictRealtime=yes\n"
> +	       "RestrictSUIDSGID=yes\n"
> +	       "SystemCallArchitectures=native\n"
> +	       "SystemCallFilter=@system-service\n";
> +	if (fprintf(file, unit, exec_path, exec_path) < 0) {
> +		error(_("failed to write to '%s'"), filename);
> +		fclose(file);
> +		goto error;
> +	}
> +	if (fclose(file) == EOF) {
> +		error_errno(_("failed to flush '%s'"), filename);
> +		goto error;
> +	}
> +	return 0;
> +
> +error:
> +	free(filename);
> +	systemd_timer_delete_unit_templates();

One small comment just to show I have read the patches - I think 
systemd_timer_delete_unit_templates() ends up being called twice if we 
fail to write one of the template files as systemd_timer_setup_units() 
calls it if this function fails. I don't think this matters and there is 
no need to change it - from a future maintenance perspective it is 
probably safer to leave it as is.

Best Wishes

Phillip

> +	return -1;
> +}
> +
> +static int systemd_timer_setup_units(void)
> +{
> +	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);
> +	if (ret)
> +		systemd_timer_delete_units();
> +	return ret;
> +}
> +
> +static int systemd_timer_update_schedule(int run_maintenance, int fd)
> +{
> +	if (run_maintenance)
> +		return systemd_timer_setup_units();
> +	else
> +		return systemd_timer_delete_units();
> +}
> +
>   enum scheduler {
>   	SCHEDULER_INVALID = -1,
>   	SCHEDULER_AUTO,
>   	SCHEDULER_CRON,
> +	SCHEDULER_SYSTEMD,
>   	SCHEDULER_LAUNCHCTL,
>   	SCHEDULER_SCHTASKS,
>   };
> @@ -2058,6 +2269,11 @@ static const struct {
>   		.is_available = is_crontab_available,
>   		.update_schedule = crontab_update_schedule,
>   	},
> +	[SCHEDULER_SYSTEMD] = {
> +		.name = "systemctl",
> +		.is_available = is_systemd_timer_available,
> +		.update_schedule = systemd_timer_update_schedule,
> +	},
>   	[SCHEDULER_LAUNCHCTL] = {
>   		.name = "launchctl",
>   		.is_available = is_launchctl_available,
> @@ -2078,6 +2294,9 @@ static enum scheduler parse_scheduler(const char *value)
>   		return SCHEDULER_AUTO;
>   	else if (!strcasecmp(value, "cron") || !strcasecmp(value, "crontab"))
>   		return SCHEDULER_CRON;
> +	else if (!strcasecmp(value, "systemd") ||
> +		 !strcasecmp(value, "systemd-timer"))
> +		return SCHEDULER_SYSTEMD;
>   	else if (!strcasecmp(value, "launchctl"))
>   		return SCHEDULER_LAUNCHCTL;
>   	else if (!strcasecmp(value, "schtasks"))
> @@ -2116,6 +2335,15 @@ static void resolve_auto_scheduler(enum scheduler *scheduler)
>   	*scheduler = SCHEDULER_SCHTASKS;
>   	return;
>   
> +#elif defined(__linux__)
> +	if (is_systemd_timer_available())
> +		*scheduler = SCHEDULER_SYSTEMD;
> +	else if (is_crontab_available())
> +		*scheduler = SCHEDULER_CRON;
> +	else
> +		die(_("neither systemd timers nor crontab are available"));
> +	return;
> +
>   #else
>   	*scheduler = SCHEDULER_CRON;
>   	return;
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index b36b7f5fb0..b289cae6b9 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -20,6 +20,18 @@ test_xmllint () {
>   	fi
>   }
>   
> +test_lazy_prereq SYSTEMD_ANALYZE '
> +	systemd-analyze --help >out &&
> +	grep verify out
> +'
> +
> +test_systemd_analyze_verify () {
> +	if test_have_prereq SYSTEMD_ANALYZE
> +	then
> +		systemd-analyze verify "$@"
> +	fi
> +}
> +
>   test_expect_success 'help text' '
>   	test_expect_code 129 git maintenance -h 2>err &&
>   	test_i18ngrep "usage: git maintenance <subcommand>" err &&
> @@ -634,15 +646,56 @@ test_expect_success 'start and stop Windows maintenance' '
>   	test_cmp expect args
>   '
>   
> +test_expect_success 'start and stop Linux/systemd maintenance' '
> +	write_script print-args <<-\EOF &&
> +	printf "%s\n" "$*" >>args
> +	EOF
> +
> +	XDG_CONFIG_HOME="$PWD" &&
> +	export XDG_CONFIG_HOME &&
> +	rm -f args &&
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer &&
> +
> +	# start registers the repo
> +	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
> +
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
> +
> +	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> +	test_cmp expect args &&
> +
> +	rm -f args &&
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop &&
> +
> +	# 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" &&
> +
> +	printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> +	test_cmp expect args
> +'
> +
>   test_expect_success 'start and stop when several schedulers are available' '
>   	write_script print-args <<-\EOF &&
>   	printf "%s\n" "$*" | sed "s:gui/[0-9][0-9]*:gui/[UID]:; s:\(schtasks /create .* /xml\).*:\1:;" >>args
>   	EOF
>   
>   	rm -f args &&
> -	GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl &&
> -	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=systemd-timer &&
> +	printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
>   		hourly daily weekly >expect &&
> +	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> +		hourly daily weekly >>expect &&
> +	printf -- "systemctl --user enable --now git-maintenance@%s.timer\n" hourly daily weekly >>expect &&
> +	test_cmp expect args &&
> +
> +	rm -f args &&
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=launchctl &&
> +	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
> +	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
> +		hourly daily weekly >>expect &&
>   	for frequency in hourly daily weekly
>   	do
>   		PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> @@ -652,17 +705,19 @@ test_expect_success 'start and stop when several schedulers are available' '
>   	test_cmp expect args &&
>   
>   	rm -f args &&
> -	GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks &&
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance start --scheduler=schtasks &&
> +	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>   	printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> -		hourly daily weekly >expect &&
> +		hourly daily weekly >>expect &&
>   	printf "schtasks /create /tn Git Maintenance (%s) /f /xml\n" \
>   		hourly daily weekly >>expect &&
>   	test_cmp expect args &&
>   
>   	rm -f args &&
> -	GIT_TEST_MAINT_SCHEDULER="launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop &&
> +	GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args systemctl,launchctl:./print-args launchctl,schtasks:./print-args schtasks" git maintenance stop &&
> +	printf -- "systemctl --user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>   	printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> -		hourly daily weekly >expect &&
> +		hourly daily weekly >>expect &&
>   	printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
>   		hourly daily weekly >>expect &&
>   	test_cmp expect args
>