Libguestfs - May 2018 - Re: [PATCH v2] daemon: Move lvmetad to early in the appliance boot process.

When the daemon starts up it creates a fresh (empty) LVM configuration
and starts up lvmetad (which depends on the LVM configuration).

However this appears to cause problems: Some types of PV seem to
require lvmetad and don't work without it
(https://bugzilla.redhat.com/show_bug.cgi?id=1581810).  If we don't
start lvmetad earlier, the device nodes are not created.

Therefore move the whole initialization step into appliance/init.

Two further changes had to be made:

Now we are using lvmetad all the time, using vgchange is incorrect.
With lvmetad activated early we must use ‘pvscan --cache --activate ay’
to scan all disks for PVs and activate any VGs on them (although the
documentation is complex, confusing and contradictory so I'm not
completely sure about this).

The ‘lvm_system_dir’ local variable in ‘daemon/lvm-filter.c’
previously contained the path of the directory above $LVM_SYSTEM_DIR
(eg. $LVM_SYSTEM_DIR = "/etc/lvm", lvm_system_dir = "/etc").
As this
was highly confusing, I have changed it so the local variable and the
environment variable have identical contents.  This involved removing
the ‘lvm/’ component from a couple of paths since it is now included
in the local variable.
---
 appliance/init      | 11 +++++++-
 daemon/daemon.h     |  4 ---
 daemon/guestfsd.c   |  8 ------
 daemon/lvm-filter.c | 75 +++++++++--------------------------------------------
 4 files changed, 22 insertions(+), 76 deletions(-)

diff --git a/appliance/init b/appliance/init
index 6a61a8d6e..4f2b55822 100755
--- a/appliance/init
+++ b/appliance/init
@@ -133,9 +133,17 @@ fi
 # Scan for MDs but don't run arrays unless all expected drives are present
 mdadm -As --auto=yes --no-degraded
 
+# Set up a clean LVM environment.
+# Empty LVM configuration file means "all defaults".
+mkdir -p /tmp/lvm
+touch /tmp/lvm/lvm.conf
+LVM_SYSTEM_DIR=/tmp/lvm
+export LVM_SYSTEM_DIR
+lvmetad
+
 # Scan for LVM.
 modprobe dm_mod ||:
-lvm vgchange -aay --sysinit
+lvm pvscan --cache --activate ay
 
 # Scan for MDs and run all found arrays even they are in degraded state
 mdadm -As --auto=yes --run
@@ -149,6 +157,7 @@ if test "$guestfs_verbose" = 1 && test
"$guestfs_boot_analysis" != 1; then
     ls -lR /dev
     cat /proc/mounts
     cat /proc/mdstat
+    lvm config
     lvm pvs
     lvm vgs
     lvm lvs
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 7958ba781..faaf1237e 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -249,10 +249,6 @@ extern char *get_blkid_tag (const char *device, const char
*tag);
 /* lvm.c */
 extern int lv_canonical (const char *device, char **ret);
 
-/* lvm-filter.c */
-extern void clean_lvm_config (void);
-extern void start_lvmetad (void);
-
 /* zero.c */
 extern void wipe_device_before_mkfs (const char *device);
 
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 68d3de2ec..ae428e573 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -233,14 +233,6 @@ main (int argc, char *argv[])
   _umask (0);
 #endif
 
-  /* Make a private copy of /etc/lvm so we can change the config (see
-   * daemon/lvm-filter.c).
-   */
-  if (!test_mode) {
-    clean_lvm_config ();
-    start_lvmetad ();
-  }
-
   /* Connect to virtio-serial channel. */
   if (!channel)
     channel = VIRTIO_SERIAL_CHANNEL;
diff --git a/daemon/lvm-filter.c b/daemon/lvm-filter.c
index ad85a7cc4..1ea5ad8ab 100644
--- a/daemon/lvm-filter.c
+++ b/daemon/lvm-filter.c
@@ -36,71 +36,20 @@
 #include "daemon.h"
 #include "actions.h"
 
-/* This runs during daemon start up and creates a fresh LVM
- * configuration which we can modify as we desire.  LVM allows
- * configuration to be completely empty (meaning "all defaults").
- *
- * The final directory layout is:
- *
- *   /tmp/lvmXXXXXX                 (lvm_system_dir set to this)
- *   /tmp/lvmXXXXXX/lvm             ($LVM_SYSTEM_DIR set to this)
- *   /tmp/lvmXXXXXX/lvm/lvm.conf    (configuration file - initially empty)
- */
-static char lvm_system_dir[] = "/tmp/lvmXXXXXX";
-
-static void rm_lvm_system_dir (void);
 static void debug_lvm_config (void);
 
-void
-clean_lvm_config (void)
-{
-  char env[64], conf[64];
-  FILE *fp;
-
-  if (mkdtemp (lvm_system_dir) == NULL)
-    error (EXIT_FAILURE, errno, "mkdtemp: %s", lvm_system_dir);
-
-  snprintf (env, sizeof env, "%s/lvm", lvm_system_dir);
-  mkdir (env, 0755);
-  snprintf (conf, sizeof conf, "%s/lvm/lvm.conf", lvm_system_dir);
-  fp = fopen (conf, "w");
-  if (fp == NULL) {
-    perror ("clean_lvm_config: cannot create empty lvm.conf");
-    exit (EXIT_FAILURE);
-  }
-  fclose (fp);
-
-  /* Set environment variable so we use the clean configuration. */
-  setenv ("LVM_SYSTEM_DIR", env, 1);
-
-  /* Set a handler to remove the temporary directory at exit. */
-  atexit (rm_lvm_system_dir);
-
-  debug_lvm_config ();
-}
-
-/* Try to run lvmetad, without failing if it couldn't. */
-void
-start_lvmetad (void)
-{
-  int r;
-
-  if (verbose)
-    printf ("%s\n", "lvmetad");
-  r = system ("lvmetad");
-  if (r == -1)
-    perror ("system/lvmetad");
-  else if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
-    fprintf (stderr, "warning: lvmetad command failed\n");
-}
-
+/* Read LVM_SYSTEM_DIR environment variable, or set it to a default
+ * value if the environment variable is not set.
+ */
+static const char *lvm_system_dir;
+static void get_lvm_system_dir (void) __attribute__((constructor));
 static void
-rm_lvm_system_dir (void)
+get_lvm_system_dir (void)
 {
-  char cmd[64];
-
-  snprintf (cmd, sizeof cmd, "rm -rf %s", lvm_system_dir);
-  ignore_value (system (cmd));
+  lvm_system_dir = getenv ("LVM_SYSTEM_DIR");
+  if (!lvm_system_dir)
+    lvm_system_dir = "/etc/lvm";
+  fprintf (stderr, "lvm_system_dir = %s\n", lvm_system_dir);
 }
 
 /* Rewrite the 'filter = [ ... ]' line in lvm.conf. */
@@ -112,7 +61,7 @@ set_filter (char *const *filters)
   FILE *fp;
   size_t i, j;
 
-  if (asprintf (&conf, "%s/lvm/lvm.conf", lvm_system_dir) == -1)
{
+  if (asprintf (&conf, "%s/lvm.conf", lvm_system_dir) == -1) {
     reply_with_perror ("asprintf");
     return -1;
   }
@@ -177,7 +126,7 @@ static int
 rescan (void)
 {
   char lvm_cache[64];
-  snprintf (lvm_cache, sizeof lvm_cache, "%s/lvm/cache/.cache",
lvm_system_dir);
+  snprintf (lvm_cache, sizeof lvm_cache, "%s/cache/.cache",
lvm_system_dir);
 
   unlink (lvm_cache);
 
-- 
2.16.2

On Thursday, 24 May 2018 16:01:22 CEST Richard W.M. Jones
wrote:
> When the daemon starts up it creates a fresh (empty) LVM configuration
> and starts up lvmetad (which depends on the LVM configuration).
> 
> However this appears to cause problems: Some types of PV seem to
> require lvmetad and don't work without it
> (https://bugzilla.redhat.com/show_bug.cgi?id=1581810).  If we don't
> start lvmetad earlier, the device nodes are not created.
> 
> Therefore move the whole initialization step into appliance/init.
> 
> Two further changes had to be made:
> 
> Now we are using lvmetad all the time, using vgchange is incorrect.
> With lvmetad activated early we must use ‘pvscan --cache --activate ay’
> to scan all disks for PVs and activate any VGs on them (although the
> documentation is complex, confusing and contradictory so I'm not
> completely sure about this).
> 
> The ‘lvm_system_dir’ local variable in ‘daemon/lvm-filter.c’
> previously contained the path of the directory above $LVM_SYSTEM_DIR
> (eg. $LVM_SYSTEM_DIR = "/etc/lvm", lvm_system_dir =
"/etc").  As this
> was highly confusing, I have changed it so the local variable and the
> environment variable have identical contents.  This involved removing
> the ‘lvm/’ component from a couple of paths since it is now included
> in the local variable.
> ---
Mostly LGTM, but there is one thing to fix:
> +  lvm_system_dir = getenv ("LVM_SYSTEM_DIR");
> +  if (!lvm_system_dir)
> +    lvm_system_dir = "/etc/lvm";
> +  fprintf (stderr, "lvm_system_dir = %s\n", lvm_system_dir);
The pointer returned by getenv() can change, so it will be better to
duplicate its result, and store that instead for the daemon lifetime.

-- 
Pino Toscano

On Thu, May 24, 2018 at 04:54:49PM +0200, Pino Toscano
wrote:
> On Thursday, 24 May 2018 16:01:22 CEST Richard W.M. Jones wrote:
> > When the daemon starts up it creates a fresh (empty) LVM configuration
> > and starts up lvmetad (which depends on the LVM configuration).
> > 
> > However this appears to cause problems: Some types of PV seem to
> > require lvmetad and don't work without it
> > (https://bugzilla.redhat.com/show_bug.cgi?id=1581810).  If we
don't
> > start lvmetad earlier, the device nodes are not created.
> > 
> > Therefore move the whole initialization step into appliance/init.
> > 
> > Two further changes had to be made:
> > 
> > Now we are using lvmetad all the time, using vgchange is incorrect.
> > With lvmetad activated early we must use ‘pvscan --cache --activate
ay’
> > to scan all disks for PVs and activate any VGs on them (although the
> > documentation is complex, confusing and contradictory so I'm not
> > completely sure about this).
> > 
> > The ‘lvm_system_dir’ local variable in ‘daemon/lvm-filter.c’
> > previously contained the path of the directory above $LVM_SYSTEM_DIR
> > (eg. $LVM_SYSTEM_DIR = "/etc/lvm", lvm_system_dir =
"/etc").  As this
> > was highly confusing, I have changed it so the local variable and the
> > environment variable have identical contents.  This involved removing
> > the ‘lvm/’ component from a couple of paths since it is now included
> > in the local variable.
> > ---
> 
> Mostly LGTM, but there is one thing to fix:
> 
> > +  lvm_system_dir = getenv ("LVM_SYSTEM_DIR");
> > +  if (!lvm_system_dir)
> > +    lvm_system_dir = "/etc/lvm";
> > +  fprintf (stderr, "lvm_system_dir = %s\n",
lvm_system_dir);
> 
> The pointer returned by getenv() can change, so it will be better to
> duplicate its result, and store that instead for the daemon lifetime.
I pushed this, with a small change to ensure that it strdups the
string in this function.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v