* [PATCH v2 1/6] maintenance: use systemd timers builtin randomization
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-22 22:11 ` [PATCH v2 2/6] maintenance: use packaged systemd units Max Gautier
` (5 subsequent siblings)
6 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Junio C Hamano
Commit daa787010c (maintenance: use random minute in systemd scheduler,
2023-08-10) hooked the systemd timers with the get_random_minute()
function, which is used to spread the load generated by clients, while
keeping one hour between each run (for the hourly run).
There is two problems with this approach:
1. The random minute is the same for all timers, which can lead to
overlapping execution, and in practice skipped execution for some of
the timers.
2. The timers are overwritten on each execution of `git maintenance
start`, with a new random minute, which can lead to missing a daily
or weekly timer.
Consider this (unlikely) sequence of events:
- weekly timer schedule is "Mon 0:52:00".
- user run `git maintenance start` Monday, at 0:30:00.
- the random minute happens to be 22, making the new weekly timer
schedule "Mon 0:22:00".
- the weekly timer does not fire until the next Monday.
Commit c97ec0378b (maintenance: fix systemd schedule overlaps,
2023-08-10) resolve problem 1 by skipping some executions of the timers
to avoid overlap, but not problem 2.
Revert both commits to instead use two native systemd timers properties:
- RandomizedDelaySec: as the name suggest, this adds a random offset
to each timer execution. Use 3540 seconds (59 minutes) to spread
execution as much as possible.
- FixedRandomDelay: this makes the offset from the previous setting
deterministic, depending on machine ID, timer name and user
identifier.
(For more details on these settings, see man 5 systemd.timer)
This give us timers which:
- don't overlap (schedule depend on timer name)
- are spread out by user (schedule depend on machine ID and user ID)
- happen at fixed intervals
This also restore the ability to use the systemd templating system, and
keep the systemd unit strings embedded in the C code simpler, with no
need for format strings.
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
builtin/gc.c | 154 +++++++----------------------------------
t/t7900-maintenance.sh | 15 +---
2 files changed, 27 insertions(+), 142 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb..dac59414f0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2308,54 +2308,29 @@ static char *xdg_config_home_systemd(const char *filename)
return xdg_config_home_for("systemd/user", filename);
}
-#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s"
-
-static int systemd_timer_delete_timer_file(enum schedule_priority priority)
+static int systemd_timer_delete_unit_templates(void)
{
int ret = 0;
- const char *frequency = get_frequency(priority);
- char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
- char *filename = xdg_config_home_systemd(local_timer_name);
-
+ 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);
- free(filename);
- free(local_timer_name);
- return ret;
-}
-
-static int systemd_timer_delete_service_template(void)
-{
- int ret = 0;
- char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
- char *filename = xdg_config_home_systemd(local_service_name);
+ 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);
- free(local_service_name);
return ret;
}
-/*
- * Write the schedule information into a git-maintenance@<schedule>.timer
- * file using a custom minute. This timer file cannot use the templating
- * system, so we generate a specific file for each.
- */
-static int systemd_timer_write_timer_file(enum schedule_priority schedule,
- int minute)
+static int systemd_timer_write_unit_templates(const char *exec_path)
{
- int res = -1;
char *filename;
FILE *file;
const char *unit;
- char *schedule_pattern = NULL;
- const char *frequency = get_frequency(schedule);
- char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer");
-
- filename = xdg_config_home_systemd(local_timer_name);
+ filename = xdg_config_home_systemd("git-maintenance@.timer");
if (safe_create_leading_directories(filename)) {
error(_("failed to create directories for '%s'"), filename);
goto error;
@@ -2364,23 +2339,6 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
if (!file)
goto error;
- switch (schedule) {
- case SCHEDULE_HOURLY:
- schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute);
- break;
-
- case SCHEDULE_DAILY:
- schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute);
- break;
-
- case SCHEDULE_WEEKLY:
- schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
- break;
-
- default:
- BUG("Unhandled schedule_priority");
- }
-
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"
@@ -2389,12 +2347,14 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
"Description=Optimize Git repositories data\n"
"\n"
"[Timer]\n"
- "OnCalendar=%s\n"
+ "OnCalendar=%i\n"
"Persistent=true\n"
+ "RandomizedDelaySecond=3540\n"
+ "FixedRandomDelay=true\n"
"\n"
"[Install]\n"
"WantedBy=timers.target\n";
- if (fprintf(file, unit, schedule_pattern) < 0) {
+ if (fputs(unit, file) == EOF) {
error(_("failed to write to '%s'"), filename);
fclose(file);
goto error;
@@ -2403,36 +2363,9 @@ static int systemd_timer_write_timer_file(enum schedule_priority schedule,
error_errno(_("failed to flush '%s'"), filename);
goto error;
}
-
- res = 0;
-
-error:
- free(schedule_pattern);
- free(local_timer_name);
free(filename);
- return res;
-}
-/*
- * No matter the schedule, we use the same service and can make use of the
- * templating system. When installing git-maintenance@<schedule>.timer,
- * systemd will notice that git-maintenance@.service exists as a template
- * and will use this file and insert the <schedule> into the template at
- * the position of "%i".
- */
-static int systemd_timer_write_service_template(const char *exec_path)
-{
- int res = -1;
- char *filename;
- FILE *file;
- const char *unit;
- char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service");
-
- filename = xdg_config_home_systemd(local_service_name);
- if (safe_create_leading_directories(filename)) {
- error(_("failed to create directories for '%s'"), filename);
- goto error;
- }
+ filename = xdg_config_home_systemd("git-maintenance@.service");
file = fopen_or_warn(filename, "w");
if (!file)
goto error;
@@ -2465,18 +2398,17 @@ static int systemd_timer_write_service_template(const char *exec_path)
error_errno(_("failed to flush '%s'"), filename);
goto error;
}
-
- res = 0;
+ free(filename);
+ return 0;
error:
- free(local_service_name);
free(filename);
- return res;
+ systemd_timer_delete_unit_templates();
+ return -1;
}
static int systemd_timer_enable_unit(int enable,
- enum schedule_priority schedule,
- int minute)
+ enum schedule_priority schedule)
{
const char *cmd = "systemctl";
struct child_process child = CHILD_PROCESS_INIT;
@@ -2493,14 +2425,12 @@ static int systemd_timer_enable_unit(int enable,
*/
if (!enable)
child.no_stderr = 1;
- else if (systemd_timer_write_timer_file(schedule, minute))
- return -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, SYSTEMD_UNIT_FORMAT, frequency, "timer");
+ strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency);
if (start_command(&child))
return error(_("failed to start systemctl"));
@@ -2517,58 +2447,24 @@ static int systemd_timer_enable_unit(int enable,
return 0;
}
-/*
- * A previous version of Git wrote the timer units as template files.
- * Clean these up, if they exist.
- */
-static void systemd_timer_delete_stale_timer_templates(void)
-{
- char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer");
- char *filename = xdg_config_home_systemd(timer_template_name);
-
- if (unlink(filename) && !is_missing_file_error(errno))
- warning(_("failed to delete '%s'"), filename);
-
- free(filename);
- free(timer_template_name);
-}
-
-static int systemd_timer_delete_unit_files(void)
-{
- systemd_timer_delete_stale_timer_templates();
-
- /* Purposefully not short-circuited to make sure all are called. */
- return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
- systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
- systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
- systemd_timer_delete_service_template();
-}
-
static int systemd_timer_delete_units(void)
{
- int minute = get_random_minute();
- /* Purposefully not short-circuited to make sure all are called. */
- return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |
- systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |
- systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |
- systemd_timer_delete_unit_files();
+ 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_setup_units(void)
{
- int minute = get_random_minute();
const char *exec_path = git_exec_path();
- int ret = systemd_timer_write_service_template(exec_path) ||
- systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||
- systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||
- systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);
-
+ 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();
- else
- systemd_timer_delete_stale_timer_templates();
-
return ret;
}
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0943dfa18a..37aa408d26 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -790,15 +790,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
# start registers the repo
git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
- for schedule in hourly daily weekly
- do
- test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1
- done &&
- test_path_is_file "systemd/user/git-maintenance@.service" &&
-
- test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" &&
- test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
- test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&
+ 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 &&
@@ -809,10 +801,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
# stop does not unregister the repo
git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
- for schedule in hourly daily weekly
- do
- test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1
- done &&
+ 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 &&
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 2/6] maintenance: use packaged systemd units
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
2024-03-22 22:11 ` [PATCH v2 1/6] maintenance: use systemd timers builtin randomization Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-23 8:38 ` Eric Sunshine
2024-03-22 22:11 ` [PATCH v2 3/6] maintenance: simplify systemctl calls Max Gautier
` (4 subsequent siblings)
6 siblings, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Johannes Schindelin, Junio C Hamano
Currently, `git maintenance start`, if it uses the systemd scheduler,
will create systemd user units in $XDG_CONFIG_HOME, and overwrite the
files on each invocation.
Conversely, `git maintenance stop` will remove the unit files.
This is problematic mostly because it steps on user toes (and their
configuration, also). You can't mask the git maintenance systemd units,
because they are in $XDG_CONFIG_HOME and you can only mask "vendor"
units (see man systemctl), you can't override them either.
Additionally, it necessitates creating, modifying and removing files
from builtin/gc.c, which adds code and some avoidable complexity.
Package the systemd user units (timer and service) with git in
$(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
and remove code for writing and deleting the units from builtin/gc.c.
Determine the correct git path at install time by for the service unit.
Detect systemd timers support (at install time) by relying on systemctl
presence, since we use it as the control interface for the systemd
scheduler.
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
Notes:
I'm not completely sure if it's ok to do install time templating like
this, but I couldn't find a similar enough example in the Makefile. Any
suggestion for a better way ?
Makefile | 5 +
builtin/gc.c | 117 +----------------------
config.mak.uname | 10 ++
systemd/user/git-maintenance@.service.in | 17 ++++
systemd/user/git-maintenance@.timer | 12 +++
5 files changed, 49 insertions(+), 112 deletions(-)
create mode 100644 systemd/user/git-maintenance@.service.in
create mode 100644 systemd/user/git-maintenance@.timer
diff --git a/Makefile b/Makefile
index 4e255c81f2..4fb015478e 100644
--- a/Makefile
+++ b/Makefile
@@ -3469,6 +3469,11 @@ install: all
$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
+ifdef SYSTEMD_USER_UNIT_DIR
+ $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
+ sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
+ $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
+endif
ifdef MSVC
# We DO NOT install the individual foo.o.pdb files because they
diff --git a/builtin/gc.c b/builtin/gc.c
index dac59414f0..199c8e6240 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,110 +2303,6 @@ static int is_systemd_timer_available(void)
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_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_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)
- goto error;
-
- 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"
- "RandomizedDelaySecond=3540\n"
- "FixedRandomDelay=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;
- }
- free(filename);
-
- filename = xdg_config_home_systemd("git-maintenance@.service");
- file = fopen_or_warn(filename, "w");
- if (!file)
- goto error;
-
- 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 AF_VSOCK\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;
- }
- free(filename);
- return 0;
-
-error:
- free(filename);
- systemd_timer_delete_unit_templates();
- return -1;
-}
-
static int systemd_timer_enable_unit(int enable,
enum schedule_priority schedule)
{
@@ -2447,24 +2343,21 @@ static int systemd_timer_enable_unit(int enable,
return 0;
}
-static int systemd_timer_delete_units(void)
+static int systemd_timer_disable_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();
+ systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);
}
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) ||
+ int ret = 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();
+ systemd_timer_disable_units();
return ret;
}
@@ -2473,7 +2366,7 @@ static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
if (run_maintenance)
return systemd_timer_setup_units();
else
- return systemd_timer_delete_units();
+ return systemd_timer_disable_units();
}
enum scheduler {
diff --git a/config.mak.uname b/config.mak.uname
index d0dcca2ec5..35ca236874 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
ifneq ($(findstring .el7.,$(uname_R)),)
BASIC_CFLAGS += -std=c99
endif
+ ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
+ XDG_DATA_HOME ?= $(HOME)/.local/share
+ # systemd user units of programm installed in the home directory
+ # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
+ # (from man 5 systemd.unit)
+ SYSTEMD_USER_UNIT_DIR = $(strip $(if $(and \
+ $(findstring $(prefix),$(HOME)),\
+ $(findstring $(HOME),$(prefix))),\
+ $(XDG_DATA_HOME),$(prefix)/$(lib)))/systemd/user
+ endif
endif
ifeq ($(uname_S),GNU/kFreeBSD)
HAVE_ALLOCA_H = YesPlease
diff --git a/systemd/user/git-maintenance@.service.in b/systemd/user/git-maintenance@.service.in
new file mode 100644
index 0000000000..649ba87e7e
--- /dev/null
+++ b/systemd/user/git-maintenance@.service.in
@@ -0,0 +1,17 @@
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Service]
+Type=oneshot
+ExecStart=@BINDIR@/git for-each-repo --config=maintenance.repo \
+ maintenance run --schedule=%i
+LockPersonality=yes
+MemoryDenyWriteExecute=yes
+NoNewPrivileges=yes
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
+RestrictNamespaces=yes
+RestrictRealtime=yes
+RestrictSUIDSGID=yes
+SystemCallArchitectures=native
+SystemCallFilter=@system-service
diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
new file mode 100644
index 0000000000..2834bac365
--- /dev/null
+++ b/systemd/user/git-maintenance@.timer
@@ -0,0 +1,12 @@
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Timer]
+OnCalendar=%i
+Persistent=true
+RandomizedDelaySec=3540
+FixedRandomDelay=true
+
+[Install]
+WantedBy=timers.target
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 2/6] maintenance: use packaged systemd units
2024-03-22 22:11 ` [PATCH v2 2/6] maintenance: use packaged systemd units Max Gautier
@ 2024-03-23 8:38 ` Eric Sunshine
2024-03-23 9:52 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Eric Sunshine @ 2024-03-23 8:38 UTC (permalink / raw)
To: Max Gautier
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Johannes Schindelin, Junio C Hamano
On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:
> [...]
> Package the systemd user units (timer and service) with git in
> $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
> and remove code for writing and deleting the units from builtin/gc.c.
> Determine the correct git path at install time by for the service unit.
>
> Detect systemd timers support (at install time) by relying on systemctl
> presence, since we use it as the control interface for the systemd
> scheduler.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
> Notes:
> I'm not completely sure if it's ok to do install time templating like
> this, but I couldn't find a similar enough example in the Makefile. Any
> suggestion for a better way ?
>
> diff --git a/Makefile b/Makefile
> @@ -3469,6 +3469,11 @@ install: all
> +ifdef SYSTEMD_USER_UNIT_DIR
> + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
> + sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
> + $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
> +endif
This is the first use of /dev/stdin in the project and I might worry a
bit about portability. Granted, a system in which systemd is installed
is likely to have /dev/stdin available, but it's often a good idea to
be cautious when introducing something new into the project.
I would think it would be possible to instead generate the
`git-maintenance@.service` file locally from the template
`git-maintenance@.service.in` as part of the normal build process, and
then install the built `git-maintenance@.service` at "install" time.
That seems more in line with how other resources are handled, avoids
the novel use of /dev/stdin, and answers the question you ask above.
> diff --git a/config.mak.uname b/config.mak.uname
> @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
> + ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
What is "?&&"?
> + XDG_DATA_HOME ?= $(HOME)/.local/share
> + # systemd user units of programm installed in the home directory
> + # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
> + # (from man 5 systemd.unit)
s/programm/program/
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/6] maintenance: use packaged systemd units
2024-03-23 8:38 ` Eric Sunshine
@ 2024-03-23 9:52 ` Max Gautier
0 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-23 9:52 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Johannes Schindelin, Junio C Hamano
On Sat, Mar 23, 2024 at 04:38:44AM -0400, Eric Sunshine wrote:
> On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:
> > [...]
> > Package the systemd user units (timer and service) with git in
> > $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
> > and remove code for writing and deleting the units from builtin/gc.c.
> > Determine the correct git path at install time by for the service unit.
> >
> > Detect systemd timers support (at install time) by relying on systemctl
> > presence, since we use it as the control interface for the systemd
> > scheduler.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> > Notes:
> > I'm not completely sure if it's ok to do install time templating like
> > this, but I couldn't find a similar enough example in the Makefile. Any
> > suggestion for a better way ?
> >
> > diff --git a/Makefile b/Makefile
> > @@ -3469,6 +3469,11 @@ install: all
> > +ifdef SYSTEMD_USER_UNIT_DIR
> > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
> > + sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
> > + $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
> > +endif
>
> This is the first use of /dev/stdin in the project and I might worry a
> bit about portability. Granted, a system in which systemd is installed
> is likely to have /dev/stdin available, but it's often a good idea to
> be cautious when introducing something new into the project.
>
> I would think it would be possible to instead generate the
> `git-maintenance@.service` file locally from the template
> `git-maintenance@.service.in` as part of the normal build process, and
> then install the built `git-maintenance@.service` at "install" time.
> That seems more in line with how other resources are handled, avoids
> the novel use of /dev/stdin, and answers the question you ask above.
Ok. It's not completely obvious to me how the "Detect prefix change
logic" works, but using other rules as a model, I think I can do
something like that:
systemd/user/git-maintenance@.service: systemd/user/git-maintenance@.service.in GIT-PREFIX
sed 's+@BINDIR@+$(bindir_SQ)+' $< > $@
and depending on GIT-PREFIX should regenerate the service when changing
the prefix, correct ?
I'll need to add that to .gitignore as well.
>
> > diff --git a/config.mak.uname b/config.mak.uname
> > @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
> > + ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
>
> What is "?&&"?
>
Hum, a typo, sorry that slipped through. It apparently works regardless,
because shell expansion does something with it I guess. Curious. I'll
clean that up as well.
> > + XDG_DATA_HOME ?= $(HOME)/.local/share
> > + # systemd user units of programm installed in the home directory
> > + # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
> > + # (from man 5 systemd.unit)
>
> s/programm/program/
Ack.
That should even be 'programs' I think, that's a general rule.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 3/6] maintenance: simplify systemctl calls
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
2024-03-22 22:11 ` [PATCH v2 1/6] maintenance: use systemd timers builtin randomization Max Gautier
2024-03-22 22:11 ` [PATCH v2 2/6] maintenance: use packaged systemd units Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-22 23:09 ` Eric Sunshine
2024-03-22 22:11 ` [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user Max Gautier
` (3 subsequent siblings)
6 siblings, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Junio C Hamano, Eric Sunshine, Jeff King
The systemctl invocation to enable or disable the git maintenance timers
is needlessly complicated:
- systemctl does not mind at all enabling already enabled units, nor
disabling already disabled units.
- it can act on several units at once instead of only one.
Use only one systemctl invocation per `git maintenance start/stop`.
Transparently pass its status and output.
Add the --force option to override conflicting symlinks to previous
instances of our units files which lived in $XDG_CONFIG_HOME.
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
builtin/gc.c | 63 ++++++++++++----------------------------------------
1 file changed, 14 insertions(+), 49 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 199c8e6240..aaee91451a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
return real_is_systemd_timer_available();
}
-static int systemd_timer_enable_unit(int enable,
- enum schedule_priority schedule)
+static int systemd_set_units_state(int enable)
{
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);
+
+ strvec_pushl(&child.args, "--user", "--force", "--now",
+ enable ? "enable" : "disable",
+ "git-maintenance@hourly.timer",
+ "git-maintenance@daily.timer",
+ "git-maintenance@weekly.timer", NULL);
+ /*
+ * --force override existing conflicting symlinks
+ * We need it because the units have changed location (~/.config ->
+ * /usr/lib)
+ */
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 error(_("failed to run systemctl"));
return 0;
}
-static int systemd_timer_disable_units(void)
-{
- return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
- systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
- systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);
-}
-
-static int systemd_timer_setup_units(void)
-{
-
- int ret = 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_disable_units();
- return ret;
-}
-
static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
{
- if (run_maintenance)
- return systemd_timer_setup_units();
- else
- return systemd_timer_disable_units();
+ return systemd_set_units_state(run_maintenance);
}
enum scheduler {
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 3/6] maintenance: simplify systemctl calls
2024-03-22 22:11 ` [PATCH v2 3/6] maintenance: simplify systemctl calls Max Gautier
@ 2024-03-22 23:09 ` Eric Sunshine
2024-03-23 10:25 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Eric Sunshine @ 2024-03-22 23:09 UTC (permalink / raw)
To: Max Gautier
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Junio C Hamano, Jeff King
On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> The systemctl invocation to enable or disable the git maintenance timers
> is needlessly complicated:
> - systemctl does not mind at all enabling already enabled units, nor
> disabling already disabled units.
Has systemctl behavior changed...
> Use only one systemctl invocation per `git maintenance start/stop`.
> Transparently pass its status and output.
> Add the --force option to override conflicting symlinks to previous
> instances of our units files which lived in $XDG_CONFIG_HOME.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
> - * 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.
... since this and...
> - * Disabling an already disabled systemd unit makes
> - * systemctl fail.
> - * Let's ignore this failure.
... this were written?
If so, do we still need to worry about older systems in which
systemctl prints errors and/or fails outright?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/6] maintenance: simplify systemctl calls
2024-03-22 23:09 ` Eric Sunshine
@ 2024-03-23 10:25 ` Max Gautier
0 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-23 10:25 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Junio C Hamano, Jeff King, Phillip Wood
On Fri, Mar 22, 2024 at 07:09:18PM -0400, Eric Sunshine wrote:
> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> > The systemctl invocation to enable or disable the git maintenance timers
> > is needlessly complicated:
> > - systemctl does not mind at all enabling already enabled units, nor
> > disabling already disabled units.
>
> Has systemctl behavior changed...
>
> > Use only one systemctl invocation per `git maintenance start/stop`.
> > Transparently pass its status and output.
> > Add the --force option to override conflicting symlinks to previous
> > instances of our units files which lived in $XDG_CONFIG_HOME.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
> > ---
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void)
> > - * 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.
>
> ... since this and...
>
> > - * Disabling an already disabled systemd unit makes
> > - * systemctl fail.
> > - * Let's ignore this failure.
>
> ... this were written?
>
> If so, do we still need to worry about older systems in which
> systemctl prints errors and/or fails outright?
I tried the following on systemd source
$ git log -L :do_unit_file_disable:src/shared/install.c
$ git log -L :unit_file_disable:src/shared/install.c`
$ git log --grep 'already disabled'
$ git log --grep 're disable'
$ git log --grep 'disabled unit'
$ git log --grep 'systemctl disable'
That yields nothing indicating a change (in the commit messages, I
didn't look at the code in detail) when disabling already disabled
units, and testing on systemd 255 disable already disabled units (normal
and templated, same thing) without complaining or an error status.
Nothing relevant I could find on github in the issues or PRs either.
systemctl does error out or print warnings in other conditions, like
missing [Install] section, but that's not something we should ignore.
Philip Wood asked that question here in the original thread:
> What is the exit code of systemctl if a unit is already enabled and we
> try to enbale it again (and the same for disabling a disabled unit)?
https://lore.kernel.org/git/3fd17223-8667-24be-2e65-f1970d411bdf@gmail.com/
But I can't find a follow-up email from Lénaïc, and searching for
"disabled" in the whole thread only yield the comment I'm removing. Not
sure if Lénaïc still follows the list, but maybe he can comment on that
?
To me it looks like it was not necessary to begin with, but I might have
just missed the discussion about it.
Adding Phillip to the discussion as well.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
` (2 preceding siblings ...)
2024-03-22 22:11 ` [PATCH v2 3/6] maintenance: simplify systemctl calls Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-22 22:38 ` Kristoffer Haugsbakk
2024-03-23 11:07 ` Max Gautier
2024-03-22 22:11 ` [PATCH v2 5/6] maintenance: update systemd scheduler docs Max Gautier
` (2 subsequent siblings)
6 siblings, 2 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Jeff King, Junio C Hamano
Before commit 976640edbb (maintenance: use packaged systemd units,
2024-03-21), we we're putting systemd unit files in $XDG_CONFIG_HOME ;
these could mask those we are now distributing as part of git.
Remove all the systemd units possibly created by previous version of
git when running `git maintenance start/stop`.
Avoid overwriting units we didn't write, by comparing the first line
with the start of the comment we added to our unit files previously.
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
Notes:
How should I refer to a commit which is part of the same patch series ?
The commit id will change so the message won't be correct anymore, right
?
builtin/gc.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/builtin/gc.c b/builtin/gc.c
index aaee91451a..99b158e481 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2329,8 +2329,50 @@ static int systemd_set_units_state(int enable)
return 0;
}
+/*
+ * TODO: in the future (~2026 ?) remove this cleanup code
+ */
+static void systemd_delete_user_unit(char const *unit)
+{
+ char const file_start_stale[] = "# This file was created and is"
+ " maintained by Git.";
+ char file_start_user[sizeof(file_start_stale)] = {'\0'};
+
+ char *filename = xdg_config_home_for("systemd/user", unit);
+ int handle = open(filename, O_RDONLY);
+
+ /*
+ * Check this is actually our file and we're not removing a legitimate
+ * user override.
+ */
+ if (handle == -1 && !is_missing_file_error(errno))
+ warning(_("failed to delete '%s'"), filename);
+ else {
+ read(handle, file_start_user, sizeof(file_start_stale) - 1);
+ close(handle);
+ if (strcmp(file_start_stale, file_start_user) == 0) {
+ if (unlink(filename) == 0)
+ warning(_("deleted stale unit file '%s'"), filename);
+ else if (!is_missing_file_error(errno))
+ warning(_("failed to delete '%s'"), filename);
+ }
+ }
+
+ free(filename);
+}
+
static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
{
+ /*
+ * A previous version of Git wrote the units in the user configuration
+ * directory. Clean these up, if they exist.
+ * TODO: in the future (~2026 ?) remove this cleanup code
+ */
+ systemd_delete_user_unit("git-maintenance@hourly.timer");
+ systemd_delete_user_unit("git-maintenance@daily.timer");
+ systemd_delete_user_unit("git-maintenance@weekly.timer");
+ systemd_delete_user_unit("git-maintenance@.timer");
+ systemd_delete_user_unit("git-maintenance@.service");
return systemd_set_units_state(run_maintenance);
}
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-22 22:11 ` [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user Max Gautier
@ 2024-03-22 22:38 ` Kristoffer Haugsbakk
2024-03-22 22:43 ` Junio C Hamano
2024-03-23 11:07 ` Max Gautier
1 sibling, 1 reply; 52+ messages in thread
From: Kristoffer Haugsbakk @ 2024-03-22 22:38 UTC (permalink / raw)
To: Max Gautier
Cc: Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano, git
On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:
> Notes:
> How should I refer to a commit which is part of the same patch series ?
> The commit id will change so the message won't be correct anymore, right
> ?
It looks like a fair few say “in the previous commit” for the one just
before this one and “in a previous commit” for some commit that was
before this in the series but not the immediate previous one. I guess
that’s okay, no?
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-22 22:38 ` Kristoffer Haugsbakk
@ 2024-03-22 22:43 ` Junio C Hamano
0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2024-03-22 22:43 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Jeff King, git
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Fri, Mar 22, 2024, at 23:11, Max Gautier wrote:
>> Notes:
>> How should I refer to a commit which is part of the same patch series ?
>> The commit id will change so the message won't be correct anymore, right
>> ?
>
> It looks like a fair few say “in the previous commit” for the one just
> before this one and “in a previous commit” for some commit that was
> before this in the series but not the immediate previous one. I guess
> that’s okay, no?
I saw some series say "Earlier in the series, X has learned to do Y,
so now we take advantage of that new feature", and found it quite
readable.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-22 22:11 ` [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user Max Gautier
2024-03-22 22:38 ` Kristoffer Haugsbakk
@ 2024-03-23 11:07 ` Max Gautier
2024-03-24 15:45 ` Phillip Wood
1 sibling, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-23 11:07 UTC (permalink / raw)
To: git
Cc: Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano
On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>
> +/*
> + * TODO: in the future (~2026 ?) remove this cleanup code
> + */
> +static void systemd_delete_user_unit(char const *unit)
> +{
> + char const file_start_stale[] = "# This file was created and is"
> + " maintained by Git.";
> + char file_start_user[sizeof(file_start_stale)] = {'\0'};
> +
> + char *filename = xdg_config_home_for("systemd/user", unit);
> + int handle = open(filename, O_RDONLY);
> +
> + /*
> + * Check this is actually our file and we're not removing a legitimate
> + * user override.
> + */
> + if (handle == -1 && !is_missing_file_error(errno))
> + warning(_("failed to delete '%s'"), filename);
> + else {
> + read(handle, file_start_user, sizeof(file_start_stale) - 1);
Actually that fails -Werror because I don't check read return.
Alternative below (on top of this one), with one question:
Are VLA using size_t const OK ? It's folded to a constant array by gcc
but I don't know if that causes portability problem with other platforms
? I can always repeat the sizeof expr if it's a problematic construct.
-- >8 --
diff --git a/builtin/gc.c b/builtin/gc.c
index 99b158e481..7fb25ea2b1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
/*
* TODO: in the future (~2026 ?) remove this cleanup code
*/
+
static void systemd_delete_user_unit(char const *unit)
{
char const file_start_stale[] = "# This file was created and is"
" maintained by Git.";
- char file_start_user[sizeof(file_start_stale)] = {'\0'};
+ size_t const length = sizeof(file_start_stale);
+ char file_start_user[length] = {'\0'};
+
char *filename = xdg_config_home_for("systemd/user", unit);
int handle = open(filename, O_RDONLY);
@@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
if (handle == -1 && !is_missing_file_error(errno))
warning(_("failed to delete '%s'"), filename);
else {
- read(handle, file_start_user, sizeof(file_start_stale) - 1);
- close(handle);
- if (strcmp(file_start_stale, file_start_user) == 0) {
+ if (length - 1 == read(handle, file_start_user, length - 1) &&
+ strcmp(file_start_stale, file_start_user) == 0) {
if (unlink(filename) == 0)
warning(_("deleted stale unit file '%s'"), filename);
else if (!is_missing_file_error(errno))
warning(_("failed to delete '%s'"), filename);
}
+ close(handle);
}
free(filename);
--
Max Gautier
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-23 11:07 ` Max Gautier
@ 2024-03-24 15:45 ` Phillip Wood
2024-03-25 8:36 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Phillip Wood @ 2024-03-24 15:45 UTC (permalink / raw)
To: Max Gautier, git
Cc: Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano
Hi Max
On 23/03/2024 11:07, Max Gautier wrote:
> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>
>> +/*
>> + * TODO: in the future (~2026 ?) remove this cleanup code
>> + */
That is rather optimistic - users only run "git maintenance start" once
so any unit files that have been written in the past will exist well
beyond 2026.
>> +static void systemd_delete_user_unit(char const *unit)
>> +{
>> + char const file_start_stale[] = "# This file was created and is"
>> + " maintained by Git.";
>> + char file_start_user[sizeof(file_start_stale)] = {'\0'};
>> +
>> + char *filename = xdg_config_home_for("systemd/user", unit);
>> + int handle = open(filename, O_RDONLY);
>> +
>> + /*
>> + * Check this is actually our file and we're not removing a legitimate
>> + * user override.
>> + */
>> + if (handle == -1 && !is_missing_file_error(errno))
>> + warning(_("failed to delete '%s'"), filename);
>> + else {
>> + read(handle, file_start_user, sizeof(file_start_stale) - 1);
>
> Actually that fails -Werror because I don't check read return.
> Alternative below (on top of this one), with one question:
> Are VLA using size_t const OK ? It's folded to a constant array by gcc
> but I don't know if that causes portability problem with other platforms
> ? I can always repeat the sizeof expr if it's a problematic construct.
I think it would be easier to use strbuf_read_file() instead - it is
only a small file so there is not really any advantage in just reading
the first line. Something like
static int systemd_delete_user_unit(const char* unit)
{
int ret = 0;
struct strbuf buf = STRBUF_INIT;
char *filename = xdg_config_home_for("systemd/user", unit);
if (strbuf_read_file(&buf, filename, 0) < 0) {
if (errno != ENOENT)
ret = error_errno(_("could not read '%s'", filename));
goto out;
}
if (starts_with(buf.buf,
"# This file was created and is maintained by git.\n") &&
unlink(filename))
ret = error_errno(_("could not remove '%s', filename));
out:
free(filename);
strbuf_release(&buf);
return ret;
}
Best Wishes
Phillip
> -- >8 --
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 99b158e481..7fb25ea2b1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2332,11 +2332,14 @@ static int systemd_set_units_state(int enable)
> /*
> * TODO: in the future (~2026 ?) remove this cleanup code
> */
> +
> static void systemd_delete_user_unit(char const *unit)
> {
> char const file_start_stale[] = "# This file was created and is"
> " maintained by Git.";
> - char file_start_user[sizeof(file_start_stale)] = {'\0'};
> + size_t const length = sizeof(file_start_stale);
> + char file_start_user[length] = {'\0'};
> +
>
> char *filename = xdg_config_home_for("systemd/user", unit);
> int handle = open(filename, O_RDONLY);
> @@ -2348,14 +2351,14 @@ static void systemd_delete_user_unit(char const *unit)
> if (handle == -1 && !is_missing_file_error(errno))
> warning(_("failed to delete '%s'"), filename);
> else {
> - read(handle, file_start_user, sizeof(file_start_stale) - 1);
> - close(handle);
> - if (strcmp(file_start_stale, file_start_user) == 0) {
> + if (length - 1 == read(handle, file_start_user, length - 1) &&
> + strcmp(file_start_stale, file_start_user) == 0) {
> if (unlink(filename) == 0)
> warning(_("deleted stale unit file '%s'"), filename);
> else if (!is_missing_file_error(errno))
> warning(_("failed to delete '%s'"), filename);
> }
> + close(handle);
> }
>
> free(filename);
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-24 15:45 ` Phillip Wood
@ 2024-03-25 8:36 ` Max Gautier
2024-03-25 16:39 ` Phillip Wood
0 siblings, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-25 8:36 UTC (permalink / raw)
To: phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano
On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
> Hi Max
>
> On 23/03/2024 11:07, Max Gautier wrote:
> > On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
> > > +/*
> > > + * TODO: in the future (~2026 ?) remove this cleanup code
> > > + */
>
> That is rather optimistic - users only run "git maintenance start" once so
> any unit files that have been written in the past will exist well beyond
> 2026.
In that case, should we hook the cleanup (in it's final form) in more
place ? `git maintenance register` for instance ?
>
> > > +static void systemd_delete_user_unit(char const *unit)
> > > +{
> > > + char const file_start_stale[] = "# This file was created and is"
> > > + " maintained by Git.";
> > > + char file_start_user[sizeof(file_start_stale)] = {'\0'};
> > > +
> > > + char *filename = xdg_config_home_for("systemd/user", unit);
> > > + int handle = open(filename, O_RDONLY);
> > > +
> > > + /*
> > > + * Check this is actually our file and we're not removing a legitimate
> > > + * user override.
> > > + */
> > > + if (handle == -1 && !is_missing_file_error(errno))
> > > + warning(_("failed to delete '%s'"), filename);
> > > + else {
> > > + read(handle, file_start_user, sizeof(file_start_stale) - 1);
> >
> > Actually that fails -Werror because I don't check read return.
> > Alternative below (on top of this one), with one question:
> > Are VLA using size_t const OK ? It's folded to a constant array by gcc
> > but I don't know if that causes portability problem with other platforms
> > ? I can always repeat the sizeof expr if it's a problematic construct.
>
> I think it would be easier to use strbuf_read_file() instead - it is only a
> small file so there is not really any advantage in just reading the first
> line. Something like
>
> static int systemd_delete_user_unit(const char* unit)
> {
> int ret = 0;
> struct strbuf buf = STRBUF_INIT;
> char *filename = xdg_config_home_for("systemd/user", unit);
>
> if (strbuf_read_file(&buf, filename, 0) < 0) {
> if (errno != ENOENT)
> ret = error_errno(_("could not read '%s'", filename));
> goto out;
> }
> if (starts_with(buf.buf,
> "# This file was created and is maintained by git.\n") &&
> unlink(filename))
> ret = error_errno(_("could not remove '%s', filename));
>
> out:
> free(filename);
> strbuf_release(&buf);
> return ret;
> }
Thanks, this is exactly what I needed, I looked at the strbuf API but
couldn't find this somehow.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-25 8:36 ` Max Gautier
@ 2024-03-25 16:39 ` Phillip Wood
2024-03-27 16:20 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Phillip Wood @ 2024-03-25 16:39 UTC (permalink / raw)
To: Max Gautier, phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano
Hi Max
On 25/03/2024 08:36, Max Gautier wrote:
> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
>> Hi Max
>>
>> On 23/03/2024 11:07, Max Gautier wrote:
>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>>> +/*
>>>> + * TODO: in the future (~2026 ?) remove this cleanup code
>>>> + */
>>
>> That is rather optimistic - users only run "git maintenance start" once so
>> any unit files that have been written in the past will exist well beyond
>> 2026.
>
> In that case, should we hook the cleanup (in it's final form) in more
> place ? `git maintenance register` for instance ?
I'm not sure if that is needed if we leave the code to delete the unit
files in place.
Best Wishes
Phillip
>>>> +static void systemd_delete_user_unit(char const *unit)
>>>> +{
>>>> + char const file_start_stale[] = "# This file was created and is"
>>>> + " maintained by Git.";
>>>> + char file_start_user[sizeof(file_start_stale)] = {'\0'};
>>>> +
>>>> + char *filename = xdg_config_home_for("systemd/user", unit);
>>>> + int handle = open(filename, O_RDONLY);
>>>> +
>>>> + /*
>>>> + * Check this is actually our file and we're not removing a legitimate
>>>> + * user override.
>>>> + */
>>>> + if (handle == -1 && !is_missing_file_error(errno))
>>>> + warning(_("failed to delete '%s'"), filename);
>>>> + else {
>>>> + read(handle, file_start_user, sizeof(file_start_stale) - 1);
>>>
>>> Actually that fails -Werror because I don't check read return.
>>> Alternative below (on top of this one), with one question:
>>> Are VLA using size_t const OK ? It's folded to a constant array by gcc
>>> but I don't know if that causes portability problem with other platforms
>>> ? I can always repeat the sizeof expr if it's a problematic construct.
>>
>> I think it would be easier to use strbuf_read_file() instead - it is only a
>> small file so there is not really any advantage in just reading the first
>> line. Something like
>>
>> static int systemd_delete_user_unit(const char* unit)
>> {
>> int ret = 0;
>> struct strbuf buf = STRBUF_INIT;
>> char *filename = xdg_config_home_for("systemd/user", unit);
>>
>> if (strbuf_read_file(&buf, filename, 0) < 0) {
>> if (errno != ENOENT)
>> ret = error_errno(_("could not read '%s'", filename));
>> goto out;
>> }
>> if (starts_with(buf.buf,
>> "# This file was created and is maintained by git.\n") &&
>> unlink(filename))
>> ret = error_errno(_("could not remove '%s', filename));
>>
>> out:
>> free(filename);
>> strbuf_release(&buf);
>> return ret;
>> }
>
> Thanks, this is exactly what I needed, I looked at the strbuf API but
> couldn't find this somehow.
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
2024-03-25 16:39 ` Phillip Wood
@ 2024-03-27 16:20 ` Max Gautier
0 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-27 16:20 UTC (permalink / raw)
To: phillip.wood, Phillip Wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Jeff King, Junio C Hamano
Le 25 mars 2024 17:39:23 GMT+01:00, Phillip Wood <phillip.wood123@gmail.com> a écrit :
>Hi Max
>
>On 25/03/2024 08:36, Max Gautier wrote:
>> On Sun, Mar 24, 2024 at 03:45:45PM +0000, Phillip Wood wrote:
>>> Hi Max
>>>
>>> On 23/03/2024 11:07, Max Gautier wrote:
>>>> On Fri, Mar 22, 2024 at 11:11:09PM +0100, Max Gautier wrote:
>>>>> +/*
>>>>> + * TODO: in the future (~2026 ?) remove this cleanup code
>>>>> + */
>>>
>>> That is rather optimistic - users only run "git maintenance start" once so
>>> any unit files that have been written in the past will exist well beyond
>>> 2026.
>>
>> In that case, should we hook the cleanup (in it's final form) in more
>> place ? `git maintenance register` for instance ?
>
>I'm not sure if that is needed if we leave the code to delete the unit files in place.
>
>Best Wishes
But don't we want to remove it, eventually ?
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 5/6] maintenance: update systemd scheduler docs
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
` (3 preceding siblings ...)
2024-03-22 22:11 ` [PATCH v2 4/6] maintenance: cleanup $XDG_CONFIG_HOME/systemd/user Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-22 22:11 ` [PATCH v2 6/6] maintenance: update tests for systemd scheduler Max Gautier
2024-03-24 14:54 ` [PATCH v2 0/6] maintenance: use packaged systemd units Phillip Wood
6 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Junio C Hamano
The `git maintenance` systemd scheduler no longer writes units in
$XDG_CONFIG_HOME.
Describe the new behavior.
Instead of explaining manual ways to modify the timer, suggest the
systemd standard tool: `systemctl edit`.
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
Documentation/git-maintenance.txt | 33 +++++++++++++++----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 51d0f7e94b..6511c3f3f1 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -304,10 +304,9 @@ 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:
+In this case, `git maintenance start` will enable user systemd timer units
+and start them. The current list of user-scheduled tasks can be found by
+running `systemctl --user list-timers`. These timers are similar to this:
-----------------------------------------------------------------------
$ systemctl --user list-timers
@@ -317,25 +316,25 @@ Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago
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.
+One timer instance is enabled for each `--schedule=<frequency>` option.
-The definition of the systemd units can be inspected in the following files:
+The definition of the systemd units can be inspected this way:
-----------------------------------------------------------------------
-~/.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
+$ systemctl cat --user git-maintenance@.timer
+$ systemctl cat --user git-maintenance@.service
-----------------------------------------------------------------------
-`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.
+Customization of the timer or service can be performed with the usual systemd
+tooling:
+-----------------------------------------------------------------------
+$ systemctl edit --user git-maintenance@.timer # all the timers
+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer
+$ systemctl edit --user git-maintenance@.service # all the services
+$ systemctl edit --user git-maintenance@hourly.service # the hourly run
+-----------------------------------------------------------------------
-`git maintenance stop` will stop the user systemd timers and delete
-the above mentioned files.
+`git maintenance stop` will disable and stop the user systemd timers.
For more details, see `systemd.timer(5)`.
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH v2 6/6] maintenance: update tests for systemd scheduler
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
` (4 preceding siblings ...)
2024-03-22 22:11 ` [PATCH v2 5/6] maintenance: update systemd scheduler docs Max Gautier
@ 2024-03-22 22:11 ` Max Gautier
2024-03-22 23:02 ` Eric Sunshine
2024-03-24 14:54 ` [PATCH v2 0/6] maintenance: use packaged systemd units Phillip Wood
6 siblings, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-22 22:11 UTC (permalink / raw)
To: git
Cc: Max Gautier, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt, Eric Sunshine, Junio C Hamano
The systemd units are now in the source tree, rather than produced when
running git maitenance start. There is no need anymore to couple
validating the units and testing `git maintenance start`.
Adjust the test to verify the new `systemctl` command used, discard
checks for presence/absence of unit files in $XDG_CONFIG_HOME.
Validate the systemd units in the source tree, with one test per unit to
have more distinct failures.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Max Gautier <mg@max.gautier.name>
---
Notes:
I encountered the same issue that
cover.1697319294.git.code@khaugsbakk.name tried to solve; I was
expecting multiples tests to be independent while they weren't (in
particular the MacOS ones).
Cc: Kristoffer Haugsbakk <code@khaugsbakk.name>
t/t7900-maintenance.sh | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 37aa408d26..dc0bf39250 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -24,13 +24,6 @@ test_lazy_prereq SYSTEMD_ANALYZE '
systemd-analyze verify /lib/systemd/system/basic.target
'
-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 >actual &&
test_grep "usage: git maintenance <subcommand>" actual &&
@@ -776,23 +769,32 @@ test_expect_success 'start and stop Windows maintenance' '
hourly daily weekly >expect &&
test_cmp expect args
'
+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd service unit' '
+ git_path=$(command -v git) &&
+ sed "s+@BINDIR@/git+${git_path}+" "$TEST_DIRECTORY"/../systemd/user/git-maintenance@.service.in > git-maintenance@.service &&
+ systemd-analyze verify git-maintenance@hourly.service git-maintenance@daily.service git-maintenance@weekly.service &&
+ rm git-maintenance@.service &&
+ unset git_path
+'
+
+test_expect_success SYSTEMD_ANALYZE 'validate maintenance systemd timer unit' '
+ SYSTEMD_UNIT_PATH="$TEST_DIRECTORY"/../systemd/user/: systemd-analyze verify git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer
+ # ':' at the end of SYSTEMD_UNIT_PATH appends the default systemd search path
+ # This is needed because analyze tries to load implicit / default unit dependencies
+'
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 &&
+ echo "--user --force --now enable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
test_cmp expect args &&
rm -f args &&
@@ -801,10 +803,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
# 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 &&
+ echo "--user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
test_cmp expect args
'
@@ -819,12 +818,12 @@ test_expect_success 'start and stop when several schedulers are available' '
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 &&
+ echo "systemctl --user --force --now enable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >>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 &&
+ echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
printf "schtasks /delete /tn Git Maintenance (%s) /f\n" \
hourly daily weekly >>expect &&
for frequency in hourly daily weekly
@@ -837,7 +836,7 @@ test_expect_success 'start and stop when several schedulers are available' '
rm -f args &&
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 &&
+ echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
printf "launchctl bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
hourly daily weekly >>expect &&
printf "schtasks /create /tn Git Maintenance (%s) /f /xml\n" \
@@ -846,7 +845,7 @@ test_expect_success 'start and stop when several schedulers are available' '
rm -f args &&
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 &&
+ echo "systemctl --user --force --now disable" git-maintenance@hourly.timer git-maintenance@daily.timer git-maintenance@weekly.timer >expect &&
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" \
--
2.44.0
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler
2024-03-22 22:11 ` [PATCH v2 6/6] maintenance: update tests for systemd scheduler Max Gautier
@ 2024-03-22 23:02 ` Eric Sunshine
2024-03-23 10:28 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Eric Sunshine @ 2024-03-22 23:02 UTC (permalink / raw)
To: Max Gautier
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Junio C Hamano
On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> The systemd units are now in the source tree, rather than produced when
> running git maitenance start. There is no need anymore to couple
> validating the units and testing `git maintenance start`.
s/maitenance/maintenance/
> Adjust the test to verify the new `systemctl` command used, discard
> checks for presence/absence of unit files in $XDG_CONFIG_HOME.
>
> Validate the systemd units in the source tree, with one test per unit to
> have more distinct failures.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
In a patch series, in order to preserve "bisectability", we want to
ensure that the entire test suite continues to pass after each patch
is applied. A such, we normally update tests -- to ensure that they
continue passing -- in each patch which changes some
testable/observable behavior. However, this series only updates test
in the final patch. Doesn't that break bisectability? Or am I
misunderstanding something?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 6/6] maintenance: update tests for systemd scheduler
2024-03-22 23:02 ` Eric Sunshine
@ 2024-03-23 10:28 ` Max Gautier
0 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-23 10:28 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Junio C Hamano
On Fri, Mar 22, 2024 at 07:02:36PM -0400, Eric Sunshine wrote:
> On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote:
> > The systemd units are now in the source tree, rather than produced when
> > running git maitenance start. There is no need anymore to couple
> > validating the units and testing `git maintenance start`.
>
> s/maitenance/maintenance/
>
Ack
> > Adjust the test to verify the new `systemctl` command used, discard
> > checks for presence/absence of unit files in $XDG_CONFIG_HOME.
> >
> > Validate the systemd units in the source tree, with one test per unit to
> > have more distinct failures.
> >
> > Signed-off-by: Max Gautier <mg@max.gautier.name>
>
> In a patch series, in order to preserve "bisectability", we want to
> ensure that the entire test suite continues to pass after each patch
> is applied. A such, we normally update tests -- to ensure that they
> continue passing -- in each patch which changes some
> testable/observable behavior. However, this series only updates test
> in the final patch. Doesn't that break bisectability? Or am I
> misunderstanding something?
No you're right, good point. I'll split this one up and fold the parts
where the behavior was changed.
And I'll keep that in mind in the future.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-22 22:11 ` [PATCH v2 0/6] maintenance: use packaged systemd units Max Gautier
` (5 preceding siblings ...)
2024-03-22 22:11 ` [PATCH v2 6/6] maintenance: update tests for systemd scheduler Max Gautier
@ 2024-03-24 14:54 ` Phillip Wood
2024-03-24 17:03 ` Eric Sunshine
2024-03-25 8:32 ` Max Gautier
6 siblings, 2 replies; 52+ messages in thread
From: Phillip Wood @ 2024-03-24 14:54 UTC (permalink / raw)
To: Max Gautier, git
Cc: Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
Hi Max
On 22/03/2024 22:11, Max Gautier wrote:
> * Distribute the systemd timers used by the `git maintenance start` with
> the systemd scheduler as part of git, rather than writing them in
> $XDG_CONFIG_HOME.
>
> This allows users to override the units if they wish, and is more
> in-line with the usual practices of distribution for systemd units.
Thanks for suggesting this, I think this is a useful change, but the
implementation could be improved.
> We also move away from using the random minute, and instead rely on
> systemd features to achieve the same goal (see patch 2). This allows us
> to go back to using unit templating for the timers. This is also a
> prerequisite to have static unit files.
>
> Note that even if we really need more specific OnCalendar= settings for
> each timer, we should still do it that way, but instead distribute
> override alongside the template, for instance for weekly:
>
> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> [Timer]
> OnCalendar=<daily specific calendar spec>
We should definitely do that. Using systemd's random delay does not
prevent the different maintenance jobs from running concurrently as one
job may be started before a previous job has finished. It is important
to only have one job running at a time because the first thing "git
maintenance run" does is to try and acquire a lock file so if the hourly
job is running when the daily jobs tries to start the daily job will not
be run. As the daily job is a superset of the hourly job and the weekly
job is a superset of the daily job so it does not make sense to run more
than one job per hour.
> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
> and takes care of not removing legitimate user overrides, by checking
> the file start.
This points to an alternate strategy for supporting user overrides -
don't overwrite the unit files if the user has edited them. I think that
there is still a benefit to moving to system wide unit files though as
it means that if we improve the unit files in the future systemd will
pick up these improvements automatically. That is an improvement over
the status quo where the users' unit files are written once and never
updated.
I think it would help to reorder the changes in this series as follows:
1 - simplify the invocation of "systemctl --user"
This would be the current patch 3 without adding "--force" or
moving "--now" combined with the relevant test changes from patch 6.
This would make it clear that those changes are a simple clean up that
is independent of the other changes made in this series.
2 - don't delete user edited unit files
This would be based on the current patch 4 and would show that we can
avoid deleting unit files that the user has edited without the other
changes in this series. This change should have an associated test.
3 - start using systemd's random delay function
This would be the current patch 1 without the template changes and the
commit message should explain that it is in preparation for disturbing
system-wide unit files.
4 - install system-wide systemd unit files
This would be based on the current patch 2 with the addition of
overrides to prevent more than one job running per hour. The unit
files should be installed under $XDG_DATA_HOME when $(prefix) starts
with $(HOME), not just when they are equal. The associated test
changes from patch 6 should be moved here as well as the "--force"
change from patch 3.
5 - documentation updates
I'm on the fence about having these in a separate commit like the
current patch 5 or updating the documentation when the code is
changed.
Best Wishes
Phillip
> Testing:
> The simplest way to simulate having the units in /usr/lib is probably to
> copy them in /etc/systemd/user.
>
> Changes since v1:
> - Reorganization of the commits and their messages to try to address
> review comments
> - Dropped the DON'T APPLY PATCH, added a TODO to the cleanup code
> instead
> - Updated the git-maintenance tests to work with the new logic.
> - Conditional installation of the units files
> - Fixing some style/consistency issues
> - template the systemd service file to use $(bindir)
>
> Max Gautier (6):
> maintenance: use systemd timers builtin randomization
> maintenance: use packaged systemd units
> maintenance: simplify systemctl calls
> maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
> maintenance: update systemd scheduler docs
> maintenance: update tests for systemd scheduler
>
> Documentation/git-maintenance.txt | 33 ++-
> Makefile | 5 +
> builtin/gc.c | 298 ++++-------------------
> config.mak.uname | 10 +
> systemd/user/git-maintenance@.service.in | 17 ++
> systemd/user/git-maintenance@.timer | 12 +
> t/t7900-maintenance.sh | 50 ++--
> 7 files changed, 126 insertions(+), 299 deletions(-)
> create mode 100644 systemd/user/git-maintenance@.service.in
> create mode 100644 systemd/user/git-maintenance@.timer
>
> Range-diff against v1:
> 1: ea54a6e50e < -: ---------- maintenance: package systemd units
> 2: b29dbb9fdd < -: ---------- maintenance: use packaged systemd units
> 3: 47bd6712b8 < -: ---------- maintenance: add fixed random delay to systemd timers
> -: ---------- > 1: 42d88c7f81 maintenance: use systemd timers builtin randomization
> -: ---------- > 2: 18d51b1dd1 maintenance: use packaged systemd units
> -: ---------- > 3: 3aa7446e95 maintenance: simplify systemctl calls
> -: ---------- > 4: daff7b4d60 maintenance: cleanup $XDG_CONFIG_HOME/systemd/user
> 4: fac57db55e ! 5: 5f6a8e141f maintenance: update systemd scheduler docs
> @@ Metadata
> ## Commit message ##
> maintenance: update systemd scheduler docs
>
> + The `git maintenance` systemd scheduler no longer writes units in
> + $XDG_CONFIG_HOME.
> +
> + Describe the new behavior.
> + Instead of explaining manual ways to modify the timer, suggest the
> + systemd standard tool: `systemctl edit`.
> +
> Signed-off-by: Max Gautier <mg@max.gautier.name>
>
> ## Documentation/git-maintenance.txt ##
> 5: d888fbd0c3 < -: ---------- DON'T APPLY YET: maintenance: remove cleanup code
> -: ---------- > 6: 4d4bcd6233 maintenance: update tests for systemd scheduler
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-24 14:54 ` [PATCH v2 0/6] maintenance: use packaged systemd units Phillip Wood
@ 2024-03-24 17:03 ` Eric Sunshine
2024-03-25 10:08 ` phillip.wood123
2024-03-25 8:32 ` Max Gautier
1 sibling, 1 reply; 52+ messages in thread
From: Eric Sunshine @ 2024-03-24 17:03 UTC (permalink / raw)
To: phillip.wood
Cc: Max Gautier, git, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt
On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 5 - documentation updates
> I'm on the fence about having these in a separate commit like the
> current patch 5 or updating the documentation when the code is
> changed.
It's generally more reviewer-friendly to bundle documentation change
into the patch which changes the observable behavior. This way, a
reviewer has the behavior change fresh in mind and can verify that the
revised documentation matches the new implementation. Same goes for
revising tests in the same patch which changes behavior (though, of
course, revising tests at the same time as the change of behavior is
also mandatory for maintaining bisectability).
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-24 17:03 ` Eric Sunshine
@ 2024-03-25 10:08 ` phillip.wood123
0 siblings, 0 replies; 52+ messages in thread
From: phillip.wood123 @ 2024-03-25 10:08 UTC (permalink / raw)
To: Eric Sunshine, phillip.wood
Cc: Max Gautier, git, Lénaïc Huard, Derrick Stolee,
Patrick Steinhardt
On 24/03/2024 17:03, Eric Sunshine wrote:
> On Sun, Mar 24, 2024 at 10:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> 5 - documentation updates
>> I'm on the fence about having these in a separate commit like the
>> current patch 5 or updating the documentation when the code is
>> changed.
>
> It's generally more reviewer-friendly to bundle documentation change
> into the patch which changes the observable behavior. This way, a
> reviewer has the behavior change fresh in mind and can verify that the
> revised documentation matches the new implementation. Same goes for
> revising tests in the same patch which changes behavior (though, of
> course, revising tests at the same time as the change of behavior is
> also mandatory for maintaining bisectability).
Good point, I think a couple of the documentation changes like
recommending "systemctl --user edit" were improving the existing docs
and so they should be in a separate commit at the start of the series.
The other patches should update the documentation as the code changes.
Thanks
Phillip
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-24 14:54 ` [PATCH v2 0/6] maintenance: use packaged systemd units Phillip Wood
2024-03-24 17:03 ` Eric Sunshine
@ 2024-03-25 8:32 ` Max Gautier
2024-03-25 10:06 ` phillip.wood123
1 sibling, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-25 8:32 UTC (permalink / raw)
To: phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
> Hi Max
>
> On 22/03/2024 22:11, Max Gautier wrote:
> > * Distribute the systemd timers used by the `git maintenance start` with
> > the systemd scheduler as part of git, rather than writing them in
> > $XDG_CONFIG_HOME.
> >
> > This allows users to override the units if they wish, and is more
> > in-line with the usual practices of distribution for systemd units.
>
> Thanks for suggesting this, I think this is a useful change, but the
> implementation could be improved.
>
> > We also move away from using the random minute, and instead rely on
> > systemd features to achieve the same goal (see patch 2). This allows us
> > to go back to using unit templating for the timers. This is also a
> > prerequisite to have static unit files.
> >
> > Note that even if we really need more specific OnCalendar= settings for
> > each timer, we should still do it that way, but instead distribute
> > override alongside the template, for instance for weekly:
> >
> > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> > [Timer]
> > OnCalendar=<daily specific calendar spec>
>
> We should definitely do that. Using systemd's random delay does not prevent
> the different maintenance jobs from running concurrently as one job may be
> started before a previous job has finished. It is important to only have one
> job running at a time because the first thing "git maintenance run" does is
> to try and acquire a lock file so if the hourly job is running when the
> daily jobs tries to start the daily job will not be run.
Thinking about that, it occurs to me that the current scheme does not
prevent concurrent execution either: the timers all use Persistent=true,
which means they can fire concurrently on machine boot, if two or more
would have been triggered during the time the machine was powered off
(or just the user logged out, since it's a user unit).
So maybe there should be a more robust mechanism to avoid concurrent
execution ? I assume from what you say above the lock is acquired in a
non-blocking way. Could going to a blocking one be a solution ?
> As the daily job is
> a superset of the hourly job and the weekly job is a superset of the daily
> job so it does not make sense to run more than one job per hour.
Is that set in stone, or could they perform disjoint set of tasks
instead ?
>
> > The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
> > and takes care of not removing legitimate user overrides, by checking
> > the file start.
>
> This points to an alternate strategy for supporting user overrides - don't
> overwrite the unit files if the user has edited them. I think that there is
> still a benefit to moving to system wide unit files though as it means that
> if we improve the unit files in the future systemd will pick up these
> improvements automatically. That is an improvement over the status quo where
> the users' unit files are written once and never updated.
>
> I think it would help to reorder the changes in this series as follows:
>
> 1 - simplify the invocation of "systemctl --user"
> This would be the current patch 3 without adding "--force" or
> moving "--now" combined with the relevant test changes from patch 6.
> This would make it clear that those changes are a simple clean up that
> is independent of the other changes made in this series.
>
> 2 - don't delete user edited unit files
> This would be based on the current patch 4 and would show that we can
> avoid deleting unit files that the user has edited without the other
> changes in this series. This change should have an associated test.
>
> 3 - start using systemd's random delay function
> This would be the current patch 1 without the template changes and the
> commit message should explain that it is in preparation for disturbing
> system-wide unit files.
>
> 4 - install system-wide systemd unit files
> This would be based on the current patch 2 with the addition of
> overrides to prevent more than one job running per hour. The unit
> files should be installed under $XDG_DATA_HOME when $(prefix) starts
> with $(HOME), not just when they are equal. The associated test
> changes from patch 6 should be moved here as well as the "--force"
> change from patch 3.
>
> 5 - documentation updates
> I'm on the fence about having these in a separate commit like the
> current patch 5 or updating the documentation when the code is
> changed.
I had started cooking v3, I'll take into account, thanks !
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 8:32 ` Max Gautier
@ 2024-03-25 10:06 ` phillip.wood123
2024-03-25 12:27 ` Max Gautier
2024-03-25 13:45 ` Max Gautier
0 siblings, 2 replies; 52+ messages in thread
From: phillip.wood123 @ 2024-03-25 10:06 UTC (permalink / raw)
To: Max Gautier, phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
Hi Max
On 25/03/2024 08:32, Max Gautier wrote:
> On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
>> Hi Max
>>
>> On 22/03/2024 22:11, Max Gautier wrote:
>>> * Distribute the systemd timers used by the `git maintenance start` with
>>> the systemd scheduler as part of git, rather than writing them in
>>> $XDG_CONFIG_HOME.
>>>
>>> This allows users to override the units if they wish, and is more
>>> in-line with the usual practices of distribution for systemd units.
>>
>> Thanks for suggesting this, I think this is a useful change, but the
>> implementation could be improved.
>>
>>> We also move away from using the random minute, and instead rely on
>>> systemd features to achieve the same goal (see patch 2). This allows us
>>> to go back to using unit templating for the timers. This is also a
>>> prerequisite to have static unit files.
>>>
>>> Note that even if we really need more specific OnCalendar= settings for
>>> each timer, we should still do it that way, but instead distribute
>>> override alongside the template, for instance for weekly:
>>>
>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
>>> [Timer]
>>> OnCalendar=<daily specific calendar spec>
>>
>> We should definitely do that. Using systemd's random delay does not prevent
>> the different maintenance jobs from running concurrently as one job may be
>> started before a previous job has finished. It is important to only have one
>> job running at a time because the first thing "git maintenance run" does is
>> to try and acquire a lock file so if the hourly job is running when the
>> daily jobs tries to start the daily job will not be run.
>
> Thinking about that, it occurs to me that the current scheme does not
> prevent concurrent execution either: the timers all use Persistent=true,
> which means they can fire concurrently on machine boot, if two or more
> would have been triggered during the time the machine was powered off
> (or just the user logged out, since it's a user unit).
Interesting, I wonder if the other schedulers suffer from the same problem.
> So maybe there should be a more robust mechanism to avoid concurrent
> execution ? I assume from what you say above the lock is acquired in a
> non-blocking way. Could going to a blocking one be a solution ?
It is possible to wait on a lock file but I'd be worried about building
up an endless queue of processes if the process holding the lock file
crashed leaving it in place without anyway to automatically remove it.
I don't think we need to solve that problem as part of this patch series
but we should take care not to make it worse. Long term we may be better
scheduling a single job and have "git maintenance run" decide which jobs
to run based on the last time it run, rather than trying to schedule
different jobs with the os scheduler.
>> As the daily job is
>> a superset of the hourly job and the weekly job is a superset of the daily
>> job so it does not make sense to run more than one job per hour.
>
> Is that set in stone, or could they perform disjoint set of tasks
> instead ?
All of the schedulers are set up to run a single job each hour, I don't
see why we'd start running disjoint sets of tasks in the different jobs.
>>> The cleanup code for the units written in $XDG_CONFIG_HOME is adapted,
>>> and takes care of not removing legitimate user overrides, by checking
>>> the file start.
>>
>> This points to an alternate strategy for supporting user overrides - don't
>> overwrite the unit files if the user has edited them. I think that there is
>> still a benefit to moving to system wide unit files though as it means that
>> if we improve the unit files in the future systemd will pick up these
>> improvements automatically. That is an improvement over the status quo where
>> the users' unit files are written once and never updated.
>>
>> I think it would help to reorder the changes in this series as follows:
>>
>> 1 - simplify the invocation of "systemctl --user"
>> This would be the current patch 3 without adding "--force" or
>> moving "--now" combined with the relevant test changes from patch 6.
>> This would make it clear that those changes are a simple clean up that
>> is independent of the other changes made in this series.
>>
>> 2 - don't delete user edited unit files
>> This would be based on the current patch 4 and would show that we can
>> avoid deleting unit files that the user has edited without the other
>> changes in this series. This change should have an associated test.
>>
>> 3 - start using systemd's random delay function
>> This would be the current patch 1 without the template changes and the
>> commit message should explain that it is in preparation for disturbing
>> system-wide unit files.
>>
>> 4 - install system-wide systemd unit files
>> This would be based on the current patch 2 with the addition of
>> overrides to prevent more than one job running per hour. The unit
>> files should be installed under $XDG_DATA_HOME when $(prefix) starts
>> with $(HOME), not just when they are equal. The associated test
>> changes from patch 6 should be moved here as well as the "--force"
>> change from patch 3.
>>
>> 5 - documentation updates
>> I'm on the fence about having these in a separate commit like the
>> current patch 5 or updating the documentation when the code is
>> changed.
>
> I had started cooking v3, I'll take into account, thanks !
Thanks
Phillip
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 10:06 ` phillip.wood123
@ 2024-03-25 12:27 ` Max Gautier
2024-03-25 16:39 ` Phillip Wood
2024-03-25 13:45 ` Max Gautier
1 sibling, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-25 12:27 UTC (permalink / raw)
To: phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
> Hi Max
>
> On 25/03/2024 08:32, Max Gautier wrote:
> > On Sun, Mar 24, 2024 at 02:54:58PM +0000, Phillip Wood wrote:
> > > Hi Max
> > >
> > > On 22/03/2024 22:11, Max Gautier wrote:
> > > > * Distribute the systemd timers used by the `git maintenance start` with
> > > > the systemd scheduler as part of git, rather than writing them in
> > > > $XDG_CONFIG_HOME.
> > > >
> > > > This allows users to override the units if they wish, and is more
> > > > in-line with the usual practices of distribution for systemd units.
> > >
> > > Thanks for suggesting this, I think this is a useful change, but the
> > > implementation could be improved.
> > >
> > > > We also move away from using the random minute, and instead rely on
> > > > systemd features to achieve the same goal (see patch 2). This allows us
> > > > to go back to using unit templating for the timers. This is also a
> > > > prerequisite to have static unit files.
> > > >
> > > > Note that even if we really need more specific OnCalendar= settings for
> > > > each timer, we should still do it that way, but instead distribute
> > > > override alongside the template, for instance for weekly:
> > > >
> > > > /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
> > > > [Timer]
> > > > OnCalendar=<daily specific calendar spec>
> > >
> > > We should definitely do that. Using systemd's random delay does not prevent
> > > the different maintenance jobs from running concurrently as one job may be
> > > started before a previous job has finished. It is important to only have one
> > > job running at a time because the first thing "git maintenance run" does is
> > > to try and acquire a lock file so if the hourly job is running when the
> > > daily jobs tries to start the daily job will not be run.
> >
> > Thinking about that, it occurs to me that the current scheme does not
> > prevent concurrent execution either: the timers all use Persistent=true,
> > which means they can fire concurrently on machine boot, if two or more
> > would have been triggered during the time the machine was powered off
> > (or just the user logged out, since it's a user unit).
>
> Interesting, I wonder if the other schedulers suffer from the same problem.
From what I can find (didn't dig much):
- cron does not have the problem, because it will just miss the timers
if the machine was powered off. Not really better ^
- anacron though is another implementation of cron which apparently
support that semantic and is the default on ubuntu [1]
I can't find if there is something to avoid the same problem that
Persitent=true imply
- same goes for launchctl (Effect of Sleeping and Powering Off at the
bottom of the page [2])
- for schtasks it's apparently possible to have a similar mechanism than
Persistent [3]. There is a policy apparently to handle multiples
instances [4] but I'm not completely sure whether or not theses
instances can have different parameters.
It's currently defined that way for the schtasks scheduler:
"<MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy>\n". I
don't think it would prevent parallel execution between the different
schedule though, it seems to me they are different tasks.
[1]: https://serverfault.com/a/52338
[2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html
[3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off
[4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances
>
> > So maybe there should be a more robust mechanism to avoid concurrent
> > execution ? I assume from what you say above the lock is acquired in a
> > non-blocking way. Could going to a blocking one be a solution ?
>
> It is possible to wait on a lock file but I'd be worried about building up
> an endless queue of processes if the process holding the lock file crashed
> leaving it in place without anyway to automatically remove it.
>
At least with systemd we have some mechanisms to deal with that.
- systemd timers don't touch an already running unit, so that won't
trigger a new hourly or daily if the previous one is still running.
- for the automatically removing it, we could:
- use XDG_RUNTIME_DIR ("%t" in systemd units) which is removed on
logout
- optionally add a tmpfiles fragments to delete locks which are really
too old (tmpfiles won't delete files on which a lock is taken)
- I thought about using a common RuntimeDirectory (see systemd.exec),
but this is not possible due to [5]
[5]: https://github.com/systemd/systemd/issues/5394
> I don't think we need to solve that problem as part of this patch series but
> we should take care not to make it worse. Long term we may be better
> scheduling a single job and have "git maintenance run" decide which jobs to
> run based on the last time it run, rather than trying to schedule different
> jobs with the os scheduler.
>
> > > As the daily job is
> > > a superset of the hourly job and the weekly job is a superset of the daily
> > > job so it does not make sense to run more than one job per hour.
> >
> > Is that set in stone, or could they perform disjoint set of tasks
> > instead ?
>
> All of the schedulers are set up to run a single job each hour, I don't see
> why we'd start running disjoint sets of tasks in the different jobs.
I was wondering if running distinct tasks would allow overlapping
execution, or if the different tasks are not safe to run concurrently
anyway. I'm not familiar enough with them and the git internals to tell.
Another option if the tasks set was distinct for each service instance
would be to use dependencies and ordering directives like this:
weekly.service
```
[Unit]
Requires=daily.service
After=daily.service
[Service]
ExecStart=<run only weekly stuff>
```
daily.service
```
[Unit]
Requires=hourly.service
After=hourly.service
[Service]
ExecStart=<run only daily stuff>
```
hourly.service
```
[Service]
ExecStart=<run only hourly stuff>
```
That would avoid concurrent execution I think.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 12:27 ` Max Gautier
@ 2024-03-25 16:39 ` Phillip Wood
0 siblings, 0 replies; 52+ messages in thread
From: Phillip Wood @ 2024-03-25 16:39 UTC (permalink / raw)
To: Max Gautier, phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
On 25/03/2024 12:27, Max Gautier wrote:
> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>>>> Note that even if we really need more specific OnCalendar= settings for
>>>>> each timer, we should still do it that way, but instead distribute
>>>>> override alongside the template, for instance for weekly:
>>>>>
>>>>> /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:
>>>>> [Timer]
>>>>> OnCalendar=<daily specific calendar spec>
>>>>
>>>> We should definitely do that. Using systemd's random delay does not prevent
>>>> the different maintenance jobs from running concurrently as one job may be
>>>> started before a previous job has finished. It is important to only have one
>>>> job running at a time because the first thing "git maintenance run" does is
>>>> to try and acquire a lock file so if the hourly job is running when the
>>>> daily jobs tries to start the daily job will not be run.
>>>
>>> Thinking about that, it occurs to me that the current scheme does not
>>> prevent concurrent execution either: the timers all use Persistent=true,
>>> which means they can fire concurrently on machine boot, if two or more
>>> would have been triggered during the time the machine was powered off
>>> (or just the user logged out, since it's a user unit).
>>
>> Interesting, I wonder if the other schedulers suffer from the same problem.
>
> From what I can find (didn't dig much):
Thanks for looking at this
> - cron does not have the problem, because it will just miss the timers
> if the machine was powered off. Not really better ^
Yes, skipping the jobs is not great. On debian at least the job will be
run if it is less than three hours since it should have been run. See
https://manpages.debian.org/bookworm/cron/cron.8.en.html
> - anacron though is another implementation of cron which apparently
> support that semantic and is the default on ubuntu [1]
> I can't find if there is something to avoid the same problem that
> Persitent=true imply
> - same goes for launchctl (Effect of Sleeping and Powering Off at the
> bottom of the page [2])
As I read it the job is rescheduled if the computer was asleep when it
should have run, but not if it was powered off.
> - for schtasks it's apparently possible to have a similar mechanism than
> Persistent [3]. There is a policy apparently to handle multiples
> instances [4] but I'm not completely sure whether or not theses
> instances can have different parameters.
> It's currently defined that way for the schtasks scheduler:
> "<MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy>\n". I
> don't think it would prevent parallel execution between the different
> schedule though, it seems to me they are different tasks.
>
> [1]: https://serverfault.com/a/52338
> [2]: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html
> [3]: https://learn.microsoft.com/en-us/troubleshoot/windows-server/system-management-components/scheduled-task-not-run-upon-reboot-machine-off
> [4]: https://learn.microsoft.com/en-us/windows/win32/taskschd/tasksettings-multipleinstances
We should have a think about what to do about this once your patches to
move to system-wide unit files are merged. We'll need to come up with
something that works for all the schedulers so that we don't miss the
daily and weekly jobs when the computer is powered off and ensures we
don't run concurrent jobs.
Best Wishes
Phillip
>>> So maybe there should be a more robust mechanism to avoid concurrent
>>> execution ? I assume from what you say above the lock is acquired in a
>>> non-blocking way. Could going to a blocking one be a solution ?
>>
>> It is possible to wait on a lock file but I'd be worried about building up
>> an endless queue of processes if the process holding the lock file crashed
>> leaving it in place without anyway to automatically remove it.
>>
>
> At least with systemd we have some mechanisms to deal with that.
> - systemd timers don't touch an already running unit, so that won't
> trigger a new hourly or daily if the previous one is still running.
> - for the automatically removing it, we could:
> - use XDG_RUNTIME_DIR ("%t" in systemd units) which is removed on
> logout
> - optionally add a tmpfiles fragments to delete locks which are really
> too old (tmpfiles won't delete files on which a lock is taken)
> - I thought about using a common RuntimeDirectory (see systemd.exec),
> but this is not possible due to [5]
>
>
> [5]: https://github.com/systemd/systemd/issues/5394
>
>> I don't think we need to solve that problem as part of this patch series but
>> we should take care not to make it worse. Long term we may be better
>> scheduling a single job and have "git maintenance run" decide which jobs to
>> run based on the last time it run, rather than trying to schedule different
>> jobs with the os scheduler.
>>
>>>> As the daily job is
>>>> a superset of the hourly job and the weekly job is a superset of the daily
>>>> job so it does not make sense to run more than one job per hour.
>>>
>>> Is that set in stone, or could they perform disjoint set of tasks
>>> instead ?
>>
>> All of the schedulers are set up to run a single job each hour, I don't see
>> why we'd start running disjoint sets of tasks in the different jobs.
>
> I was wondering if running distinct tasks would allow overlapping
> execution, or if the different tasks are not safe to run concurrently
> anyway. I'm not familiar enough with them and the git internals to tell.
>
> Another option if the tasks set was distinct for each service instance
> would be to use dependencies and ordering directives like this:
> weekly.service
> ```
> [Unit]
> Requires=daily.service
> After=daily.service
>
> [Service]
> ExecStart=<run only weekly stuff>
> ```
>
> daily.service
> ```
> [Unit]
> Requires=hourly.service
> After=hourly.service
>
> [Service]
> ExecStart=<run only daily stuff>
> ```
>
> hourly.service
> ```
> [Service]
> ExecStart=<run only hourly stuff>
> ```
>
> That would avoid concurrent execution I think.
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 10:06 ` phillip.wood123
2024-03-25 12:27 ` Max Gautier
@ 2024-03-25 13:45 ` Max Gautier
2024-03-25 16:39 ` Phillip Wood
1 sibling, 1 reply; 52+ messages in thread
From: Max Gautier @ 2024-03-25 13:45 UTC (permalink / raw)
To: phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>
> It is possible to wait on a lock file but I'd be worried about building up
> an endless queue of processes if the process holding the lock file crashed
> leaving it in place without anyway to automatically remove it.
Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
in POSIX.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 13:45 ` Max Gautier
@ 2024-03-25 16:39 ` Phillip Wood
2024-03-27 16:21 ` Max Gautier
0 siblings, 1 reply; 52+ messages in thread
From: Phillip Wood @ 2024-03-25 16:39 UTC (permalink / raw)
To: Max Gautier, phillip.wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
On 25/03/2024 13:45, Max Gautier wrote:
> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>
>> It is possible to wait on a lock file but I'd be worried about building up
>> an endless queue of processes if the process holding the lock file crashed
>> leaving it in place without anyway to automatically remove it.
>
> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
> in POSIX.
We need something cross-platform, I don't think there is any appetite to
move away from our current lock file implementation. We can think about
how to handle concurrent maintenance jobs once these patches are merged.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/6] maintenance: use packaged systemd units
2024-03-25 16:39 ` Phillip Wood
@ 2024-03-27 16:21 ` Max Gautier
0 siblings, 0 replies; 52+ messages in thread
From: Max Gautier @ 2024-03-27 16:21 UTC (permalink / raw)
To: phillip.wood, Phillip Wood
Cc: git, Lénaïc Huard, Derrick Stolee, Patrick Steinhardt,
Eric Sunshine
Le 25 mars 2024 17:39:47 GMT+01:00, Phillip Wood <phillip.wood123@gmail.com> a écrit :
>On 25/03/2024 13:45, Max Gautier wrote:
>> On Mon, Mar 25, 2024 at 10:06:29AM +0000, phillip.wood123@gmail.com wrote:
>>>
>>> It is possible to wait on a lock file but I'd be worried about building up
>>> an endless queue of processes if the process holding the lock file crashed
>>> leaving it in place without anyway to automatically remove it.
>>
>> Side question: we can't use flock(2) in git, or lockf(3) ? The latter is
>> in POSIX.
>
>We need something cross-platform, I don't think there is any appetite to move away from our current lock file implementation. We can think about how to handle concurrent maintenance jobs once these patches are merged.
>
>Best Wishes
>
>Phillip
>
>
Ack, I'll leave that concern aside for now.
--
Max Gautier
^ permalink raw reply [flat|nested] 52+ messages in thread