[PATCH 0/3] qemu: implementation of transient option for qcow2 file
Hello, This patchset tries to implement transient option for qcow2 file. It gets user available to set <transient/> to the domain xml file like as: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk> Any changes which the Guest does to the disk is dropped when the Guest is shutdowned. Masayoshi Mizuma (3): qemu: implementation of transient option for qcow2 file testutilsqemu: Assign qemu-img path to driver->qemuImgBinary qemublocktest: add test of transient option for qcow2 file src/qemu/qemu_block.c | 71 +++++++++++++++++++ src/qemu/qemu_block.h | 7 ++ src/qemu/qemu_domain.c | 4 ++ src/qemu/qemu_process.c | 3 + src/qemu/qemu_validate.c | 2 +- tests/qemublocktest.c | 10 +++ .../xml2json/qcow2-transient-srconly.json | 9 +++ .../xml2json/qcow2-transient.json | 13 ++++ .../xml2json/qcow2-transient.xml | 13 ++++ tests/testutilsqemu.c | 6 +- 10 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.xml -- 2.27.0
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Here is the implementation of transient option for qcow2 file. This gets available <transient/> directive in domain xml file like as: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk> The internal procedure is as follows. When the qemu command line options are built, a new qcow2 image is created with backing qcow2 image by using qemu-img command. The backing image is the qcow2 file which is set as <source>. The filename of the new qcow2 image is original-source-file.TRANSIENT. The qemu-img will be: qemu-img create -f qcow2 -F qcow2 \ -b /var/lib/libvirt/images/guest.qcow2 \ /var/lib/libvirt/images/guest.qcow2.TRANSIENT Then, it switches the disk path, virStorageSourcePtr src->path, to the new qcow2 image. The new image and the backing image is handled and the blockdev option of qemu will be built like as: -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2", "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2", "file":"libvirt-2-storage","backing":null}' \ -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT", "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2", "file":"libvirt-1-storage","backing":"libvirt-2-format"}' The new qcow2 image is removed when the Guest is shutdowned, Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_block.c | 71 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 7 ++++ src/qemu/qemu_domain.c | 4 +++ src/qemu/qemu_process.c | 3 ++ src/qemu/qemu_validate.c | 2 +- 5 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6f9c707..5eb0225 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -27,6 +27,7 @@ #include "viralloc.h" #include "virstring.h" #include "virlog.h" +#include "virqemu.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -3438,3 +3439,73 @@ qemuBlockUpdateRelativeBacking(virDomainObjPtr vm, return 0; } + +int +qemuBlockCreateTransientDisk(virStorageSourcePtr src, + qemuDomainObjPrivatePtr priv) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virCommandPtr cmd = NULL; + g_autofree char *filename = NULL; + g_autofree char *dirname = NULL; + const char *qemuImgPath; + char *newpath; + int err = -1; + + if ((src->format != VIR_STORAGE_FILE_QCOW2)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("transient option supports qcow2")); + return -1; + } + + if (!(qemuImgPath = qemuFindQemuImgBinary(priv->driver))) + return -1; + + newpath = g_strdup_printf("%s.TRANSIENT", src->path); + + if (virFileExists(newpath)) { + virReportError(VIR_ERR_INVALID_ARG, + _(" '%s' is already exists. Please remove it."), newpath); + goto cleanup; + } + + if (!(cmd = virCommandNewArgList(qemuImgPath, + "create", + "-f", + "qcow2", + "-F", + "qcow2", + "-b", + NULL))) + goto cleanup; + + virQEMUBuildBufferEscapeComma(&buf, src->path); + virCommandAddArgBuffer(cmd, &buf); + + virQEMUBuildBufferEscapeComma(&buf, newpath); + virCommandAddArgBuffer(cmd, &buf); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + VIR_DEBUG("Original disk: %s Transient disk: %s", src->path, newpath); + + g_free(src->path); + src->path = newpath; + + err = 0; + cleanup: + virBufferFreeAndReset(&buf); + virCommandFree(cmd); + if (err) + g_free(newpath); + + return err; +} + +void +qemuBlockRemoveTransientDisk(virStorageSourcePtr src) +{ + VIR_DEBUG("unlink transient disk: %s ", src->path); + unlink(src->path); +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 2ad2ce1..60c6898 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -266,3 +266,10 @@ int qemuBlockUpdateRelativeBacking(virDomainObjPtr vm, virStorageSourcePtr src, virStorageSourcePtr topsrc); + +int +qemuBlockCreateTransientDisk(virStorageSourcePtr src, + qemuDomainObjPrivatePtr priv); + +void +qemuBlockRemoveTransientDisk(virStorageSourcePtr src); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d5e3d1a..9dbf73c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8301,6 +8301,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid); + if (disk->transient) + if (qemuBlockCreateTransientDisk(src, vm->privateData) < 0) + return -1; + if (virStorageFileGetMetadata(src, uid, gid, report_broken) < 0) return -1; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 999e576..859ef82 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7597,6 +7597,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, } qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); + + if (disk->transient) + qemuBlockRemoveTransientDisk(disk->src); } } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 584d137..cdda6ac 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2098,7 +2098,7 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } } - if (disk->transient) { + if ((disk->transient) && (disk->src->format != VIR_STORAGE_FILE_QCOW2)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("transient disks not supported yet")); return -1; -- 2.27.0
On Mon, Jul 06, 2020 at 14:20:23 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Here is the implementation of transient option for qcow2 file. This gets available <transient/> directive in domain xml file like as:
<disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/guest.qcow2'/> <target dev='vda' bus='virtio'/> <transient/> </disk>
The internal procedure is as follows. When the qemu command line options are built, a new qcow2 image is created with backing qcow2 image by using qemu-img command. The backing image is the qcow2 file which is set as <source>. The filename of the new qcow2 image is original-source-file.TRANSIENT. The qemu-img will be:
qemu-img create -f qcow2 -F qcow2 \ -b /var/lib/libvirt/images/guest.qcow2 \ /var/lib/libvirt/images/guest.qcow2.TRANSIENT
Then, it switches the disk path, virStorageSourcePtr src->path, to the new qcow2 image. The new image and the backing image is handled and the blockdev option of qemu will be built like as:
-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2", "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2", "file":"libvirt-2-storage","backing":null}' \ -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT", "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2", "file":"libvirt-1-storage","backing":"libvirt-2-format"}'
The new qcow2 image is removed when the Guest is shutdowned,
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- src/qemu/qemu_block.c | 71 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 7 ++++ src/qemu/qemu_domain.c | 4 +++ src/qemu/qemu_process.c | 3 ++ src/qemu/qemu_validate.c | 2 +- 5 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6f9c707..5eb0225 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -27,6 +27,7 @@ #include "viralloc.h" #include "virstring.h" #include "virlog.h" +#include "virqemu.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -3438,3 +3439,73 @@ qemuBlockUpdateRelativeBacking(virDomainObjPtr vm,
return 0; } + +int +qemuBlockCreateTransientDisk(virStorageSourcePtr src, + qemuDomainObjPrivatePtr priv) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virCommandPtr cmd = NULL; + g_autofree char *filename = NULL; + g_autofree char *dirname = NULL; + const char *qemuImgPath; + char *newpath; + int err = -1; + + if ((src->format != VIR_STORAGE_FILE_QCOW2)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("transient option supports qcow2")); + return -1; + } + + if (!(qemuImgPath = qemuFindQemuImgBinary(priv->driver))) + return -1; + + newpath = g_strdup_printf("%s.TRANSIENT", src->path);
This assumes that 'src' is a local disk, but the code doesn't check it. If e.g. an NBD disk which can have 'path' NULL is used it would crash. Specifically since we can't even create a new NBD disk this won't even wrok.
+ + if (virFileExists(newpath)) { + virReportError(VIR_ERR_INVALID_ARG, + _(" '%s' is already exists. Please remove it."), newpath); + goto cleanup; + } + + if (!(cmd = virCommandNewArgList(qemuImgPath, + "create", + "-f", + "qcow2", + "-F", + "qcow2", + "-b", + NULL))) + goto cleanup;
I'd strongly suggest you implement this after qemu starts using the new blockdev-create blockjob before starting the cpus and install the overlays using the snapshot command. The above will not work or will require many adjustments e.g. for luks-encrypted qcow2 files. which would require substantial changes.
+ virQEMUBuildBufferEscapeComma(&buf, src->path); + virCommandAddArgBuffer(cmd, &buf); + + virQEMUBuildBufferEscapeComma(&buf, newpath); + virCommandAddArgBuffer(cmd, &buf); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + VIR_DEBUG("Original disk: %s Transient disk: %s", src->path, newpath); + + g_free(src->path); + src->path = newpath;
This must add a new virStorageSource as 'src' and move the original to src->backingStore in the live configuration object. we should not try to detect the files which we know are there.
+ + err = 0; + cleanup: + virBufferFreeAndReset(&buf); + virCommandFree(cmd); + if (err) + g_free(newpath); + + return err; +} + +void +qemuBlockRemoveTransientDisk(virStorageSourcePtr src) +{ + VIR_DEBUG("unlink transient disk: %s ", src->path); + unlink(src->path); +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 2ad2ce1..60c6898 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -266,3 +266,10 @@ int qemuBlockUpdateRelativeBacking(virDomainObjPtr vm, virStorageSourcePtr src, virStorageSourcePtr topsrc); + +int +qemuBlockCreateTransientDisk(virStorageSourcePtr src, + qemuDomainObjPrivatePtr priv); + +void +qemuBlockRemoveTransientDisk(virStorageSourcePtr src); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d5e3d1a..9dbf73c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8301,6 +8301,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid);
+ if (disk->transient) + if (qemuBlockCreateTransientDisk(src, vm->privateData) < 0) + return -1;
The function call semantically doesn't belong to qemuDomainDetermineDiskChain. Please put it in appropriate places in the callers or the function which prepares the domain disk definition. Please also note that the hotplug code path then needs to be handled separately.
+ if (virStorageFileGetMetadata(src, uid, gid, report_broken) < 0) return -1;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 999e576..859ef82 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7597,6 +7597,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, }
qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); + + if (disk->transient) + qemuBlockRemoveTransientDisk(disk->src);
Disk hot-unplug requires then the same treatment.
} }
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 584d137..cdda6ac 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2098,7 +2098,7 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } }
- if (disk->transient) { + if ((disk->transient) && (disk->src->format != VIR_STORAGE_FILE_QCOW2)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("transient disks not supported yet"));
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that. As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that). Additionally there are corner cases which this patch doesn't deal with: 1) the virDomainBlockCopy operation flattens the backing chain into the top level only. This means that <transient/> must be stripped or the operation rejected, as otherwise shutting down the VM would end up removing the disk image completely. 2) the same as above is used also for non-shared-storage migration where we use block-copy internally to transport the disks, same as above applies. Here again it requires careful consideration of the semantics, e.g whether to reject it or e.g. copy it into the original filename and strip <transient/> (we can't currently copy multiple layers). 3) active-layer virDomainBlockCommit moves the data from the transient overlay into the original (now backing image). The <transient> flag is stored in the disk struct though, so that would mean that the original disk source would be removed after stopping the VM. block commit must clear the <transient> flag. One nice-to-have but not required modification would be to allow configuration of the transient disk's overlay path.
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Additionally there are corner cases which this patch doesn't deal with:
1) the virDomainBlockCopy operation flattens the backing chain into the top level only. This means that <transient/> must be stripped or the operation rejected, as otherwise shutting down the VM would end up removing the disk image completely.
If you have marked a disk transient, does it still make sense to allow copying that disk? Rejecting the operation is easiest, as permitting it implies that even though you already said you don't care about changes to your disk, you still want to be able to back up that disk.
2) the same as above is used also for non-shared-storage migration where we use block-copy internally to transport the disks, same as above applies. Here again it requires careful consideration of the semantics, e.g whether to reject it or e.g. copy it into the original filename and strip <transient/> (we can't currently copy multiple layers).
The easiest solution is to make a transient disk a migration-blocker. Of course, this may annoy people, so migration properly creating a transient destination on top of the original base, to preserve the fact that when the migrated guest shuts down it reverts to original contents, is a nicer (but more complex) goal.
3) active-layer virDomainBlockCommit moves the data from the transient overlay into the original (now backing image). The <transient> flag is stored in the disk struct though, so that would mean that the original disk source would be removed after stopping the VM. block commit must clear the <transient> flag.
Why should commit be permitted, when you declared that disk contents shouldn't change? For that matter, external snapshots should be blocked if there is a transient disk.
One nice-to-have but not required modification would be to allow configuration of the transient disk's overlay path.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Jul 07, 2020 at 06:36:23 -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
1) the virDomainBlockCopy operation flattens the backing chain into the top level only. This means that <transient/> must be stripped or the operation rejected, as otherwise shutting down the VM would end up removing the disk image completely.
If you have marked a disk transient, does it still make sense to allow copying that disk? Rejecting the operation is easiest, as permitting it implies that even though you already said you don't care about changes to your disk, you still want to be able to back up that disk.
Back up? This is more about moving to new storage.
2) the same as above is used also for non-shared-storage migration where we use block-copy internally to transport the disks, same as above applies. Here again it requires careful consideration of the semantics, e.g whether to reject it or e.g. copy it into the original filename and strip <transient/> (we can't currently copy multiple layers).
The easiest solution is to make a transient disk a migration-blocker. Of course, this may annoy people, so migration properly creating a transient destination on top of the original base, to preserve the fact that when the migrated guest shuts down it reverts to original contents, is a nicer (but more complex) goal.
To be fair, our docs already point out that <transient> prevents migration. It still needs to be implemented. Same appliest to snapshots.
3) active-layer virDomainBlockCommit moves the data from the transient overlay into the original (now backing image). The <transient> flag is stored in the disk struct though, so that would mean that the original disk source would be removed after stopping the VM. block commit must clear the <transient> flag.
Why should commit be permitted, when you declared that disk contents shouldn't change? For that matter, external snapshots should be blocked if there is a transient disk.
I might have read it in the qemu documentation, but commit is meant to actually ... commit ... the changes user might want to keep. I'm okay with forbidding it though in libvirt.
Thank you for your review. On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings. Does that make sense? 1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), and connect it. 2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(), qemuBlockStorageSourceAttachApplyStorage(), qemuBlockStorageSourceCreateGetFormatProps() and something... 3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then close the monitor. 4) Switch the original disk to the transient disk. 5) Build the blockdev argument for qemu. 6) Run qemu And I suppose qemuBlockStorageSourceCreate() maybe useful for the hotplug... Thanks, Masa
Additionally there are corner cases which this patch doesn't deal with:
1) the virDomainBlockCopy operation flattens the backing chain into the top level only. This means that <transient/> must be stripped or the operation rejected, as otherwise shutting down the VM would end up removing the disk image completely.
If you have marked a disk transient, does it still make sense to allow copying that disk? Rejecting the operation is easiest, as permitting it implies that even though you already said you don't care about changes to your disk, you still want to be able to back up that disk.
2) the same as above is used also for non-shared-storage migration where we use block-copy internally to transport the disks, same as above applies. Here again it requires careful consideration of the semantics, e.g whether to reject it or e.g. copy it into the original filename and strip <transient/> (we can't currently copy multiple layers).
The easiest solution is to make a transient disk a migration-blocker. Of course, this may annoy people, so migration properly creating a transient destination on top of the original base, to preserve the fact that when the migrated guest shuts down it reverts to original contents, is a nicer (but more complex) goal.
3) active-layer virDomainBlockCommit moves the data from the transient overlay into the original (now backing image). The <transient> flag is stored in the disk struct though, so that would mean that the original disk source would be removed after stopping the VM. block commit must clear the <transient> flag.
Why should commit be permitted, when you declared that disk contents shouldn't change? For that matter, external snapshots should be blocked if there is a transient disk.
One nice-to-have but not required modification would be to allow configuration of the transient disk's overlay path.
On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
Thank you for your review.
On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings. Does that make sense?
1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), and connect it.
Starting a new qemu process just to format an image is extreme overkill and definitely not what we want to do.
2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(), qemuBlockStorageSourceAttachApplyStorage(), qemuBlockStorageSourceCreateGetFormatProps() and something...
3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then close the monitor.
These two steps should be exectued in the qemu process which already will run the VM prior to starting the guest CPUs.
4) Switch the original disk to the transient disk.
5) Build the blockdev argument for qemu.
And instead of this step, you use the external snapshot infrastructure to install the overlays via 'blockdev-snapshot' QMP command
6) Run qemu
And instead of this the VM cpus will be started. The above steps require factoring out snapshot code a bit. I have a few patches in that direction so I'll try posting them next week hopefully.
On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
Thank you for your review.
On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings. Does that make sense?
1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), and connect it.
Starting a new qemu process just to format an image is extreme overkill and definitely not what we want to do.
I see, thanks.
2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(), qemuBlockStorageSourceAttachApplyStorage(), qemuBlockStorageSourceCreateGetFormatProps() and something...
3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then close the monitor.
These two steps should be exectued in the qemu process which already will run the VM prior to starting the guest CPUs.
4) Switch the original disk to the transient disk.
5) Build the blockdev argument for qemu.
And instead of this step, you use the external snapshot infrastructure to install the overlays via 'blockdev-snapshot' QMP command
OK. I suppose qemuDomainSnapshotDiskPrepare() and qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the setup steps of transient disk.
6) Run qemu
And instead of this the VM cpus will be started.
Got it, I think the appropriate place is after virCommandRun() at qemuProcessLaunch(), and before qemuProcessFinishStartup().
The above steps require factoring out snapshot code a bit. I have a few patches in that direction so I'll try posting them next week hopefully.
Great, thanks! - Masa
On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
Thank you for your review.
On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings. Does that make sense?
1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), and connect it.
Starting a new qemu process just to format an image is extreme overkill and definitely not what we want to do.
I see, thanks.
2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(), qemuBlockStorageSourceAttachApplyStorage(), qemuBlockStorageSourceCreateGetFormatProps() and something...
3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then close the monitor.
These two steps should be exectued in the qemu process which already will run the VM prior to starting the guest CPUs.
4) Switch the original disk to the transient disk.
5) Build the blockdev argument for qemu.
And instead of this step, you use the external snapshot infrastructure to install the overlays via 'blockdev-snapshot' QMP command
OK. I suppose qemuDomainSnapshotDiskPrepare() and qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the setup steps of transient disk.
6) Run qemu
And instead of this the VM cpus will be started.
Got it, I think the appropriate place is after virCommandRun() at qemuProcessLaunch(), and before qemuProcessFinishStartup().
The above steps require factoring out snapshot code a bit. I have a few patches in that direction so I'll try posting them next week hopefully.
Sorry this took longer than expected, but we were dealing with the build system change. The refactor is here: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will go through the disks and find the 'transient' ones. It will then create snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked snapshot disk object. 'qemuSnapshotDiskPrepareOne' ensures that the files are created and formatted properly. You then use same algorithm as 'qemuSnapshotCreateDiskActive' (e.g. by extracting the common internals (basically everything except call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse it when starting the VM as you've described above. Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is supported. The caveats/limitations with blockjobs and snapshots still apply though. It depends on how you approach it. It's okay to limit/block the features if transient disk is used. Alternatively you can implement some form of private data to mark which image was transient and allow those operations.
On Thu, Aug 06, 2020 at 12:09:20PM +0200, Peter Krempa wrote:
On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
Thank you for your review.
On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
You can install a qcow2 overlay on top of a raw file too. IMO the implications of using <transient/> allow that.
As said above I'd strongly prefer if the overlay is created in qemu using the blockdev-create blockjob (there is already infrastructure in libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings. Does that make sense?
1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), and connect it.
Starting a new qemu process just to format an image is extreme overkill and definitely not what we want to do.
I see, thanks.
2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(), qemuBlockStorageSourceAttachApplyStorage(), qemuBlockStorageSourceCreateGetFormatProps() and something...
3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then close the monitor.
These two steps should be exectued in the qemu process which already will run the VM prior to starting the guest CPUs.
4) Switch the original disk to the transient disk.
5) Build the blockdev argument for qemu.
And instead of this step, you use the external snapshot infrastructure to install the overlays via 'blockdev-snapshot' QMP command
OK. I suppose qemuDomainSnapshotDiskPrepare() and qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the setup steps of transient disk.
6) Run qemu
And instead of this the VM cpus will be started.
Got it, I think the appropriate place is after virCommandRun() at qemuProcessLaunch(), and before qemuProcessFinishStartup().
The above steps require factoring out snapshot code a bit. I have a few patches in that direction so I'll try posting them next week hopefully.
Sorry this took longer than expected, but we were dealing with the build system change.
The refactor is here:
https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html
You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will go through the disks and find the 'transient' ones. It will then create snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked snapshot disk object.
'qemuSnapshotDiskPrepareOne' ensures that the files are created and formatted properly.
You then use same algorithm as 'qemuSnapshotCreateDiskActive' (e.g. by extracting the common internals (basically everything except call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse it when starting the VM as you've described above.
Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is supported.
The caveats/limitations with blockjobs and snapshots still apply though. It depends on how you approach it. It's okay to limit/block the features if transient disk is used. Alternatively you can implement some form of private data to mark which image was transient and allow those operations.
Thank you for the helpful advice and the patches! I'll try to implement the transient disk procedure with the refactor. Thanks! Masa
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Assign qemu-img command file path to driver->qemuImgBinary so that the unit tests can use qemu-img command. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- tests/testutilsqemu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 4dcc308..8517f31 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -104,7 +104,8 @@ char * virFindFileInPath(const char *file) { if (g_str_has_prefix(file, "qemu-system") || - g_str_equal(file, "qemu-kvm")) { + g_str_equal(file, "qemu-kvm") || + g_str_equal(file, "qemu-img")) { return g_strdup_printf("/usr/bin/%s", file); } @@ -317,6 +318,7 @@ void qemuTestDriverFree(virQEMUDriver *driver) virObjectUnref(driver->caps); virObjectUnref(driver->config); virObjectUnref(driver->securityManager); + virObjectUnref(driver->qemuImgBinary); } int qemuTestCapsCacheInsert(virFileCachePtr cache, @@ -447,6 +449,8 @@ int qemuTestDriverInit(virQEMUDriver *driver) qemuTestSetHostCPU(driver, driver->hostarch, NULL); + driver->qemuImgBinary = virFindFileInPath("qemu-img"); + return 0; error: -- 2.27.0
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Add a unit test for transient option for qcow2 file. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- tests/qemublocktest.c | 10 ++++++++++ .../xml2json/qcow2-transient-srconly.json | 9 +++++++++ .../qemublocktestdata/xml2json/qcow2-transient.json | 13 +++++++++++++ .../qemublocktestdata/xml2json/qcow2-transient.xml | 13 +++++++++++++ 4 files changed, 45 insertions(+) create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.xml diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 0cdedb9..1294c18 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -266,6 +266,7 @@ testQemuDiskXMLToProps(const void *opaque) g_autoptr(virJSONValue) formatProps = NULL; g_autoptr(virJSONValue) storageProps = NULL; g_autoptr(virJSONValue) storageSrcOnlyProps = NULL; + qemuDomainObjPrivate priv; g_autofree char *xmlpath = NULL; g_autofree char *xmlstr = NULL; @@ -288,6 +289,13 @@ testQemuDiskXMLToProps(const void *opaque) return -1; } + if (disk->transient) { + priv.driver = data->driver; + if (qemuBlockCreateTransientDisk(disk->src, &priv) < 0) + return EXIT_AM_SKIP; + unlink(disk->src->path); + } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { g_autofree char *backingstore = NULL; @@ -1226,6 +1234,8 @@ mymain(void) TEST_DISK_TO_JSON("nvme-raw-noopts"); + TEST_DISK_TO_JSON("qcow2-transient"); + #define TEST_JSON_TO_JSON(nme) \ do { \ jsontojsondata.name = nme; \ diff --git a/tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json b/tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json new file mode 100644 index 0000000..3c2de91 --- /dev/null +++ b/tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json @@ -0,0 +1,9 @@ +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/transient.qcow2.TRANSIENT" + } + backing store string: + /var/lib/libvirt/images/transient.qcow2.TRANSIENT +) diff --git a/tests/qemublocktestdata/xml2json/qcow2-transient.json b/tests/qemublocktestdata/xml2json/qcow2-transient.json new file mode 100644 index 0000000..57463e1 --- /dev/null +++ b/tests/qemublocktestdata/xml2json/qcow2-transient.json @@ -0,0 +1,13 @@ +{ + "node-name": "1234567890", + "read-only": false, + "driver": "qcow2", + "file": "1234567890" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/transient.qcow2.TRANSIENT", + "node-name": "1234567890", + "auto-read-only": true, + "discard": "unmap" +} diff --git a/tests/qemublocktestdata/xml2json/qcow2-transient.xml b/tests/qemublocktestdata/xml2json/qcow2-transient.xml new file mode 100644 index 0000000..d2e3919 --- /dev/null +++ b/tests/qemublocktestdata/xml2json/qcow2-transient.xml @@ -0,0 +1,13 @@ +<disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/transient.qcow2'> + <privateData> + <nodenames> + <nodename type='storage' name='1234567890'/> + <nodename type='format' name='1234567890'/> + </nodenames> + </privateData> + </source> + <target dev='vda' bus='virtio'/> + <transient/> +</disk> -- 2.27.0
On Mon, Jul 06, 2020 at 14:20:25 -0400, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Add a unit test for transient option for qcow2 file.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- tests/qemublocktest.c | 10 ++++++++++ .../xml2json/qcow2-transient-srconly.json | 9 +++++++++ .../qemublocktestdata/xml2json/qcow2-transient.json | 13 +++++++++++++ .../qemublocktestdata/xml2json/qcow2-transient.xml | 13 +++++++++++++ 4 files changed, 45 insertions(+) create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.json create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.xml
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 0cdedb9..1294c18 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -266,6 +266,7 @@ testQemuDiskXMLToProps(const void *opaque) g_autoptr(virJSONValue) formatProps = NULL; g_autoptr(virJSONValue) storageProps = NULL; g_autoptr(virJSONValue) storageSrcOnlyProps = NULL; + qemuDomainObjPrivate priv; g_autofree char *xmlpath = NULL; g_autofree char *xmlstr = NULL;
@@ -288,6 +289,13 @@ testQemuDiskXMLToProps(const void *opaque) return -1; }
+ if (disk->transient) { + priv.driver = data->driver; + if (qemuBlockCreateTransientDisk(disk->src, &priv) < 0)
NACK, this would create files on the system running the test suite in random paths according to the disk config. The test-suite must never do that.
participants (3)
-
Eric Blake -
Masayoshi Mizuma -
Peter Krempa