[PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints
This is basically just rebase of [1] as it was not get any attention at that time. [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html Nikolay Shirokovskiy (10): qemu: qemuDomainRenameCallback: fix sending false undefined event qemu: rename: send events only on success qemu: rename: return instead of goto if no cleanup required qemu: remove duplicate code for removing remnant files qemu: rename: support renaming snapshots directory qemu: rename: support renaming checkpoints directory qemu: update name on reverting from snapshot qemu: rename: remove snapshot/checkpoint restriction qemu: qemuDomainDefineXMLFlags: move cleanup logic to cleanup section qemu: remove possible garbage left from previous rename/undefine src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_checkpoint.h | 6 ++ src/qemu/qemu_domain.c | 43 ++++++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_driver.c | 158 ++++++++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 3 + src/qemu/qemu_snapshot.c | 10 +++ 7 files changed, 160 insertions(+), 67 deletions(-) -- 1.8.3.1
For example if saving config file with new name fails we send false undefine event currently. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cb56fbb..2f2a55f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19152,10 +19152,6 @@ qemuDomainRenameCallback(virDomainObjPtr vm, } } - event_old = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_UNDEFINED, - VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); - /* Switch name in domain definition. */ old_dom_name = vm->def->name; vm->def->name = new_dom_name; @@ -19182,6 +19178,9 @@ qemuDomainRenameCallback(virDomainObjPtr vm, } } + event_old = virDomainEventLifecycleNew(vm->def->id, old_dom_name, vm->def->uuid, + VIR_DOMAIN_EVENT_UNDEFINED, + VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); event_new = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, VIR_DOMAIN_EVENT_DEFINED_RENAMED); -- 1.8.3.1
We can simplify cleanup section by moving sending events to success path only because only on sucess path events are not NULL. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f2a55f..22df0f6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19184,11 +19184,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, event_new = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, VIR_DOMAIN_EVENT_DEFINED_RENAMED); + virObjectEventStateQueue(driver->domainEventState, event_old); + virObjectEventStateQueue(driver->domainEventState, event_new); ret = 0; cleanup: - virObjectEventStateQueue(driver->domainEventState, event_old); - virObjectEventStateQueue(driver->domainEventState, event_new); return ret; rollback: -- 1.8.3.1
Going to cleanup label is mere return -1 thus let's just return instead of goto to this label. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22df0f6..621facc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19135,20 +19135,20 @@ qemuDomainRenameCallback(virDomainObjPtr vm, new_dom_name)) || !(old_dom_cfg_file = virDomainConfigFile(cfg->configDir, vm->def->name))) - goto cleanup; + return -1; if (vm->autostart) { if (!(new_dom_autostart_link = virDomainConfigFile(cfg->autostartDir, new_dom_name)) || !(old_dom_autostart_link = virDomainConfigFile(cfg->autostartDir, vm->def->name))) - goto cleanup; + return -1; if (symlink(new_dom_cfg_file, new_dom_autostart_link) < 0) { virReportSystemError(errno, _("Failed to create symlink '%s to '%s'"), new_dom_autostart_link, new_dom_cfg_file); - goto cleanup; + return -1; } } -- 1.8.3.1
This patch also changes functionality a bit. First if unlinking of old config file is failed we rollback and return error previously and now we return success. I don't think this makes much difference. I guess in both cases on libvirtd restart we have to deal with both new and old config existing on disk with different names but same uuid. Second if unlinking of old autolink is failed we rollback previously which was not right as at this point we already unlink old config file. So this is fixed now. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 39 ++++++++------------------------------- 3 files changed, 42 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d7dbca4..7b2c165 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11094,3 +11094,32 @@ qemuDomainInterfaceSetDefaultQDisc(virQEMUDriverPtr driver, return 0; } + + +int +qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, + const char *name, + bool bestEffort) +{ + g_autofree char *cfg_file = NULL; + g_autofree char *autostart_link = NULL; + + cfg_file = virDomainConfigFile(cfg->configDir, name); + autostart_link = virDomainConfigFile(cfg->autostartDir, name); + + if (virFileExists(cfg_file) && + unlink(cfg_file) < 0) { + virReportSystemError(errno, _("Failed to unlink '%s'"), cfg_file); + if (!bestEffort) + return -1; + } + + if (virFileIsLink(autostart_link) == 1 && + unlink(autostart_link) < 0) { + virReportSystemError(errno, _("Failed to unlink '%s'"), autostart_link); + if (!bestEffort) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index fc69678..a2dfe86 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1050,3 +1050,8 @@ qemuDomainFileWrapperFDClose(virDomainObjPtr vm, int qemuDomainInterfaceSetDefaultQDisc(virQEMUDriverPtr driver, virDomainNetDefPtr net); + +int +qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, + const char *name, + bool bestEffort); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 621facc..52c42cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19112,6 +19112,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm, virObjectEventPtr event_new = NULL; virObjectEventPtr event_old = NULL; int ret = -1; + virErrorPtr err = NULL; g_autofree char *new_dom_name = NULL; g_autofree char *old_dom_name = NULL; g_autofree char *new_dom_cfg_file = NULL; @@ -19158,25 +19159,7 @@ qemuDomainRenameCallback(virDomainObjPtr vm, new_dom_name = NULL; if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) - goto rollback; - - if (virFileExists(old_dom_cfg_file) && - unlink(old_dom_cfg_file) < 0) { - virReportSystemError(errno, - _("cannot remove old domain config file %s"), - old_dom_cfg_file); - goto rollback; - } - - if (vm->autostart) { - if (virFileIsLink(old_dom_autostart_link) && - unlink(old_dom_autostart_link) < 0) { - virReportSystemError(errno, - _("Failed to delete symlink '%s'"), - old_dom_autostart_link); - goto rollback; - } - } + goto cleanup; event_old = virDomainEventLifecycleNew(vm->def->id, old_dom_name, vm->def->uuid, VIR_DOMAIN_EVENT_UNDEFINED, @@ -19189,23 +19172,17 @@ qemuDomainRenameCallback(virDomainObjPtr vm, ret = 0; cleanup: - return ret; - - rollback: - if (old_dom_name) { + if (old_dom_name && ret < 0) { new_dom_name = vm->def->name; vm->def->name = old_dom_name; old_dom_name = NULL; } - if (virFileExists(new_dom_cfg_file)) - unlink(new_dom_cfg_file); - - if (vm->autostart && - virFileExists(new_dom_autostart_link)) - unlink(new_dom_autostart_link); - - goto cleanup; + if (ret < 0) + virErrorPreserveLast(&err); + qemuDomainNamePathsCleanup(cfg, ret < 0 ? new_dom_name : old_dom_name, true); + virErrorRestore(&err); + return ret; } static int qemuDomainRename(virDomainPtr dom, -- 1.8.3.1
This is basically just saves snapshots metadata on disk after name is changed in memory as path to domain snapshot directory depends on name. After that old snapshot directory is deleted with snapshot metadata files. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7b2c165..258c8c5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11103,9 +11103,11 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, { g_autofree char *cfg_file = NULL; g_autofree char *autostart_link = NULL; + g_autofree char *snap_dir = NULL; cfg_file = virDomainConfigFile(cfg->configDir, name); autostart_link = virDomainConfigFile(cfg->autostartDir, name); + snap_dir = g_strdup_printf("%s/%s", cfg->snapshotDir, name); if (virFileExists(cfg_file) && unlink(cfg_file) < 0) { @@ -11121,5 +11123,10 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, return -1; } + if (virFileIsDir(snap_dir) && + virFileDeleteTree(snap_dir) < 0 && + !bestEffort) + return -1; + return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52c42cf..2664603 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19101,6 +19101,30 @@ qemuDomainSetUserPassword(virDomainPtr dom, } +struct qemuDomainMomentWriteMetadataData { + virQEMUDriverPtr driver; + virDomainObjPtr vm; +}; + + +static int +qemuDomainSnapshotWriteMetadataIter(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + struct qemuDomainMomentWriteMetadataData *data = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(data->driver); + int ret; + + ret = qemuDomainSnapshotWriteMetadata(data->vm, payload, + data->driver->xmlopt, + cfg->snapshotDir); + + virObjectUnref(cfg); + return ret; +} + + static int qemuDomainRenameCallback(virDomainObjPtr vm, const char *new_name, @@ -19119,6 +19143,10 @@ qemuDomainRenameCallback(virDomainObjPtr vm, g_autofree char *old_dom_cfg_file = NULL; g_autofree char *new_dom_autostart_link = NULL; g_autofree char *old_dom_autostart_link = NULL; + struct qemuDomainMomentWriteMetadataData data = { + .driver = driver, + .vm = vm, + }; virCheckFlags(0, ret); @@ -19158,6 +19186,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, vm->def->name = new_dom_name; new_dom_name = NULL; + if (virDomainSnapshotForEach(vm->snapshots, + qemuDomainSnapshotWriteMetadataIter, + &data) < 0) + goto cleanup; + if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; -- 1.8.3.1
On 11/3/20 9:00 AM, Nikolay Shirokovskiy wrote:
This is basically just saves snapshots metadata on disk after name is changed
Extra 'is'. "This basically just saves ...."
in memory as path to domain snapshot directory depends on name. After that old snapshot directory is deleted with snapshot metadata files.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7b2c165..258c8c5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11103,9 +11103,11 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, { g_autofree char *cfg_file = NULL; g_autofree char *autostart_link = NULL; + g_autofree char *snap_dir = NULL;
cfg_file = virDomainConfigFile(cfg->configDir, name); autostart_link = virDomainConfigFile(cfg->autostartDir, name); + snap_dir = g_strdup_printf("%s/%s", cfg->snapshotDir, name);
if (virFileExists(cfg_file) && unlink(cfg_file) < 0) { @@ -11121,5 +11123,10 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, return -1; }
+ if (virFileIsDir(snap_dir) && + virFileDeleteTree(snap_dir) < 0 && + !bestEffort) + return -1; + return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52c42cf..2664603 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19101,6 +19101,30 @@ qemuDomainSetUserPassword(virDomainPtr dom, }
+struct qemuDomainMomentWriteMetadataData { + virQEMUDriverPtr driver; + virDomainObjPtr vm; +}; + + +static int +qemuDomainSnapshotWriteMetadataIter(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + struct qemuDomainMomentWriteMetadataData *data = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(data->driver); + int ret; + + ret = qemuDomainSnapshotWriteMetadata(data->vm, payload, + data->driver->xmlopt, + cfg->snapshotDir); + + virObjectUnref(cfg); + return ret; +} + + static int qemuDomainRenameCallback(virDomainObjPtr vm, const char *new_name, @@ -19119,6 +19143,10 @@ qemuDomainRenameCallback(virDomainObjPtr vm, g_autofree char *old_dom_cfg_file = NULL; g_autofree char *new_dom_autostart_link = NULL; g_autofree char *old_dom_autostart_link = NULL; + struct qemuDomainMomentWriteMetadataData data = { + .driver = driver, + .vm = vm, + };
virCheckFlags(0, ret);
@@ -19158,6 +19186,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, vm->def->name = new_dom_name; new_dom_name = NULL;
+ if (virDomainSnapshotForEach(vm->snapshots, + qemuDomainSnapshotWriteMetadataIter, + &data) < 0) + goto cleanup; + if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup;
This is basically just saves checkpoints metadata on disk after name is changed in memory as path to domain checkpoints directory depends on name. After that old checkpoint directory is deleted with checkpoint metadata files. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_checkpoint.h | 6 ++++++ src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_driver.c | 23 +++++++++++++++++++++++ 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index fb76c21..4a496d5 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -76,7 +76,7 @@ qemuCheckpointObjFromCheckpoint(virDomainObjPtr vm, } -static int +int qemuCheckpointWriteMetadata(virDomainObjPtr vm, virDomainMomentObjPtr checkpoint, virDomainXMLOptionPtr xmlopt, diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h index 0d267a1..d58657e 100644 --- a/src/qemu/qemu_checkpoint.h +++ b/src/qemu/qemu_checkpoint.h @@ -79,3 +79,9 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, virJSONValuePtr actions, const char *diskdst, GSList **reopenimages); + +int +qemuCheckpointWriteMetadata(virDomainObjPtr vm, + virDomainMomentObjPtr checkpoint, + virDomainXMLOptionPtr xmlopt, + const char *checkpointDir); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 258c8c5..422b534 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11104,10 +11104,12 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, g_autofree char *cfg_file = NULL; g_autofree char *autostart_link = NULL; g_autofree char *snap_dir = NULL; + g_autofree char *chk_dir = NULL; cfg_file = virDomainConfigFile(cfg->configDir, name); autostart_link = virDomainConfigFile(cfg->autostartDir, name); snap_dir = g_strdup_printf("%s/%s", cfg->snapshotDir, name); + chk_dir = g_strdup_printf("%s/%s", cfg->checkpointDir, name); if (virFileExists(cfg_file) && unlink(cfg_file) < 0) { @@ -11128,5 +11130,10 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, !bestEffort) return -1; + if (virFileIsDir(chk_dir) && + virFileDeleteTree(chk_dir) < 0 && + !bestEffort) + return -1; + return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2664603..4e0186b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19126,6 +19126,24 @@ qemuDomainSnapshotWriteMetadataIter(void *payload, static int +qemuDomainCheckpointWriteMetadataIter(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + struct qemuDomainMomentWriteMetadataData *data = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(data->driver); + int ret; + + ret = qemuCheckpointWriteMetadata(data->vm, payload, + data->driver->xmlopt, + cfg->snapshotDir); + + virObjectUnref(cfg); + return ret; +} + + +static int qemuDomainRenameCallback(virDomainObjPtr vm, const char *new_name, unsigned int flags, @@ -19191,6 +19209,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, &data) < 0) goto cleanup; + if (virDomainCheckpointForEach(vm->checkpoints, + qemuDomainCheckpointWriteMetadataIter, + &data) < 0) + goto cleanup; + if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; -- 1.8.3.1
On 11/3/20 9:00 AM, Nikolay Shirokovskiy wrote:
This is basically just saves checkpoints metadata on disk after name is changed
"This basically just saves ..."
in memory as path to domain checkpoints directory depends on name. After that old checkpoint directory is deleted with checkpoint metadata files.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_checkpoint.h | 6 ++++++ src/qemu/qemu_domain.c | 7 +++++++ src/qemu/qemu_driver.c | 23 +++++++++++++++++++++++ 4 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index fb76c21..4a496d5 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -76,7 +76,7 @@ qemuCheckpointObjFromCheckpoint(virDomainObjPtr vm, }
-static int +int qemuCheckpointWriteMetadata(virDomainObjPtr vm, virDomainMomentObjPtr checkpoint, virDomainXMLOptionPtr xmlopt, diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h index 0d267a1..d58657e 100644 --- a/src/qemu/qemu_checkpoint.h +++ b/src/qemu/qemu_checkpoint.h @@ -79,3 +79,9 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, virJSONValuePtr actions, const char *diskdst, GSList **reopenimages); + +int +qemuCheckpointWriteMetadata(virDomainObjPtr vm, + virDomainMomentObjPtr checkpoint, + virDomainXMLOptionPtr xmlopt, + const char *checkpointDir); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 258c8c5..422b534 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11104,10 +11104,12 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, g_autofree char *cfg_file = NULL; g_autofree char *autostart_link = NULL; g_autofree char *snap_dir = NULL; + g_autofree char *chk_dir = NULL;
cfg_file = virDomainConfigFile(cfg->configDir, name); autostart_link = virDomainConfigFile(cfg->autostartDir, name); snap_dir = g_strdup_printf("%s/%s", cfg->snapshotDir, name); + chk_dir = g_strdup_printf("%s/%s", cfg->checkpointDir, name);
if (virFileExists(cfg_file) && unlink(cfg_file) < 0) { @@ -11128,5 +11130,10 @@ qemuDomainNamePathsCleanup(virQEMUDriverConfigPtr cfg, !bestEffort) return -1;
+ if (virFileIsDir(chk_dir) && + virFileDeleteTree(chk_dir) < 0 && + !bestEffort) + return -1; + return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2664603..4e0186b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19126,6 +19126,24 @@ qemuDomainSnapshotWriteMetadataIter(void *payload,
static int +qemuDomainCheckpointWriteMetadataIter(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + struct qemuDomainMomentWriteMetadataData *data = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(data->driver); + int ret; + + ret = qemuCheckpointWriteMetadata(data->vm, payload, + data->driver->xmlopt, + cfg->snapshotDir); + + virObjectUnref(cfg); + return ret; +} + + +static int qemuDomainRenameCallback(virDomainObjPtr vm, const char *new_name, unsigned int flags, @@ -19191,6 +19209,11 @@ qemuDomainRenameCallback(virDomainObjPtr vm, &data) < 0) goto cleanup;
+ if (virDomainCheckpointForEach(vm->checkpoints, + qemuDomainCheckpointWriteMetadataIter, + &data) < 0) + goto cleanup; + if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup;
If domain name is changed since snapshot we need to update it to current in config taken from snapshot. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_snapshot.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a6241ab..c9f2be0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1924,6 +1924,11 @@ qemuSnapshotRevert(virDomainObjPtr vm, driver->xmlopt, priv->qemuCaps, true); if (!config) goto endjob; + + if (STRNEQ(config->name, vm->def->name)) { + VIR_FREE(config->name); + config->name = g_strdup(vm->def->name); + } } if (snap->def->inactiveDom) { @@ -1931,6 +1936,11 @@ qemuSnapshotRevert(virDomainObjPtr vm, driver->xmlopt, priv->qemuCaps, true); if (!inactiveConfig) goto endjob; + + if (STRNEQ(inactiveConfig->name, vm->def->name)) { + VIR_FREE(inactiveConfig->name); + inactiveConfig->name = g_strdup(vm->def->name); + } } else { /* Inactive domain definition is missing: * - either this is an old active snapshot and we need to copy the -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e0186b..86ff74f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19284,18 +19284,6 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } - if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot rename domain with snapshots")); - goto endjob; - } - - if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, flags) > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot rename domain with checkpoints")); - goto endjob; - } - if (virDomainObjListRename(driver->domains, vm, new_name, flags, qemuDomainRenameCallback, driver) < 0) goto endjob; -- 1.8.3.1
Let's move objlist restoring to cleanup section so that we can handle failure of actions between virDomainObjListAdd and virDomainDefSave. We are going to add such actions in next patch. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 86ff74f..6c353a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6705,7 +6705,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) - goto cleanup; + return NULL; if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; @@ -6719,10 +6719,23 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, goto cleanup; def = NULL; + if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def, + driver->xmlopt, cfg->configDir) < 0) + goto cleanup; + vm->persistent = 1; - if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def, - driver->xmlopt, cfg->configDir) < 0) { + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_DEFINED, + !oldDef ? + VIR_DOMAIN_EVENT_DEFINED_ADDED : + VIR_DOMAIN_EVENT_DEFINED_UPDATED); + + VIR_INFO("Creating domain '%s'", vm->def->name); + dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); + + cleanup: + if (!dom && !def) { if (oldDef) { /* There is backup so this VM was defined before. * Just restore the backup. */ @@ -6735,22 +6748,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - vm->persistent = 0; qemuDomainRemoveInactiveJob(driver, vm); } - goto cleanup; } - - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_DEFINED, - !oldDef ? - VIR_DOMAIN_EVENT_DEFINED_ADDED : - VIR_DOMAIN_EVENT_DEFINED_UPDATED); - - VIR_INFO("Creating domain '%s'", vm->def->name); - dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - - cleanup: virDomainDefFree(oldDef); virDomainDefFree(def); virDomainObjEndAPI(&vm); -- 1.8.3.1
Due to failures to unlink on previous rename/undefine we can already have autolink etc files for the domain to be defined. Remove them. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_migration.c | 3 +++ 2 files changed, 9 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6c353a5..3b7d1d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6719,6 +6719,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, goto cleanup; def = NULL; + if (!oldDef && qemuDomainNamePathsCleanup(cfg, vm->def->name, false) < 0) + goto cleanup; + if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; @@ -19184,6 +19187,9 @@ qemuDomainRenameCallback(virDomainObjPtr vm, vm->def->name))) return -1; + if (qemuDomainNamePathsCleanup(cfg, new_name, false) < 0) + goto cleanup; + if (vm->autostart) { if (!(new_dom_autostart_link = virDomainConfigFile(cfg->autostartDir, new_dom_name)) || diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6f764b0..b4fb586 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5137,6 +5137,9 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver, priv->qemuCaps))) goto error; + if (!oldDef && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0) + goto error; + if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 && !ignoreSaveError) goto error; -- 1.8.3.1
On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
This is basically just rebase of [1] as it was not get any attention at that time.
[1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
Code LGTM: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Shouldn't you add some test cases for this new behavior though? I'm a bit nervous with pushing this upstream without any coverage. Thanks, DHB
Nikolay Shirokovskiy (10): qemu: qemuDomainRenameCallback: fix sending false undefined event qemu: rename: send events only on success qemu: rename: return instead of goto if no cleanup required qemu: remove duplicate code for removing remnant files qemu: rename: support renaming snapshots directory qemu: rename: support renaming checkpoints directory qemu: update name on reverting from snapshot qemu: rename: remove snapshot/checkpoint restriction qemu: qemuDomainDefineXMLFlags: move cleanup logic to cleanup section qemu: remove possible garbage left from previous rename/undefine
src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_checkpoint.h | 6 ++ src/qemu/qemu_domain.c | 43 ++++++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_driver.c | 158 ++++++++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 3 + src/qemu/qemu_snapshot.c | 10 +++ 7 files changed, 160 insertions(+), 67 deletions(-)
-- 1.8.3.1
On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
This is basically just rebase of [1] as it was not get any attention at that time.
[1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
Code LGTM:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Shouldn't you add some test cases for this new behavior though? I'm a bit nervous with pushing this upstream without any coverage.
So basically we need to test how qemuDomainRenameCallback works. Currently there is no test for this function or virDomainRename. I spent some time looking at existing tests that need to mock driver/vm objects and it looks like it requires a good deal of effort in order to prepare the test in this case. At the same time those tests has many inputs so it looks like worth heavy preparation. In case of rename the code we test does not depend greatly on inputs so may be it does not worth adding such test given heavy preparation we have to do. Of course I give the patch series some manual testing. Nikolay
On 10.11.2020 10:58, Nikolay Shirokovskiy wrote:
On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
This is basically just rebase of [1] as it was not get any attention at that time.
[1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
Code LGTM:
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Shouldn't you add some test cases for this new behavior though? I'm a bit nervous with pushing this upstream without any coverage.
So basically we need to test how qemuDomainRenameCallback works. Currently there is no test for this function or virDomainRename. I spent some time looking at existing tests that need to mock driver/vm objects and it looks like it requires a good deal of effort in order to prepare the test in this case. At the same time those tests has many inputs so it looks like worth heavy preparation. In case of rename the code we test does not depend greatly on inputs so may be it does not worth adding such test given heavy preparation we have to do.
Of course I give the patch series some manual testing.
Thanx for the review! Pushed with next hunk squashed into the last patch: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c831ae6..fef0be6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5137,7 +5137,7 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver, priv->qemuCaps))) goto error; - if (!oldDef && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0) + if (!oldPersist && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0) goto error; if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 && Nikolay
participants (2)
-
Daniel Henrique Barboza -
Nikolay Shirokovskiy