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