[libvirt] [PATCH 6/7] virsh: Allow restoring snapshot with non-persistent configuration

[John Ferlan]

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 at 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>}]
>  
>