[PATCH] modules: load modules from versioned /var/run dir

[PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

On upgrades the old .so files usually are replaced. But on the other
hand since a qemu process represents a guest instance it is usually kept
around.

That makes late addition of dynamic features e.g. 'hot-attach of a ceph
disk' fail by trying to load a new version of e.f. block-rbd.so into an
old still running qemu binary.

This adds a fallback to also load modules from a versioned directory in the
temporary /var/run path. That way qemu is providing a way for packaging
to store modules of an upgraded qemu package as needed until the next reboot.

An example how that can then be used in packaging can be seen in:
https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU

Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 util/module.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/module.c b/util/module.c
index 236a7bb52a..d2446104be 100644
--- a/util/module.c
+++ b/util/module.c
@@ -19,6 +19,7 @@
 #endif
 #include "qemu/queue.h"
 #include "qemu/module.h"
+#include "qemu-version.h"
 
 typedef struct ModuleEntry
 {
@@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
 #ifdef CONFIG_MODULES
     char *fname = NULL;
     char *exec_dir;
+    char *version_dir;
     const char *search_dir;
     char *dirs[4];
     char *module_name;
@@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
     dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
     dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
     dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
+    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
+                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
+                             '_');
+    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+
     assert(n_dirs <= ARRAY_SIZE(dirs));
 
     g_free(exec_dir);
-- 
2.25.1

Re: [PATCH] modules: load modules from versioned /var/run dir

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200306132648.27577-1-christian.ehrhardt@canonical.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

File: "/tmp/qemu-nsis\qemu-doc.html" -> no files found.
Usage: File [/nonfatal] [/a] ([/r] [/x filespec [...]] filespec [...] |
   /oname=outfile one_file_only)
Error in script "/tmp/qemu-test/src/qemu.nsi" on line 180 -- aborting creation process
make: *** [Makefile:1162: qemu-setup-4.2.50.exe] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zbg82xrb/src/docker-src.2020-03-06-09.38.41.1852:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zbg82xrb/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m8.711s
user    0m7.969s


The full log is available at
http://patchew.org/logs/20200306132648.27577-1-christian.ehrhardt@canonical.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

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

On Fri, Mar 6, 2020 at 3:42 PM <no-reply@patchew.org> wrote:

> Patchew URL:
> https://patchew.org/QEMU/20200306132648.27577-1-christian.ehrhardt@canonical.com/
>
>
>
> Hi,
>
> This series failed the docker-mingw@fedora build test. Please find the
> testing commands and
> their output below. If you have Docker installed, you can probably
> reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #! /bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
>
> File: "/tmp/qemu-nsis\qemu-doc.html" -> no files found.
> Usage: File [/nonfatal] [/a] ([/r] [/x filespec [...]] filespec [...] |
>    /oname=outfile one_file_only)
> Error in script "/tmp/qemu-test/src/qemu.nsi" on line 180 -- aborting
> creation process
> make: *** [Makefile:1162: qemu-setup-4.2.50.exe] Error 1
> Traceback (most recent call last):
>   File "./tests/docker/docker.py", line 664, in <module>
>     sys.exit(main())
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run',
> '--label', 'com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1', '-u',
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e',
> 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14',
> '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache',
> '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v',
> '/var/tmp/patchew-tester-tmp-zbg82xrb/src/docker-src.2020-03-06-09.38.41.1852:/var/tmp/qemu:z,ro',
> 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit
> status 2.
>
> filter=--filter=label=com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zbg82xrb/src'
> make: *** [docker-run-test-mingw@fedora] Error 2
>
> real    3m8.711s
> user    0m7.969s
>
>
> The full log is available at
>
> http://patchew.org/logs/20200306132648.27577-1-christian.ehrhardt@canonical.com/testing.docker-mingw@fedora/?type=message
> .
>

This is a false-positive - at least not due to my proposed patch.
I've not even remotely touched qemu-doc.

The test build even runs with config:
  module support    no
Which even means all my changes except the include are removed by the
preprocessor.

Do I need to do anything to reset the state of this patch to not be marked
as failed?


> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 4140 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

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

On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> On upgrades the old .so files usually are replaced. But on the other
> hand since a qemu process represents a guest instance it is usually kept
> around.
> 
> That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> disk' fail by trying to load a new version of e.f. block-rbd.so into an
> old still running qemu binary.
> 
> This adds a fallback to also load modules from a versioned directory in the
> temporary /var/run path. That way qemu is providing a way for packaging
> to store modules of an upgraded qemu package as needed until the next reboot.
> 
> An example how that can then be used in packaging can be seen in:
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  util/module.c | 7 +++++++
>  1 file changed, 7 insertions(+)

CCing Debian, Fedora, and Red Hat package maintainers in case they have
comments.

The use of /var/run makes me a little uneasy.  I guess it's related to
wanting to uninstall the old package so the .so in their original
location cannot be used (even if they had a versioned path)?

I'm not a package maintainer though so I hope the others will make
suggestions if there are other solutions :).

> 
> diff --git a/util/module.c b/util/module.c
> index 236a7bb52a..d2446104be 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -19,6 +19,7 @@
>  #endif
>  #include "qemu/queue.h"
>  #include "qemu/module.h"
> +#include "qemu-version.h"
>  
>  typedef struct ModuleEntry
>  {
> @@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
>  #ifdef CONFIG_MODULES
>      char *fname = NULL;
>      char *exec_dir;
> +    char *version_dir;
>      const char *search_dir;
>      char *dirs[4];
>      char *module_name;
> @@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
>      dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
>      dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
>      dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
> +    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
> +                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
> +                             '_');
> +    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
> +
>      assert(n_dirs <= ARRAY_SIZE(dirs));
>  
>      g_free(exec_dir);
> -- 
> 2.25.1
> 
> 

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

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

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

On Tue, Mar 10, 2020 at 10:39 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > On upgrades the old .so files usually are replaced. But on the other
> > hand since a qemu process represents a guest instance it is usually kept
> > around.
> >
> > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > old still running qemu binary.
> >
> > This adds a fallback to also load modules from a versioned directory in
> the
> > temporary /var/run path. That way qemu is providing a way for packaging
> > to store modules of an upgraded qemu package as needed until the next
> reboot.
> >
> > An example how that can then be used in packaging can be seen in:
> >
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  util/module.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
>
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
>

Yeah that seems worth, thanks Stefan.

The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?
>

Yes you'd want to uninstall the old bits from disk - even if there would be
a versioned path.
/var/run was considered a nice place as it is auto-cleaned on a reboot
avoiding a massive
and most likely broken logic trying to detect which qemu versions still
have running binaries.

And no distro will have so many qemu updates that N*~<100k for the .so
files will really grow too big.

Also if the path would end up to be the major concern and we can't agree
on a single one we can consider making this:
1. not checking an extra path if not configured
2. on configure packaging can set a path of their choice

I have not done so yet as I was hoping for a simpler patch and
everyone knowing what to expect across e.g. distros.
It also will make isolation easier for example I also have apparmor changes
for libvirt allowing that path which is more complex if it ends up
configurable.

And finally this has to be considered an "offer" by qemu to the packagers
to fix a real field issue.
The packaging does not "have to" exploit this, every Distro is free to just
ignore it.

I'm not a package maintainer though so I hope the others will make
> suggestions if there are other solutions :).
>
> >
> > diff --git a/util/module.c b/util/module.c
> > index 236a7bb52a..d2446104be 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -19,6 +19,7 @@
> >  #endif
> >  #include "qemu/queue.h"
> >  #include "qemu/module.h"
> > +#include "qemu-version.h"
> >
> >  typedef struct ModuleEntry
> >  {
> > @@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char
> *lib_name)
> >  #ifdef CONFIG_MODULES
> >      char *fname = NULL;
> >      char *exec_dir;
> > +    char *version_dir;
> >      const char *search_dir;
> >      char *dirs[4];
> >      char *module_name;
> > @@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char
> *lib_name)
> >      dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
> >      dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
> >      dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
> > +    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
> > +                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS
> "+-.~",
> > +                             '_');
> > +    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
> > +
> >      assert(n_dirs <= ARRAY_SIZE(dirs));
> >
> >      g_free(exec_dir);
> > --
> > 2.25.1
> >
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 5837 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

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

On Tue, Mar 10, 2020 at 12:47:49PM +0100, Christian Ehrhardt wrote:
> On Tue, Mar 10, 2020 at 10:39 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> And finally this has to be considered an "offer" by qemu to the packagers
> to fix a real field issue.
> The packaging does not "have to" exploit this, every Distro is free to just
> ignore it.

I understand.  My intention is just to draw the attention of the right
people so that other distros are aware of the problem and improvements
can be discussed.

From my own perspective it seems fine to merge this or a similar patch
into qemu.git.

Stefan

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

Re: [PATCH] modules: load modules from versioned /var/run dir

[Daniel P. Berrangé]

On Tue, Mar 10, 2020 at 09:39:10AM +0000, Stefan Hajnoczi wrote:
> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > On upgrades the old .so files usually are replaced. But on the other
> > hand since a qemu process represents a guest instance it is usually kept
> > around.
> > 
> > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > old still running qemu binary.
> > 
> > This adds a fallback to also load modules from a versioned directory in the
> > temporary /var/run path. That way qemu is providing a way for packaging
> > to store modules of an upgraded qemu package as needed until the next reboot.
> > 
> > An example how that can then be used in packaging can be seen in:
> > https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> > 
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  util/module.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> 
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
> 
> The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?
> 
> I'm not a package maintainer though so I hope the others will make
> suggestions if there are other solutions :).

My first concern is that this only partially solves the quoted problem.

Consider the QEMU RBD module which is

   /usr/lib64/qemu/block-rbd.so

This links to

   /usr/lib64/librbd.so.1

On host OS upgrade, just before uninstalling old QEMU we copy RBD
module into:

   /var/run/qemu-$VER/block-rbd.so

....but the host OS upgrade also upgrades RBD itself to a new
major version which ships

   /usr/lib64/librbd.so.2

We've got /var/run/qemu-$VER/block-rbd.so saved, but we didn't
transitively save all the things that this linked to, so there's
no guarantee it will still be possible to load it.

IOW, this approach of saving QEMU block modules to a scratch dir
works, *provided* everything else that this QEMU block module needs
still exists in a compatible form.

Some distros may have a policy that no incompatible soname bumps
are permitted in updates, but that's not universal.

On the other hand this is not a big amount of new code, so is not
a huge maint problem even if only a few people ever use it. I would
be a bit more comfortable if this search path addition was perhaps
enabled by an opt-in configure argument, rather than being always
present.


I'm also uneasy about the idea of using a search path ordering for
doing this.  This means that an old running QEMU is always going
to first try to load the block-rbd.so from the new QEMU, expect
to see that fail, and only then fallback to load the block-rbd.so
that actually matches its build.

IIRC, we have an embedded build-id in the modules that should
guarantee that the first load attempt always fails, but I'm still
uneasy about that at a conceptual level.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

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

On Tue, Mar 10, 2020 at 1:10 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Mar 10, 2020 at 09:39:10AM +0000, Stefan Hajnoczi wrote:
> > On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > > On upgrades the old .so files usually are replaced. But on the other
> > > hand since a qemu process represents a guest instance it is usually
> kept
> > > around.
> > >
> > > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > > old still running qemu binary.
> > >
> > > This adds a fallback to also load modules from a versioned directory
> in the
> > > temporary /var/run path. That way qemu is providing a way for packaging
> > > to store modules of an upgraded qemu package as needed until the next
> reboot.
> > >
> > > An example how that can then be used in packaging can be seen in:
> > >
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> > >
> > > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > ---
> > >  util/module.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> >
> > CCing Debian, Fedora, and Red Hat package maintainers in case they have
> > comments.
> >
> > The use of /var/run makes me a little uneasy.  I guess it's related to
> > wanting to uninstall the old package so the .so in their original
> > location cannot be used (even if they had a versioned path)?
> >
> > I'm not a package maintainer though so I hope the others will make
> > suggestions if there are other solutions :).
>
> My first concern is that this only partially solves the quoted problem.
>
> Consider the QEMU RBD module which is
>
>    /usr/lib64/qemu/block-rbd.so
>
> This links to
>
>    /usr/lib64/librbd.so.1
>
> On host OS upgrade, just before uninstalling old QEMU we copy RBD
> module into:
>
>    /var/run/qemu-$VER/block-rbd.so
>
> ....but the host OS upgrade also upgrades RBD itself to a new
> major version which ships
>
>    /usr/lib64/librbd.so.2



We've got /var/run/qemu-$VER/block-rbd.so saved, but we didn't
> transitively save all the things that this linked to, so there's
> no guarantee it will still be possible to load it.
>
> IOW, this approach of saving QEMU block modules to a scratch dir
> works, *provided* everything else that this QEMU block module needs
> still exists in a compatible form.
>
> Some distros may have a policy that no incompatible soname bumps
> are permitted in updates, but that's not universal.
>

Hi Daniel,
Yeah for "us" being Ubuntu that would certainly be true.
An upgrade of librbd.so.1 to librbd.so.2 would only happen at a major
upgrade.
For example in Ubuntu terms e.g. 18.04 to 20.04. Those upgrades usually
come with a requirement to reboot anyway - I'm not trying to solve these.

The much more common and frequent small updates to qemu with individual
fixes are what triggers this bug and what my proposal would solve.

On the other hand this is not a big amount of new code, so is not
> a huge maint problem even if only a few people ever use it. I would
> be a bit more comfortable if this search path addition was perhaps
> enabled by an opt-in configure argument, rather than being always
> present.
>

I already suggested opt-in configure before.
I'd keep this just enable/disable unless we really can't agree on a path.
That should be no big issue to add this in v2.

I'm also uneasy about the idea of using a search path ordering for
> doing this.  This means that an old running QEMU is always going
> to first try to load the block-rbd.so from the new QEMU, expect
> to see that fail, and only then fallback to load the block-rbd.so
> that actually matches its build.
>
> IIRC, we have an embedded build-id in the modules that should
> guarantee that the first load attempt always fails, but I'm still

uneasy about that at a conceptual level.
>

Yes that build-id is what prevents it loading the modules of the new
package.

The intention was to keep this change small and not a burden for anyone.
I'm not yet having an idea for a conceptual change that would avoid to rely
on the build-id AND at the same time not make this a huge patch with
probably many new issues we'll find out only much later.

Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 7485 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Michael Tokarev]

10.03.2020 12:39, Stefan Hajnoczi wrote:
> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
>> On upgrades the old .so files usually are replaced. But on the other
>> hand since a qemu process represents a guest instance it is usually kept
>> around.
>>
>> That makes late addition of dynamic features e.g. 'hot-attach of a ceph
>> disk' fail by trying to load a new version of e.f. block-rbd.so into an
>> old still running qemu binary.
>>
>> This adds a fallback to also load modules from a versioned directory in the
>> temporary /var/run path. That way qemu is providing a way for packaging
>> to store modules of an upgraded qemu package as needed until the next reboot.
>>
>> An example how that can then be used in packaging can be seen in:
>> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
>>
>> Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>> ---
>>  util/module.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
> 
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
> 
> The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?

BTW, this is /run nowadays, not /var/run, as far as I can see.

/mjt

Re: [PATCH] modules: load modules from versioned /var/run dir

[Paolo Bonzini]

On 13/03/20 08:34, Michael Tokarev wrote:
>> The use of /var/run makes me a little uneasy.  I guess it's related to
>> wanting to uninstall the old package so the .so in their original
>> location cannot be used (even if they had a versioned path)?
> 
> BTW, this is /run nowadays, not /var/run, as far as I can see.

/var/run is still symlinked to /run.  QEMU generally uses /var/run,
though we could consider switching sooner or later.

Paolo

Re: [PATCH] modules: load modules from versioned /var/run dir

[Daniel P. Berrangé]

On Fri, Mar 13, 2020 at 08:45:21AM +0100, Paolo Bonzini wrote:
> On 13/03/20 08:34, Michael Tokarev wrote:
> >> The use of /var/run makes me a little uneasy.  I guess it's related to
> >> wanting to uninstall the old package so the .so in their original
> >> location cannot be used (even if they had a versioned path)?
> > 
> > BTW, this is /run nowadays, not /var/run, as far as I can see.
> 
> /var/run is still symlinked to /run.  QEMU generally uses /var/run,
> though we could consider switching sooner or later.

Only Linux commonly uses /run, others still use /var/run. 

We really only need a "configure --rundir=/run"  arg to override the
default, in the same way distros pick /etc and /var, instead of
/usr/etc and /usr/var.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Best practices to handle shared objects through qemu upgrades?

[Christian Ehrhardt]

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

On Fri, Nov 1, 2019 at 10:34 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Nov 01, 2019 at 08:14:08AM +0100, Christian Ehrhardt wrote:
> > Hi everyone,
> > we've got a bug report recently - on handling qemu .so's through
> > upgrades - that got me wondering how to best handle it.
> > After checking with Paolo yesterday that there is no obvious solution
> > that I missed we agreed this should be brought up on the list for
> > wider discussion.
> > Maybe there already is a good best practise out there, or if it
> > doesn't exist we might want to agree upon one going forward.
> > Let me outline the case and the ideas brought up so far.
> >
> > Case
> > - You have qemu representing a Guest
> > - Due to other constraints e.g. PT you can't live migrate (which would
> > be preferred)
> > - You haven't used a specific shared object yet - lets say RBD storage
> > driver as example
> > - Qemu gets an update, packaging replaces the .so files on disk
> > - The Qemu process and the .so files on disk now have a mismatch in
> $buildid
> > - If you hotplug an RBD device it will fail to load the (now new) .so
>
> What happens when it fails to load ?  Does the user get a graceful
> error message or does QEMU abort ? I'd hope the former.
>
> >
> > On almost any other service than "qemu representing a VM" the answer
> > is "restart it", some even re-exec in place to keep things up and
> > running.
> >
> > Ideas so far:
> > a) Modules are checked by build-id, so keep them in a per build-id dir
> on disk
> >   - qemu could be made looking preferred in -$buildid dir first
> >   - do not remove the packages with .so's on upgrades
> >   - needs a not-too-complex way to detect which buildids running qemu
> processes
> >     have for packaging to be able to "autoclean later"
> >   - Needs some dependency juggling for Distro packaging but IMHO can be
> made
> >     to work if above simple "probing buildid of running qemu" would exist
>
> So this needs a bunch of special QEMU hacks in package mgmt tools
> to prevent the package upgrade & cleanup later. This does not look
> like a viable strategy to me.
>
> >
> > b) Preload the modules before upgrade
> >   - One could load the .so files before upgrade
> >   - The open file reference will keep the content around even with the
> > on disk file gone
> >   - lacking a 'load-module' command that would require fake hotplugs
> > which seems wrong
> >   - Required additional upgrade pre-planning
> >   - kills most benefits of modular code without an actual need for it
> > being loaded
>
> Well there's two benefits to modular approach
>
>  - Allow a single build to be selectively installed on a host or container
>    image, such that the install disk footprint is reduced
>  - Allow a faster startup such that huge RBD libraries dont slow down
>    startup of VMs not using RBD disks.
>
> Preloading the modules before upgrade doesn't have to the second benefit.
> We just have to make sure the pre loading doesn't impact the VM startup
> performance.
>
> IOW, register a SIGUSR2 handler which preloads all modules it finds on
> disk. Have a pre-uninstall option on the .so package that sends SIGUSR2
> to all QEMU processes. The challenge of course is that signals are
> async. You might suggest a QMP command, but only 1 process can have the
> QMP monitor open at any time and that's libvirt. Adding a second QMP
> monitor instance is possible but kind of gross for this purpose.
>
> Another option would be to pre-load the modules during startup, but
> do it asynchronously, so that its not blocking overall VM startup.
> eg just before starting the mainloop, spawn a background thread to
> load all remaining modules.
>
> This will potentially degrade performance of the guest CPUs a bit,
> but avoids the latency spike from being synchronous in the startup
> path.
>
>
> > c) go back to non modular build
> >   - No :-)
> >
> > d) anything else out there?
>
> e) Don't do upgrades on a host with running VMs :-)
>
>    Upgrades can break the running VM even ignoring this particular
>    QEMU module scenario.
>
> f) Simply document that if you upgrade with running VMs that some
>    features like hotplug of RBD will become unavialable. Users can
>    then avoid upgrades if that matters to them.
>

Hi,
I've come back to this after a while and now think all the pre-load or
load-command Ideas we had were in vain.
They would be overly complex and need a lot of integration into different
places to trigger them.
All of that would not be well integrated in the trigger of the issue itself
which usually is a package upgrade.

But qemu already does try to load modules from different places and with a
slight extension there I think we could
provide something that packaging (the actual place knowing about upgrades)
can use to avoid this issue.

I'll reply to this thread with a patch for your consideration in a few
minutes.

There is already a Ubuntu 20.04 test build with the qemu and packaging
changes in [1].
The related Debian/Ubuntu packaging changes themselves can be seen in [2].
I hope that helps to illustrate how it would work overall

[1]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3961
[2]:
https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU



> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 7670 bytes --]

[PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

On upgrades the old .so files usually are replaced. But on the other
hand since a qemu process represents a guest instance it is usually kept
around.

That makes late addition of dynamic features e.g. 'hot-attach of a ceph
disk' fail by trying to load a new version of e.f. block-rbd.so into an
old still running qemu binary.

This adds a fallback to also load modules from a versioned directory in the
temporary /var/run path. That way qemu is providing a way for packaging
to store modules of an upgraded qemu package as needed until the next reboot.

An example how that can then be used in packaging can be seen in:
https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU

Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 util/module.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/module.c b/util/module.c
index 236a7bb52a..d2446104be 100644
--- a/util/module.c
+++ b/util/module.c
@@ -19,6 +19,7 @@
 #endif
 #include "qemu/queue.h"
 #include "qemu/module.h"
+#include "qemu-version.h"
 
 typedef struct ModuleEntry
 {
@@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
 #ifdef CONFIG_MODULES
     char *fname = NULL;
     char *exec_dir;
+    char *version_dir;
     const char *search_dir;
     char *dirs[4];
     char *module_name;
@@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
     dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
     dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
     dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
+    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
+                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
+                             '_');
+    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+
     assert(n_dirs <= ARRAY_SIZE(dirs));
 
     g_free(exec_dir);
-- 
2.25.1

Re: [PATCH] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

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

On Wed, Mar 04, 2020 at 10:39:46AM +0100, Christian Ehrhardt wrote:

Please start a new email thread.  Patches sent as replies to existing
email threads are easily missed by humans and tooling also doesn't
recognize them.

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

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

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

On Fri, Mar 6, 2020 at 11:54 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Mar 04, 2020 at 10:39:46AM +0100, Christian Ehrhardt wrote:
>
> Please start a new email thread.  Patches sent as replies to existing
> email threads are easily missed by humans and tooling also doesn't
> recognize them.
>

Sure, thanks Stefan for the hint about how that will be processed/looked at
by maintainers and reviewers.

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 900 bytes --]