Re: [PATCH v2 1/1] maintenance: use systemd timers on Linux

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2021-05-09 23:32:17+0200, Lénaïc Huard <lenaic@lhuard.fr> 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.
> 
> +static int systemd_timer_enable_unit(int enable,
> +				     enum schedule_priority schedule,
> +				     const char *cmd)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	const char *frequency = get_frequency(schedule);
> +
> +	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))
> +		die(_("failed to run systemctl"));
> +	return finish_command(&child);
> +}
> +
> +static int systemd_timer_delete_unit_templates(void)
> +{
> +	char *filename = xdg_config_home_systemd("git-maintenance@.timer");
> +	unlink(filename);
> +	free(filename);
> +
> +	filename = xdg_config_home_systemd("git-maintenance@.service");
> +	unlink(filename);
> +	free(filename);
> +
> +	return 0;
> +}
> +
> +static int systemd_timer_delete_units(const char *cmd)
> +{
> +	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, cmd) ||
> +	       systemd_timer_enable_unit(0, SCHEDULE_DAILY, cmd) ||
> +	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, cmd) ||
> +	       systemd_timer_delete_unit_templates();
> +}

I'm not using any systemd-based distros. However, isn't this try to
enable all systemd's {hourly,daily,weekly} user's timer, then delete
the templates?^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W^W

Argh, we're disabling those systemd timer units first, by passing 0 as
first argument of systemd_timer_delete_units.

The fact that I read that twice, and still wrote down above reply
makes me think that above code is not self-explanatory enough.
May we switch to something else? Let's say using enum?


> +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))
> +		die(_("failed to create directories for '%s'"), filename);

This message is used by other codes, less works for translator, nice!

> +	file = xfopen(filename, "w");
> +	free(filename);

I'm sure if we should use FREE_AND_NULL(filename) instead?
Since, filename will be reused later.

> +
> +	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";
> +	fputs(unit, file);
> +	fclose(file);
> +
> +	filename = xdg_config_home_systemd("git-maintenance@.service");
> +	if (safe_create_leading_directories(filename))
> +		die(_("failed to create directories for '%s'"), filename);
> +	file = xfopen(filename, "w");
> +	free(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=\"%1$s/git\" --exec-path=\"%1$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";
> +	fprintf(file, unit, exec_path);

I think others have strong opinion on not using "%1$s",
and prefer simple "%s" and using "exec_path" twice instead.

> +	fclose(file);
> +
> +	return 0;
> +}
> +
> +static int systemd_timer_setup_units(const char *cmd)
> +{
> +	const char *exec_path = git_exec_path();
> +
> +	return systemd_timer_write_unit_templates(exec_path) ||
> +	       systemd_timer_enable_unit(1, SCHEDULE_HOURLY, cmd) ||
> +	       systemd_timer_enable_unit(1, SCHEDULE_DAILY, cmd) ||
> +	       systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, cmd);
> +}
> +
> +static int systemd_timer_update_schedule(int run_maintenance, int fd,
> +					 const char *cmd)
> +{
> +	if (run_maintenance)
> +		return systemd_timer_setup_units(cmd);
> +	else
> +		return systemd_timer_delete_units(cmd);
> +}
> +
> +enum scheduler {
> +	SCHEDULER_INVALID = -1,
> +	SCHEDULER_AUTO = 0,
> +	SCHEDULER_CRON = 1,
> +	SCHEDULER_SYSTEMD = 2,
> +	SCHEDULER_LAUNCHCTL = 3,
> +	SCHEDULER_SCHTASKS = 4,

I think explicitly writing down values doesn't make things clearer,
-1 would be nice, not a strong opinion, though.

Anyway, would it be better to move those type declaration to top of
file?

> +};
> +
> +static const struct {
> +	int (*is_available)(const char *cmd);
> +	int (*update_schedule)(int run_maintenance, int fd, const char *cmd);
> +	const char *cmd;
> +} scheduler_fn[] = {
> +	[SCHEDULER_CRON] = { is_crontab_available, crontab_update_schedule,
> +			     "crontab" },
> +	[SCHEDULER_SYSTEMD] = { is_systemd_timer_available,
> +				systemd_timer_update_schedule, "systemctl" },
> +	[SCHEDULER_LAUNCHCTL] = { is_launchctl_available,
> +				  launchctl_update_schedule, "launchctl" },
> +	[SCHEDULER_SCHTASKS] = { is_schtasks_available,
> +				 schtasks_update_schedule, "schtasks" },
> +};
> +
> +static enum scheduler parse_scheduler(const char *value)
> +{
> +	if (!value)
> +		return SCHEDULER_INVALID;
> +	else if (!strcasecmp(value, "auto"))
> +		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"))
> +		return SCHEDULER_SCHTASKS;
> +	else
> +		return SCHEDULER_INVALID;
> +}
> +
> +static int maintenance_opt_scheduler(const struct option *opt, const char *arg,
> +				     int unset)
> +{
> +	enum scheduler *scheduler = opt->value;
> +
> +	if (unset)
> +		die(_("--no-scheduler is not allowed"));

I think it's better to use OPT_CALLBACK_F in the options list and
we will write below code instead:

	BUG_ON_OPT_NEG(unset)

> +
> +	*scheduler = parse_scheduler(arg);
> +
> +	if (*scheduler == SCHEDULER_INVALID)
> +		die(_("unrecognized --scheduler argument '%s'"), arg);

Most of other callbacks do this instead:

	return error(_("messsage.... '%s'"), arg);

> +
> +	return 0;
> +}
> +
> +struct maintenance_start_opts {
> +	enum scheduler scheduler;
> +};
> +
> +static void resolve_auto_scheduler(enum scheduler *scheduler)
> +{
> +	if (*scheduler != SCHEDULER_AUTO)
> +		return;
> +
>  #if defined(__APPLE__)
> -static const char platform_scheduler[] = "launchctl";
> +	*scheduler = SCHEDULER_LAUNCHCTL;
> +	return;
> +
>  #elif defined(GIT_WINDOWS_NATIVE)
> -static const char platform_scheduler[] = "schtasks";
> +	*scheduler = SCHEDULER_SCHTASKS;
> +	return;
> +
> +#elif defined(__linux__)
> +	if (is_systemd_timer_available("systemctl"))
> +		*scheduler = SCHEDULER_SYSTEMD;
> +	else if (is_crontab_available("crontab"))
> +		*scheduler = SCHEDULER_CRON;
> +	else
> +		die(_("neither systemd timers nor crontab are available"));
> +	return;
> +
>  #else
> -static const char platform_scheduler[] = "crontab";
> +	*scheduler = SCHEDULER_CRON;
> +	return;
>  #endif
> +}
>  
> -static int update_background_schedule(int enable)
> +static void validate_scheduler(enum scheduler scheduler)
>  {
> -	int result;
> -	const char *scheduler = platform_scheduler;
> -	const char *cmd = scheduler;
> +	const char *cmd;
> +
> +	if (scheduler == SCHEDULER_INVALID)
> +		BUG("invalid scheduler");
> +	if (scheduler == SCHEDULER_AUTO)
> +		BUG("resolve_auto_scheduler should have been called before");
> +
> +	cmd = scheduler_fn[scheduler].cmd;
> +	if (!scheduler_fn[scheduler].is_available(cmd))
> +		die(_("%s scheduler is not available"), cmd);
> +}
> +
> +static int update_background_schedule(const struct maintenance_start_opts *opts,
> +				      int enable)
> +{
> +	unsigned int i;
> +	int res, result = 0;
> +	enum scheduler scheduler;
> +	const char *cmd = NULL;
>  	char *testing;
>  	struct lock_file lk;
>  	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
>  
> +	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
> +		return error(_("another process is scheduling background maintenance"));
> +
>  	testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
>  	if (testing) {
>  		char *sep = strchr(testing, ':');
>  		if (!sep)
>  			die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing);
>  		*sep = '\0';
> -		scheduler = testing;
> +		scheduler = parse_scheduler(testing);
>  		cmd = sep + 1;
> +		result = scheduler_fn[scheduler].update_schedule(
> +			enable, get_lock_file_fd(&lk), cmd);
> +		goto done;
>  	}
>  
> -	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
> -		return error(_("another process is scheduling background maintenance"));
> -
> -	if (!strcmp(scheduler, "launchctl"))
> -		result = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd);
> -	else if (!strcmp(scheduler, "schtasks"))
> -		result = schtasks_update_schedule(enable, get_lock_file_fd(&lk), cmd);
> -	else if (!strcmp(scheduler, "crontab"))
> -		result = crontab_update_schedule(enable, get_lock_file_fd(&lk), cmd);
> -	else
> -		die("unknown background scheduler: %s", scheduler);
> +	for (i = 1; i < ARRAY_SIZE(scheduler_fn); i++) {
> +		int enable_scheduler = enable && (opts->scheduler == i);
> +		cmd = scheduler_fn[i].cmd;
> +		if (!scheduler_fn[i].is_available(cmd))
> +			continue;
> +		res = scheduler_fn[i].update_schedule(
> +			enable_scheduler, get_lock_file_fd(&lk), cmd);
> +		if (enable_scheduler)
> +			result = res;
> +	}
>  
> +done:
>  	rollback_lock_file(&lk);
>  	free(testing);
>  	return result;
>  }
>  
> -static int maintenance_start(void)
> +static const char *const builtin_maintenance_start_usage[] = {
> +	N_("git maintenance start [--scheduler=<scheduler>]"), NULL
> +};
> +
> +static int maintenance_start(int argc, const char **argv, const char *prefix)
>  {
> +	struct maintenance_start_opts opts;
> +	struct option builtin_maintenance_start_options[] = {
> +		OPT_CALLBACK(
> +			0, "scheduler", &opts.scheduler, N_("scheduler"),
> +			N_("scheduler to use to trigger git maintenance run"),
> +			maintenance_opt_scheduler),

Following up my comment above, we're better to use:

		OPT_CALLBACK_F(0, "scheduler", &opts.scheduler, N_("scheduler"),
			N_("............"),
			PARSE_OPT_NONEG, maintenance_opt_scheduler),

> +		OPT_END()
> +	};
> +	memset(&opts, 0, sizeof(opts));
> +
> +	argc = parse_options(argc, argv, prefix,
> +			     builtin_maintenance_start_options,
> +			     builtin_maintenance_start_usage, 0);
> +
> +	resolve_auto_scheduler(&opts.scheduler);
> +	validate_scheduler(opts.scheduler);
> +
> +	if (argc > 0)
> +		usage_with_options(builtin_maintenance_start_usage,
> +				   builtin_maintenance_start_options);
> +
>  	if (maintenance_register())
>  		warning(_("failed to add repo to global config"));
> -
> -	return update_background_schedule(1);
> +	return update_background_schedule(&opts, 1);
>  }
>  
>  static int maintenance_stop(void)
>  {
> -	return update_background_schedule(0);
> +	return update_background_schedule(NULL, 0);
>  }
>  
>  static const char builtin_maintenance_usage[] =	N_("git maintenance <subcommand> [<options>]");
> @@ -2027,7 +2354,7 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[1], "run"))
>  		return maintenance_run(argc - 1, argv + 1, prefix);
>  	if (!strcmp(argv[1], "start"))
> -		return maintenance_start();
> +		return maintenance_start(argc - 1, argv + 1, prefix);
>  	if (!strcmp(argv[1], "stop"))
>  		return maintenance_stop();
>  	if (!strcmp(argv[1], "register"))
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 2412d8c5c0..6e6316cd90 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -20,6 +20,20 @@ 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 "$@"
> +	else
> +		true

The "else" leg is not necessary.

> +	fi
> +}
> +
>  test_expect_success 'help text' '
>  	test_expect_code 129 git maintenance -h 2>err &&
>  	test_i18ngrep "usage: git maintenance <subcommand>" err &&
> @@ -615,6 +629,43 @@ 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 &&
> +	echo $* >>args

To avoid any possible incompatibility with zillion echo implementation
out there. printf should be prefered over echo. Not a in this test
case, however, it costs us nothing anyway.

	printf "%s\n" "$*"

> +	EOF
> +
> +	rm -f args &&
> +	GIT_TEST_MAINT_SCHEDULER="systemd:./print-args" git maintenance start &&
> +
> +	# start registers the repo
> +	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
> +
> +	test_systemd_analyze_verify "$HOME/.config/systemd/user/git-maintenance@.service" &&
> +
> +	rm -f expect &&
> +	for frequency in hourly daily weekly
> +	do
> +		echo "--user enable --now git-maintenance@${frequency}.timer" >>expect || return 1
> +	done &&

And here, we can have a nicer syntax with printf:

	printf "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&

With printf, we don't even need "rm -f expect" above.

> +	test_cmp expect args &&
> +
> +	rm -f args &&
> +	GIT_TEST_MAINT_SCHEDULER="systemd:./print-args" git maintenance stop &&
> +
> +	# stop does not unregister the repo
> +	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
> +
> +	test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.timer" &&
> +	test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.service" &&
> +
> +	rm -f expect &&
> +	for frequency in hourly daily weekly
> +	do
> +		echo "--user disable --now git-maintenance@${frequency}.timer" >>expect || return 1
> +	done &&

Ditto.

All of this was written without testing, because I don't have any
systemd based system near my hand, right now.

So, please take it with a grain of salt.


-- 
Danh