Re: [RFC nfs-utils PATCH 0/2] add systemd generator for the rpc_pipefs mountpoint

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 8515 bytes --]

On Mon, Apr 03 2017, Scott Mayhew wrote:

> On Mon, 03 Apr 2017, NeilBrown wrote:
>
>> On Fri, Mar 31 2017, Scott Mayhew wrote:
>> 
>> > These patches aim to make it a little easier to change the mountpoint.
>> > Right now if you change the pipefs-directory in /etc/nfs.conf, you still
>> > need to manually override the dependencies in the systemd unit files in
>> > order for the change to actually work.  
>> >
>> > The first patch moves rpc.idmapd's (mostly) undocumented
>> > pipefs-directory from /etc/idmapd.conf to /etc/nfs.conf, which rpc.gssd
>> > already can use for it's pipefs-directory configuration.
>> >
>> > The second patch adds a systemd generator that reads the
>> > pipefs-directory configurations from /etc/nfs.conf, and if they differ
>> > from the default it will automatically 1) create a systemd mount unit
>> > file for the pipefs mountpoint and 2) it will create a drop-in
>> > configuration file to override the Requires= and After= directives for
>> > that service.
>> >
>> > I did run into a bit of a snag though.  Depsite overriding the
>> > dependencies for both idmapd and gssd, I wind up with two pipefs
>> > filesystems mounted:
>> >
>> > [root@coeurl ~]# grep pipefs /proc/mounts
>> > sunrpc /var/lib/nfs/rpc_pipefs rpc_pipefs rw,relatime 0 0
>> > sunrpc /run/rpc_pipefs rpc_pipefs rw,relatime 0 0
>> >
>> > systemd still shows the dependency on the default pipefs mountpoint:
>> >
>> > [root@coeurl ~]# systemctl list-dependencies --before var-lib-nfs-rpc_pipefs.mount
>> > var-lib-nfs-rpc_pipefs.mount
>> > ● ├─nfs-idmapd.service
>> > ● └─rpc-gssd.service
>> >
>> > as well as the new one:
>> >
>> > [root@coeurl ~]# systemctl list-dependencies --before run-rpc_pipefs.mount
>> > run-rpc_pipefs.mount
>> > ● ├─nfs-idmapd.service
>> > ● └─rpc-gssd.service
>> >
>> > The drop-in configs to override the pipefs mountpoint look correct.  I'm
>> > clearing both Requires= and After= before setting them:
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/nfs-idmapd.service.d/10-pipefs.conf 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Requires=
>> > Requires=run-rpc_pipefs.mount
>> > After=
>> > After=run-rpc_pipefs.mount local-fs.target
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/rpc-gssd.service.d/10-pipefs.conf 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Requires=
>> > Requires=run-rpc_pipefs.mount
>> > After=
>> > After=run-rpc_pipefs.mount
>> >
>> > The generated mount unit file also looks correct:
>> >
>> > [root@coeurl ~]# cat /run/systemd/generator/run-rpc_pipefs.mount 
>> > # Automatically generated by rpc-pipefs-generator
>> >
>> > [Unit]
>> > Description=RPC Pipe File System
>> > DefaultDependencies=no
>> > After=systemd-tmpfiles-setup.service
>> > Conflicts=umount.target
>> >
>> > [Mount]
>> > What=sunrpc
>> > Where=/run/rpc_pipefs
>> > Type=rpc_pipefs
>> >
>> > systemd shows that the drop-in config was picked up:
>> >
>> > [root@coeurl ~]# systemctl status nfs-idmapd
>> > ● nfs-idmapd.service - NFSv4 ID-name mapping service
>> >    Loaded: loaded (/usr/lib/systemd/system/nfs-idmapd.service; static; vendor preset: disabled)
>> >   Drop-In: /run/systemd/generator/nfs-idmapd.service.d
>> >            └─10-pipefs.conf
>> >    Active: active (running) since Fri 2017-03-31 16:54:24 EDT; 5min ago
>> >   Process: 27831 ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS (code=exited, status=0/SUCCESS)
>> >  Main PID: 27832 (rpc.idmapd)
>> >     Tasks: 1 (limit: 4915)
>> >    CGroup: /system.slice/nfs-idmapd.service
>> >            └─27832 /usr/sbin/rpc.idmapd
>> >
>> > and lsof shows that the correct mountpoint is being used:
>> >
>> > [root@coeurl ~]# lsof -p 27832 2>/dev/null | grep pipefs
>> > rpc.idmap 27832 root   10r      DIR               0,42        0        103 /run/rpc_pipefs/nfs
>> >
>> > The same for gssd:
>> >
>> > [root@coeurl ~]# systemctl status rpc-gssd
>> > ● rpc-gssd.service - RPC security service for NFS client and server
>> >    Loaded: loaded (/usr/lib/systemd/system/rpc-gssd.service; static; vendor preset: disabled)
>> >   Drop-In: /run/systemd/generator/rpc-gssd.service.d
>> >            └─10-pipefs.conf
>> >    Active: active (running) since Fri 2017-03-31 16:54:29 EDT; 6min ago
>> >   Process: 27839 ExecStart=/usr/sbin/rpc.gssd $RPCGSSDARGS (code=exited, status=0/SUCCESS)
>> >  Main PID: 27840 (rpc.gssd)
>> >     Tasks: 1 (limit: 4915)
>> >    CGroup: /system.slice/rpc-gssd.service
>> >            └─27840 /usr/sbin/rpc.gssd
>> >
>> > [root@coeurl ~]# lsof -p 27840 2>/dev/null | grep pipefs
>> > rpc.gssd 27840 root  cwd       DIR               0,42        0   24637 /run/rpc_pipefs
>> > rpc.gssd 27840 root    7r      DIR               0,42        0   24637 /run/rpc_pipefs
>> > rpc.gssd 27840 root   11u     FIFO               0,42      0t0     112 /run/rpc_pipefs/gssd/clntXX/gssd
>> >
>> > So it looks like systemd is using both sets of dependencies, even though
>> > the programs themselves are only looking for what's specified in
>> > /etc/nfs.conf.  I'm not sure what to do about that.  Maybe remove the
>> > var-lib-nfs-rpc_pipefs.mount unit as well as the dependencies in the
>> > nfs-idmapd.service and rpc-gssd.service files, and have the generator
>> > create those automatically as well?
>> >
>> 
>> Towards the end of the systemd.unit man page is the text:
>> 
>>       Note that dependencies (After=, etc.) cannot be reset to an empty list,
>>        so dependencies can only be added in drop-ins. If you want to remove
>>        dependencies, you have to override the entire unit.
>> 
>> 
>> which is consistent with what you discovered.
>
> Doh, that's what I get for not reading all the way to the end.
>
>> 
>> Maybe create a "rpc_pipefs.target" which
>>   Requires=var-lib-nfs-rpc_pipefs.mount
>>   After=var-lib-nfs-rpc_pipefs.mount
>> and have all the other unit files specified their dependencies
>> against this target.
>> 
>> Then your generator would conditionally create a new "rpc_pipefs.target"
>> and matching foo.mount.  The new .target would depend in the foo.mount,
>> and the service files would already depend on that.
>> 
>> Might work.
>
> That looks like it works, but what should happen then if programs
> were to intentionally specify two different values for the pipefs-directory
> in nfs.conf?  Or is that something that wouldn't make sense to do and
> should be prevented?  I can't think of a reason why you'd want more than
> one rpc_pipefs filesystem mounted...

I agree.  I cannot think of any good reason.
I'm not sure it would be such a bad idea to fix a mount point at compile
time, but not that we have run-time configuration it is safe to stick
with it.

>
> Maybe it would make sense to create a 'global' section and have the
> programs all look at that instead of looking in their specific config
> sections... or maybe have the program check global section and then a
> program-specific section, but include a big fat warning that if you
> specify it in the program-specific section then you need to override all
> of them with the same value.

I support a "global" section of rpc-pipefs mount point.
I probably should have put pipefs-directory in a "global" section to
start with.  I don't recall why I didn't.

Thanks,
NeilBrown


>
> -Scott
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>> > -Scott
>> >
>> > Scott Mayhew (2):
>> >   idmapd: move the pipefs-directory config option to nfs.conf
>> >   systemd: add a generator for the rpc_pipefs mountpoint
>> >
>> >  .gitignore                     |   1 +
>> >  nfs.conf                       |   3 +
>> >  systemd/Makefile.am            |   4 +-
>> >  systemd/nfs.conf.man           |   9 ++
>> >  systemd/rpc-pipefs-generator.c | 256 +++++++++++++++++++++++++++++++++++++++++
>> >  systemd/rpc-svcgssd.service    |   3 +-
>> >  utils/idmapd/idmapd.c          |  35 +++---
>> >  utils/idmapd/idmapd.man        |  19 ++-
>> >  8 files changed, 305 insertions(+), 25 deletions(-)
>> >  create mode 100644 systemd/rpc-pipefs-generator.c
>> >
>> > -- 
>> > 2.9.3
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]