[libvirt] [PATCH v2 1/2] visrh dump compression support
Sorry, email was empty.. == Add dump_image_format[] to qemu.conf and support compressed dump at virsh dump. coredump compression is important for saving disk space in an environment where multiple guest run. (In general, "disk space for dump" is specially allocated and will be a dead space in the system. It's used only at emergency. So, it's better to have both of save_image_format and dump_image_format. "save" is done in scheduled manner with enough calculated disk space for it.) This code reuses some of save_image_format[] and supports the same format with virsh save. --- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 11 +++++++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 30 +++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 5 deletions(-) Index: libvirt-0.8.4/src/qemu/qemu_conf.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c +++ libvirt-0.8.4/src/qemu/qemu_conf.c @@ -324,6 +324,17 @@ int qemudLoadDriverConfig(struct qemud_d } } + p = virConfGetValue (conf, "dump_image_format"); + CHECK_TYPE ("dump_image_format", VIR_CONF_STRING); + if (p && p->str) { + VIR_FREE(driver->dumpImageFormat); + if (!(driver->dumpImageFormat = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { Index: libvirt-0.8.4/src/qemu/qemu_conf.h =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.h +++ libvirt-0.8.4/src/qemu/qemu_conf.h @@ -159,6 +159,7 @@ struct qemud_driver { virSecurityDriverPtr securitySecondaryDriver; char *saveImageFormat; + char *dumpImageFormat; pciDeviceList *activePciHostdevs; Index: libvirt-0.8.4/src/qemu/qemu.conf =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu.conf +++ libvirt-0.8.4/src/qemu/qemu.conf @@ -144,7 +144,11 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # +# save_image_format is used when you use 'virsh save' at scheduled saving. +# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# # save_image_format = "raw" +# dump_image_format = "raw" # If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is Index: libvirt-0.8.4/src/qemu/qemu_driver.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_driver.c +++ libvirt-0.8.4/src/qemu/qemu_driver.c @@ -5716,11 +5716,15 @@ static int qemudDomainCoreDump(virDomain int resume = 0, paused = 0; int ret = -1, fd = -1; virDomainEventPtr event = NULL; - const char *args[] = { - "cat", - NULL, - }; + int compress; qemuDomainObjPrivatePtr priv; + /* + * We reuse "save" flag for "dump" here. Then, we can support the same + * format in "save" and "dump". + */ + compress = QEMUD_SAVE_FORMAT_RAW; + if (driver->dumpImageFormat) + compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5787,9 +5791,25 @@ static int qemudDomainCoreDump(virDomain } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorMigrateToFile(priv->mon, + if (compress == QEMUD_SAVE_FORMAT_RAW) { + const char *args[] = { + "cat", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0); + } else { + const char *prog = qemudSaveCompressionTypeToString(compress); + const char *args[] = { + prog, + "-c", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto endjob;
At compression, external programs are used but it is not checked whether the program is available or not. Check it at parsing qemu.conf and find problems in early stage. --- src/qemu/qemu_conf.c | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) Index: libvirt-0.8.4/src/qemu/qemu_conf.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c +++ libvirt-0.8.4/src/qemu/qemu_conf.c @@ -316,22 +316,54 @@ int qemudLoadDriverConfig(struct qemud_d p = virConfGetValue (conf, "save_image_format"); CHECK_TYPE ("save_image_format", VIR_CONF_STRING); if (p && p->str) { - VIR_FREE(driver->saveImageFormat); - if (!(driver->saveImageFormat = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + int find = 1; + if (strcmp(p->str, "raw")) { + char *c; + c = virFindFileInPath(p->str); + if (!c) + find = 0; + else + VIR_FREE(c); + } + VIR_FREE(driver->saveImageFormat); + if (find) { + if (!(driver->saveImageFormat = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "save_image_format cannot find program %s", p->str); + virConfFree(conf); + return -1; + } } p = virConfGetValue (conf, "dump_image_format"); CHECK_TYPE ("dump_image_format", VIR_CONF_STRING); if (p && p->str) { + int find = 1; + if (strcmp(p->str, "raw")) { + char *c; + c = virFindFileInPath(p->str); + if (!c) + find = 0; + else + VIR_FREE(c); + } VIR_FREE(driver->dumpImageFormat); - if (!(driver->dumpImageFormat = strdup(p->str))) { + if (find) { + if (!(driver->dumpImageFormat = strdup(p->str))) { virReportOOMError(); virConfFree(conf); return -1; + } + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "dump_image_format cannot find program %s", p->str); + virConfFree(conf); + return -1; } }
On Mon, Oct 25, 2010 at 09:05:29AM +0900, KAMEZAWA Hiroyuki wrote:
At compression, external programs are used but it is not checked whether the program is available or not. Check it at parsing qemu.conf and find problems in early stage.
The problem with doing the error check here is that it will cause the entire libvirtd to fail to startup, and just syslog the error. We've found admins often miss these types of problem in syslog and just file bugs complaining that libvirtd doesn't start. I think we should put an explicit check for existance of the compression program in the API code instead. That way the error message will get fed to the end user who will easily notice it.
--- src/qemu/qemu_conf.c | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-)
Index: libvirt-0.8.4/src/qemu/qemu_conf.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c +++ libvirt-0.8.4/src/qemu/qemu_conf.c @@ -316,22 +316,54 @@ int qemudLoadDriverConfig(struct qemud_d p = virConfGetValue (conf, "save_image_format"); CHECK_TYPE ("save_image_format", VIR_CONF_STRING); if (p && p->str) { - VIR_FREE(driver->saveImageFormat); - if (!(driver->saveImageFormat = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + int find = 1; + if (strcmp(p->str, "raw")) { + char *c; + c = virFindFileInPath(p->str); + if (!c) + find = 0; + else + VIR_FREE(c); + } + VIR_FREE(driver->saveImageFormat); + if (find) { + if (!(driver->saveImageFormat = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "save_image_format cannot find program %s", p->str); + virConfFree(conf); + return -1; + } }
p = virConfGetValue (conf, "dump_image_format"); CHECK_TYPE ("dump_image_format", VIR_CONF_STRING); if (p && p->str) { + int find = 1; + if (strcmp(p->str, "raw")) { + char *c; + c = virFindFileInPath(p->str); + if (!c) + find = 0; + else + VIR_FREE(c); + } VIR_FREE(driver->dumpImageFormat); - if (!(driver->dumpImageFormat = strdup(p->str))) { + if (find) { + if (!(driver->dumpImageFormat = strdup(p->str))) { virReportOOMError(); virConfFree(conf); return -1; + } + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "dump_image_format cannot find program %s", p->str); + virConfFree(conf); + return -1; } }
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, 25 Oct 2010 16:20:50 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Oct 25, 2010 at 09:05:29AM +0900, KAMEZAWA Hiroyuki wrote:
At compression, external programs are used but it is not checked whether the program is available or not. Check it at parsing qemu.conf and find problems in early stage.
The problem with doing the error check here is that it will cause the entire libvirtd to fail to startup, and just syslog the error. We've found admins often miss these types of problem in syslog and just file bugs complaining that libvirtd doesn't start.
Hmm, yes.
I think we should put an explicit check for existance of the compression program in the API code instead. That way the error message will get fed to the end user who will easily notice it.
Ok, I'll try it. I wonder which is better to make the command fail or run it in "raw" format when compression program cannot be found. Because virsh dump cannot be stopped by Ctrl-C, starting the dump can be critical decision. I think I should just make it fail and add "--raw" option to virsh.. Thanks, -Kame
On Tue, Oct 26, 2010 at 08:45:27AM +0900, KAMEZAWA Hiroyuki wrote:
On Mon, 25 Oct 2010 16:20:50 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Oct 25, 2010 at 09:05:29AM +0900, KAMEZAWA Hiroyuki wrote:
At compression, external programs are used but it is not checked whether the program is available or not. Check it at parsing qemu.conf and find problems in early stage.
The problem with doing the error check here is that it will cause the entire libvirtd to fail to startup, and just syslog the error. We've found admins often miss these types of problem in syslog and just file bugs complaining that libvirtd doesn't start.
Hmm, yes.
I think we should put an explicit check for existance of the compression program in the API code instead. That way the error message will get fed to the end user who will easily notice it.
Ok, I'll try it. I wonder which is better to make the command fail or run it in "raw" format when compression program cannot be found. Because virsh dump cannot be stopped by Ctrl-C, starting the dump can be critical decision.
The API impl should check whether the compression program exists, before trying to start the dump operation. If it doesn't exist, then send an error back to the user.
I think I should just make it fail and add "--raw" option to virsh..
Yes, we could add a VIR_DOMAIN_DUMP_RAW flag that forces a raw dump regardless of default compression setting. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Oct 25, 2010 at 09:04:10AM +0900, KAMEZAWA Hiroyuki wrote:
Sorry, email was empty..
== Add dump_image_format[] to qemu.conf and support compressed dump at virsh dump. coredump compression is important for saving disk space in an environment where multiple guest run. (In general, "disk space for dump" is specially allocated and will be a dead space in the system. It's used only at emergency. So, it's better to have both of save_image_format and dump_image_format. "save" is done in scheduled manner with enough calculated disk space for it.)
This code reuses some of save_image_format[] and supports the same format with virsh save.
--- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 11 +++++++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 30 +++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 5 deletions(-)
This all looks good, but it is also neccessary to add the new option to the augeas files libvirtd_qemu.aug and test_libvirtd_qemu.aug
Index: libvirt-0.8.4/src/qemu/qemu_conf.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c +++ libvirt-0.8.4/src/qemu/qemu_conf.c @@ -324,6 +324,17 @@ int qemudLoadDriverConfig(struct qemud_d } }
+ p = virConfGetValue (conf, "dump_image_format"); + CHECK_TYPE ("dump_image_format", VIR_CONF_STRING); + if (p && p->str) { + VIR_FREE(driver->dumpImageFormat); + if (!(driver->dumpImageFormat = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { Index: libvirt-0.8.4/src/qemu/qemu_conf.h =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.h +++ libvirt-0.8.4/src/qemu/qemu_conf.h @@ -159,6 +159,7 @@ struct qemud_driver { virSecurityDriverPtr securitySecondaryDriver;
char *saveImageFormat; + char *dumpImageFormat;
pciDeviceList *activePciHostdevs;
Index: libvirt-0.8.4/src/qemu/qemu.conf =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu.conf +++ libvirt-0.8.4/src/qemu/qemu.conf @@ -144,7 +144,11 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # +# save_image_format is used when you use 'virsh save' at scheduled saving. +# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# # save_image_format = "raw" +# dump_image_format = "raw"
# If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is Index: libvirt-0.8.4/src/qemu/qemu_driver.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_driver.c +++ libvirt-0.8.4/src/qemu/qemu_driver.c @@ -5716,11 +5716,15 @@ static int qemudDomainCoreDump(virDomain int resume = 0, paused = 0; int ret = -1, fd = -1; virDomainEventPtr event = NULL; - const char *args[] = { - "cat", - NULL, - }; + int compress; qemuDomainObjPrivatePtr priv; + /* + * We reuse "save" flag for "dump" here. Then, we can support the same + * format in "save" and "dump". + */ + compress = QEMUD_SAVE_FORMAT_RAW; + if (driver->dumpImageFormat) + compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat);
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5787,9 +5791,25 @@ static int qemudDomainCoreDump(virDomain }
qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorMigrateToFile(priv->mon, + if (compress == QEMUD_SAVE_FORMAT_RAW) { + const char *args[] = { + "cat", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0); + } else { + const char *prog = qemudSaveCompressionTypeToString(compress); + const char *args[] = { + prog, + "-c", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + }
The whitespace indentation looks wrong here, seems to be using tabs instead of spaces. You can verify coding style by running 'make syntax-check' Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, 25 Oct 2010 16:18:07 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Oct 25, 2010 at 09:04:10AM +0900, KAMEZAWA Hiroyuki wrote:
Sorry, email was empty..
== Add dump_image_format[] to qemu.conf and support compressed dump at virsh dump. coredump compression is important for saving disk space in an environment where multiple guest run. (In general, "disk space for dump" is specially allocated and will be a dead space in the system. It's used only at emergency. So, it's better to have both of save_image_format and dump_image_format. "save" is done in scheduled manner with enough calculated disk space for it.)
This code reuses some of save_image_format[] and supports the same format with virsh save.
--- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 11 +++++++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 30 +++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 5 deletions(-)
This all looks good, but it is also neccessary to add the new option to the augeas files libvirtd_qemu.aug and test_libvirtd_qemu.aug
Ah, I missed it. I'll look into.
Index: libvirt-0.8.4/src/qemu/qemu_conf.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c +++ libvirt-0.8.4/src/qemu/qemu_conf.c @@ -324,6 +324,17 @@ int qemudLoadDriverConfig(struct qemud_d } }
+ p = virConfGetValue (conf, "dump_image_format"); + CHECK_TYPE ("dump_image_format", VIR_CONF_STRING); + if (p && p->str) { + VIR_FREE(driver->dumpImageFormat); + if (!(driver->dumpImageFormat = strdup(p->str))) { + virReportOOMError(); + virConfFree(conf); + return -1; + } + } + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { Index: libvirt-0.8.4/src/qemu/qemu_conf.h =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_conf.h +++ libvirt-0.8.4/src/qemu/qemu_conf.h @@ -159,6 +159,7 @@ struct qemud_driver { virSecurityDriverPtr securitySecondaryDriver;
char *saveImageFormat; + char *dumpImageFormat;
pciDeviceList *activePciHostdevs;
Index: libvirt-0.8.4/src/qemu/qemu.conf =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu.conf +++ libvirt-0.8.4/src/qemu/qemu.conf @@ -144,7 +144,11 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # +# save_image_format is used when you use 'virsh save' at scheduled saving. +# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# # save_image_format = "raw" +# dump_image_format = "raw"
# If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is Index: libvirt-0.8.4/src/qemu/qemu_driver.c =================================================================== --- libvirt-0.8.4.orig/src/qemu/qemu_driver.c +++ libvirt-0.8.4/src/qemu/qemu_driver.c @@ -5716,11 +5716,15 @@ static int qemudDomainCoreDump(virDomain int resume = 0, paused = 0; int ret = -1, fd = -1; virDomainEventPtr event = NULL; - const char *args[] = { - "cat", - NULL, - }; + int compress; qemuDomainObjPrivatePtr priv; + /* + * We reuse "save" flag for "dump" here. Then, we can support the same + * format in "save" and "dump". + */ + compress = QEMUD_SAVE_FORMAT_RAW; + if (driver->dumpImageFormat) + compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat);
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5787,9 +5791,25 @@ static int qemudDomainCoreDump(virDomain }
qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorMigrateToFile(priv->mon, + if (compress == QEMUD_SAVE_FORMAT_RAW) { + const char *args[] = { + "cat", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0); + } else { + const char *prog = qemudSaveCompressionTypeToString(compress); + const char *args[] = { + prog, + "-c", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + }
The whitespace indentation looks wrong here, seems to be using tabs instead of spaces. You can verify coding style by running 'make syntax-check'
Ok. and check my .emacs.. Thank you for review. Regards, -Kame
On Tue, Oct 26, 2010 at 08:40:46AM +0900, KAMEZAWA Hiroyuki wrote:
On Mon, 25 Oct 2010 16:18:07 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote:
+ } else { + const char *prog = qemudSaveCompressionTypeToString(compress); + const char *args[] = { + prog, + "-c", + NULL, + }; + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); + }
The whitespace indentation looks wrong here, seems to be using tabs instead of spaces. You can verify coding style by running 'make syntax-check'
Ok. and check my .emacs..
This is what I use in .emacs (defun libvirt-c-mode () "C mode with adjusted defaults for use with libvirt." (interactive) (c-set-style "K&R") (setq indent-tabs-mode nil) ; indent using spaces, not TABs (setq c-indent-level 4) (setq c-basic-offset 4)) (add-hook 'c-mode-hook '(lambda () (if (string-match "/libvirt" (buffer-file-name)) (libvirt-c-mode)))) Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
KAMEZAWA Hiroyuki