Merge ~ahasenack/ubuntu/+source/samba:jammy-ctdb-nfs-mvp-fixes into ubuntu/+source/samba:ubuntu/devel

[Andreas Hasenack]

Small, if not minimal, set of changes to make ctdb compatible with the new nfs-utils NFS server/client in jammy. Without these, it still somewhat works, but there are corner cases due to the debian packaging not removing the now obsolete /etc/default/nfs-* config files.

Some remarks:

# quota rpc service
The debian/ubuntu/ quota rpc (rpc.rquotad) service has a quirk that it will scan /etc/fstab and only start if it detects relevant quota mount options. When no such options exist, the service will stop with exit 0. ctdb eventually detects that rpc.rquotad isn't running, and tries to start it again, and this loops forever.
I contacted upstream[1] for a way to signal the scripts that quota isn't mandatory, and there is a series of commits[2] to address that, but too fresh for jammy I think, and somewhat of a feature change. I then went with the workaround that Rafael had used already, which is to not define nfs_rquotad_service, in which case ctdb will try to handle it manually and not via the service command.
For users who know they will really not need rpc.rquotad running, they can then just remove the /etc/ctdb/nfs-checks.d/50.rquotad.check file, and ctdb won't check (nor restart) this quota service anymore.

# the nfs_*_service variables in the nfs-linux-kernel-callout script
The content of those variables, if defined, will be used as the service name in the service command, like "service $nfs_rquotad_service start".
Some nfs services, however, start other services. NFS is that complicated, yes. So if a service variable is not defined, it doesn't really mean it won't be started. They can still be started by the checks, individually. That's what happened with the rpc.rquotad service described above.
The previous version of the fix-nfs-service-name-to-nfs-kernel-server was undefining basically all service variables, and then having the script rely on its own parsing of /etc/default/nfs-* scripts to find out which command line options it should use, and start the services manually. I chose to define the variables instead, so normal systemd units would be used, and no command-line option figuring out is needed, because the daemons all read from /etc/nfs.conf{,.d/*}. The exception being rpc.rquotad, as explained above, but it uses its own config in /etc/default/quota, so not really affected by the nfs-utils config file change.

# nfsconf changes
The meat of this MP. I tried the less invasive alternative for this. I changed Rafael's patch, isntead of creating a new one on top of it with the nfsconf changes. The patch name isn't that great anymore, but I thought renaming the patch would be worse for a review. If you prefer, I can add a patch on top of the original one that just changes the service name.

# ctdb "service" name
There is a mistake in the original patch where it was renaming "nfs" to "nfs-common" or "nfs-kernel-server", but in some cases that was incorrect. The "nfs" key was used for a directory in /var/lib//var/lib/ctdb/scripts/service/<key>, and as a result the previous package, and this one (because I didn't change it) have:

/var/lib/ctdb/scripts/service/nfs
/var/lib/ctdb/scripts/service/nfs-kernel-server

Fixing that now would mean moving state files from nfs-kernel-server into nfs in a maintainer script, and that's too big a change for this MVP branch. As far as I can tell, things are working as they are, fortunately or unfortunately.

# the enable-nfs.sh
This is a script written by Rafael back when he first tackled ctdb in ubuntu and made it work. It came as a result of his work in bug #722201[3] and this discourse post[4]. I changed it for the new nfs-utils version we have, but didn't make other big improvements to the script. Keep in mind it's an example script. It complements and implements parts of the upstream documentation[5].

PPA: https://launchpad.net/~ahasenack/+archive/ubuntu/samba-ctdb-nfs-mvp-fixes/+packages

If you want to test this on your own, get in touch. That discourse post is a start, but there are many details.

1. https://lists.samba.org/archive/samba-technical/2022-March/137176.html
2. https://git.samba.org/?p=martins/samba.git;a=shortlog;h=refs/heads/ctdb-nfs
3. https://bugs.launchpad.net/debian/+source/ctdb/+bug/722201
4. https://discourse.ubuntu.com/t/ctdb-create-a-3-node-nfs-ha-backed-by-a-clustered-filesystem/11608
5. https://wiki.samba.org/index.php/Setting_up_CTDB_for_Clustered_NFS

[Miriam España Acebal (mirespace)]

Hi Andreas,

intense merge proposal! As spoken, I did a review of the code without testing itself, checking for typos or missed things and trying to understand the a bit the logic.

I don't find any strange. At the beginning, the changes around nfs-kernel-server files confuse me a bit because I was seeing service and conf didn't match or removed but after reading carefully your "# ctdb "service" name" I totally agree that is out of the scope of this MP.

Also, I checked that the new files are shipped in the *.deb from your PPA (I've checked what I might have forgotten).

Thanks for this MP.

review: Approve

[Andreas Hasenack (ahasenack)]

Thanks a lot Miriam!

diff --git a/debian/changelog b/debian/changelog
index 7f8f905..88d399e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,21 @@
+samba (2:4.15.5~dfsg-0ubuntu3) jammy; urgency=medium
+
+  * Update nfs scripts for new nfs.conf config (LP: #1961840):
+    - d/p/fix-nfs-service-name-to-nfs-kernel-server.patch: updated to use
+      nfsconf(8) if it's available, instead of parsing the old config
+      files in /etc/default/nfs-*
+    - d/ctdb.example.nfs.conf: /etc/nfs.conf to be used by the example
+      enable-nfs.sh example script
+    - d/ctdb.example.quota: quota config file to be used by the example
+      enable-nfs.sh script
+    - d/ctdb.example.nfs-{common,kernel-server}: obsolete, replaced by
+      nfs.conf
+    - d/ctdb.example.enable.nfs.sh: handle new nfs.conf and other
+      changes in the new nfs server packages
+    - d/rules: install the new/changed ctdb example nfs files
+
+ -- Andreas Hasenack <[e-mail]>  Mon, 21 Mar 2022 11:55:54 -0300
+
 samba (2:4.15.5~dfsg-0ubuntu2) jammy; urgency=medium
 
   * d/p/lp-1951490-fix-printing-KB5006743.patch: Fix printing after
diff --git a/debian/ctdb.example.enable.nfs.sh b/debian/ctdb.example.enable.nfs.sh
index 2a99074..f719e63 100755
--- a/debian/ctdb.example.enable.nfs.sh
+++ b/debian/ctdb.example.enable.nfs.sh
@@ -19,6 +19,14 @@ backupfile() {
     [ -f $1 ] && cp $1 $1.prvctdb || true
 }
 
+renamefiles() {
+    for f; do
+        [ -f "$f" ] || continue
+        echo "Renaming $f to $f.prvctdb"
+        mv "$f" "$f".prvctdb
+    done
+}
+
 checkservice() {
     (systemctl list-unit-files | grep -q $1.service) || die "service $1 not found"
 }
@@ -46,20 +54,6 @@ appendfile() {
     cat $base/$origfile >> $replfile
 }
 
-appendnfsenv() {
-
-    file=$1 ; [ -f $file ] || die "inexistent file $file";
-
-    echo appending NFS_HOSTNAME to $file...
-
-    grep -q "NFS_HOSTNAME" $file || \
-    {
-        echo
-        echo "echo NFS_HOSTNAME=\\\"\$NFS_HOSTNAME\\\"" \>\> \/run\/sysconfig\/nfs-utils
-        echo
-    } >> $file
-}
-
 execnfsenv() {
 
     file=$1 ; [ -f $file ] || due "inexistent file $file";
@@ -71,7 +65,7 @@ execnfsenv() {
 
 fixnfshostname() {
 
-    file=$1 ; [ -f $file ] || due "inexistent file $file";
+    type nfsconf > /dev/null 2>&1 || die "nfsconf(8) not found"
 
     if [ "$ghostname" == "" ]; then
         echo "What is the FQDN for the public IP address of this host ?"
@@ -79,8 +73,8 @@ fixnfshostname() {
         read ghostname
     fi
 
-    echo placing hostname $ghostname into $file...
-    sed -i "s:PLACE_HOSTNAME_HERE:$ghostname:g" $file
+    echo "Setting $ghostname in nfs.conf..."
+    nfsconf --set statd name "$ghostname"
 }
 
 # end of functions --
@@ -90,13 +84,14 @@ fixnfshostname() {
 echo """
 This script will enable CTDB NFS HA by changing the following files:
 
-(1) /etc/default/nfs-common                   ( replace )
-(2) /etc/default/nfs-kernel-server            ( replace )
+(1) /etc/nfs.conf                             ( replace )
+(2) /etc/nfs.conf.d/*.conf                    ( rename  )
 (3) /etc/services                             ( append  )
 (4) /etc/sysctl.d/99-nfs-static-ports.conf    ( create  )
-(5) /usr/lib/systemd/scripts/nfs-utils_env.sh ( modify  )
+(5) /etc/default/quota                        ( replace )
 
-and disabling the following services:
+and disabling the following services, as they will be managed
+by ctdb:
 
 (1) rpcbind
 (2) nfs-kernel-server
@@ -124,10 +119,10 @@ checkservice rpcbind
 echo "requirements okay!"
 echo
 
-backupfile /etc/default/nfs-common
-backupfile /etc/default/nfs-kernel-server
+backupfile /etc/nfs.conf
+renamefiles /etc/nfs.conf.d/*.conf
 backupfile /etc/services
-backupfile /usr/lib/systemd/scripts/nfs-utils_env.sh
+backupfile /etc/default/quota
 echo
 
 set +e
@@ -150,20 +145,15 @@ echo
 
 set -e
 
-replacefile nfs-common /etc/default/nfs-common
-replacefile nfs-kernel-server /etc/default/nfs-kernel-server
+replacefile nfs.conf /etc/nfs.conf
 replacefile 99-nfs-static-ports.conf /etc/sysctl.d/99-nfs-static-ports.conf
+replacefile quota /etc/default/quota
 echo
 
 appendfile services /etc/services
 echo
 
-fixnfshostname /etc/default/nfs-common
-fixnfshostname /etc/default/nfs-kernel-server
-echo
-
-appendnfsenv /usr/lib/systemd/scripts/nfs-utils_env.sh
-execnfsenv /usr/lib/systemd/scripts/nfs-utils_env.sh
+fixnfshostname
 echo
 
 sysctlrefresh
diff --git a/debian/ctdb.example.nfs-common b/debian/ctdb.example.nfs-common
 deleted file mode 100644
index 9d4f22c..0000000
--- a/debian/ctdb.example.nfs-common
+++ /dev/null
@@ -1,19 +0,0 @@
-# CTDB: /etc/default/nfs-common for clustering
-
-NFS_HOSTNAME="PLACE_HOSTNAME_HERE"
-
-# rpc.statd - daemon listening for reboot notifications (locks related)
-NEED_STATD="yes"
-STATDOPTS="-n ${NFS_HOSTNAME} -p 32765 -o 32766 -H /etc/ctdb/statd-callout -T 32768 -U 32768"
-STATD_HOSTNAME="$NFS_HOSTNAME"
-
-# rpc.gssd - security context for rpc connections
-NEED_GSSD="no"
-
-# rpc.idmapd - NFSv4 <-> name mapping daemon (fallback nowadays)
-# recent kernels use nfsidmap(8) instead
-NEED_IDMAPD="no"
-
-# rpc.quota - usage quota
-RPCRQUOTADOPTS="-p 32769"
-
diff --git a/debian/ctdb.example.nfs-kernel-server b/debian/ctdb.example.nfs-kernel-server
 deleted file mode 100644
index 6aa5df9..0000000
--- a/debian/ctdb.example.nfs-kernel-server
+++ /dev/null
@@ -1,16 +0,0 @@
-# CTDB: /etc/default/nfs-kernel-server for clustering
-
-NFS_HOSTNAME="PLACE_HOSTNAME_HERE"
-
-# rpc.nfsd - user level part of nfs service (kernel: nfsd module)
-RPCNFSDPRIORITY=0
-RPCNFSDCOUNT=8
-RPCNFSDOPTS="-N 4"
-
-# rpc.mountd - server side of nfs mount protocol
-RPCMOUNTDOPTS="-p 32767 --manage-gids --no-nfs-version 4"
-
-# rpc.svcgssd - userspace daemon to handle sec context for kernel rpcsec_gss
-NEED_SVCGSSD="no"
-RPCSVCGSSDOPTS=""
-
diff --git a/debian/ctdb.example.nfs.conf b/debian/ctdb.example.nfs.conf
 new file mode 100644
index 0000000..5cfa13a
--- /dev/null
+++ b/debian/ctdb.example.nfs.conf
@@ -0,0 +1,20 @@
+[general]
+pipefs-directory = /run/rpc_pipefs
+
+[lockd]
+port = 32768
+udp-port = 32768
+
+[mountd]
+manage-gids = 1
+port = 32767
+
+[nfsd]
+threads = 8
+vers4 = n
+
+[statd]
+ha-callout = /etc/ctdb/statd-callout
+name = @NFS_HOSTNAME@
+outgoing-port = 32766
+port = 32765
diff --git a/debian/ctdb.example.quota b/debian/ctdb.example.quota
 new file mode 100644
index 0000000..00eab66
--- /dev/null
+++ b/debian/ctdb.example.quota
@@ -0,0 +1,5 @@
+# Set to "true" if warnquota should be run in cron.daily
+run_warnquota=
+
+# Add options to rpc.rquotad here
+RPCRQUOTADOPTS="-p 32769"
diff --git a/debian/patches/fix-nfs-service-name-to-nfs-kernel-server.patch b/debian/patches/fix-nfs-service-name-to-nfs-kernel-server.patch
index 70afeba..0010ab2 100644
--- a/debian/patches/fix-nfs-service-name-to-nfs-kernel-server.patch
+++ b/debian/patches/fix-nfs-service-name-to-nfs-kernel-server.patch
@@ -4,13 +4,15 @@ Subject: fix nfs related service names
 Upstream defines nfs related service names based on the Linux
 distribution. This patch fixes the names for Debian and derivatives.
 
+Update by Andreas Hasenack <[e-mail]> (LP: #1961840):
+Use nfsconf(8) if it's available, instead of parsing the old config
+files in /etc/default/nfs-*
+
 Bug-Debian: https://bugs.debian.org/929931
 Bug-Ubuntu: https://bugs.launchpad.net/bugs/722201
-Last-Update: 2018-08-05
-Index: samba/ctdb/config/events/legacy/06.nfs.script
-===================================================================
---- samba.orig/ctdb/config/events/legacy/06.nfs.script	2020-11-24 18:11:53.506104058 -0500
-+++ samba/ctdb/config/events/legacy/06.nfs.script	2020-11-24 18:11:53.502104093 -0500
+Last-Update: 2022-03-16
+--- a/ctdb/config/events/legacy/06.nfs.script
++++ b/ctdb/config/events/legacy/06.nfs.script
 @@ -6,7 +6,7 @@
  
  . "${CTDB_BASE}/functions"
@@ -20,11 +22,9 @@ Index: samba/ctdb/config/events/legacy/06.nfs.script
  
  load_script_options "service" "60.nfs"
  
-Index: samba/ctdb/config/events/legacy/60.nfs.script
-===================================================================
---- samba.orig/ctdb/config/events/legacy/60.nfs.script	2020-11-24 18:11:53.506104058 -0500
-+++ samba/ctdb/config/events/legacy/60.nfs.script	2020-11-24 18:11:53.502104093 -0500
-@@ -6,9 +6,9 @@
+--- a/ctdb/config/events/legacy/60.nfs.script
++++ b/ctdb/config/events/legacy/60.nfs.script
+@@ -6,9 +6,11 @@
  
  . "${CTDB_BASE}/functions"
  
@@ -32,14 +32,14 @@ Index: samba/ctdb/config/events/legacy/60.nfs.script
 +service_name="nfs-kernel-server"
  
 -load_system_config "nfs"
-+load_system_config "nfs-kernel-server"
++if ! type nfsconf > /dev/null 2>&1; then
++    load_system_config "nfs-kernel-server"
++fi
  
  load_script_options
  
-Index: samba/ctdb/config/nfs-linux-kernel-callout
-===================================================================
---- samba.orig/ctdb/config/nfs-linux-kernel-callout	2020-11-24 18:11:53.506104058 -0500
-+++ samba/ctdb/config/nfs-linux-kernel-callout	2020-11-24 18:11:53.502104093 -0500
+--- a/ctdb/config/nfs-linux-kernel-callout
++++ b/ctdb/config/nfs-linux-kernel-callout
 @@ -14,7 +14,7 @@
  
  # As above, edit the default value below.  CTDB_NFS_DISTRO_STYLE is a
@@ -49,31 +49,40 @@ Index: samba/ctdb/config/nfs-linux-kernel-callout
  
  case "$nfs_distro_style" in
  systemd-*)
-@@ -33,6 +33,14 @@
+@@ -32,7 +32,22 @@
+ 		: # Defaults only
  		;;
  	*-debian)
- 		nfs_rquotad_service="quotarpc"
-+		nfs_lock_service=""
-+		nfs_lock_service=""
-+		nfs_mountd_service=""
-+		nfs_status_service=""
+-		nfs_rquotad_service="quotarpc"
++		# XXX
++		# Undefine nfs_rquotad_services because the quotarpc service won't
++		# start unless there are specific "quota" mount options in /etc/fstab.
++		# In this way, we let ctdb start it up manually once the
++		# /etc/ctdb/nfs-checks.d/50.rquotad.check detects rpc.rquotad isn't
++		# running.
++		# Users who really don't want rpc.rquotad running should then move
++		# the 50.rquotad.check script away.
 +		nfs_rquotad_service=""
 +		nfs_service="nfs-kernel-server"
-+		nfs_config="/etc/default/nfs-kernel-server"
++		if type nfsconf >/dev/null 2>&1; then
++			nfs_config=""
++		else
++			nfs_config="/etc/default/nfs-kernel-server"
++		fi
 +		nfs_rquotad_config="/etc/default/quota"
  		;;
  	*)
  		echo "Internal error"
-Index: samba/ctdb/config/statd-callout
-===================================================================
---- samba.orig/ctdb/config/statd-callout	2020-11-24 18:11:53.506104058 -0500
-+++ samba/ctdb/config/statd-callout	2020-11-24 18:11:53.502104093 -0500
-@@ -29,7 +29,7 @@
+--- a/ctdb/config/statd-callout
++++ b/ctdb/config/statd-callout
+@@ -29,7 +29,9 @@
  }
  
  # Try different variables to find config file for NFS_HOSTNAME
 -load_system_config "nfs" "nfs-common"
-+load_system_config "nfs-kernel-server"
++if ! type nfsconf > /dev/null 2>&1; then
++    load_system_config "nfs-common" "nfs-kernel-server"
++fi
  
  # If NFS_HOSTNAME not set then try to pull it out of /etc/nfs.conf
  if [ -z "$NFS_HOSTNAME" ] && type nfsconf >/dev/null 2>&1 ; then
diff --git a/debian/rules b/debian/rules
index 26d2df6..655be1a 100755
--- a/debian/rules
+++ b/debian/rules
@@ -222,8 +222,8 @@ override_dh_installexamples-arch:
 	# CTDB config examples and helper scripts
 	mkdir -p ctdb/doc/examples/nfs-kernel-server
 	cp debian/ctdb.example.enable.nfs.sh ctdb/doc/examples/nfs-kernel-server/enable-nfs.sh
-	cp debian/ctdb.example.nfs-common ctdb/doc/examples/nfs-kernel-server/nfs-common
-	cp debian/ctdb.example.nfs-kernel-server ctdb/doc/examples/nfs-kernel-server/nfs-kernel-server
+	cp debian/ctdb.example.nfs.conf ctdb/doc/examples/nfs-kernel-server/nfs.conf
+	cp debian/ctdb.example.quota ctdb/doc/examples/nfs-kernel-server/quota
 	cp debian/ctdb.example.services ctdb/doc/examples/nfs-kernel-server/services
 	cp debian/ctdb.example.sysctl-nfs-static-ports.conf ctdb/doc/examples/nfs-kernel-server/99-nfs-static-ports.conf
 	dh_installexamples