[libvirt] [PATCH v1 3/5] qemu.conf: Introduce memory_predictable_file_names

[Daniel P. Berrange]

On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote:
> In some cases management application needs to allocate memory for
> qemu upfront and then just let qemu use that. Since we don't want
> to expose path for memory-backend-file anywhere in the domain
> XML, we can have a configuration knob that when enabled generated
> predictable paths. In this case:
> 
>   $memoryBackingDir/libvirt/qemu/$shortName/$alias
>
> where $shortName is result of virDomainObjGetShortName().

Looking at the current test cases, it seems that for huge pages
we currently create a directory

  $hugepagesMountPoint/libvirt/qemu/$shortName

and for non-hugepages we just use

   $memoryBackingDir  (== /var/lib/libvirt/ram)

In both cases, we then just let QEMU create a random tempfile
name within these directories.

So, IIUC, your proposal here is just making the non-hugepages
case work the same way the hugepages case works, except that
it is adding an extra level of $alias in the directory path.

I don't think this extra level of nesting is really desirable,
or needed. We should just give QEMU a filename, so that it
doesn't create a random tempfile. This is achieved by simply
doing a

   mkdir($memoryBackingDir/libvirt/qemu/$shortName)

but then giving a

  mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias

QEMU will try to open that, and will get ENOENT, at which
point it to O_CREAT that file with the exact name we've
given, instead of using a tempfile name.


> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/libvirtd_qemu.aug         |   1 +
>  src/qemu/qemu.conf                 |   5 ++
>  src/qemu/qemu_command.c            |   9 +--
>  src/qemu/qemu_conf.c               |  76 +++++++++++++++++++-
>  src/qemu/qemu_conf.h               |  10 ++-
>  src/qemu/qemu_driver.c             |  19 +++++
>  src/qemu/qemu_hotplug.c            |   2 +-
>  src/qemu/qemu_process.c            | 141 ++++++++++++++++++++++++++-----------
>  src/qemu/qemu_process.h            |   8 +--
>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
>  10 files changed, 220 insertions(+), 52 deletions(-)

I'd expect to see current tests updated wrt the new naming scheme
for paths

> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index c19bf3a43..77d466b14 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -114,6 +114,7 @@ module Libvirtd_qemu =
>     let gluster_debug_level_entry = int_entry "gluster_debug_level"
>  
>     let memory_entry = str_entry "memory_backing_dir"
> +                 | bool_entry "memory_predictable_file_names"
>  
>     let vxhs_entry = bool_entry "vxhs_tls"
>                   | str_entry "vxhs_tls_x509_cert_dir"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 2e8370a5a..dc3098148 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -769,3 +769,8 @@
>  # This directory is used for memoryBacking source if configured as file.
>  # NOTE: big files will be stored here
>  #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
> +
> +# Use predictable file names. If this is enabled, Libvirt constructs
> +# full paths for memory-backing-file objects. This is experimental
> +# feature and generated paths can change across releases. Don't use it.
> +#memory_predictable_file_names = 1

I don't think we should expose this in the config file at all - it means
we get 2 codepaths, one of which will never be tested by 99% of the
deployments.

I think we should just make normal mem paths uses the same naming
scheme as hugepage mempaths unconditionally. There's no  benefit to
keeping the old naming scheme around.

At the same time though, we don't need to promise anything wrt the new
naming scheme. If someone wants to rely on it they can, but we're not
going to promise forever compat.


> +static int
> +qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> +                                       virDomainDefPtr def,
> +                                       const char *path,
> +                                       bool build)
> +{
> +    if (build) {
> +        if (virFileExists(path))
> +            return 0;
> +
> +        if (virFileMakePathWithMode(path, 0700) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Unable to create %s"),
> +                                 path);
> +            return -1;
> +        }

NB, this creates potentially many levels in the dir hiearchy

  $memoryBackingDir/libvirt
  $memoryBackingDir/libvirt/qemu/
  $memoryBackingDir/libvirt/qemu/$shortName
  $memoryBackingDir/libvirt/qemu/$shortName/$alias

> +
> +        if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> +                                           def, path) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                            _("Unable to label %s"), path);
> +            return -1;
> +        }
> +    } else {
> +        if (rmdir(path) < 0 &&
> +            errno != ENOENT)
> +            VIR_WARN("Unable to remove hugepage path: %s (errno=%d)",
> +                     path, errno);

This only deletes

  $memoryBackingDir/libvirt/qemu/$shortName/$alias

so we're leaving around

  $memoryBackingDir/libvirt/qemu/$shortName

forever. So we need to delete both levels - leaving 
$memoryBackingDir/libvirt/qemu is ok for other guests
though.


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 :|