Re: [PATCH v2 4/4] nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf value

[Chuck Lever <chuck.lever@oracle.com>]

> On Jul 20, 2020, at 1:54 PM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> Hello,
> 
> On 7/19/20 3:57 PM, Alice Mitchell wrote:
>> Hi Chuck,
>> I must have missed the discussion on Trond's work sorry, and I agree
>> that having it fixed in a way that is both automatic and transparent to
>> the user is far preferable to the solution I have posted. Do we have
>> any timeline on this yet ?
> I too did missed  the discussion... Chuck or Trond can you give us more 
> context on how this is going to work automatically and transparent?
> Is there a thread you can point us to?

https://lore.kernel.org/linux-nfs/20190611180832.119488-1-trond.myklebust@hammerspace.com/


>> My proposed solution would therefore be a stop-gap if required, as it
>> does not force any specific solution upon the system and merely adds a
>> few small features in order to assist the administrator if they choose
>> to make use of the existing kernel module option, in a way which would
>> preserve the idea of centralised configuration.
> I think I agree with Chuck... Once we add something to a 
> configuration file it is awful hard to back it out... 
> 
> I'm not against setting it in nfs.conf... But if it can be
> set easier from the kernel... so be it!
> 
>> 
>> As an aside I was also going to propose the use of this same mechanism
>> to address the issue of the lockd options for port numbers, which as it
>> currently stands are manually set in both modprobe.d and in nfs.conf,
>> which as i understand it both must match for successful operation.  A
>> small addition to the scripts I have posted could see the modules.d
>> options automatically generated from the nfs.conf options, thus
>> reducing the scope for mistakes if the administrator chooses to alter
>> those values and further solidifying the idea of gathering all the
>> configuration in a single location.
> I kinda like the idea to be able to set lockd ports from
> nfs.conf. We've done that in the past which was lost
> when we moved systemd. 
> 
> steved.
> 
>> 
>> Your thoughts as always are appreciated.
>> 
>> -Alice 
>> 
>> 
>> On Thu, 2020-07-16 at 10:02 -0400, Chuck Lever wrote:
>>> Hi Alice-
>>> 
>>> I agree that selecting a unique nfs4_client_id string is a problem.
>>> 
>>> However, I thought that Trond is working on a udev-based mechanism
>>> for automatically choosing one that uniquifies containers as well
>>> as stand-alone clients.
>>> 
>>> I'd prefer if we stuck with one mechanism for doing this rather than
>>> having both.
>>> 
>>> Is there rationale for having this in nfs.conf instead of being
>>> completely opaque to the administrator? I don't see a compelling
>>> need for an administrator to adjust this if it is truly a random
>>> string of bytes. Do you know of one?
>>> 
>>> 
>>>> On Jul 16, 2020, at 9:56 AM, Alice Mitchell <ajmitchell@redhat.com>
>>>> wrote:
>>>> 
>>>> This reintroduces the nfs-config.service in order to ensure
>>>> that values are taken from nfs.conf and fed to the kernel
>>>> module if it is loaded and modprobe.d config incase it is not
>>>> 
>>>> Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
>>>> ---
>>>> nfs.conf                      |  1 +
>>>> systemd/Makefile.am           |  3 +++
>>>> systemd/README                |  5 +++++
>>>> systemd/nfs-conf-export.sh    | 28 ++++++++++++++++++++++++++++
>>>> systemd/nfs-config.service.in | 17 +++++++++++++++++
>>>> systemd/nfs.conf.man          | 12 +++++++++++-
>>>> 6 files changed, 65 insertions(+), 1 deletion(-)
>>>> create mode 100755 systemd/nfs-conf-export.sh
>>>> create mode 100644 systemd/nfs-config.service.in
>>>> 
>>>> diff --git a/nfs.conf b/nfs.conf
>>>> index 186a5b19..8bb41227 100644
>>>> --- a/nfs.conf
>>>> +++ b/nfs.conf
>>>> @@ -4,6 +4,7 @@
>>>> #
>>>> [general]
>>>> # pipefs-directory=/var/lib/nfs/rpc_pipefs
>>>> +# nfs4_unique_id = ${machine-id}
>>>> #
>>>> [exports]
>>>> # rootdir=/export
>>>> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
>>>> index 75cdd9f5..51acdc3f 100644
>>>> --- a/systemd/Makefile.am
>>>> +++ b/systemd/Makefile.am
>>>> @@ -9,6 +9,7 @@ unit_files =  \
>>>>    nfs-mountd.service \
>>>>    nfs-server.service \
>>>>    nfs-utils.service \
>>>> +    nfs-config.service \
>>>>    rpc-statd-notify.service \
>>>>    rpc-statd.service \
>>>>    \
>>>> @@ -69,4 +70,6 @@ genexec_PROGRAMS = nfs-server-generator rpc-
>>>> pipefs-generator
>>>> install-data-hook: $(unit_files)
>>>> 	mkdir -p $(DESTDIR)/$(unitdir)
>>>> 	cp $(unit_files) $(DESTDIR)/$(unitdir)
>>>> +	mkdir -p $(DESTDIR)/$(libexecdir)/nfs-utils
>>>> +	install  nfs-conf-export.sh $(DESTDIR)/$(libexecdir)/nfs-utils/
>>>> endif
>>>> diff --git a/systemd/README b/systemd/README
>>>> index da23d6f6..56108b10 100644
>>>> --- a/systemd/README
>>>> +++ b/systemd/README
>>>> @@ -28,6 +28,11 @@ by a suitable 'preset' setting:
>>>>    If enabled, then blkmapd will be run when nfs-client.target is
>>>>    started.
>>>> 
>>>> + nfs-config.service
>>>> +    Invoked by nfs-client.target to export values from nfs.conf to
>>>> +    any kernel modules that require it, such as setting
>>>> nfs4_unique_id
>>>> +    for the nfs client modules
>>>> +
>>>> Another special unit is "nfs-utils.service".  This doesn't really
>>>> do
>>>> anything, but exists so that other units may declare themselves as
>>>> "PartOf" nfs-utils.service.
>>>> diff --git a/systemd/nfs-conf-export.sh b/systemd/nfs-conf-
>>>> export.sh
>>>> new file mode 100755
>>>> index 00000000..486e8df9
>>>> --- /dev/null
>>>> +++ b/systemd/nfs-conf-export.sh
>>>> @@ -0,0 +1,28 @@
>>>> +#!/bin/bash
>>>> +#
>>>> +# This script pulls values out of /etc/nfs.conf and configures
>>>> +# the appropriate kernel modules which cannot read it directly
>>>> +
>>>> +NFSMOD=/sys/module/nfs/parameters/nfs4_unique_id
>>>> +NFSPROBE=/etc/modprobe.d/nfs.conf
>>>> +
>>>> +# Now read the values from nfs.conf
>>>> +MACHINEID=`nfsconf --get general nfs4_unique_id`
>>>> +if [ $? -ne 0 ] || [ "$MACHINEID" == "" ]
>>>> +then
>>>> +# No config vaue found, assume blank
>>>> +MACHINEID=""
>>>> +fi
>>>> +
>>>> +# Kernel module is already loaded, update the live one
>>>> +if [ -e $NFSMOD ]; then
>>>> +echo -n "$MACHINEID" >> $NFSMOD
>>>> +fi
>>>> +
>>>> +# Rewrite the modprobe file for next reboot
>>>> +echo "# This file is overwritten by systemd nfs-config.service" >
>>>> $NFSPROBE
>>>> +echo "# with values taken from /etc/nfs.conf" >> $NFSPROBE
>>>> +echo "# Do not hand modify" >> $NFSPROBE
>>>> +echo "options nfs nfs4_unique_id=\"$MACHINEID\"" >> $NFSPROBE
>>>> +
>>>> +echo "Set to: $MACHINEID"
>>>> diff --git a/systemd/nfs-config.service.in b/systemd/nfs-
>>>> config.service.in
>>>> new file mode 100644
>>>> index 00000000..c5ef1024
>>>> --- /dev/null
>>>> +++ b/systemd/nfs-config.service.in
>>>> @@ -0,0 +1,17 @@
>>>> +[Unit]
>>>> +Description=Preprocess NFS configuration
>>>> +PartOf=nfs-client.target
>>>> +After=nfs-client.target
>>>> +DefaultDependencies=no
>>>> +
>>>> +[Service]
>>>> +Type=oneshot
>>>> +# This service needs to run any time any nfs service
>>>> +# is started, so changes to local config files get
>>>> +# incorporated.  Having "RemainAfterExit=no" (the default)
>>>> +# ensures this happens.
>>>> +RemainAfterExit=no
>>>> +ExecStart=@_libexecdir@/nfs-utils/nfs-conf-export.sh
>>>> +
>>>> +[Install]
>>>> +WantedBy=nfs-client.target
>>>> diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
>>>> index 28dbaa99..fb9d2dab 100644
>>>> --- a/systemd/nfs.conf.man
>>>> +++ b/systemd/nfs.conf.man
>>>> @@ -101,8 +101,11 @@ When a list is given, the members should be
>>>> comma-separated.
>>>> .TP
>>>> .B general
>>>> Recognized values:
>>>> -.BR pipefs-directory .
>>>> +.BR pipefs-directory ,
>>>> +.BR nfs4_unique_id .
>>>> 
>>>> +For 
>>>> +.BR pipefs-directory
>>>> See
>>>> .BR blkmapd (8),
>>>> .BR rpc.idmapd (8),
>>>> @@ -110,6 +113,13 @@ and
>>>> .BR rpc.gssd (8)
>>>> for details.
>>>> 
>>>> +The
>>>> +.BR nfs4_unique_id
>>>> +value is used by the NFS4 client when identifying itself to
>>>> servers and
>>>> +can be used to ensure that this value is unique when the local
>>>> system name
>>>> +perhaps is not. For full details please refer to the kernel
>>>> Documentation
>>>> +.I filesystems/nfs/nfs.txt
>>>> +
>>>> .TP
>>>> .B exports
>>>> Recognized values:
>>>> -- 
>>>> 2.18.1
>>>> 
>>>> 
>>> 
>>> --
>>> Chuck Lever
>>> 
>>> 
>>> 
>> 
> 

--
Chuck Lever