Re: [PATCH 1/2] qemu: add a max_core setting to qemu.conf for core dump size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 17, 2016 at 05:04:48PM -0400, John Ferlan wrote:
> 
> 
> On 08/03/2016 11:31 AM, Daniel P. Berrange wrote:
> > Currently the QEMU processes inherit their core dump rlimit
> > from libvirtd, which is really suboptimal. This change allows
> > their limit to be directly controlled from qemu.conf instead.
> > ---
> >  src/libvirt_private.syms           |  2 ++
> >  src/qemu/libvirtd_qemu.aug         |  4 ++++
> >  src/qemu/qemu.conf                 | 23 ++++++++++++++++++++++-
> >  src/qemu/qemu_conf.c               | 17 +++++++++++++++++
> >  src/qemu/qemu_conf.h               |  1 +
> >  src/qemu/qemu_process.c            |  1 +
> >  src/qemu/test_libvirtd_qemu.aug.in |  1 +
> >  src/util/vircommand.c              | 14 ++++++++++++++
> >  src/util/vircommand.h              |  1 +
> >  src/util/virprocess.c              | 36 ++++++++++++++++++++++++++++++++++++
> >  src/util/virprocess.h              |  1 +
> >  11 files changed, 100 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 419c33d..6dc2b23 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1389,6 +1389,7 @@ virCommandSetErrorFD;
> >  virCommandSetGID;
> >  virCommandSetInputBuffer;
> >  virCommandSetInputFD;
> > +virCommandSetMaxCoreSize;
> >  virCommandSetMaxFiles;
> >  virCommandSetMaxMemLock;
> >  virCommandSetMaxProcesses;
> > @@ -2199,6 +2200,7 @@ virProcessRunInMountNamespace;
> >  virProcessSchedPolicyTypeFromString;
> >  virProcessSchedPolicyTypeToString;
> >  virProcessSetAffinity;
> > +virProcessSetMaxCoreSize;
> >  virProcessSetMaxFiles;
> >  virProcessSetMaxMemLock;
> >  virProcessSetMaxProcesses;
> > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> > index 8bc23ba..9ec8250 100644
> > --- a/src/qemu/libvirtd_qemu.aug
> > +++ b/src/qemu/libvirtd_qemu.aug
> > @@ -22,6 +22,9 @@ module Libvirtd_qemu =
> >     let int_entry       (kw:string) = [ key kw . value_sep . int_val ]
> >     let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ]
> >  
> > +   let unlimited_val =  del /\"/ "\"" . store /unlimited/ . del /\"/ "\""
> > +   let limits_entry (kw:string) = [ key kw . value_sep . unlimited_val ] |  [ key kw . value_sep . int_val ]
> > +
> 
> Is there no 'uulong_val'?  At the very least uint?  I don't know much
> about this aug stuff, so I defer to you on this.

There's no built-in types at all in augeas - we just define the grammar
entirely ourselves.

> >     (* Config entry grouped by function - same order as example config *)
> >     let vnc_entry = str_entry "vnc_listen"
> > @@ -72,6 +75,7 @@ module Libvirtd_qemu =
> >                   | bool_entry "set_process_name"
> >                   | int_entry "max_processes"
> >                   | int_entry "max_files"
> > +                 | limits_entry "max_core"
> >                   | str_entry "stdio_handler"
> >  
> >     let device_entry = bool_entry "mac_filter"
> > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > index 7964273..abf9938 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -401,7 +401,28 @@
> >  #max_processes = 0
> >  #max_files = 0
> >  
> > -
> > +# If max_core is set to a positive integer, then QEMU will be
> > +# permitted to create core dumps when it crashes, provided its
> > +# RAM size is smaller than the limit set.
> 
> Providing a negative value will cause libvirtd startup failure... The
> way this is worded could lead one to believe they could provide a signed
> value.

Ok, i'll reword it.

> > +# Be warned that the core dump will include a full copy of the
> > +# guest RAM, unless it has been disabled via the guest XML by
> > +# setting:
> > +#
> > +#   <memory dumpcore="off">...guest ram...</memory>
> > +#
> > +# If guest RAM is to be included, ensure the max_core limit
> > +# is set to at least the size of the largest expected guest
> > +# plus another 1GB for any QEMU host side memory mappings.
> > +#
> > +# As a special case it can be set to the string "unlimited" to
> > +# to allow arbitrarily sized core dumps.
> > +#
> > +# By default the core dump size is set to 0 disabling all dumps
> > +#
> > +# Size is in bytes or string "unlimited"
> > +#
> > +#max_core = "unlimited"
> >  
> 
> Would it be overly complicated to alter max_core to be "unlimited" or a
> sized value?  I would think it would be easier to type/see "5G" rather
> than do the math! or fat-finger or cut-n-paste wrong...

Currently our config file format is essentially python variable
initialization syntax, so you can have unquoted numbers, or quoted
strings. So we could not have a bare 5G - it would have to be input
as a string which I think would be confusing - eg users would expect
these to work:

  max_core = 5368709120
  max_core = 5G

but would in fact have to use

  max_core = 5368709120
  max_core = "5G"


we could solve this by extending the virConf parsing code to deal
with bare sized values, but I don't fancy attacking that problem
right now.


> > @@ -1105,6 +1110,15 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files)
> >      cmd->maxFiles = files;
> >  }
> >  
> > +void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes)
> > +{
> > +    if (!cmd || cmd->has_error)
> > +        return;
> > +
> > +    cmd->maxCore = bytes;
> 
> Should this check if bytes == 0 before setting to true?  If not, then
> another change probably would be necessary below... [1]

Yep, we can check for 0.


> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index 09dd3c9..2b71445 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -914,6 +914,42 @@ virProcessSetMaxFiles(pid_t pid ATTRIBUTE_UNUSED, unsigned int files)
> >  }
> >  #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_NOFILE)) */
> >  
> > +#if HAVE_SETRLIMIT && defined(RLIMIT_CORE)
> > +int
> > +virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes)
> > +{
> > +    struct rlimit rlim;
> > +
> > +    rlim.rlim_cur = rlim.rlim_max = bytes;
> > +    if (pid == 0) {
> > +        if (setrlimit(RLIMIT_CORE, &rlim) < 0) {
> > +            virReportSystemError(errno,
> > +                                 _("cannot limit core file size to %llu"),
> > +                                 bytes);
> > +            return -1;
> > +        }
> > +    } else {
> > +        if (virProcessPrLimit(pid, RLIMIT_CORE, &rlim, NULL) < 0) {
> > +            virReportSystemError(errno,
> > +                                 _("cannot limit core file size "
> > +                                   "of process %lld to %llu"),
> > +                                 (long long int)pid, bytes);
> > +            return -1;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +#else /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */
> > +int
> > +virProcessSetMaxCoreSize(pid_t pid ATTRIBUTE_UNUSED,
> > +                         unsigned long long bytes ATTRIBUTE_UNUSED)
> > +{
> 
> [1] Similar to other such functions should this have a :
> 
>     if (!bytes)
>         return 0;
> 
> and of course a removed ATTRIBUTE_UNUSED on bytes above...

Yep.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]