2016-03-12 12:05:50

by Benjamin Coddington

[permalink] [raw]
Subject: [PATCH nfs-utils] systemd: remove the nfs-config service

The nfs-config service exists to translate distro-specific startup
configuration into the command line arguments used by systemd to start nfs
utilities. Unfortunately, it is not clear to most users that this service
must be recycled every time startup configurations have been modified, as
this requirement is a change for at least one distro.

We can get rid of nfs-config by generating the startup arguments in an
ExecStartPre option for each service that needs them. We'll also have to
break out the EnvironmentFile into a separate file for each service to
avoid races overwriting this file.

A distro taking this change should also modify their
/usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
EnvironmentFile location for each service.

Signed-off-by: Benjamin Coddington <[email protected]>
---
systemd/Makefile.am | 1 -
systemd/README | 8 +++++---
systemd/nfs-blkmap.service | 4 +++-
systemd/nfs-idmapd.service | 7 +++----
systemd/nfs-mountd.service | 7 +++----
systemd/nfs-server.service | 7 +++----
systemd/rpc-gssd.service | 7 +++----
systemd/rpc-statd-notify.service | 7 +++----
systemd/rpc-statd.service | 7 +++----
systemd/rpc-svcgssd.service | 7 +++----
10 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 03f96e9..6f59dfc 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
unit_files = \
nfs-client.target \
\
- nfs-config.service \
nfs-mountd.service \
nfs-server.service \
nfs-utils.service \
diff --git a/systemd/README b/systemd/README
index bbd7790..1508ea7 100644
--- a/systemd/README
+++ b/systemd/README
@@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.

Distro specific commandline configuration can be provided by
installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
-This should write /run/sysconfig/nfs-utils based on configuration
-information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
-It is run once by nfs-config.service.
+It will be provided the environment variable SERVICE with the short
+name of the service being started, and should write
+/run/sysconfig/nfs-utils-${SERVICE} based on configuration
+information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
+is run just prior to starting each service.

rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
is present.
diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
index ddbf4e9..254b005 100644
--- a/systemd/nfs-blkmap.service
+++ b/systemd/nfs-blkmap.service
@@ -10,7 +10,9 @@ PartOf=nfs-utils.service
[Service]
Type=forking
PIDFile=/var/run/blkmapd.pid
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=blkmapd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS

[Install]
diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
index df3dd9d..f58a58d 100644
--- a/systemd/nfs-idmapd.service
+++ b/systemd/nfs-idmapd.service
@@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target

BindsTo=nfs-server.service

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=idmapd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
Type=forking
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
index 8a39f3e..5392429 100644
--- a/systemd/nfs-mountd.service
+++ b/systemd/nfs-mountd.service
@@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
After=network.target local-fs.target
BindsTo=nfs-server.service

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=mountd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
Type=forking
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
index 317e5d6..493e41e 100644
--- a/systemd/nfs-server.service
+++ b/systemd/nfs-server.service
@@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
# start/stop server before/after client
Before=remote-fs-pre.target

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=nfsd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd

Type=oneshot
RemainAfterExit=yes
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStartPre=/usr/sbin/exportfs -r
ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
ExecStop=/usr/sbin/rpc.nfsd 0
diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
index d4a3819..f945661 100644
--- a/systemd/rpc-gssd.service
+++ b/systemd/rpc-gssd.service
@@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab

PartOf=nfs-utils.service

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=gssd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-gssd

Type=forking
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
index 89ba36c..b33c92e 100644
--- a/systemd/rpc-statd-notify.service
+++ b/systemd/rpc-statd-notify.service
@@ -10,10 +10,9 @@ After=nfs-server.service

PartOf=nfs-utils.service

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=sm-notify"
+EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
Type=forking
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
index f16ea42..1af3bc8 100644
--- a/systemd/rpc-statd.service
+++ b/systemd/rpc-statd.service
@@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service

PartOf=nfs-utils.service

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=statd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-statd
Type=forking
PIDFile=/var/run/rpc.statd.pid
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
index 41177b6..ace6ec5 100644
--- a/systemd/rpc-svcgssd.service
+++ b/systemd/rpc-svcgssd.service
@@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
ConditionPathExists=/etc/krb5.keytab

-Wants=nfs-config.service
-After=nfs-config.service
-
[Service]
-EnvironmentFile=-/run/sysconfig/nfs-utils
+Environment="SERVICE=svcgssd"
+EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
Type=forking
+ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
--
1.7.1



2016-03-15 14:24:32

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

Also cc'ing Neil, as he added nfs-config originally.--b.

On Sat, Mar 12, 2016 at 07:05:48AM -0500, Benjamin Coddington wrote:
> The nfs-config service exists to translate distro-specific startup
> configuration into the command line arguments used by systemd to start nfs
> utilities. Unfortunately, it is not clear to most users that this service
> must be recycled every time startup configurations have been modified, as
> this requirement is a change for at least one distro.
>
> We can get rid of nfs-config by generating the startup arguments in an
> ExecStartPre option for each service that needs them. We'll also have to
> break out the EnvironmentFile into a separate file for each service to
> avoid races overwriting this file.
>
> A distro taking this change should also modify their
> /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> EnvironmentFile location for each service.
>
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> systemd/Makefile.am | 1 -
> systemd/README | 8 +++++---
> systemd/nfs-blkmap.service | 4 +++-
> systemd/nfs-idmapd.service | 7 +++----
> systemd/nfs-mountd.service | 7 +++----
> systemd/nfs-server.service | 7 +++----
> systemd/rpc-gssd.service | 7 +++----
> systemd/rpc-statd-notify.service | 7 +++----
> systemd/rpc-statd.service | 7 +++----
> systemd/rpc-svcgssd.service | 7 +++----
> 10 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> index 03f96e9..6f59dfc 100644
> --- a/systemd/Makefile.am
> +++ b/systemd/Makefile.am
> @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> unit_files = \
> nfs-client.target \
> \
> - nfs-config.service \
> nfs-mountd.service \
> nfs-server.service \
> nfs-utils.service \
> diff --git a/systemd/README b/systemd/README
> index bbd7790..1508ea7 100644
> --- a/systemd/README
> +++ b/systemd/README
> @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
>
> Distro specific commandline configuration can be provided by
> installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> -This should write /run/sysconfig/nfs-utils based on configuration
> -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> -It is run once by nfs-config.service.
> +It will be provided the environment variable SERVICE with the short
> +name of the service being started, and should write
> +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> +is run just prior to starting each service.
>
> rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> is present.
> diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> index ddbf4e9..254b005 100644
> --- a/systemd/nfs-blkmap.service
> +++ b/systemd/nfs-blkmap.service
> @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> [Service]
> Type=forking
> PIDFile=/var/run/blkmapd.pid
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=blkmapd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
>
> [Install]
> diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> index df3dd9d..f58a58d 100644
> --- a/systemd/nfs-idmapd.service
> +++ b/systemd/nfs-idmapd.service
> @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
>
> BindsTo=nfs-server.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=idmapd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> index 8a39f3e..5392429 100644
> --- a/systemd/nfs-mountd.service
> +++ b/systemd/nfs-mountd.service
> @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> After=network.target local-fs.target
> BindsTo=nfs-server.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=mountd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> index 317e5d6..493e41e 100644
> --- a/systemd/nfs-server.service
> +++ b/systemd/nfs-server.service
> @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> # start/stop server before/after client
> Before=remote-fs-pre.target
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=nfsd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
>
> Type=oneshot
> RemainAfterExit=yes
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStartPre=/usr/sbin/exportfs -r
> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> ExecStop=/usr/sbin/rpc.nfsd 0
> diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> index d4a3819..f945661 100644
> --- a/systemd/rpc-gssd.service
> +++ b/systemd/rpc-gssd.service
> @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=gssd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
>
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> index 89ba36c..b33c92e 100644
> --- a/systemd/rpc-statd-notify.service
> +++ b/systemd/rpc-statd-notify.service
> @@ -10,10 +10,9 @@ After=nfs-server.service
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=sm-notify"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> index f16ea42..1af3bc8 100644
> --- a/systemd/rpc-statd.service
> +++ b/systemd/rpc-statd.service
> @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=statd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> Type=forking
> PIDFile=/var/run/rpc.statd.pid
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> index 41177b6..ace6ec5 100644
> --- a/systemd/rpc-svcgssd.service
> +++ b/systemd/rpc-svcgssd.service
> @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> ConditionPathExists=/etc/krb5.keytab
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=svcgssd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-03-15 15:13:37

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Tue, 15 Mar 2016, J. Bruce Fields wrote:

> Also cc'ing Neil, as he added nfs-config originally.--b.

Ah, thanks Bruce! I should have looked up the original author.

For a point of reference on the work needed within a distro: here's the
smallest change required for Fedora's nfs-utils_env.sh:

--- /usr/lib/systemd/scripts/nfs-utils_env.sh 2016-03-15 11:08:34.641326549 -0400
+++ /usr/lib/systemd/scripts/nfs-utils_env.sh.orig 2016-03-15 11:07:10.974514047 -0400
@@ -67,4 +67,4 @@
echo SVCGSSDARGS=\"$RPCSVCGSSDARGS\"
echo BLKMAPDARGS=\"$BLKMAPDARGS\"
echo GSS_USE_PROXY=\"$GSS_USE_PROXY\"
-} > /run/sysconfig/nfs-utils
+} > /run/sysconfig/nfs-utils-${SERVICE}

This small change just generates the entire set of variables into a separate
file for each service, so it is quite dumb. More sensible would be to only
generate the required set of environment variables for each service.

Ben

> On Sat, Mar 12, 2016 at 07:05:48AM -0500, Benjamin Coddington wrote:
> > The nfs-config service exists to translate distro-specific startup
> > configuration into the command line arguments used by systemd to start nfs
> > utilities. Unfortunately, it is not clear to most users that this service
> > must be recycled every time startup configurations have been modified, as
> > this requirement is a change for at least one distro.
> >
> > We can get rid of nfs-config by generating the startup arguments in an
> > ExecStartPre option for each service that needs them. We'll also have to
> > break out the EnvironmentFile into a separate file for each service to
> > avoid races overwriting this file.
> >
> > A distro taking this change should also modify their
> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> > EnvironmentFile location for each service.
> >
> > Signed-off-by: Benjamin Coddington <[email protected]>
> > ---
> > systemd/Makefile.am | 1 -
> > systemd/README | 8 +++++---
> > systemd/nfs-blkmap.service | 4 +++-
> > systemd/nfs-idmapd.service | 7 +++----
> > systemd/nfs-mountd.service | 7 +++----
> > systemd/nfs-server.service | 7 +++----
> > systemd/rpc-gssd.service | 7 +++----
> > systemd/rpc-statd-notify.service | 7 +++----
> > systemd/rpc-statd.service | 7 +++----
> > systemd/rpc-svcgssd.service | 7 +++----
> > 10 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> > index 03f96e9..6f59dfc 100644
> > --- a/systemd/Makefile.am
> > +++ b/systemd/Makefile.am
> > @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> > unit_files = \
> > nfs-client.target \
> > \
> > - nfs-config.service \
> > nfs-mountd.service \
> > nfs-server.service \
> > nfs-utils.service \
> > diff --git a/systemd/README b/systemd/README
> > index bbd7790..1508ea7 100644
> > --- a/systemd/README
> > +++ b/systemd/README
> > @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
> >
> > Distro specific commandline configuration can be provided by
> > installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> > -This should write /run/sysconfig/nfs-utils based on configuration
> > -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> > -It is run once by nfs-config.service.
> > +It will be provided the environment variable SERVICE with the short
> > +name of the service being started, and should write
> > +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> > +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> > +is run just prior to starting each service.
> >
> > rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> > is present.
> > diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> > index ddbf4e9..254b005 100644
> > --- a/systemd/nfs-blkmap.service
> > +++ b/systemd/nfs-blkmap.service
> > @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> > [Service]
> > Type=forking
> > PIDFile=/var/run/blkmapd.pid
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=blkmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
> >
> > [Install]
> > diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> > index df3dd9d..f58a58d 100644
> > --- a/systemd/nfs-idmapd.service
> > +++ b/systemd/nfs-idmapd.service
> > @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> >
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=idmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> > diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> > index 8a39f3e..5392429 100644
> > --- a/systemd/nfs-mountd.service
> > +++ b/systemd/nfs-mountd.service
> > @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> > After=network.target local-fs.target
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=mountd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> > diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> > index 317e5d6..493e41e 100644
> > --- a/systemd/nfs-server.service
> > +++ b/systemd/nfs-server.service
> > @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> > # start/stop server before/after client
> > Before=remote-fs-pre.target
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=nfsd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
> >
> > Type=oneshot
> > RemainAfterExit=yes
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStartPre=/usr/sbin/exportfs -r
> > ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> > ExecStop=/usr/sbin/rpc.nfsd 0
> > diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> > index d4a3819..f945661 100644
> > --- a/systemd/rpc-gssd.service
> > +++ b/systemd/rpc-gssd.service
> > @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=gssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
> >
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> > diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> > index 89ba36c..b33c92e 100644
> > --- a/systemd/rpc-statd-notify.service
> > +++ b/systemd/rpc-statd-notify.service
> > @@ -10,10 +10,9 @@ After=nfs-server.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=sm-notify"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> > diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> > index f16ea42..1af3bc8 100644
> > --- a/systemd/rpc-statd.service
> > +++ b/systemd/rpc-statd.service
> > @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=statd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> > Type=forking
> > PIDFile=/var/run/rpc.statd.pid
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> > diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> > index 41177b6..ace6ec5 100644
> > --- a/systemd/rpc-svcgssd.service
> > +++ b/systemd/rpc-svcgssd.service
> > @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> > ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> > ConditionPathExists=/etc/krb5.keytab
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=svcgssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> > --
> > 1.7.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2016-03-15 20:59:45

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Sat, Mar 12 2016, Benjamin Coddington wrote:

> The nfs-config service exists to translate distro-specific startup
> configuration into the command line arguments used by systemd to start nfs
> utilities. Unfortunately, it is not clear to most users that this service
> must be recycled every time startup configurations have been modified, as
> this requirement is a change for at least one distro.
>
> We can get rid of nfs-config by generating the startup arguments in an
> ExecStartPre option for each service that needs them. We'll also have to
> break out the EnvironmentFile into a separate file for each service to
> avoid races overwriting this file.
>
> A distro taking this change should also modify their
> /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> EnvironmentFile location for each service.
>
> Signed-off-by: Benjamin Coddington <[email protected]>

I can't say I really like this, but it does make sense and solves a real
problem.
Maybe: I would never have written it myself, but I'm kind-a glad you did
:-)

Acked-by: NeilBrown <[email protected]>

Thanks,
NeilBrown


> ---
> systemd/Makefile.am | 1 -
> systemd/README | 8 +++++---
> systemd/nfs-blkmap.service | 4 +++-
> systemd/nfs-idmapd.service | 7 +++----
> systemd/nfs-mountd.service | 7 +++----
> systemd/nfs-server.service | 7 +++----
> systemd/rpc-gssd.service | 7 +++----
> systemd/rpc-statd-notify.service | 7 +++----
> systemd/rpc-statd.service | 7 +++----
> systemd/rpc-svcgssd.service | 7 +++----
> 10 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> index 03f96e9..6f59dfc 100644
> --- a/systemd/Makefile.am
> +++ b/systemd/Makefile.am
> @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> unit_files = \
> nfs-client.target \
> \
> - nfs-config.service \
> nfs-mountd.service \
> nfs-server.service \
> nfs-utils.service \
> diff --git a/systemd/README b/systemd/README
> index bbd7790..1508ea7 100644
> --- a/systemd/README
> +++ b/systemd/README
> @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
>
> Distro specific commandline configuration can be provided by
> installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> -This should write /run/sysconfig/nfs-utils based on configuration
> -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> -It is run once by nfs-config.service.
> +It will be provided the environment variable SERVICE with the short
> +name of the service being started, and should write
> +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> +is run just prior to starting each service.
>
> rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> is present.
> diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> index ddbf4e9..254b005 100644
> --- a/systemd/nfs-blkmap.service
> +++ b/systemd/nfs-blkmap.service
> @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> [Service]
> Type=forking
> PIDFile=/var/run/blkmapd.pid
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=blkmapd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
>
> [Install]
> diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> index df3dd9d..f58a58d 100644
> --- a/systemd/nfs-idmapd.service
> +++ b/systemd/nfs-idmapd.service
> @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
>
> BindsTo=nfs-server.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=idmapd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> index 8a39f3e..5392429 100644
> --- a/systemd/nfs-mountd.service
> +++ b/systemd/nfs-mountd.service
> @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> After=network.target local-fs.target
> BindsTo=nfs-server.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=mountd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> index 317e5d6..493e41e 100644
> --- a/systemd/nfs-server.service
> +++ b/systemd/nfs-server.service
> @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> # start/stop server before/after client
> Before=remote-fs-pre.target
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=nfsd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
>
> Type=oneshot
> RemainAfterExit=yes
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStartPre=/usr/sbin/exportfs -r
> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> ExecStop=/usr/sbin/rpc.nfsd 0
> diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> index d4a3819..f945661 100644
> --- a/systemd/rpc-gssd.service
> +++ b/systemd/rpc-gssd.service
> @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=gssd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
>
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> index 89ba36c..b33c92e 100644
> --- a/systemd/rpc-statd-notify.service
> +++ b/systemd/rpc-statd-notify.service
> @@ -10,10 +10,9 @@ After=nfs-server.service
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=sm-notify"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> index f16ea42..1af3bc8 100644
> --- a/systemd/rpc-statd.service
> +++ b/systemd/rpc-statd.service
> @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
>
> PartOf=nfs-utils.service
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=statd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> Type=forking
> PIDFile=/var/run/rpc.statd.pid
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> index 41177b6..ace6ec5 100644
> --- a/systemd/rpc-svcgssd.service
> +++ b/systemd/rpc-svcgssd.service
> @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> ConditionPathExists=/etc/krb5.keytab
>
> -Wants=nfs-config.service
> -After=nfs-config.service
> -
> [Service]
> -EnvironmentFile=-/run/sysconfig/nfs-utils
> +Environment="SERVICE=svcgssd"
> +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> Type=forking
> +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Attachments:
signature.asc (818.00 B)

2016-03-15 21:10:28

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Wed, 16 Mar 2016, NeilBrown wrote:

> On Sat, Mar 12 2016, Benjamin Coddington wrote:
>
> > The nfs-config service exists to translate distro-specific startup
> > configuration into the command line arguments used by systemd to start nfs
> > utilities. Unfortunately, it is not clear to most users that this service
> > must be recycled every time startup configurations have been modified, as
> > this requirement is a change for at least one distro.
> >
> > We can get rid of nfs-config by generating the startup arguments in an
> > ExecStartPre option for each service that needs them. We'll also have to
> > break out the EnvironmentFile into a separate file for each service to
> > avoid races overwriting this file.
> >
> > A distro taking this change should also modify their
> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> > EnvironmentFile location for each service.
> >
> > Signed-off-by: Benjamin Coddington <[email protected]>
>
> I can't say I really like this, but it does make sense and solves a real
> problem.
> Maybe: I would never have written it myself, but I'm kind-a glad you did
> :-)

Which bits are unlikeable? I am guessing that all these little intermediate
files are the issue; that is what I hate. I admit that it is a least-effort
approach to fixing the problem. What does the ideal fix look like to you?

Ben

>
> Acked-by: NeilBrown <[email protected]>
>
> Thanks,
> NeilBrown
>
>
> > ---
> > systemd/Makefile.am | 1 -
> > systemd/README | 8 +++++---
> > systemd/nfs-blkmap.service | 4 +++-
> > systemd/nfs-idmapd.service | 7 +++----
> > systemd/nfs-mountd.service | 7 +++----
> > systemd/nfs-server.service | 7 +++----
> > systemd/rpc-gssd.service | 7 +++----
> > systemd/rpc-statd-notify.service | 7 +++----
> > systemd/rpc-statd.service | 7 +++----
> > systemd/rpc-svcgssd.service | 7 +++----
> > 10 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> > index 03f96e9..6f59dfc 100644
> > --- a/systemd/Makefile.am
> > +++ b/systemd/Makefile.am
> > @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> > unit_files = \
> > nfs-client.target \
> > \
> > - nfs-config.service \
> > nfs-mountd.service \
> > nfs-server.service \
> > nfs-utils.service \
> > diff --git a/systemd/README b/systemd/README
> > index bbd7790..1508ea7 100644
> > --- a/systemd/README
> > +++ b/systemd/README
> > @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
> >
> > Distro specific commandline configuration can be provided by
> > installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> > -This should write /run/sysconfig/nfs-utils based on configuration
> > -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> > -It is run once by nfs-config.service.
> > +It will be provided the environment variable SERVICE with the short
> > +name of the service being started, and should write
> > +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> > +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> > +is run just prior to starting each service.
> >
> > rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> > is present.
> > diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> > index ddbf4e9..254b005 100644
> > --- a/systemd/nfs-blkmap.service
> > +++ b/systemd/nfs-blkmap.service
> > @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> > [Service]
> > Type=forking
> > PIDFile=/var/run/blkmapd.pid
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=blkmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
> >
> > [Install]
> > diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> > index df3dd9d..f58a58d 100644
> > --- a/systemd/nfs-idmapd.service
> > +++ b/systemd/nfs-idmapd.service
> > @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> >
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=idmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> > diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> > index 8a39f3e..5392429 100644
> > --- a/systemd/nfs-mountd.service
> > +++ b/systemd/nfs-mountd.service
> > @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> > After=network.target local-fs.target
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=mountd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> > diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> > index 317e5d6..493e41e 100644
> > --- a/systemd/nfs-server.service
> > +++ b/systemd/nfs-server.service
> > @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> > # start/stop server before/after client
> > Before=remote-fs-pre.target
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=nfsd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
> >
> > Type=oneshot
> > RemainAfterExit=yes
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStartPre=/usr/sbin/exportfs -r
> > ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> > ExecStop=/usr/sbin/rpc.nfsd 0
> > diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> > index d4a3819..f945661 100644
> > --- a/systemd/rpc-gssd.service
> > +++ b/systemd/rpc-gssd.service
> > @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=gssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
> >
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> > diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> > index 89ba36c..b33c92e 100644
> > --- a/systemd/rpc-statd-notify.service
> > +++ b/systemd/rpc-statd-notify.service
> > @@ -10,10 +10,9 @@ After=nfs-server.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=sm-notify"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> > diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> > index f16ea42..1af3bc8 100644
> > --- a/systemd/rpc-statd.service
> > +++ b/systemd/rpc-statd.service
> > @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=statd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> > Type=forking
> > PIDFile=/var/run/rpc.statd.pid
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> > diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> > index 41177b6..ace6ec5 100644
> > --- a/systemd/rpc-svcgssd.service
> > +++ b/systemd/rpc-svcgssd.service
> > @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> > ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> > ConditionPathExists=/etc/krb5.keytab
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=svcgssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> > --
> > 1.7.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2016-03-15 21:52:02

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Wed, Mar 16 2016, Benjamin Coddington wrote:

> On Wed, 16 Mar 2016, NeilBrown wrote:
>
>> On Sat, Mar 12 2016, Benjamin Coddington wrote:
>>
>> > The nfs-config service exists to translate distro-specific startup
>> > configuration into the command line arguments used by systemd to start nfs
>> > utilities. Unfortunately, it is not clear to most users that this service
>> > must be recycled every time startup configurations have been modified, as
>> > this requirement is a change for at least one distro.
>> >
>> > We can get rid of nfs-config by generating the startup arguments in an
>> > ExecStartPre option for each service that needs them. We'll also have to
>> > break out the EnvironmentFile into a separate file for each service to
>> > avoid races overwriting this file.
>> >
>> > A distro taking this change should also modify their
>> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
>> > EnvironmentFile location for each service.
>> >
>> > Signed-off-by: Benjamin Coddington <[email protected]>
>>
>> I can't say I really like this, but it does make sense and solves a real
>> problem.
>> Maybe: I would never have written it myself, but I'm kind-a glad you did
>> :-)
>
> Which bits are unlikeable? I am guessing that all these little intermediate
> files are the issue; that is what I hate. I admit that it is a least-effort
> approach to fixing the problem. What does the ideal fix look like to you?

I don't like having multiple intermediate config files that are all
identical, and I don't like having to run the config script every time
any daemon is (re)started.

I'm not sure what "ideal" would be.

One improvement would be if systemd could be given an executable in
place of an environment file with the implication that it would run the
executable and read environment variables from the stdout.
That would remove the multiple config files.
It would still mean the script is run every time, but I could be
convinced that isn't so bad.

An alternate idea is that maybe systemd could have a directive so that
in any transaction were "this" unit is started, "that" unit will also be
started. Then we could replace "Wants=nfs-config.server" with
"Prepare=nfs-config.service" so if systemd ever needed to start any
nfs-related service, it would make preparations by first running
nfs-config.service - only once per transaction.

But unless you want to hack on systemd, I think what you have is good
enough.


.... hmmm. I wonder what would happen if we just removed
"RemainAfterExit=y" from nfs-config.service. Would it cause things to
fail, or would it cause nfs-config.service to be run every time.

Hey, that looks like it works! Can you double check for me?
i.e. revert your patch, remove "RemainAfterExit=y" from
nfs-config.service, and then see if nfs-config.service gets run any time
any nfs server is started.

Thanks,
NeilBrown


Attachments:
signature.asc (818.00 B)

2016-03-15 23:33:37

by Malahal Naineni

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

Ben, if I understand, EnvironmentFile must be sourced after running
ExecStartPre for this method to work, correct? I was planning on doing
something like this for ganesha project but I couldn't find enough
documentation that says this.

Regards, Malahal.

J. Bruce Fields [[email protected]] wrote:
> Also cc'ing Neil, as he added nfs-config originally.--b.
>
> On Sat, Mar 12, 2016 at 07:05:48AM -0500, Benjamin Coddington wrote:
> > The nfs-config service exists to translate distro-specific startup
> > configuration into the command line arguments used by systemd to start nfs
> > utilities. Unfortunately, it is not clear to most users that this service
> > must be recycled every time startup configurations have been modified, as
> > this requirement is a change for at least one distro.
> >
> > We can get rid of nfs-config by generating the startup arguments in an
> > ExecStartPre option for each service that needs them. We'll also have to
> > break out the EnvironmentFile into a separate file for each service to
> > avoid races overwriting this file.
> >
> > A distro taking this change should also modify their
> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> > EnvironmentFile location for each service.
> >
> > Signed-off-by: Benjamin Coddington <[email protected]>
> > ---
> > systemd/Makefile.am | 1 -
> > systemd/README | 8 +++++---
> > systemd/nfs-blkmap.service | 4 +++-
> > systemd/nfs-idmapd.service | 7 +++----
> > systemd/nfs-mountd.service | 7 +++----
> > systemd/nfs-server.service | 7 +++----
> > systemd/rpc-gssd.service | 7 +++----
> > systemd/rpc-statd-notify.service | 7 +++----
> > systemd/rpc-statd.service | 7 +++----
> > systemd/rpc-svcgssd.service | 7 +++----
> > 10 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> > index 03f96e9..6f59dfc 100644
> > --- a/systemd/Makefile.am
> > +++ b/systemd/Makefile.am
> > @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> > unit_files = \
> > nfs-client.target \
> > \
> > - nfs-config.service \
> > nfs-mountd.service \
> > nfs-server.service \
> > nfs-utils.service \
> > diff --git a/systemd/README b/systemd/README
> > index bbd7790..1508ea7 100644
> > --- a/systemd/README
> > +++ b/systemd/README
> > @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
> >
> > Distro specific commandline configuration can be provided by
> > installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> > -This should write /run/sysconfig/nfs-utils based on configuration
> > -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> > -It is run once by nfs-config.service.
> > +It will be provided the environment variable SERVICE with the short
> > +name of the service being started, and should write
> > +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> > +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> > +is run just prior to starting each service.
> >
> > rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> > is present.
> > diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> > index ddbf4e9..254b005 100644
> > --- a/systemd/nfs-blkmap.service
> > +++ b/systemd/nfs-blkmap.service
> > @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> > [Service]
> > Type=forking
> > PIDFile=/var/run/blkmapd.pid
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=blkmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
> >
> > [Install]
> > diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> > index df3dd9d..f58a58d 100644
> > --- a/systemd/nfs-idmapd.service
> > +++ b/systemd/nfs-idmapd.service
> > @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> >
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=idmapd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> > diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> > index 8a39f3e..5392429 100644
> > --- a/systemd/nfs-mountd.service
> > +++ b/systemd/nfs-mountd.service
> > @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> > After=network.target local-fs.target
> > BindsTo=nfs-server.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=mountd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> > diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> > index 317e5d6..493e41e 100644
> > --- a/systemd/nfs-server.service
> > +++ b/systemd/nfs-server.service
> > @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> > # start/stop server before/after client
> > Before=remote-fs-pre.target
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=nfsd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
> >
> > Type=oneshot
> > RemainAfterExit=yes
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStartPre=/usr/sbin/exportfs -r
> > ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> > ExecStop=/usr/sbin/rpc.nfsd 0
> > diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> > index d4a3819..f945661 100644
> > --- a/systemd/rpc-gssd.service
> > +++ b/systemd/rpc-gssd.service
> > @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=gssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
> >
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> > diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> > index 89ba36c..b33c92e 100644
> > --- a/systemd/rpc-statd-notify.service
> > +++ b/systemd/rpc-statd-notify.service
> > @@ -10,10 +10,9 @@ After=nfs-server.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=sm-notify"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> > diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> > index f16ea42..1af3bc8 100644
> > --- a/systemd/rpc-statd.service
> > +++ b/systemd/rpc-statd.service
> > @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
> >
> > PartOf=nfs-utils.service
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=statd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> > Type=forking
> > PIDFile=/var/run/rpc.statd.pid
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> > diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> > index 41177b6..ace6ec5 100644
> > --- a/systemd/rpc-svcgssd.service
> > +++ b/systemd/rpc-svcgssd.service
> > @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> > ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> > ConditionPathExists=/etc/krb5.keytab
> >
> > -Wants=nfs-config.service
> > -After=nfs-config.service
> > -
> > [Service]
> > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > +Environment="SERVICE=svcgssd"
> > +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> > Type=forking
> > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> > --
> > 1.7.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2016-03-16 00:16:29

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

> On Wed, Mar 16 2016, Benjamin Coddington wrote:
>
> > On Wed, 16 Mar 2016, NeilBrown wrote:
> >
> >> On Sat, Mar 12 2016, Benjamin Coddington wrote:
> >>
> >> > The nfs-config service exists to translate distro-specific startup
> >> > configuration into the command line arguments used by systemd to start nfs
> >> > utilities. Unfortunately, it is not clear to most users that this service
> >> > must be recycled every time startup configurations have been modified, as
> >> > this requirement is a change for at least one distro.
> >> >
> >> > We can get rid of nfs-config by generating the startup arguments in an
> >> > ExecStartPre option for each service that needs them. We'll also have to
> >> > break out the EnvironmentFile into a separate file for each service to
> >> > avoid races overwriting this file.
> >> >
> >> > A distro taking this change should also modify their
> >> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> >> > EnvironmentFile location for each service.
> >> >
> >> > Signed-off-by: Benjamin Coddington <[email protected]>
> >>
> >> I can't say I really like this, but it does make sense and solves a real
> >> problem.
> >> Maybe: I would never have written it myself, but I'm kind-a glad you did
> >> :-)
> >
> > Which bits are unlikeable? I am guessing that all these little intermediate
> > files are the issue; that is what I hate. I admit that it is a least-effort
> > approach to fixing the problem. What does the ideal fix look like to you?
>
> I don't like having multiple intermediate config files that are all
> identical, and I don't like having to run the config script every time
> any daemon is (re)started.
>
> I'm not sure what "ideal" would be.
>
> One improvement would be if systemd could be given an executable in
> place of an environment file with the implication that it would run the
> executable and read environment variables from the stdout.
> That would remove the multiple config files.
> It would still mean the script is run every time, but I could be
> convinced that isn't so bad.
>
> An alternate idea is that maybe systemd could have a directive so that
> in any transaction were "this" unit is started, "that" unit will also be
> started. Then we could replace "Wants=nfs-config.server" with
> "Prepare=nfs-config.service" so if systemd ever needed to start any
> nfs-related service, it would make preparations by first running
> nfs-config.service - only once per transaction.
>
> But unless you want to hack on systemd, I think what you have is good
> enough.
>
>
> .... hmmm. I wonder what would happen if we just removed
> "RemainAfterExit=y" from nfs-config.service. Would it cause things to
> fail, or would it cause nfs-config.service to be run every time.
>
> Hey, that looks like it works! Can you double check for me?
> i.e. revert your patch, remove "RemainAfterExit=y" from
> nfs-config.service, and then see if nfs-config.service gets run any time
> any nfs server is started.

Yep.. that appears to work just fine. Seems like a simpler solution than
this change. It appears to me ( from systemd.service(5) ) that systemd's
state transitions for Type=Oneshot will avoid races that might overwrite the
environment file. Nice find!

Ben

2016-03-16 00:19:37

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Wed, Mar 16 2016, Malahal Naineni wrote:

> Ben, if I understand, EnvironmentFile must be sourced after running
> ExecStartPre for this method to work, correct? I was planning on doing
> something like this for ganesha project but I couldn't find enough
> documentation that says this.

The only documentation I found was the source code :-)

NeilBrown


Attachments:
signature.asc (818.00 B)

2016-03-16 00:23:46

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Tue, 15 Mar 2016, Malahal Naineni wrote:

> Ben, if I understand, EnvironmentFile must be sourced after running
> ExecStartPre for this method to work, correct? I was planning on doing
> something like this for ganesha project but I couldn't find enough
> documentation that says this.
>
> Regards, Malahal.

Right, the EnvironmentFile would need to be sourced after running
ExecStartPre. Regarding EnvironmentFile, systemd.service(5) states:

The files listed with this directive will be read shortly before the process
is executed (more specifically, after all processes from a previous unit
state terminated. This means you can generate these files in one unit state,
and read it with this option in the next).

I suspect that the EnvironmentFile is sourced before every exec, including
each ExecStartPre.

It looks now like we'll perhaps not use this method after all, as Neil Brown
has found that making the nfs-config.service into a RemainAfterExit=no, and
Type=oneshot means it will get re-started before each service that has it
listed in Wants=. That's nicer because it means we can change less, use
less files, and systemd (I expect) handles the state transitions such that
it isn't run concurrently.

Ben

>
> J. Bruce Fields [[email protected]] wrote:
> > Also cc'ing Neil, as he added nfs-config originally.--b.
> >
> > On Sat, Mar 12, 2016 at 07:05:48AM -0500, Benjamin Coddington wrote:
> > > The nfs-config service exists to translate distro-specific startup
> > > configuration into the command line arguments used by systemd to start nfs
> > > utilities. Unfortunately, it is not clear to most users that this service
> > > must be recycled every time startup configurations have been modified, as
> > > this requirement is a change for at least one distro.
> > >
> > > We can get rid of nfs-config by generating the startup arguments in an
> > > ExecStartPre option for each service that needs them. We'll also have to
> > > break out the EnvironmentFile into a separate file for each service to
> > > avoid races overwriting this file.
> > >
> > > A distro taking this change should also modify their
> > > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
> > > EnvironmentFile location for each service.
> > >
> > > Signed-off-by: Benjamin Coddington <[email protected]>
> > > ---
> > > systemd/Makefile.am | 1 -
> > > systemd/README | 8 +++++---
> > > systemd/nfs-blkmap.service | 4 +++-
> > > systemd/nfs-idmapd.service | 7 +++----
> > > systemd/nfs-mountd.service | 7 +++----
> > > systemd/nfs-server.service | 7 +++----
> > > systemd/rpc-gssd.service | 7 +++----
> > > systemd/rpc-statd-notify.service | 7 +++----
> > > systemd/rpc-statd.service | 7 +++----
> > > systemd/rpc-svcgssd.service | 7 +++----
> > > 10 files changed, 29 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> > > index 03f96e9..6f59dfc 100644
> > > --- a/systemd/Makefile.am
> > > +++ b/systemd/Makefile.am
> > > @@ -5,7 +5,6 @@ MAINTAINERCLEANFILES = Makefile.in
> > > unit_files = \
> > > nfs-client.target \
> > > \
> > > - nfs-config.service \
> > > nfs-mountd.service \
> > > nfs-server.service \
> > > nfs-utils.service \
> > > diff --git a/systemd/README b/systemd/README
> > > index bbd7790..1508ea7 100644
> > > --- a/systemd/README
> > > +++ b/systemd/README
> > > @@ -54,9 +54,11 @@ client and systemd cannot specify is two-pronged reverse dependency.
> > >
> > > Distro specific commandline configuration can be provided by
> > > installing a script /usr/lib/systemd/scripts/nfs-utils_env.sh
> > > -This should write /run/sysconfig/nfs-utils based on configuration
> > > -information such as in /etc/sysconfig/nfs or /etc/defaults/nfs.
> > > -It is run once by nfs-config.service.
> > > +It will be provided the environment variable SERVICE with the short
> > > +name of the service being started, and should write
> > > +/run/sysconfig/nfs-utils-${SERVICE} based on configuration
> > > +information such as in /etc/sysconfig/nfs or /etc/defaults/nfs. It
> > > +is run just prior to starting each service.
> > >
> > > rpc.gssd and rpc.svcgssd are assumed to be needed if /etc/krb5.keytab
> > > is present.
> > > diff --git a/systemd/nfs-blkmap.service b/systemd/nfs-blkmap.service
> > > index ddbf4e9..254b005 100644
> > > --- a/systemd/nfs-blkmap.service
> > > +++ b/systemd/nfs-blkmap.service
> > > @@ -10,7 +10,9 @@ PartOf=nfs-utils.service
> > > [Service]
> > > Type=forking
> > > PIDFile=/var/run/blkmapd.pid
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=blkmapd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-blkmapd
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/blkmapd $BLKMAPDARGS
> > >
> > > [Install]
> > > diff --git a/systemd/nfs-idmapd.service b/systemd/nfs-idmapd.service
> > > index df3dd9d..f58a58d 100644
> > > --- a/systemd/nfs-idmapd.service
> > > +++ b/systemd/nfs-idmapd.service
> > > @@ -6,10 +6,9 @@ After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> > >
> > > BindsTo=nfs-server.service
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=idmapd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-idmapd
> > > Type=forking
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> > > diff --git a/systemd/nfs-mountd.service b/systemd/nfs-mountd.service
> > > index 8a39f3e..5392429 100644
> > > --- a/systemd/nfs-mountd.service
> > > +++ b/systemd/nfs-mountd.service
> > > @@ -6,10 +6,9 @@ After=proc-fs-nfsd.mount
> > > After=network.target local-fs.target
> > > BindsTo=nfs-server.service
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=mountd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-mountd
> > > Type=forking
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/rpc.mountd $RPCMOUNTDARGS
> > > diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> > > index 317e5d6..493e41e 100644
> > > --- a/systemd/nfs-server.service
> > > +++ b/systemd/nfs-server.service
> > > @@ -18,14 +18,13 @@ After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
> > > # start/stop server before/after client
> > > Before=remote-fs-pre.target
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=nfsd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-nfsd
> > >
> > > Type=oneshot
> > > RemainAfterExit=yes
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStartPre=/usr/sbin/exportfs -r
> > > ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS
> > > ExecStop=/usr/sbin/rpc.nfsd 0
> > > diff --git a/systemd/rpc-gssd.service b/systemd/rpc-gssd.service
> > > index d4a3819..f945661 100644
> > > --- a/systemd/rpc-gssd.service
> > > +++ b/systemd/rpc-gssd.service
> > > @@ -9,11 +9,10 @@ ConditionPathExists=/etc/krb5.keytab
> > >
> > > PartOf=nfs-utils.service
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=gssd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-gssd
> > >
> > > Type=forking
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/rpc.gssd $GSSDARGS
> > > diff --git a/systemd/rpc-statd-notify.service b/systemd/rpc-statd-notify.service
> > > index 89ba36c..b33c92e 100644
> > > --- a/systemd/rpc-statd-notify.service
> > > +++ b/systemd/rpc-statd-notify.service
> > > @@ -10,10 +10,9 @@ After=nfs-server.service
> > >
> > > PartOf=nfs-utils.service
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=sm-notify"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-sm-notify
> > > Type=forking
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=-/usr/sbin/sm-notify $SMNOTIFYARGS
> > > diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> > > index f16ea42..1af3bc8 100644
> > > --- a/systemd/rpc-statd.service
> > > +++ b/systemd/rpc-statd.service
> > > @@ -7,11 +7,10 @@ After=network.target nss-lookup.target rpcbind.service
> > >
> > > PartOf=nfs-utils.service
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=statd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-statd
> > > Type=forking
> > > PIDFile=/var/run/rpc.statd.pid
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/rpc.statd --no-notify $STATDARGS
> > > diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> > > index 41177b6..ace6ec5 100644
> > > --- a/systemd/rpc-svcgssd.service
> > > +++ b/systemd/rpc-svcgssd.service
> > > @@ -11,10 +11,9 @@ ConditionPathExists=|!/run/gssproxy.pid
> > > ConditionPathExists=|!/proc/net/rpc/use-gss-proxy
> > > ConditionPathExists=/etc/krb5.keytab
> > >
> > > -Wants=nfs-config.service
> > > -After=nfs-config.service
> > > -
> > > [Service]
> > > -EnvironmentFile=-/run/sysconfig/nfs-utils
> > > +Environment="SERVICE=svcgssd"
> > > +EnvironmentFile=-/run/sysconfig/nfs-utils-svcgssd
> > > Type=forking
> > > +ExecStartPre=-/usr/lib/systemd/scripts/nfs-utils_env.sh
> > > ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS
> > > --
> > > 1.7.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>

2016-03-16 01:35:30

by NeilBrown

[permalink] [raw]
Subject: Re: [PATCH nfs-utils] systemd: remove the nfs-config service

On Wed, Mar 16 2016, Benjamin Coddington wrote:

>> On Wed, Mar 16 2016, Benjamin Coddington wrote:
>>
>> > On Wed, 16 Mar 2016, NeilBrown wrote:
>> >
>> >> On Sat, Mar 12 2016, Benjamin Coddington wrote:
>> >>
>> >> > The nfs-config service exists to translate distro-specific startup
>> >> > configuration into the command line arguments used by systemd to start nfs
>> >> > utilities. Unfortunately, it is not clear to most users that this service
>> >> > must be recycled every time startup configurations have been modified, as
>> >> > this requirement is a change for at least one distro.
>> >> >
>> >> > We can get rid of nfs-config by generating the startup arguments in an
>> >> > ExecStartPre option for each service that needs them. We'll also have to
>> >> > break out the EnvironmentFile into a separate file for each service to
>> >> > avoid races overwriting this file.
>> >> >
>> >> > A distro taking this change should also modify their
>> >> > /usr/lib/systemd/scripts/nfs-utils_env.sh script to include the new
>> >> > EnvironmentFile location for each service.
>> >> >
>> >> > Signed-off-by: Benjamin Coddington <[email protected]>
>> >>
>> >> I can't say I really like this, but it does make sense and solves a real
>> >> problem.
>> >> Maybe: I would never have written it myself, but I'm kind-a glad you did
>> >> :-)
>> >
>> > Which bits are unlikeable? I am guessing that all these little intermediate
>> > files are the issue; that is what I hate. I admit that it is a least-effort
>> > approach to fixing the problem. What does the ideal fix look like to you?
>>
>> I don't like having multiple intermediate config files that are all
>> identical, and I don't like having to run the config script every time
>> any daemon is (re)started.
>>
>> I'm not sure what "ideal" would be.
>>
>> One improvement would be if systemd could be given an executable in
>> place of an environment file with the implication that it would run the
>> executable and read environment variables from the stdout.
>> That would remove the multiple config files.
>> It would still mean the script is run every time, but I could be
>> convinced that isn't so bad.
>>
>> An alternate idea is that maybe systemd could have a directive so that
>> in any transaction were "this" unit is started, "that" unit will also be
>> started. Then we could replace "Wants=nfs-config.server" with
>> "Prepare=nfs-config.service" so if systemd ever needed to start any
>> nfs-related service, it would make preparations by first running
>> nfs-config.service - only once per transaction.
>>
>> But unless you want to hack on systemd, I think what you have is good
>> enough.
>>
>>
>> .... hmmm. I wonder what would happen if we just removed
>> "RemainAfterExit=y" from nfs-config.service. Would it cause things to
>> fail, or would it cause nfs-config.service to be run every time.
>>
>> Hey, that looks like it works! Can you double check for me?
>> i.e. revert your patch, remove "RemainAfterExit=y" from
>> nfs-config.service, and then see if nfs-config.service gets run any time
>> any nfs server is started.
>
> Yep.. that appears to work just fine. Seems like a simpler solution than
> this change. It appears to me ( from systemd.service(5) ) that systemd's
> state transitions for Type=Oneshot will avoid races that might overwrite the
> environment file. Nice find!
>

Great. Thanks for testing and thanks for prodding me to explain
myself. Very happy with the outcome!

I've posted a patch to Steve.

NeilBrown


Attachments:
signature.asc (818.00 B)