On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

Inorder to capture the exact state of domain, inactive configuration

In order

is needed along with active configuration. This patch stores inactive domain configuration when creating snapshot of a running domain. It also captures the inactive snapshot configuration when a snapshot is redefined.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- src/conf/snapshot_conf.c | 13 +++++++++++++ src/conf/snapshot_conf.h | 1 + src/qemu/qemu_driver.c | 10 ++++++++++ 3 files changed, 24 insertions(+)

Caveat emptor - I'm not 100% familiar with all/any of the snapshot_conf code and processing, but I'll look at the patches to provide some feedback... Editorial comment... Using "newDef" has always been a bit "confusing" to say the least. I know at one point there was a comment on list in some review that we should rename it to something else to indicate what it really is, but I have forgotten what the recommendation was. Hopefully, someone else can remember! Perhaps we can "avoid" continuing the confusion with this set of changes. Even more hopefully, someone spends the time to change domain newDef ;-)! That might even fix what I'm finding confusing about these patches... Looking at existing code prior to these changes and thinking about the context of just these changes as they relate to def/newDef - I'm not sure you're capturing "everything". For instance, I see code in qemuDomainSnapshotDiskDataCollect makes specific decisions about which disks to collect, but there's no adjustment in that code with this series. There's a comment in qemuDomainRevertToSnapshot from a hunk of code just above the !REVERT_ACTIVE_ONLY from patch 2: " /* Prepare to copy the snapshot inactive xml as the config of this * domain. * * XXX Should domain snapshots track live xml rather * than inactive xml? */ " and from qemuDomainSnapshotCreateXML above where you add the newDef copy in this patch: " /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ " So what's confusing (to say the least) is - which "def" is really being used. The code at times says inactive, but IIUC newDef is generally treated as the persistent def, but that only matters if something has caused it to be created. Of course, I assume this confusion is what you're trying to fix based on the cover letter comments! If the intention of these patches is to essentially "mimic" the domain def/newDef functionality, then perhaps thinking in terms of something like virDomainObjGetPersistentDef may be helpful to "hide" which is which and when one applies over the other. (does that even make sense?)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f0e852c..bfe3d6c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) virDomainSnapshotDiskDefClear(&def->disks[i]); VIR_FREE(def->disks); virDomainDefFree(def->dom); + virDomainDefFree(def->newDom); virObjectUnref(def->cookie); VIR_FREE(def); } @@ -1336,6 +1337,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } }

+ if (other->def->newDom) { + if (def->newDom) { + if (!virDomainDefCheckABIStability(other->def->newDom, + def->newDom, xmlopt)) + goto cleanup; + } else { + /* Transfer the inactive domain def */ + def->newDom = other->def->newDom; + other->def->newDom = NULL;

/* Steal the inactive domain def */ VIR_STEAL_PTR(def->newDom, other->def->newDom); [yes, I realize this is just stealing the code a few lines above, but the more recent desire is to use the macro]

+ } + } + if (other == vm->current_snapshot) { *update_current = true; vm->current_snapshot = NULL; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1d663c7..0bc915f 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -75,6 +75,7 @@ struct _virDomainSnapshotDef { virDomainSnapshotDiskDef *disks;

virDomainDefPtr dom; + virDomainDefPtr newDom;

virObjectPtr cookie;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 74fdfdb..4ffec70 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15035,6 +15035,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob;

IIUC, the above hunk is designed to grab from vm->def anything that was felt to be in the domain def making it as if it was some sort of inactive definition. But since it's copying the live definition it perhaps copies portions of the live definition that have changed, such as via hotplug. That leaves a couple of questions in my mind related to whether restoration from a snapshot would/should "restore" that hotplug element especially if it doesn't apply/exist any more. The subsequent hunk essentially copies the persistent definition, so does "PARSE_INACTIVE" make sense? John BTW: I'll respond individually to other patches, but it may take a few hours as I have some errands to run today - so I'll be "in and out" - figured it was better to get this out there first though.

+ if (vm->newDef) { + if (!(xml = qemuDomainDefFormatLive(driver, vm->newDef, priv->origCPU, + true, true)) || + !(def->newDom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + goto endjob; + } + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false;

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

By default, active and inactive XMl snapshot configurations are

*XML

assigned to domain definition. This will make sure that all the non-persistent configurations of the snapshot are restored back as it is. This patch will also make sure that user has a choice

s/it is/they were/ ??

to choose of using active XML configuration of snapshot as both active and inactive XML configurations of the restoring domain.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- include/libvirt/libvirt-domain-snapshot.h | 10 +++++++--- src/qemu/qemu_driver.c | 20 +++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 0f73f24..67ccb59 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -184,9 +184,13 @@ int virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, unsigned int flags);

typedef enum { - VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */ - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ - VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */ + VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */ + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ + VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */ + VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY = 1 << 3, /* Use active snapshot + configurations as both + active and inactive + domain configurations*/

Previous def only had 1 space between longest line of "RUNNING" and "="; this has 2 spaces between "ONLY" and "=". Just go with 1. Also the tailing "*/" should gain a space. The comment doesn't quite make sense though related to the name used for the enum. I'm having trouble understanding the meaning of "as both active and inactive" as it relates to REVERT_ACTIVE_ONLY. I think you're trying to indicate that by setting this flag, when reverting from the snapshot, that only the "saved" active definition would be restored and if there was a saved persistent one, then it would be skipped. IOW: "VIR_DOMAIN_SNAPSHOT_REVERT_SKIP_PERSISTENT"

} virDomainSnapshotRevertFlags;

/* Revert the domain to a point-in-time snapshot. The diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4ffec70..aecfcff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15577,6 +15577,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr newConfig = NULL;

persistentConfig?

virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_running = false; @@ -15586,7 +15587,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,

virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | - VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1); + VIR_DOMAIN_SNAPSHOT_REVERT_FORCE | + VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY, -1);

/* We have the following transitions, which create the following events: * 1. inactive -> inactive: none @@ -15688,6 +15690,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; }

This is why I ended up writing that long comment in patch 1.

+ /* Prepare to copy snapshot inactive xml as inactive configuration + * of this domain unless user exclusively specify not to copy it */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY) && + snap->def->newDom) { + newConfig = virDomainDefCopy(snap->def->newDom, caps, + driver->xmlopt, NULL, true); + if (!newConfig) + goto endjob; + } + cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;

switch ((virDomainState) snap->def->state) { @@ -15785,12 +15797,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virCPUDefFree(priv->origCPU); VIR_STEAL_PTR(priv->origCPU, origCPU); } + if (newConfig) + vm->newDef = newConfig; } else { /* Transitions 2, 3 */ load: was_stopped = true; if (config) virDomainObjAssignDef(vm, config, false, NULL); + if (newConfig) + vm->newDef = newConfig;

/* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. @@ -15884,6 +15900,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } if (config) virDomainObjAssignDef(vm, config, false, NULL); + if (newConfig) + vm->newDef = newConfig;

Shall I assume you're sure that for all assignments that vm->newDef isn't already populated with something? This is why I started thinking about the various existing convenience API's that managed the def/newDef. John

if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

This patch will allow user to edit the inactive XML snapshot configuration when it is available in the snapshot.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 2 ++ src/conf/snapshot_conf.c | 35 ++++++++++++++++++++++++++++++++++- src/qemu/qemu_driver.c | 3 ++- 5 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4048acf..e70c664 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1546,6 +1546,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_ACTIVE_ONLY = (1 << 4), /* dump active XML and avoid inactive XML in snapshot */

Not liking the name - makes me wonder why the negation of the existing VIR_DOMAIN_XML_INACTIVE wasn't used in some way... It doesn't scream use this only for SNAPSHOT manipulation. Consider: VIR_DOMAIN_XML_SNAPSHOT_ACTIVE

} virDomainXMLFlags;

char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 77c20c6..36cebe5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25687,8 +25687,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, - -1); + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, -1);

if (!(type = virDomainVirtTypeToString(def->virtType))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -26472,6 +26472,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; if (flags & VIR_DOMAIN_XML_MIGRATABLE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; + if (flags & VIR_DOMAIN_XML_ACTIVE_ONLY) + formatFlags |= VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY;

return formatFlags; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 38de70b..0659220 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2853,6 +2853,8 @@ typedef enum { VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6, VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7, VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8, + /* format active XML and avoid inactive XML */ + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY = 1 << 9, } virDomainDefFormatFlags;

/* Use these flags to skip specific domain ABI consistency checks done diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index bfe3d6c..3cb7cd4 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -290,6 +290,29 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } + + /* Older snapshots were created without inactive domain configuration. + * In that case, leave the newDom NULL. */ + if ((tmp = virXPathString("string(./inactiveDomain/domain/@type)", ctxt))) { + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) + domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS; + xmlNodePtr domainNode = virXPathNode("./inactiveDomain/domain", ctxt); + + VIR_FREE(tmp); + if (domainNode) { + def->newDom = virDomainDefParseNode(ctxt->node->doc, domainNode, + caps, xmlopt, NULL, domainflags); + if (!def->newDom) + goto cleanup; + } else { + VIR_WARN("missing inactive domain in snapshot");

But if dom->newDef never existed, then why would snapshotDef->newDom exist?

+ } + } else { + VIR_WARN("parsing older snapshot that lacks inactive domain");

So? You need to be able to handle a NULL snapshotDef->newDom being NULL anyway, so not sure the WARN is appropriate here.

+ } + } else { def->creationTime = tv.tv_sec; } @@ -705,7 +728,8 @@ virDomainSnapshotDefFormat(const char *domain_uuid, virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i;

- virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, NULL);

flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;

@@ -757,6 +781,15 @@ virDomainSnapshotDefFormat(const char *domain_uuid, virBufferAddLit(&buf, "</domain>\n"); }

+ if (def->newDom && !(flags & VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY)) { + virBufferAddLit(&buf, "<inactiveDomain>\n"); + virBufferAdjustIndent(&buf, 2); + if (virDomainDefFormatInternal(def->newDom, caps, flags, &buf) < 0) + goto error; + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</inactiveDomain>\n"); + } +

So after all this, it's not clear why there needs to be a special flag for snapshot and why having this flag allows editing (IOW: The commit message perhaps is misleading). It's even more problematic since domain->newDef isn't necessarily defined and thus def->newDom wouldn't be either. I think perhaps the Parse and Format code should be their own patch anyway, so that part is useful; however, the new flag, I'm not sure yet. If your goal is to mimic the domain inactive flag for snapshot, then follow how domain command line processing goes. Inventing an "active-only" (subsequent patch) is not something I'm sure is desired (yet). Consistency between various commands is (to me at least) far more desirable. Currently it seems usage if "--inactive" on the dumpxml command line is preferred (domain, net-*, iface-*, pool-*, etc). If "today" what someone is doing is editing/dumping the active XML, then what you're allowing in the future is to allow someone to edit/dump the inactive XML. For that purpose, there are existing flags. John

if (virSaveCookieFormatBuf(&buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aecfcff..a0a4384 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15469,7 +15469,8 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN];

- virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_ACTIVE_ONLY, NULL);

if (!(vm = qemuDomObjFromSnapshot(snapshot))) return NULL;

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

Now, snapshot-dumpxml will display inactive XML configuration of snapshot along with active XML configuration. When --active-only flag is used the inactive XML configuration will not be displayed.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- tools/virsh-snapshot.c | 7 +++++++ tools/virsh.pod | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+)

Hmm... ahhh... so patch 3 is dumping the inactive XML in the middle of the active XML... And we're doing that for what reason? Why would the snapshot XML need to be different? If one can use --inactive flags for dumpxml to specifically dump the "newDom" if it exists (or the "dom" if it doesn't), then it would essentially follow how the domain does this (see qemuDomainFormatXML). Looking back at patch 3 now, I guess generating XML for the inactive within the context of active just doesn't seem right. John

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 24cd4ab..4b0a18d 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1667,6 +1667,10 @@ static const vshCmdOptDef opts_snapshot_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("include security sensitive information in XML dump") }, + {.name = "active-only", + .type = VSH_OT_BOOL, + .help = N_("dump only active XML configuration and avoid inactive XML") + }, {.name = NULL} };

@@ -1683,6 +1687,9 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "security-info")) flags |= VIR_DOMAIN_XML_SECURE;

+ if (vshCommandOptBool(cmd, "active-only")) + flags |= VIR_DOMAIN_XML_ACTIVE_ONLY; + if (vshCommandOptStringReq(ctl, cmd, "snapshotname", &name) < 0) return false;

diff --git a/tools/virsh.pod b/tools/virsh.pod index 69cc423..f899da7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4489,11 +4489,31 @@ is specified, the list will be filtered to snapshots that use external files for disk images or memory state.

=item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] +[I<--active-only>]

Output the snapshot XML for the domain's snapshot named I<snapshot>. Using I<--security-info> will also include security sensitive information. Use B<snapshot-current> to easily access the XML of the current snapshot.

+If I<--active-only> is specified, only active XML configuration of the +snapshot is displayed. Otherwise, both active and inactive XML +configuration of the snapshot will be displayed. + +When both active and inactive XML snapshot configurations are displayed, +as inactive XML configuration will have same structure as active XML +it is embedded within <inactiveDomain> tag as shown below: + + <domainsnapshot> + .... + .... + <inactiveDomain> + <domain> + .... + .... + </domain> + </inactiveDomain> + </domainsnapshot> + =item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>}

Output the name of the parent snapshot, if any, for the given

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

Now, snapshot-edit will allow editing inactive XML configuration of snapshot along with active XML configuration. When --active-only flag is used the inactive XML will not be displayed and will be removed from snapshot. --active-only flag is used when user doesn't what any non-persistent configuration in domain after restoring the snapshot.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- tools/virsh-snapshot.c | 7 +++++++ tools/virsh.pod | 8 +++++++- 2 files changed, 14 insertions(+), 1 deletion(-)

Similar comment here as in patch 4... Also note additional minor edit from below...

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 4b0a18d..48fc034 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -522,6 +522,10 @@ static const vshCmdOptDef opts_snapshot_edit[] = { .type = VSH_OT_BOOL, .help = N_("allow cloning to new name") }, + {.name = "active-only", + .type = VSH_OT_BOOL, + .help = N_("allow editing active XML configuration and remove inactive XML") + }, {.name = NULL} };

@@ -545,6 +549,9 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) vshCommandOptBool(cmd, "snapshotname")) define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;

+ if (vshCommandOptBool(cmd, "active-only")) + getxml_flags |= VIR_DOMAIN_XML_ACTIVE_ONLY; + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false;

diff --git a/tools/virsh.pod b/tools/virsh.pod index f899da7..0578f8f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4400,7 +4400,7 @@ With I<snapshotname>, this is a request to make the existing named snapshot become the current snapshot, without reverting the domain.

=item B<snapshot-edit> I<domain> [I<snapshotname>] [I<--current>] -{[I<--rename>] | [I<--clone>]} +{[I<--rename>] | [I<--clone>]} [I<--active-only>]

Edit the XML configuration file for I<snapshotname> of a domain. If both I<snapshotname> and I<--current> are specified, also force the @@ -4427,6 +4427,12 @@ a snapshot name must be done with care, since the contents of some snapshots, such as internal snapshots within a single qcow2 file, are accessible only from the original name.

+If I<--active-only> is specified, only active XML configuration of the +snapshot is displayed to edit. Otherwise, both active and inactive XML +configuration of the snapshot will be displayed to edit. When domain +snapshot is edited with I<--active-only> flag, inactive XML configuration +will be removed from snapshot.

FWIW: git am tells me above line had whitespace at the end... John

+ =item B<snapshot-info> I<domain> {I<snapshot> | I<--current>}

Output basic information about a named <snapshot>, or the current snapshot

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

Now, snapshot-restore will allow restoring snapshots with non-persistent configuration as both active and inactive XML configurations are saved in snapshot. User can discard non-persistent configuratin of a domain

configuration

using --active-only flag. When --active-only flag is used, active XML configuration of the snapshot is used as both the active and inactive XML configuration of the domain after restore.

really? My recollection is, newDom is generated based on whether newDef exists. There isn't any code that would generate newDom from dom unless I'm missing something subtle.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- tools/virsh-snapshot.c | 6 ++++++ tools/virsh.pod | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)

Wish this was "closer to" patch 2 - short term memory loss already ;-) So what would be the reason to only restore ->dom and not ->newDom? Still, consider "remove-inactive" as a flag or "remove-persistent". In the long run it's all about describing things consistently - if a domain is active and persistent, then shouldn't the snapshot follow suit? The more recent domain flags make consistent use of "config" and "live". And there's a lot of "behavio[u]r is different depending on hypervisor".. I would think "historically" consumers would be thinking they were managing the active snapshot - to have active switches which end up being negative logic for the persistent portion is a bit confusing.

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 48fc034..7f6a231 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1811,6 +1811,10 @@ static const vshCmdOptDef opts_snapshot_revert[] = { .type = VSH_OT_BOOL, .help = N_("try harder on risky reverts") }, + {.name = "active-only", + .type = VSH_OT_BOOL, + .help = N_("use only active snapshot configuration when restoring") + }, {.name = NULL} };

@@ -1835,6 +1839,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) * when the error says it will make a difference. */ if (vshCommandOptBool(cmd, "force")) force = true; + if (vshCommandOptBool(cmd, "active-only")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY;

dom = virshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) diff --git a/tools/virsh.pod b/tools/virsh.pod index 0578f8f..791014e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4526,7 +4526,7 @@ Output the name of the parent snapshot, if any, for the given I<snapshot>, or for the current snapshot with I<--current>.

=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused>}] [I<--force>] +[{I<--running> | I<--paused>}] [I<--force>] [I<--active-only>]

Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -4559,6 +4559,13 @@ snapshot that uses a provably incompatible configuration, as well as with an inactive snapshot that is combined with the I<--start> or I<--pause> flag.

+When inactive XML configuration of a snapshot is available along with +active XML configuration both the inactive and active XMl configurations +are used to restore the snapshot. This will keep the non-persistent +configuration alive after restoring a snapshot. User can kill the

s/kill/remove/

+non-persistent configuration by issuing I<--active-only> flag. This will

"non-persistent" - now I'm getting more confused. This is why it's nice to have the code together, but looking back at patch 2 the flag would mean that newDom is not restored and thus newDef not filled in which to me says, the persistent def wouldn't be restored.

+use active XML configuraton alone to revert the snapshot.

configuration (at least you were consistent!) John

+ =item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>} [I<--metadata>] [{I<--children> | I<--children-only>}]

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:

Alter the schema of domainsnapshot to add inactive XML of a snapshot. As, snapshot already has active XML configuration of domain, the inactive XMl is embedded in <inactiveDomain> tags. Sample XML is:

<domainsnapshot> .... .... <domain> </domain> <inactiveDomain> <domain> </domain> </inactiveDomain> </domainsnapshot>

Alter the domainsnapshotxml2xmltest to validate the format.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- docs/schemas/domainsnapshot.rng | 19 +++++> .../full_domain_withinactive.xml | 83 ++++++++++++++++++++++

probably should be "with_inactive" because a quick read I got "within active" ;-)

tests/domainsnapshotxml2xmltest.c | 1 + 3 files changed, 103 insertions(+) create mode 100644 tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml

This should have been paired with the snapshot_conf.c change(s) to parse and format the XML. I think it would have helped me a lot when reading the virsh: patches. Typically a series also includes a docs/formatsnapshot.html.in change to describe the new XML on the website. A subsequent patch would change "docs/news.xml" to describe the adjustments in general. Now that I've read everything, it would seem to me that this "inactive" (or persistent) is the new piece. So given that, I would think it's what should be used to formulate various surrounding flags and command switches. Before this code, the dump/edit would display/edit the "live" or "active" or "primary" def data. That doesn't necessarily change, but the ability to specify the 'inactive' (persistent/new) data would. Some more thoughts... 1. Always save off the persistent def in the output and always restore with it if it exists. 2. If someone wants to remove the inactive/persistent, then they can edit it out. 3. It's too bad virDomainSnapshotDelete couldn't do something with a flag to remove the inactive/persistent for the named snapshot if it existed. 4. Continue to only format the active (or non persistent) data unless someone uses the --inactive switch or some INACTIVE flag. That means the current format code would need to check the INACTIVE flag before always printing ->dom, but there's a gotcha with the UUID option. 5. Perhaps add an --all flag/ALL option to format *both*. 6. Not sure I agree with having Revert be able to remove the inactive/ persistent (newDom) if it existed. That's one of those trying to do too much with one command that will end up biting us later on, somehow. 7. It may be interesting to look at the how the disks are handled in the current code in order to think about them from the viewpoint of having both dom and newDom type data. 8. Again - think in terms of how the domain has handled "hiding" def and newDef at least w/r/t using def for persistent in the event that newDef is NULL. That model perhaps will help you through the thought process of snapshot history for active vs. inactive data. If inactive isn't there, then all we can assume is it wasn't saved or necessary at save time. Hopefully someone else will also jump in and provide some feedback. I don't believe the patches are far off, but I do believe need some tweaking. Designing by patch submission is always difficult.

diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 2680887..2a58a84 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -84,6 +84,25 @@ </choice> </optional> <optional> + <choice> + <element name='inactiveDomain'> + <choice> + <element name='domain'> + <element name='uuid'> + <ref name="UUID"/> + </element>

Is this used for the inactive - don't believe so from how I read the Parse and Format code. If supplied one would hope it's the same! John

+ </element> + <!-- Nested grammar ensures that any of our overrides of + storagecommon/domaincommon defines do not conflict + with any domain.rng overrides. --> + <grammar> + <include href='domain.rng'/> + </grammar> + </choice> + </element> + </choice> + </optional> + <optional> <element name='parent'> <element name='name'> <text/> diff --git a/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml new file mode 100644 index 0000000..d6d1b39 --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml @@ -0,0 +1,83 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <state>running</state> + <parent> + <name>earlier_snap</name> + </parent> + <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> + </domain> + <inactiveDomain> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> + </domain> + </inactiveDomain> + <active>1</active> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 3a6f86b..ebec2de 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -205,6 +205,7 @@ mymain(void) DO_TEST_OUT("all_parameters", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", true); DO_TEST_OUT("disk_snapshot_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", true); DO_TEST_OUT("full_domain", "c7a5fdbd-edaf-9455-926a-d65c16db1809", true); + DO_TEST_OUT("full_domain_withinactive", "c7a5fdbd-edaf-9455-926a-d65c16db1809", true); DO_TEST_OUT("noparent_nodescription_noactive", NULL, false); DO_TEST_OUT("noparent_nodescription", NULL, true); DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false);