| Message ID | 20210501145220.2082670-1-lenaic@lhuard.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintenance: use systemd timers on Linux | expand |
On 2021-05-01 at 14:52:20, Lénaïc Huard wrote: > The existing mechanism for scheduling background maintenance is done > through cron. On Linux systems managed by systemd, systemd provides an > alternative to schedule recurring tasks: systemd timers. > > The main motivations to implement systemd timers in addition to cron > are: > * cron is optional and Linux systems running systemd might not have it > installed. > * The execution of `crontab -l` can tell us if cron is installed but not > if the daemon is actually running. > * With systemd, each service is run in its own cgroup and its logs are > tagged by the service inside journald. With cron, all scheduled tasks > are running in the cron daemon cgroup and all the logs of the > user-scheduled tasks are pretended to belong to the system cron > service. > Concretely, a user that doesn’t have access to the system logs won’t > have access to the log of its own tasks scheduled by cron whereas he > will have access to the log of its own tasks scheduled by systemd > timer. I would prefer to see this as a configurable option. I have systemd installed (because it's not really optional to have a functional desktop on Linux) but I want to restrict it to starting and stopping services, not performing the tasks of cron. cron is portable across a wide variety of systems, including Linux variants (and WSL) that don't use systemd, and I prefer to use more standard tooling when possible.
On 01/05/21 21.52, Lénaïc Huard wrote: > The existing mechanism for scheduling background maintenance is done > through cron. On Linux systems managed by systemd, systemd provides an > alternative to schedule recurring tasks: systemd timers. > > The main motivations to implement systemd timers in addition to cron > are: > * cron is optional and Linux systems running systemd might not have it > installed. Supposed that I have Linux box with systemd and classical cron. Should systemd timers be preferred over cron? > * The execution of `crontab -l` can tell us if cron is installed but not > if the daemon is actually running. > * With systemd, each service is run in its own cgroup and its logs are > tagged by the service inside journald. With cron, all scheduled tasks > are running in the cron daemon cgroup and all the logs of the > user-scheduled tasks are pretended to belong to the system cron > service. > Concretely, a user that doesn’t have access to the system logs won’t > have access to the log of its own tasks scheduled by cron whereas he > will have access to the log of its own tasks scheduled by systemd > timer. > > In order to schedule git maintenance, we need two unit template files: > * ~/.config/systemd/user/git-maintenance@.service > to define the command to be started by systemd and > * ~/.config/systemd/user/git-maintenance@.timer > to define the schedule at which the command should be run. > > Those units are templates that are parametrized by the frequency. > > Based on those templates, 3 timers are started: > * git-maintenance@hourly.timer > * git-maintenance@daily.timer > * git-maintenance@weekly.timer > > The command launched by those three timers are the same than with the > other scheduling methods: > > git for-each-repo --config=maintenance.repo maintenance run > --schedule=%i > > with the full path for git to ensure that the version of git launched > for the scheduled maintenance is the same as the one used to run > `maintenance start`. > > The timer unit contains `Persistent=true` so that, if the computer is > powered down when a maintenance task should run, the task will be run > when the computer is back powered on. > > Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> Nevertheless, because we are dealing with external dependency (systemd), it should makes sense to enforce this dependency requirement when user choose to use systemd timers so that users on non-systemd boxes (such as Gentoo with OpenRC) don't see errors that forcing them to use systemd.
On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: > The existing mechanism for scheduling background maintenance is done > through cron. On Linux systems managed by systemd, systemd provides an > alternative to schedule recurring tasks: systemd timers. Thanks for working on this. While `cron` has been the go-to standard for decades, `systemd` is certainly widespread enough that it makes sense to support it, as well. > The main motivations to implement systemd timers in addition to cron > are: > * cron is optional and Linux systems running systemd might not have it > installed. > * The execution of `crontab -l` can tell us if cron is installed but not > if the daemon is actually running. > * With systemd, each service is run in its own cgroup and its logs are > tagged by the service inside journald. With cron, all scheduled tasks > are running in the cron daemon cgroup and all the logs of the > user-scheduled tasks are pretended to belong to the system cron > service. > Concretely, a user that doesn’t have access to the system logs won’t > have access to the log of its own tasks scheduled by cron whereas he > will have access to the log of its own tasks scheduled by systemd > timer. The last point is somewhat compelling. A potential counterargument is that `cron` does send email to the user by default if any output is generated by the cron job. However, it seems quite likely these days that many systems either won't have local mail service enabled or the user won't bother checking the local mailbox. It's a minor point, but if you re-roll it might make sense for the commit message to expand the last point by saying that although `cron` attempts to send email, that email may go unseen by the user. > In order to schedule git maintenance, we need two unit template files: > * ~/.config/systemd/user/git-maintenance@.service > to define the command to be started by systemd and > * ~/.config/systemd/user/git-maintenance@.timer > to define the schedule at which the command should be run. > [...] > The timer unit contains `Persistent=true` so that, if the computer is > powered down when a maintenance task should run, the task will be run > when the computer is back powered on. It would be nice for the commit message to also give some high-level information about how git-maintenance chooses between `cron` and `systemd` and whether the user can influence that decision. (I know the answer because I read the patch, but this is the sort of information which is good to have in the commit message; readers want to know why certain choices were made.) Although I avoid Linux distros with `systemd`, my knee-jerk reaction, like brian's upthread, is that there should be some escape hatch or direct mechanism to allow the user to choose between `systemd` and `cron`. The patch itself is straightforward enough and nicely follows the pattern established for already-implemented schedulers, so I don't have a lot to say about it. I did leave a few comments below, most of which are subjective nits and minor observations, though there are two or three actionable items. > Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> > --- > diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt > @@ -279,6 +279,55 @@ schedule to ensure you are executing the correct binaries in your > +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMD > +----------------------------------------------- Is there a reason for the duplicated "SYSTEMD" that I'm missing? I suppose you probably mean "SYSTEMD SYSTEMS". > +In this case, `git maintenance start` will create user systemd timer units > +and start the timers. The current list of user-scheduled tasks can be found > +by running `systemctl --user list-timers`. The timers written by `git > +maintenance start` are similar to this: > + > +----------------------------------------------------------------------- > +$ systemctl --user list-timers > +NEXT LEFT LAST PASSED UNIT ACTIVATES > +Thu 2021-04-29 19:00:00 CEST 42min left Thu 2021-04-29 18:00:11 CEST 17min ago git-maintenance@hourly.timer git-maintenance@hourly.service > +Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago git-maintenance@daily.timer git-maintenance@daily.service > +Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service > + > +3 timers listed. > +Pass --all to see loaded but inactive timers, too. > +----------------------------------------------------------------------- I suspect that the "3 timers listed" and "Pass --all" lines don't add value and can be dropped without hurting the example. > +`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 > +`~/.config/systemd/user/git-maintenance@.service.d/*.conf`. Will `systemd` users generally understand what filename to create in the "...@.service.d/" directory, and will they know what to populate the file with? (Genuine question; I've never dealt with that.) > diff --git a/builtin/gc.c b/builtin/gc.c > @@ -1872,6 +1872,25 @@ static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd > +static int is_crontab_available(const char *cmd) > +{ > + struct child_process child = CHILD_PROCESS_INIT; > + > + strvec_split(&child.args, cmd); > + strvec_push(&child.args, "-l"); > + child.no_stdin = 1; > + child.no_stdout = 1; > + child.no_stderr = 1; > + child.silent_exec_failure = 1; > + > + if (start_command(&child)) > + return 0; > + /* Ignore exit code, as an empty crontab will return error. */ > + finish_command(&child); > + > + return 1; > +} Ignoring the error from `crontab -l` is an already-established idiom in this file. Okay. Nit: There doesn't seem to be a need for the blank line before `return 1`, and other maintenance-related functions don't have such a blank line. The same comment about blank lines before `return` applies to other newly-added functions, as well. But it's subjective, and not necessarily worth changing. > +static char *systemd_timer_timer_filename() > +{ > + const char *filename = "~/.config/systemd/user/git-maintenance@.timer"; > + char *expanded = expand_user_path(filename, 0); > + if (!expanded) > + die(_("failed to expand path '%s'"), filename); > + > + return expanded; > +} I was curious whether this would fail if `.config/systemd/user/` didn't already exist, but looking at the implementation of expand_user_path() , I see that it doesn't require the path to already exist if you pass 0 for the second argument as you do here. Okay. > +static char *systemd_timer_service_filename() > +{ > + const char *filename = > + "~/.config/systemd/user/git-maintenance@.service"; > + char *expanded = expand_user_path(filename, 0); > + if (!expanded) > + die(_("failed to expand path '%s'"), filename); > + > + return expanded; > +} The duplication of code between systemd_timer_timer_filename() and systemd_timer_service_filename() is probably too minor to worry about. Okay. > +static int systemd_timer_enable_unit(int enable, > + enum schedule_priority schedule, > + const char *cmd) > +{ > + struct child_process child = CHILD_PROCESS_INIT; > + const char *frequency = get_frequency(schedule); > + > + strvec_split(&child.args, cmd); > + strvec_push(&child.args, "--user"); > + if (enable) > + strvec_push(&child.args, "enable"); > + else > + strvec_push(&child.args, "disable"); It's subjective, but this might be more nicely expressed as: strvec_push(&child.args, enable ? "enable" : "disable"); > + strvec_push(&child.args, "--now"); > + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); > + > + if (start_command(&child)) > + die(_("failed to run systemctl")); > + return finish_command(&child); > +} > +static int systemd_timer_write_unit_templates(const char *exec_path) > +{ > + unit = "[Unit]\n" > + "Description=Optimize Git repositories data\n" > + "\n" > + "[Service]\n" > + "Type=oneshot\n" > + "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" I see that it's in POSIX, but do we use this `%n$s` directive elsewhere in the Git source code? If not, I'd be cautious of introducing it here. Maybe it's better to just use plain `%s` twice... > + "LockPersonality=yes\n" > + "MemoryDenyWriteExecute=yes\n" > + "NoNewPrivileges=yes\n" > + "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n" > + "RestrictNamespaces=yes\n" > + "RestrictRealtime=yes\n" > + "RestrictSUIDSGID=yes\n" > + "SystemCallArchitectures=native\n" > + "SystemCallFilter=@system-service\n"; > + fprintf(file, unit, exec_path); ... and then: fprintf(file, unit, exec_path, exec_path); > + fclose(file); > + > + return 0; > +} > @@ -1986,6 +2159,15 @@ static int update_background_schedule(int enable) > + if (!strcmp(scheduler, "crontab_or_systemctl")) { > + if (is_systemd_timer_available("systemctl")) > + scheduler = cmd = "systemctl"; > + else if (is_crontab_available("crontab")) > + scheduler = cmd = "crontab"; > + else > + die(_("Neither systemd timers nor crontab are available")); > + } Other messages emitted by git-maintenance are entirely lowercase, so downcasing "Neither" would be appropriate. > @@ -1995,10 +2177,14 @@ static int update_background_schedule(int enable) > - die("unknown background scheduler: %s", scheduler); > + die(_("unknown background scheduler: %s"), scheduler); This change is unrelated to the rest of the patch. Normally, such a "fix" would be made as a separate patch. This one is somewhat minor, so perhaps it doesn't matter whether it's in this patch... > rollback_lock_file(&lk); > + free(lock_path); > free(testing); > return result; ... however, this leak fix probably deserves its own patch. Or, at the very least, mention these two fixes in this commit message. > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > @@ -20,6 +20,20 @@ test_xmllint () { > +test_lazy_prereq SYSTEMD_ANALYZE ' > + systemd-analyze --help >out && > + grep -w verify out > +' Unportable use of `grep -w`. It's neither in POSIX nor understood by BSD-lineage `grep` (including macOS `grep`).
On Sun, May 2, 2021 at 1:28 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > On 01/05/21 21.52, Lénaïc Huard wrote: > > The existing mechanism for scheduling background maintenance is done > > through cron. On Linux systems managed by systemd, systemd provides an > > alternative to schedule recurring tasks: systemd timers. > > > > The main motivations to implement systemd timers in addition to cron > > are: > > * cron is optional and Linux systems running systemd might not have it > > installed. > > Supposed that I have Linux box with systemd and classical cron. Should > systemd timers be preferred over cron? The implementation in this patch unconditionally prefers `systemd` over `cron`. Whether that's a good idea is subject to question (as both brian and I mentioned in our reviews). > Nevertheless, because we are dealing with external dependency (systemd), it > should makes sense to enforce this dependency requirement when user choose to use > systemd timers so that users on non-systemd boxes (such as Gentoo with OpenRC) > don't see errors that forcing them to use systemd. If you scan through the patch itself, you will find that it is careful to choose the appropriate scheduler and not to spit out errors when one or the other scheduler is unavailable.
On 01/05/21 21.52, Lénaïc Huard wrote: > +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMD > +----------------------------------------------- > + systemd was repeated twice above. It should be `BACKGROUND MAINTENANCE WITH SYSTEMD TIMERS` > test_expect_success 'help text' ' > test_expect_code 129 git maintenance -h 2>err && Why exit code 129 there?
On 02/05/2021 07:45, Eric Sunshine wrote: > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: >> The existing mechanism for scheduling background maintenance is done >> through cron. On Linux systems managed by systemd, systemd provides an >> alternative to schedule recurring tasks: systemd timers. > > Thanks for working on this. While `cron` has been the go-to standard > for decades, `systemd` is certainly widespread enough that it makes > sense to support it, as well. Yes, thank you for working on this, it will be very useful to users like me who use a linux distribution that does not install a cron daemon by default but relies on systemd instead. >> The main motivations to implement systemd timers in addition to cron >> are: >> * cron is optional and Linux systems running systemd might not have it >> installed. >> * The execution of `crontab -l` can tell us if cron is installed but not >> if the daemon is actually running. Can we use systemctl to see if it is running (and enabled so we know it will be restarted after a reboot)? >> * With systemd, each service is run in its own cgroup and its logs are >> tagged by the service inside journald. With cron, all scheduled tasks >> are running in the cron daemon cgroup and all the logs of the >> user-scheduled tasks are pretended to belong to the system cron >> service. >> Concretely, a user that doesn’t have access to the system logs won’t >> have access to the log of its own tasks scheduled by cron whereas he >> will have access to the log of its own tasks scheduled by systemd >> timer. > > The last point is somewhat compelling. A potential counterargument is > that `cron` does send email to the user by default if any output is > generated by the cron job. However, it seems quite likely these days > that many systems either won't have local mail service enabled or the > user won't bother checking the local mailbox. It's a minor point, but > if you re-roll it might make sense for the commit message to expand > the last point by saying that although `cron` attempts to send email, > that email may go unseen by the user. > >> In order to schedule git maintenance, we need two unit template files: >> * ~/.config/systemd/user/git-maintenance@.service >> to define the command to be started by systemd and >> * ~/.config/systemd/user/git-maintenance@.timer >> to define the schedule at which the command should be run. >> [...] >> The timer unit contains `Persistent=true` so that, if the computer is >> powered down when a maintenance task should run, the task will be run >> when the computer is back powered on. > > It would be nice for the commit message to also give some high-level > information about how git-maintenance chooses between `cron` and > `systemd` and whether the user can influence that decision. (I know > the answer because I read the patch, but this is the sort of > information which is good to have in the commit message; readers want > to know why certain choices were made.) > > Although I avoid Linux distros with `systemd`, my knee-jerk reaction, > like brian's upthread, is that there should be some escape hatch or > direct mechanism to allow the user to choose between `systemd` and > `cron`. I agree that if both are present the user should be able to choose one. I'm not sure what the default should be in that case - before I read the commit message and Eric's comments I was inclined to say that if the user has cron installed we should take that as a sign they preferred cron over systemd timers and use that. However, given the arguments above about not knowing if cron is running and the user not necessarily getting emails from cron or being able to read the logs than maybe defaulting to systemd timers makes sense. > The patch itself is straightforward enough and nicely follows the > pattern established for already-implemented schedulers, so I don't > have a lot to say about it. I did leave a few comments below, most of > which are subjective nits and minor observations, though there are two > or three actionable items. > >> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> >> --- >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt >> @@ -279,6 +279,55 @@ schedule to ensure you are executing the correct binaries in your >> +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMD >> +----------------------------------------------- > > Is there a reason for the duplicated "SYSTEMD" that I'm missing? I > suppose you probably mean "SYSTEMD SYSTEMS". > >> +In this case, `git maintenance start` will create user systemd timer units >> +and start the timers. The current list of user-scheduled tasks can be found >> +by running `systemctl --user list-timers`. The timers written by `git >> +maintenance start` are similar to this: >> + >> +----------------------------------------------------------------------- >> +$ systemctl --user list-timers >> +NEXT LEFT LAST PASSED UNIT ACTIVATES >> +Thu 2021-04-29 19:00:00 CEST 42min left Thu 2021-04-29 18:00:11 CEST 17min ago git-maintenance@hourly.timer git-maintenance@hourly.service >> +Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago git-maintenance@daily.timer git-maintenance@daily.service >> +Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service >> + >> +3 timers listed. >> +Pass --all to see loaded but inactive timers, too. >> +----------------------------------------------------------------------- > > I suspect that the "3 timers listed" and "Pass --all" lines don't add > value and can be dropped without hurting the example. If the idea is to show the output of `systemctl --user list-timers` then I don't think we should be editing it. I also think having the column headers helps as it shows what the fields are. >> +`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 >> +`~/.config/systemd/user/git-maintenance@.service.d/*.conf`. > > Will `systemd` users generally understand what filename to create in > the "...@.service.d/" directory, and will they know what to populate > the file with? (Genuine question; I've never dealt with that.) I think it would be helpful to explicitly mention the file names (I don't think I could tell you what they are without reading the relevant systemd man page) >> diff --git a/builtin/gc.c b/builtin/gc.c >> @@ -1872,6 +1872,25 @@ static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd >> +static int is_crontab_available(const char *cmd) >> +{ >> + struct child_process child = CHILD_PROCESS_INIT; >> + >> + strvec_split(&child.args, cmd); >> + strvec_push(&child.args, "-l"); >> + child.no_stdin = 1; >> + child.no_stdout = 1; >> + child.no_stderr = 1; >> + child.silent_exec_failure = 1; >> + >> + if (start_command(&child)) >> + return 0; >> + /* Ignore exit code, as an empty crontab will return error. */ >> + finish_command(&child); >> + >> + return 1; >> +} > > Ignoring the error from `crontab -l` is an already-established idiom > in this file. Okay. > > Nit: There doesn't seem to be a need for the blank line before `return > 1`, and other maintenance-related functions don't have such a blank > line. The same comment about blank lines before `return` applies to > other newly-added functions, as well. But it's subjective, and not > necessarily worth changing. > >> +static char *systemd_timer_timer_filename() >> +{ >> + const char *filename = "~/.config/systemd/user/git-maintenance@.timer"; >> + char *expanded = expand_user_path(filename, 0); >> + if (!expanded) >> + die(_("failed to expand path '%s'"), filename); >> + >> + return expanded; >> +} > > I was curious whether this would fail if `.config/systemd/user/` > didn't already exist, but looking at the implementation of > expand_user_path() , I see that it doesn't require the path to already > exist if you pass 0 for the second argument as you do here. Okay. Do we need to worry about $XDG_CONFIG_HOME rather than hard coding "~/.config/". There is a function xdg_config_home() that takes care of this. Thanks again for working on this Best Wishes Phillip
On 5/1/2021 10:52 AM, Lénaïc Huard wrote: > The existing mechanism for scheduling background maintenance is done > through cron. On Linux systems managed by systemd, systemd provides an > alternative to schedule recurring tasks: systemd timers. > > The main motivations to implement systemd timers in addition to cron > are: ... Thank you for working on this. Users have questioned "why cron?" since the release of background maintenance, so I appreciate you taking the time to port this feature to systemd. I won't do a deep code review here since that seems to already be covered, and a v2 seems required. Ensuring that users can choose which of the two backends is a good idea. We might even want to start with 'cron' as the default and 'systemd' as an opt-in. The other concern I wanted to discuss was the upgrade scenario. If users have already enabled background maintenance with the cron backend, how can we help users disable the cron backend before they upgrade to the systemd version? I imagine that we should disable cron when enabling systemd, using crontab_update_schedule() with run_maintenance given as 0. We might want to enable quiet errors in that method for the case that cron does not exist. It is important to make it clear that we only accept one scheduler at a time, since they would be competing to run 'git for-each-repo' over the same list of repos. A single user cannot schedule some repositories in 'cron' and another set in 'systemd'. This seems like an acceptable technical limitation. Thanks, and I look forward to your v2! -Stolee
On Sun, May 02 2021, Eric Sunshine wrote: > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: >> + strvec_push(&child.args, "--now"); >> + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); >> + >> + if (start_command(&child)) >> + die(_("failed to run systemctl")); >> + return finish_command(&child); >> +} >> +static int systemd_timer_write_unit_templates(const char *exec_path) >> +{ >> + unit = "[Unit]\n" >> + "Description=Optimize Git repositories data\n" >> + "\n" >> + "[Service]\n" >> + "Type=oneshot\n" >> + "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" > > I see that it's in POSIX, but do we use this `%n$s` directive > elsewhere in the Git source code? If not, I'd be cautious of > introducing it here. Maybe it's better to just use plain `%s` twice... We use it in po/, so for sprintf() on systems that don't have NO_GETTEXT=Y we already test it in the wild. But no, I don't think anything in the main source uses it, FWIW I have a WIP series in my own fork that I've cooked for a while that uses this, I haven't run into any issues with it in either GitHub's CI (e.g. Windows), or on the systems I myself test on. I think it would be a useful canary to just take a change like this, we can always change it to the form you suggest if it doesn't work out.
On 2021-05-02 15:10:05+0100, Phillip Wood <phillip.wood123@gmail.com> wrote: > On 02/05/2021 07:45, Eric Sunshine wrote: > > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: > > > The existing mechanism for scheduling background maintenance is done > > > through cron. On Linux systems managed by systemd, systemd provides an > > > alternative to schedule recurring tasks: systemd timers. > > > > Thanks for working on this. While `cron` has been the go-to standard > > for decades, `systemd` is certainly widespread enough that it makes > > sense to support it, as well. > > Yes, thank you for working on this, it will be very useful to users like me > who use a linux distribution that does not install a cron daemon by default > but relies on systemd instead. > > > > The main motivations to implement systemd timers in addition to cron > > > are: > > > * cron is optional and Linux systems running systemd might not have it > > > installed. > > > * The execution of `crontab -l` can tell us if cron is installed but not > > > if the daemon is actually running. > > Can we use systemctl to see if it is running (and enabled so we know it will > be restarted after a reboot)? Not sure if I understand this suggestion. However, non-systemd systems doesn't have systemctl command to begin with. > > > * With systemd, each service is run in its own cgroup and its logs are > > > tagged by the service inside journald. With cron, all scheduled tasks > > > are running in the cron daemon cgroup and all the logs of the > > > user-scheduled tasks are pretended to belong to the system cron > > > service. > > > Concretely, a user that doesn’t have access to the system logs won’t > > > have access to the log of its own tasks scheduled by cron whereas he > > > will have access to the log of its own tasks scheduled by systemd > > > timer. > > > > The last point is somewhat compelling. A potential counterargument is > > that `cron` does send email to the user by default if any output is > > generated by the cron job. However, it seems quite likely these days > > that many systems either won't have local mail service enabled or the > > user won't bother checking the local mailbox. It's a minor point, but > > if you re-roll it might make sense for the commit message to expand > > the last point by saying that although `cron` attempts to send email, > > that email may go unseen by the user. > > > > > In order to schedule git maintenance, we need two unit template files: > > > * ~/.config/systemd/user/git-maintenance@.service > > > to define the command to be started by systemd and > > > * ~/.config/systemd/user/git-maintenance@.timer > > > to define the schedule at which the command should be run. I think it would be better to change ~/.config here to $XDG_CONFIG_HOME, as others also points out in another comments. [..snip..] > > > +`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 > > > +`~/.config/systemd/user/git-maintenance@.service.d/*.conf`. Ditto. > > Will `systemd` users generally understand what filename to create in > > the "...@.service.d/" directory, and will they know what to populate > > the file with? (Genuine question; I've never dealt with that.) > > I think it would be helpful to explicitly mention the file names (I don't > think I could tell you what they are without reading the relevant systemd > man page) [..snip..] > > > +static char *systemd_timer_timer_filename() > > > +{ > > > + const char *filename = "~/.config/systemd/user/git-maintenance@.timer"; > > > + char *expanded = expand_user_path(filename, 0); > > > + if (!expanded) > > > + die(_("failed to expand path '%s'"), filename); > > > + > > > + return expanded; > > > +} > > > > I was curious whether this would fail if `.config/systemd/user/` > > didn't already exist, but looking at the implementation of > > expand_user_path() , I see that it doesn't require the path to already > > exist if you pass 0 for the second argument as you do here. Okay. > > Do we need to worry about $XDG_CONFIG_HOME rather than hard coding > "~/.config/". There is a function xdg_config_home() that takes care of this.
Hi Đoàn On 05/05/2021 13:19, Đoàn Trần Công Danh wrote: > On 2021-05-02 15:10:05+0100, Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 02/05/2021 07:45, Eric Sunshine wrote: >>> On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: >>>> The existing mechanism for scheduling background maintenance is done >>>> through cron. On Linux systems managed by systemd, systemd provides an >>>> alternative to schedule recurring tasks: systemd timers. >>> >>> Thanks for working on this. While `cron` has been the go-to standard >>> for decades, `systemd` is certainly widespread enough that it makes >>> sense to support it, as well. >> >> Yes, thank you for working on this, it will be very useful to users like me >> who use a linux distribution that does not install a cron daemon by default >> but relies on systemd instead. >> >>>> The main motivations to implement systemd timers in addition to cron >>>> are: >>>> * cron is optional and Linux systems running systemd might not have it >>>> installed. >>>> * The execution of `crontab -l` can tell us if cron is installed but not >>>> if the daemon is actually running. >> >> Can we use systemctl to see if it is running (and enabled so we know it will >> be restarted after a reboot)? > > Not sure if I understand this suggestion. > However, non-systemd systems doesn't have systemctl command to begin > with. I was wondering if on systems with both cron and systemd installed we could use systemctl to determine if crond is actually running as Lénaïc pointed out that being able to run `crontab -l` does not tell us if crond is running. Best Wishes Phillip >>>> * With systemd, each service is run in its own cgroup and its logs are >>>> tagged by the service inside journald. With cron, all scheduled tasks >>>> are running in the cron daemon cgroup and all the logs of the >>>> user-scheduled tasks are pretended to belong to the system cron >>>> service. >>>> Concretely, a user that doesn’t have access to the system logs won’t >>>> have access to the log of its own tasks scheduled by cron whereas he >>>> will have access to the log of its own tasks scheduled by systemd >>>> timer. >>> >>> The last point is somewhat compelling. A potential counterargument is >>> that `cron` does send email to the user by default if any output is >>> generated by the cron job. However, it seems quite likely these days >>> that many systems either won't have local mail service enabled or the >>> user won't bother checking the local mailbox. It's a minor point, but >>> if you re-roll it might make sense for the commit message to expand >>> the last point by saying that although `cron` attempts to send email, >>> that email may go unseen by the user. >>> >>>> In order to schedule git maintenance, we need two unit template files: >>>> * ~/.config/systemd/user/git-maintenance@.service >>>> to define the command to be started by systemd and >>>> * ~/.config/systemd/user/git-maintenance@.timer >>>> to define the schedule at which the command should be run. > > I think it would be better to change ~/.config here to > $XDG_CONFIG_HOME, as others also points out in another comments. > > [..snip..] > >>>> +`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 >>>> +`~/.config/systemd/user/git-maintenance@.service.d/*.conf`. > > Ditto. > >>> Will `systemd` users generally understand what filename to create in >>> the "...@.service.d/" directory, and will they know what to populate >>> the file with? (Genuine question; I've never dealt with that.) >> >> I think it would be helpful to explicitly mention the file names (I don't >> think I could tell you what they are without reading the relevant systemd >> man page) > > [..snip..] > >>>> +static char *systemd_timer_timer_filename() >>>> +{ >>>> + const char *filename = "~/.config/systemd/user/git-maintenance@.timer"; >>>> + char *expanded = expand_user_path(filename, 0); >>>> + if (!expanded) >>>> + die(_("failed to expand path '%s'"), filename); >>>> + >>>> + return expanded; >>>> +} >>> >>> I was curious whether this would fail if `.config/systemd/user/` >>> didn't already exist, but looking at the implementation of >>> expand_user_path() , I see that it doesn't require the path to already >>> exist if you pass 0 for the second argument as you do here. Okay. >> >> Do we need to worry about $XDG_CONFIG_HOME rather than hard coding >> "~/.config/". There is a function xdg_config_home() that takes care of this. >
Le mercredi 5 mai 2021, 14:01:25 CEST Ævar Arnfjörð Bjarmason a écrit : > On Sun, May 02 2021, Eric Sunshine wrote: > > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: > >> + strvec_push(&child.args, "--now"); > >> + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); > >> + > >> + if (start_command(&child)) > >> + die(_("failed to run systemctl")); > >> + return finish_command(&child); > >> +} > >> +static int systemd_timer_write_unit_templates(const char *exec_path) > >> +{ > >> + unit = "[Unit]\n" > >> + "Description=Optimize Git repositories data\n" > >> + "\n" > >> + "[Service]\n" > >> + "Type=oneshot\n" > >> + "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo > >> --config=maintenance.repo maintenance run --schedule=%%i\n"> > > I see that it's in POSIX, but do we use this `%n$s` directive > > elsewhere in the Git source code? If not, I'd be cautious of > > introducing it here. Maybe it's better to just use plain `%s` twice... > > We use it in po/, so for sprintf() on systems that don't have > NO_GETTEXT=Y we already test it in the wild. > > But no, I don't think anything in the main source uses it, FWIW I have a > WIP series in my own fork that I've cooked for a while that uses this, I > haven't run into any issues with it in either GitHub's CI > (e.g. Windows), or on the systems I myself test on. > > I think it would be a useful canary to just take a change like this, we > can always change it to the form you suggest if it doesn't work out. Based on this latest comment, I left the `%n$s` directive in the v2 of the patch. Let me know if that’s still OK. Otherwise, I’d be happy to implement Eric’s suggestion. Note however that this would be a “poor” canary to check if that directive is supported on all the platforms on which git has been ported. Indeed, this code is executed only on systemd platforms, which means quite recent Linux systems. Should this directive not be supported, I suppose it would be on more exotic systems.
On Mon, May 10 2021, Lénaïc Huard wrote: > Le mercredi 5 mai 2021, 14:01:25 CEST Ævar Arnfjörð Bjarmason a écrit : >> On Sun, May 02 2021, Eric Sunshine wrote: >> > On Sat, May 1, 2021 at 10:59 AM Lénaïc Huard <lenaic@lhuard.fr> wrote: >> >> + strvec_push(&child.args, "--now"); >> >> + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); >> >> + >> >> + if (start_command(&child)) >> >> + die(_("failed to run systemctl")); >> >> + return finish_command(&child); >> >> +} >> >> +static int systemd_timer_write_unit_templates(const char *exec_path) >> >> +{ >> >> + unit = "[Unit]\n" >> >> + "Description=Optimize Git repositories data\n" >> >> + "\n" >> >> + "[Service]\n" >> >> + "Type=oneshot\n" >> >> + "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo >> >> --config=maintenance.repo maintenance run --schedule=%%i\n"> >> > I see that it's in POSIX, but do we use this `%n$s` directive >> > elsewhere in the Git source code? If not, I'd be cautious of >> > introducing it here. Maybe it's better to just use plain `%s` twice... >> >> We use it in po/, so for sprintf() on systems that don't have >> NO_GETTEXT=Y we already test it in the wild. >> >> But no, I don't think anything in the main source uses it, FWIW I have a >> WIP series in my own fork that I've cooked for a while that uses this, I >> haven't run into any issues with it in either GitHub's CI >> (e.g. Windows), or on the systems I myself test on. >> >> I think it would be a useful canary to just take a change like this, we >> can always change it to the form you suggest if it doesn't work out. > > Based on this latest comment, I left the `%n$s` directive in the v2 of the > patch. > > Let me know if that’s still OK. Otherwise, I’d be happy to implement Eric’s > suggestion. > > Note however that this would be a “poor” canary to check if that directive is > supported on all the platforms on which git has been ported. > Indeed, this code is executed only on systemd platforms, which means quite > recent Linux systems. > Should this directive not be supported, I suppose it would be on more exotic > systems. Indeed, although we compile it on non-Linux platforms, so we'd expect to get complaints from smarter non-Linux compilers if fprintf() is given an unknown formatting directive.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 80ddd33ceb..30443b417a 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -279,6 +279,55 @@ schedule to ensure you are executing the correct binaries in your schedule. +BACKGROUND MAINTENANCE ON LINUX SYSTEMD SYSTEMD +----------------------------------------------- + +While Linux supports `cron`, depending on the distribution, `cron` may +be an optional package not necessarily installed. On modern Linux +distributions, systemd timers are superseding it. + +If user systemd timers are available, they will be used as a replacement +of `cron`. + +In this case, `git maintenance start` will create user systemd timer units +and start the timers. The current list of user-scheduled tasks can be found +by running `systemctl --user list-timers`. The timers written by `git +maintenance start` are similar to this: + +----------------------------------------------------------------------- +$ systemctl --user list-timers +NEXT LEFT LAST PASSED UNIT ACTIVATES +Thu 2021-04-29 19:00:00 CEST 42min left Thu 2021-04-29 18:00:11 CEST 17min ago git-maintenance@hourly.timer git-maintenance@hourly.service +Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago git-maintenance@daily.timer git-maintenance@daily.service +Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service + +3 timers listed. +Pass --all to see loaded but inactive timers, too. +----------------------------------------------------------------------- + +One timer is registered for each `--schedule=<frequency>` option. + +The definition of the systemd units can be inspected in the following files: + +----------------------------------------------------------------------- +~/.config/systemd/user/git-maintenance@.timer +~/.config/systemd/user/git-maintenance@.service +~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer +~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer +~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer +----------------------------------------------------------------------- + +`git maintenance start` will overwrite these files and start the timer +again with `systemctl --user`, so any customization should be done by +creating a drop-in file +`~/.config/systemd/user/git-maintenance@.service.d/*.conf`. + +`git maintenance stop` will stop the user systemd timers and delete +the above mentioned files. + +For more details, see systemd.timer(5) + + BACKGROUND MAINTENANCE ON MACOS SYSTEMS --------------------------------------- diff --git a/builtin/gc.c b/builtin/gc.c index ef7226d7bc..913fcfc882 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1872,6 +1872,25 @@ static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd return schtasks_remove_tasks(cmd); } +static int is_crontab_available(const char *cmd) +{ + struct child_process child = CHILD_PROCESS_INIT; + + strvec_split(&child.args, cmd); + strvec_push(&child.args, "-l"); + child.no_stdin = 1; + child.no_stdout = 1; + child.no_stderr = 1; + child.silent_exec_failure = 1; + + if (start_command(&child)) + return 0; + /* Ignore exit code, as an empty crontab will return error. */ + finish_command(&child); + + return 1; +} + #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" #define END_LINE "# END GIT MAINTENANCE SCHEDULE" @@ -1959,10 +1978,164 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd) return result; } +static int is_systemd_timer_available(const char *cmd) +{ + struct child_process child = CHILD_PROCESS_INIT; + + strvec_split(&child.args, cmd); + strvec_pushl(&child.args, "--user", "list-timers", NULL); + child.no_stdin = 1; + child.no_stdout = 1; + child.no_stderr = 1; + child.silent_exec_failure = 1; + + if (start_command(&child)) + return 0; + if (finish_command(&child)) + return 0; + + return 1; +} + +static char *systemd_timer_timer_filename() +{ + const char *filename = "~/.config/systemd/user/git-maintenance@.timer"; + char *expanded = expand_user_path(filename, 0); + if (!expanded) + die(_("failed to expand path '%s'"), filename); + + return expanded; +} + +static char *systemd_timer_service_filename() +{ + const char *filename = + "~/.config/systemd/user/git-maintenance@.service"; + char *expanded = expand_user_path(filename, 0); + if (!expanded) + die(_("failed to expand path '%s'"), filename); + + return expanded; +} + +static int systemd_timer_enable_unit(int enable, + enum schedule_priority schedule, + const char *cmd) +{ + struct child_process child = CHILD_PROCESS_INIT; + const char *frequency = get_frequency(schedule); + + strvec_split(&child.args, cmd); + strvec_push(&child.args, "--user"); + if (enable) + strvec_push(&child.args, "enable"); + else + strvec_push(&child.args, "disable"); + strvec_push(&child.args, "--now"); + strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); + + if (start_command(&child)) + die(_("failed to run systemctl")); + return finish_command(&child); +} + +static int systemd_timer_delete_unit_templates() +{ + char *filename = systemd_timer_timer_filename(); + unlink(filename); + free(filename); + + filename = systemd_timer_service_filename(); + unlink(filename); + free(filename); + + return 0; +} + +static int systemd_timer_delete_units(const char *cmd) +{ + return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, cmd) || + systemd_timer_enable_unit(0, SCHEDULE_DAILY, cmd) || + systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, cmd) || + systemd_timer_delete_unit_templates(); +} + +static int systemd_timer_write_unit_templates(const char *exec_path) +{ + char *filename; + FILE *file; + const char *unit; + + filename = systemd_timer_timer_filename(); + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + file = xfopen(filename, "w"); + free(filename); + + unit = "[Unit]\n" + "Description=Optimize Git repositories data\n" + "\n" + "[Timer]\n" + "OnCalendar=%i\n" + "Persistent=true\n" + "\n" + "[Install]\n" + "WantedBy=timers.target\n"; + fputs(unit, file); + fclose(file); + + filename = systemd_timer_service_filename(); + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + file = xfopen(filename, "w"); + free(filename); + + unit = "[Unit]\n" + "Description=Optimize Git repositories data\n" + "\n" + "[Service]\n" + "Type=oneshot\n" + "ExecStart=\"%1$s/git\" --exec-path=\"%1$s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" + "LockPersonality=yes\n" + "MemoryDenyWriteExecute=yes\n" + "NoNewPrivileges=yes\n" + "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n" + "RestrictNamespaces=yes\n" + "RestrictRealtime=yes\n" + "RestrictSUIDSGID=yes\n" + "SystemCallArchitectures=native\n" + "SystemCallFilter=@system-service\n"; + fprintf(file, unit, exec_path); + fclose(file); + + return 0; +} + +static int systemd_timer_setup_units(const char *cmd) +{ + const char *exec_path = git_exec_path(); + + return systemd_timer_write_unit_templates(exec_path) || + systemd_timer_enable_unit(1, SCHEDULE_HOURLY, cmd) || + systemd_timer_enable_unit(1, SCHEDULE_DAILY, cmd) || + systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, cmd); +} + +static int systemd_timer_update_schedule(int run_maintenance, int fd, + const char *cmd) +{ + if (run_maintenance) + return systemd_timer_setup_units(cmd); + else + return systemd_timer_delete_units(cmd); +} + #if defined(__APPLE__) static const char platform_scheduler[] = "launchctl"; #elif defined(GIT_WINDOWS_NATIVE) static const char platform_scheduler[] = "schtasks"; +#elif defined(__linux__) +static const char platform_scheduler[] = "crontab_or_systemctl"; #else static const char platform_scheduler[] = "crontab"; #endif @@ -1986,6 +2159,15 @@ static int update_background_schedule(int enable) cmd = sep + 1; } + if (!strcmp(scheduler, "crontab_or_systemctl")) { + if (is_systemd_timer_available("systemctl")) + scheduler = cmd = "systemctl"; + else if (is_crontab_available("crontab")) + scheduler = cmd = "crontab"; + else + die(_("Neither systemd timers nor crontab are available")); + } + if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) return error(_("another process is scheduling background maintenance")); @@ -1995,10 +2177,14 @@ static int update_background_schedule(int enable) result = schtasks_update_schedule(enable, get_lock_file_fd(&lk), cmd); else if (!strcmp(scheduler, "crontab")) result = crontab_update_schedule(enable, get_lock_file_fd(&lk), cmd); + else if (!strcmp(scheduler, "systemctl")) + result = systemd_timer_update_schedule( + enable, get_lock_file_fd(&lk), cmd); else - die("unknown background scheduler: %s", scheduler); + die(_("unknown background scheduler: %s"), scheduler); rollback_lock_file(&lk); + free(lock_path); free(testing); return result; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 2412d8c5c0..dd281789f4 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -20,6 +20,20 @@ test_xmllint () { fi } +test_lazy_prereq SYSTEMD_ANALYZE ' + systemd-analyze --help >out && + grep -w verify out +' + +test_systemd_analyze_verify () { + if test_have_prereq SYSTEMD_ANALYZE + then + systemd-analyze verify "$@" + else + true + fi +} + test_expect_success 'help text' ' test_expect_code 129 git maintenance -h 2>err && test_i18ngrep "usage: git maintenance <subcommand>" err && @@ -615,6 +629,43 @@ test_expect_success 'start and stop Windows maintenance' ' test_cmp expect args ' +test_expect_success 'start and stop Linux/systemd maintenance' ' + write_script print-args <<-\EOF && + echo $* >>args + EOF + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start && + + # start registers the repo + git config --get --global --fixed-value maintenance.repo "$(pwd)" && + + test_systemd_analyze_verify "$HOME/.config/systemd/user/git-maintenance@.service" && + + rm -f expect && + for frequency in hourly daily weekly + do + echo "--user enable --now git-maintenance@${frequency}.timer" >>expect || return 1 + done && + test_cmp expect args && + + rm -f args && + GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop && + + # stop does not unregister the repo + git config --get --global --fixed-value maintenance.repo "$(pwd)" && + + test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.timer" && + test_path_is_missing "$HOME/.config/systemd/user/git-maintenance@.service" && + + rm -f expect && + for frequency in hourly daily weekly + do + echo "--user disable --now git-maintenance@${frequency}.timer" >>expect || return 1 + done && + test_cmp expect args +' + test_expect_success 'register preserves existing strategy' ' git config maintenance.strategy none && git maintenance register &&
The existing mechanism for scheduling background maintenance is done through cron. On Linux systems managed by systemd, systemd provides an alternative to schedule recurring tasks: systemd timers. The main motivations to implement systemd timers in addition to cron are: * cron is optional and Linux systems running systemd might not have it installed. * The execution of `crontab -l` can tell us if cron is installed but not if the daemon is actually running. * With systemd, each service is run in its own cgroup and its logs are tagged by the service inside journald. With cron, all scheduled tasks are running in the cron daemon cgroup and all the logs of the user-scheduled tasks are pretended to belong to the system cron service. Concretely, a user that doesn’t have access to the system logs won’t have access to the log of its own tasks scheduled by cron whereas he will have access to the log of its own tasks scheduled by systemd timer. In order to schedule git maintenance, we need two unit template files: * ~/.config/systemd/user/git-maintenance@.service to define the command to be started by systemd and * ~/.config/systemd/user/git-maintenance@.timer to define the schedule at which the command should be run. Those units are templates that are parametrized by the frequency. Based on those templates, 3 timers are started: * git-maintenance@hourly.timer * git-maintenance@daily.timer * git-maintenance@weekly.timer The command launched by those three timers are the same than with the other scheduling methods: git for-each-repo --config=maintenance.repo maintenance run --schedule=%i with the full path for git to ensure that the version of git launched for the scheduled maintenance is the same as the one used to run `maintenance start`. The timer unit contains `Persistent=true` so that, if the computer is powered down when a maintenance task should run, the task will be run when the computer is back powered on. Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> --- Documentation/git-maintenance.txt | 49 ++++++++ builtin/gc.c | 188 +++++++++++++++++++++++++++++- t/t7900-maintenance.sh | 51 ++++++++ 3 files changed, 287 insertions(+), 1 deletion(-)