Message ID | 1466874128-22828-1-git-send-email-nroach44@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
On 25-06-16 19:02, Nathaniel Roach wrote: > Use a template service file as all of the daemons use almost > identical arguments and generally appear the same to the init > system. > > We "Wants=" zebra as that's the daemon for interfacing to the > kernel, and it's not required for the other daemons to work > but it's probably going to be used in nearly all setups. > > /usr/bin/env is needed as systemd doesn't allow the instance > variable (%i) in the executable path. > > We don't enable these services by default as this would require > creating configuration and /etc/default files. (And is easily > achieved with an FS overlay) ^missing . So the goal of this patch is that the user simply can symlink to instead of having to create the service file. However, since the Wants= is added for zebra, it would make sense to create the zebra symlink, no? > > Signed-off-by: Nathaniel Roach <nroach44@gmail.com> > --- > Changes v1 -> v2: > (As suggested by Arnout Vandecappelle) > - Completely remove shim and use /usr/bin/env instead > - Don't tell quagga to fork as systemd prefers it > - Add comment to .service file about /usr/bin/env > - Explain not enabling the service on build in patch > > --- > package/quagga/quagga.mk | 3 +++ > package/quagga/quagga@.service | 20 ++++++++++++++++++++ > 2 files changed, 23 insertions(+) > create mode 100644 package/quagga/quagga@.service > > diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk > index 22e90ad..5b5623f 100644 > --- a/package/quagga/quagga.mk > +++ b/package/quagga/quagga.mk > @@ -75,6 +75,9 @@ endif > define QUAGGA_INSTALL_INIT_SYSTEMD > $(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \ > $(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf > + This empty line is redundant. > + $(INSTALL) -D -m 644 package/quagga/quagga@.service \ > + $(TARGET_DIR)/usr/lib/systemd/system/quagga@.service > endef > > $(eval $(autotools-package)) > diff --git a/package/quagga/quagga@.service b/package/quagga/quagga@.service > new file mode 100644 > index 0000000..b17af89 > --- /dev/null > +++ b/package/quagga/quagga@.service > @@ -0,0 +1,20 @@ > +[Unit] > +Description=Quagga %i routing daemon > +PartOf=quagga.service I don't understand this. Doesn't the quagga.service actually have to exist for this to be valid? > +ReloadPropagatedFrom=quagga.service > +Wants=quagga@zebra.service I just realize now: this line should probably be removed when BR2_PACKAGE_QUAGGA_ZEBRA is not set. > + > +[Service] > +PrivateTmp=true > +KillMode=mixed It would make sense to keep this together with KillSignal. > +Type=simple > +EnvironmentFile=/etc/default/quagga-%i > +# Systemd doesn't like having %i in the executable path. > +ExecStart=/usr/bin/env /usr/sbin/%i $OPTS -f /etc/quagga/%i.conf -i /var/run/quagga/%i.pid > +PIDFile=/var/run/quagga/%i.pid I repeat, I'm not very familiar with systemd, but as I understand it PID files shouldn't be needed anymore with a Type=simple daemon. Unless of course if quagga uses them somewhere else too. Regards, Arnout > +KillSignal=SIGINT > +Restart=on-failure > +RestartSec=1 > + > +[Install] > +WantedBy=multi-user.target >
On 27/06/16 05:09, Arnout Vandecappelle wrote: > On 25-06-16 19:02, Nathaniel Roach wrote: >> Use a template service file as all of the daemons use almost >> identical arguments and generally appear the same to the init >> system. >> >> We "Wants=" zebra as that's the daemon for interfacing to the >> kernel, and it's not required for the other daemons to work >> but it's probably going to be used in nearly all setups. >> >> /usr/bin/env is needed as systemd doesn't allow the instance >> variable (%i) in the executable path. >> >> We don't enable these services by default as this would require >> creating configuration and /etc/default files. (And is easily >> achieved with an FS overlay) > ^missing . > > So the goal of this patch is that the user simply can symlink to instead of > having to create the service file. > > However, since the Wants= is added for zebra, it would make sense to create the > zebra symlink, no? No, systemd will automatically start zebra when it's needed. It's not needed until one of the other daemons start (which systemd will start zebra beforehand if the daemon is local). If they are remote, the user can enable zebra themselves. > >> Signed-off-by: Nathaniel Roach <nroach44@gmail.com> >> --- >> Changes v1 -> v2: >> (As suggested by Arnout Vandecappelle) >> - Completely remove shim and use /usr/bin/env instead >> - Don't tell quagga to fork as systemd prefers it >> - Add comment to .service file about /usr/bin/env >> - Explain not enabling the service on build in patch >> >> --- >> package/quagga/quagga.mk | 3 +++ >> package/quagga/quagga@.service | 20 ++++++++++++++++++++ >> 2 files changed, 23 insertions(+) >> create mode 100644 package/quagga/quagga@.service >> >> diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk >> index 22e90ad..5b5623f 100644 >> --- a/package/quagga/quagga.mk >> +++ b/package/quagga/quagga.mk >> @@ -75,6 +75,9 @@ endif >> define QUAGGA_INSTALL_INIT_SYSTEMD >> $(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \ >> $(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf >> + > This empty line is redundant. > >> + $(INSTALL) -D -m 644 package/quagga/quagga@.service \ >> + $(TARGET_DIR)/usr/lib/systemd/system/quagga@.service >> endef >> >> $(eval $(autotools-package)) >> diff --git a/package/quagga/quagga@.service b/package/quagga/quagga@.service >> new file mode 100644 >> index 0000000..b17af89 >> --- /dev/null >> +++ b/package/quagga/quagga@.service >> @@ -0,0 +1,20 @@ >> +[Unit] >> +Description=Quagga %i routing daemon >> +PartOf=quagga.service > I don't understand this. Doesn't the quagga.service actually have to exist for > this to be valid? That is true, it's left over from my openvpn service file. Will remove that and ReloadPropagatedFrom= > >> +ReloadPropagatedFrom=quagga.service >> +Wants=quagga@zebra.service > I just realize now: this line should probably be removed when > BR2_PACKAGE_QUAGGA_ZEBRA is not set. Also true, however that's why I said Wants= and not Requires= so systemd handles the lack of zebra fine. > >> + >> +[Service] >> +PrivateTmp=true >> +KillMode=mixed > It would make sense to keep this together with KillSignal. > >> +Type=simple >> +EnvironmentFile=/etc/default/quagga-%i >> +# Systemd doesn't like having %i in the executable path. >> +ExecStart=/usr/bin/env /usr/sbin/%i $OPTS -f /etc/quagga/%i.conf -i /var/run/quagga/%i.pid >> +PIDFile=/var/run/quagga/%i.pid > I repeat, I'm not very familiar with systemd, but as I understand it PID files > shouldn't be needed anymore with a Type=simple daemon. Unless of course if > quagga uses them somewhere else too. Also correct. I left the -i argument in as I thought it was required for vtysh, but it is not. > > > Regards, > Arnout > >> +KillSignal=SIGINT >> +Restart=on-failure >> +RestartSec=1 >> + >> +[Install] >> +WantedBy=multi-user.target >> > Thanks for the feedback! Nathaniel Roach
(Mainly improving my systemd knowledge here :-) On 29-06-16 09:06, Nathaniel Roach wrote: > On 27/06/16 05:09, Arnout Vandecappelle wrote: >> On 25-06-16 19:02, Nathaniel Roach wrote: [snip] >> However, since the Wants= is added for zebra, it would make sense to create the >> zebra symlink, no? > No, systemd will automatically start zebra when it's needed. It's not needed > until one of the other daemons start (which systemd will start zebra beforehand > if the daemon is local). If they are remote, the user can enable zebra themselves. So, if there is a Wants=quagga@zebra.service line, then systemd will look for quagga@zebra.service in /usr/lib/systemd/services, and failing that it will automatically try quagga@.service? Nice! [snip] >>> +Wants=quagga@zebra.service >> I just realize now: this line should probably be removed when >> BR2_PACKAGE_QUAGGA_ZEBRA is not set. > Also true, however that's why I said Wants= and not Requires= so systemd handles > the lack of zebra fine. So in case zebra doesn't exist, systemd will still try to execute the service file, but the ExecStart command will fail because /usr/sbin/zebra doesn't exist, right? But in that case, won't it retry every second (Restart=on-failure) and spam the journal horribly? Maybe there should be a ConditionFileIsExecutable in the [Unit] section? Regards, Arnout
On 29/06/16 17:53, Arnout Vandecappelle wrote: > (Mainly improving my systemd knowledge here :-) > > On 29-06-16 09:06, Nathaniel Roach wrote: >> On 27/06/16 05:09, Arnout Vandecappelle wrote: >>> On 25-06-16 19:02, Nathaniel Roach wrote: > [snip] >>> However, since the Wants= is added for zebra, it would make sense to create the >>> zebra symlink, no? >> No, systemd will automatically start zebra when it's needed. It's not needed >> until one of the other daemons start (which systemd will start zebra beforehand >> if the daemon is local). If they are remote, the user can enable zebra themselves. > So, if there is a Wants=quagga@zebra.service line, then systemd will look for > quagga@zebra.service in /usr/lib/systemd/services, and failing that it will > automatically try quagga@.service? Nice! Pretty much! /etc/systemd/system/multi-user.target.wants/quagga@zebra.service is installed on systemctl enable [...] and it's just a symlink to quagga@.service > [snip] >>>> +Wants=quagga@zebra.service >>> I just realize now: this line should probably be removed when >>> BR2_PACKAGE_QUAGGA_ZEBRA is not set. >> Also true, however that's why I said Wants= and not Requires= so systemd handles >> the lack of zebra fine. > So in case zebra doesn't exist, systemd will still try to execute the service > file, but the ExecStart command will fail because /usr/sbin/zebra doesn't exist, > right? But in that case, won't it retry every second (Restart=on-failure) and > spam the journal horribly? > > Maybe there should be a ConditionFileIsExecutable in the [Unit] section? That was a good condition that I didn't think of. In that case the unit will fail and restart until it hits the start limit. I've since added in the Condition stanza and everything works as intended. > > > Regards, > Arnout > >
Hello, On Wed, 29 Jun 2016 20:02:47 +0800, Nathaniel Roach wrote: > > Maybe there should be a ConditionFileIsExecutable in the [Unit] section? > That was a good condition that I didn't think of. In that case the unit > will fail and restart until it hits the start limit. I've since added in > the Condition stanza and everything works as intended. Could you send an updated version that takes into account Arnout's comments and especially the addition of this condition? Thanks a lot! Thomas
diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk index 22e90ad..5b5623f 100644 --- a/package/quagga/quagga.mk +++ b/package/quagga/quagga.mk @@ -75,6 +75,9 @@ endif define QUAGGA_INSTALL_INIT_SYSTEMD $(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \ $(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf + + $(INSTALL) -D -m 644 package/quagga/quagga@.service \ + $(TARGET_DIR)/usr/lib/systemd/system/quagga@.service endef $(eval $(autotools-package)) diff --git a/package/quagga/quagga@.service b/package/quagga/quagga@.service new file mode 100644 index 0000000..b17af89 --- /dev/null +++ b/package/quagga/quagga@.service @@ -0,0 +1,20 @@ +[Unit] +Description=Quagga %i routing daemon +PartOf=quagga.service +ReloadPropagatedFrom=quagga.service +Wants=quagga@zebra.service + +[Service] +PrivateTmp=true +KillMode=mixed +Type=simple +EnvironmentFile=/etc/default/quagga-%i +# Systemd doesn't like having %i in the executable path. +ExecStart=/usr/bin/env /usr/sbin/%i $OPTS -f /etc/quagga/%i.conf -i /var/run/quagga/%i.pid +PIDFile=/var/run/quagga/%i.pid +KillSignal=SIGINT +Restart=on-failure +RestartSec=1 + +[Install] +WantedBy=multi-user.target
Use a template service file as all of the daemons use almost identical arguments and generally appear the same to the init system. We "Wants=" zebra as that's the daemon for interfacing to the kernel, and it's not required for the other daemons to work but it's probably going to be used in nearly all setups. /usr/bin/env is needed as systemd doesn't allow the instance variable (%i) in the executable path. We don't enable these services by default as this would require creating configuration and /etc/default files. (And is easily achieved with an FS overlay) Signed-off-by: Nathaniel Roach <nroach44@gmail.com> --- Changes v1 -> v2: (As suggested by Arnout Vandecappelle) - Completely remove shim and use /usr/bin/env instead - Don't tell quagga to fork as systemd prefers it - Add comment to .service file about /usr/bin/env - Explain not enabling the service on build in patch --- package/quagga/quagga.mk | 3 +++ package/quagga/quagga@.service | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 package/quagga/quagga@.service