qemuDomainSnapshotCreateActiveExternal: Remove memory snapshot on failure less frequently

[libvirt] [PATCH] qemuDomainSnapshotCreateActiveExternal: Remove memory snapshot on failure less frequently

Posted by Michal Privoznik 5 years, 7 months ago

https://bugzilla.redhat.com/show_bug.cgi?id=1589115

When creating a memory snapshot the domain is suspended and qemu
is told to dump memory into the desired file. After that we set a
flag so that the file is not left behind if a failure occurs at
some later point (e.g. when creating disk snapshot fails).
However, the way we currently handle the memory snapshot file is
way too aggressive. For instance, if resuming domain vCPUs fails
after all snapshots were done successfully, we still remove the
memory snapshot (even though we leave disk snapshot be).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a0f7c71675..2eae3dd49d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15248,6 +15248,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
         virObjectEventStateQueue(driver->domainEventState, event);
     }
 
+    memory_unlink = false;
     ret = 0;
 
  cleanup:
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainSnapshotCreateActiveExternal: Remove memory snapshot on failure less frequently

Posted by Peter Krempa 5 years, 7 months ago

On Thu, Aug 30, 2018 at 11:22:35 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> 
> When creating a memory snapshot the domain is suspended and qemu
> is told to dump memory into the desired file. After that we set a
> flag so that the file is not left behind if a failure occurs at
> some later point (e.g. when creating disk snapshot fails).
> However, the way we currently handle the memory snapshot file is
> way too aggressive. For instance, if resuming domain vCPUs fails
> after all snapshots were done successfully, we still remove the
> memory snapshot (even though we leave disk snapshot be).
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a0f7c71675..2eae3dd49d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15248,6 +15248,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>          virObjectEventStateQueue(driver->domainEventState, event);
>      }
>  
> +    memory_unlink = false;
>      ret = 0;

This does not make sense. The snapshot metadata is not saved at this
point so this file would be left over and could not be used (once we
implement external snapshot reversion).

The problem is that you can't get rid of the disk images once the
transaction is executed, but you can dispose of the memory image.

The failure points after the disk snapshot is taken are very hard to
roll back.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainSnapshotCreateActiveExternal: Remove memory snapshot on failure less frequently

Posted by Michal Privoznik 5 years, 7 months ago

On 08/30/2018 12:57 PM, Peter Krempa wrote:
> On Thu, Aug 30, 2018 at 11:22:35 +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
>>
>> When creating a memory snapshot the domain is suspended and qemu
>> is told to dump memory into the desired file. After that we set a
>> flag so that the file is not left behind if a failure occurs at
>> some later point (e.g. when creating disk snapshot fails).
>> However, the way we currently handle the memory snapshot file is
>> way too aggressive. For instance, if resuming domain vCPUs fails
>> after all snapshots were done successfully, we still remove the
>> memory snapshot (even though we leave disk snapshot be).
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a0f7c71675..2eae3dd49d 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15248,6 +15248,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>>          virObjectEventStateQueue(driver->domainEventState, event);
>>      }
>>  
>> +    memory_unlink = false;
>>      ret = 0;
> 
> This does not make sense. The snapshot metadata is not saved at this
> point so this file would be left over and could not be used (once we
> implement external snapshot reversion).
> 
> The problem is that you can't get rid of the disk images once the
> transaction is executed, but you can dispose of the memory image.
> 
> The failure points after the disk snapshot is taken are very hard to
> roll back.
> 

So what are you saying is that this is expected behaviour? Okay. Thanks
anyway.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainSnapshotCreateActiveExternal: Remove memory snapshot on failure less frequently

Posted by Peter Krempa 5 years, 7 months ago

On Thu, Aug 30, 2018 at 13:06:40 +0200, Michal Privoznik wrote:
> On 08/30/2018 12:57 PM, Peter Krempa wrote:
> > On Thu, Aug 30, 2018 at 11:22:35 +0200, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> >>
> >> When creating a memory snapshot the domain is suspended and qemu
> >> is told to dump memory into the desired file. After that we set a
> >> flag so that the file is not left behind if a failure occurs at
> >> some later point (e.g. when creating disk snapshot fails).
> >> However, the way we currently handle the memory snapshot file is
> >> way too aggressive. For instance, if resuming domain vCPUs fails
> >> after all snapshots were done successfully, we still remove the
> >> memory snapshot (even though we leave disk snapshot be).
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_driver.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index a0f7c71675..2eae3dd49d 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -15248,6 +15248,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
> >>          virObjectEventStateQueue(driver->domainEventState, event);
> >>      }
> >>  
> >> +    memory_unlink = false;
> >>      ret = 0;
> > 
> > This does not make sense. The snapshot metadata is not saved at this
> > point so this file would be left over and could not be used (once we
> > implement external snapshot reversion).
> > 
> > The problem is that you can't get rid of the disk images once the
> > transaction is executed, but you can dispose of the memory image.
> > 
> > The failure points after the disk snapshot is taken are very hard to
> > roll back.
> > 
> 
> So what are you saying is that this is expected behaviour? Okay. Thanks
> anyway.

Well yes. The snapshot was _not_ taken at this point. The API is going
to return failure and we rolled back what we could. Leaving the memory
image behind is pointless since it's not used by qemu after it's taken
and wrong since it would use up space on the disk unbeknownst to the
user.

Unfortunately that can't be done to disk images as they became part of
the backing chain and e.g. in case if resuming vcpus failed we
theoretically could remove them, but qemu does not give us an mechanism
to do that.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list