[libvirt] [PATCH] qemu: Do not unlink managedsave image if restoring fails.
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem. --- src/qemu/qemu_driver.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..22c29e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,20 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); - } + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); - if (ret == 0) goto cleanup; + } else { + VIR_WARN("Failed to restore from the managed state %s", managed_save); + } } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks. Jirka
于 2011年04月05日 21:20, Jiri Denemark 写道:
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks.
Jirka
Hum, it makes sense, that's what I was worried about, I think failing domain start will be better than data loss anyway, so will update with that. Thanks for the reviewing Regards Osier
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. The fix is to fail the domain startup if the restoring fails, but doesn't unlink()s the save state. v1 - v2: * Fails the domain startup and throws error instead of throwing a warning. --- src/qemu/qemu_driver.c | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..ef4d277 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,21 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore from the managed state %s"), + managed_save); } - - if (ret == 0) - goto cleanup; + goto cleanup; } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4
On 04/05/2011 07:20 AM, Jiri Denemark wrote:
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks.
What's more, I think we should consider removing the saved-state file on success for 'virsh restore file' - once a state has been restored, the guest is running and has likely modified its disks, which means that the saved (memory) state is no longer consistent with the new disk state, and a second restore of the saved file is asking for a different type of data corruption. That is, I think: virsh save dom file virsh restore file should leave file intact if and only if the restore failed, and: virsh managedsave dom virsh start should either fail but leave the (hidden) state file intact, or succeed and remove the state file. We have virsh managedsave-remove to properly delete the state file if the user determines that they want a fresh start rather than retrying the (hidden) state file. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月05日 22:55, Eric Blake 写道:
On 04/05/2011 07:20 AM, Jiri Denemark wrote:
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks.
What's more, I think we should consider removing the saved-state file on success for 'virsh restore file' - once a state has been restored, the guest is running and has likely modified its disks, which means that the saved (memory) state is no longer consistent with the new disk state, and a second restore of the saved file is asking for a different type of data corruption.
Hmm, I didn't think of this (data corruption), v3 comes.
That is, I think:
virsh save dom file virsh restore file
should leave file intact if and only if the restore failed, and:
virsh managedsave dom virsh start
should either fail but leave the (hidden) state file intact, or succeed and remove the state file. We have virsh managedsave-remove to properly delete the state file if the user determines that they want a fresh start rather than retrying the (hidden) state file.
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote:
On 04/05/2011 07:20 AM, Jiri Denemark wrote:
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks.
What's more, I think we should consider removing the saved-state file on success for 'virsh restore file' - once a state has been restored, the guest is running and has likely modified its disks, which means that the saved (memory) state is no longer consistent with the new disk state, and a second restore of the saved file is asking for a different type of data corruption.
That is, I think:
virsh save dom file virsh restore file
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing. In general I would tend to not change the established operation behaviour, especially on suddenly removing files owned by the user without asking. For managed save that's completely different due to the fact the file is owned by libvir, and removing is fine. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
于 2011年04月06日 16:50, Daniel Veillard 写道:
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote:
On 04/05/2011 07:20 AM, Jiri Denemark wrote:
On Tue, Apr 05, 2011 at 14:47:22 +0800, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss.
However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem.
I think this patch is risky. You should either remove the state on error (which is the current state) or fail domain start if managed state is present but resuming from it fails. If you do something in the middle (your patch) you will certainly end up corrupting domain's disks.
What's more, I think we should consider removing the saved-state file on success for 'virsh restore file' - once a state has been restored, the guest is running and has likely modified its disks, which means that the saved (memory) state is no longer consistent with the new disk state, and a second restore of the saved file is asking for a different type of data corruption.
That is, I think:
virsh save dom file virsh restore file
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
Somehow this is true, but as we have API and virsh commands for snapshot specificly, is it fine to make a change on the virsh manual to tell the user about we remove the state file if restoring succeeded? I did that when doing v3 patch, :-)
In general I would tend to not change the established operation behaviour, especially on suddenly removing files owned by the user without asking. For managed save that's completely different due to the fact the file is owned by libvir, and removing is fine.
Daniel
On Wed, Apr 06, 2011 at 05:02:36PM +0800, Osier Yang wrote:
于 2011年04月06日 16:50, Daniel Veillard 写道:
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote:
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
Somehow this is true, but as we have API and virsh commands for snapshot specificly, is it fine to make a change on the virsh manual to tell the user about we remove the state file if restoring succeeded? I did that when doing v3 patch, :-)
Still that can be considered a regression, that behaviour is the same since the introduction of the API years ago, we can't change it now just because we think it's cool. Document the fact that in general once the restore suceeded the file should be removed since the data are unlikely to be reusable without loss of integrity, but you can't change the behaviour. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/06/2011 06:51 AM, Daniel Veillard wrote:
On Wed, Apr 06, 2011 at 05:02:36PM +0800, Osier Yang wrote:
于 2011年04月06日 16:50, Daniel Veillard 写道:
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote:
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
Somehow this is true, but as we have API and virsh commands for snapshot specificly, is it fine to make a change on the virsh manual to tell the user about we remove the state file if restoring succeeded? I did that when doing v3 patch, :-)
Still that can be considered a regression, that behaviour is the same since the introduction of the API years ago, we can't change it now just because we think it's cool. Document the fact that in general once the restore suceeded the file should be removed since the data are unlikely to be reusable without loss of integrity, but you can't change the behaviour.
What about this proposal, for being less severe? If we can open the file read-write, then we modify it on a successful restore to change a bit to mark that the image is suspect. Loading then looks for that bit, and fails to restore an image with the bit set unless you pass a --force option. Of course, if the image is read-only, we can't mark the bit stating that the image has been successfully restored, so the best we can do in that case is just print a warning suggesting that since the restore is successful, the user should remove the file. But I agree that since we have already got releases in the wild that do not delete the file, that we cannot change behavior to delete it at this point. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月06日 22:21, Eric Blake 写道:
On 04/06/2011 06:51 AM, Daniel Veillard wrote:
On Wed, Apr 06, 2011 at 05:02:36PM +0800, Osier Yang wrote:
于 2011年04月06日 16:50, Daniel Veillard 写道:
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote:
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
Somehow this is true, but as we have API and virsh commands for snapshot specificly, is it fine to make a change on the virsh manual to tell the user about we remove the state file if restoring succeeded? I did that when doing v3 patch, :-)
Still that can be considered a regression, that behaviour is the same since the introduction of the API years ago, we can't change it now just because we think it's cool.
Agree
Document the fact that in general once the restore suceeded the file should be removed since the data are unlikely to be reusable without loss of integrity, but you can't change the behaviour.
Agree
What about this proposal, for being less severe? If we can open the file read-write, then we modify it on a successful restore to change a bit to mark that the image is suspect. Loading then looks for that bit, and fails to restore an image with the bit set unless you pass a --force option.
Sounds a good idea, but doesn't it also breaks the behaviour? it means all the apps need to introduce a new flag then if I understand your meaning right.
Of course, if the image is read-only, we can't mark the bit stating that the image has been successfully restored, so the best we can do in that case is just print a warning suggesting that since the restore is successful, the user should remove the file.
But I agree that since we have already got releases in the wild that do not delete the file, that we cannot change behavior to delete it at this point.
I'm thinking: 1) don't change the behaviour, which means won't try to remove the state file when restoring succeeded. 2) VIR_WARN to tell user the state file should be removed once the restoring succeeded. 3) Update docs to tell the state file should be remove once the restoring succeeded. Regards Osier
On Wed, Apr 06, 2011 at 08:21:56AM -0600, Eric Blake wrote:
On 04/06/2011 06:51 AM, Daniel Veillard wrote:
On Wed, Apr 06, 2011 at 05:02:36PM +0800, Osier Yang wrote:
于 2011年04月06日 16:50, Daniel Veillard 写道:
On Tue, Apr 05, 2011 at 08:55:01AM -0600, Eric Blake wrote: Somehow this is true, but as we have API and virsh commands for snapshot specificly, is it fine to make a change on the virsh manual to tell the user about we remove the state file if restoring succeeded? I did that when doing v3 patch, :-)
Still that can be considered a regression, that behaviour is the same since the introduction of the API years ago, we can't change it now just because we think it's cool. Document the fact that in general once the restore suceeded the file should be removed since the data are unlikely to be reusable without loss of integrity, but you can't change the behaviour.
What about this proposal, for being less severe? If we can open the file read-write, then we modify it on a successful restore to change a bit to mark that the image is suspect.
technically the beginning of the image is an XML dump of the domain if I remember correctly, we could try to do something smart in there to do the detection.
Loading then looks for that bit, and fails to restore an image with the bit set unless you pass a --force option.
That's still a regression, existing code using for example the LVM for snapshot independantly of our API will still break.
But I agree that since we have already got releases in the wild that do not delete the file, that we cannot change behavior to delete it at this point.
Same for being able to restore, as there are legitimate uses. I would be okay for a warning to be emitted if we detect a subsequent restore but we cannot make the operation to fail by default in my opinion. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Le mercredi 06 avril 2011 à 16:50 +0800, Daniel Veillard a écrit :
That is, I think:
virsh save dom file virsh restore file
should leave file intact if and only if the restore failed, and:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
I completely agree, virsh restore shouldn't remove the saved state. I often use this and restore several time the same dump, because I independently save (and restore) the VM disk. I use this as domain snapshots function because of the current limitations of the snapshot API (only working for qcow2 image, and I only use LVM for performance reason, I'm pretty sure a lot of user also use LVM, and would like to keep the possibility to restore a saved state multiple time). Regards, Daniel
In general I would tend to not change the established operation behaviour, especially on suddenly removing files owned by the user without asking. For managed save that's completely different due to the fact the file is owned by libvir, and removing is fine.
Daniel
-- Daniel Berteaud FIREWALL-SERVICES SARL. Société de Services en Logiciels Libres Technopôle Montesquieu 33650 MARTILLAC Tel : 05 56 64 15 32 Fax : 05 56 64 15 32 Mail: daniel@firewall-services.com Web : http://www.firewall-services.com
On 04/06/2011 07:11 AM, Daniel Berteaud wrote:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
I completely agree, virsh restore shouldn't remove the saved state. I often use this and restore several time the same dump, because I independently save (and restore) the VM disk.
Hmm, I should read the full thread before replying. Altering a saved image to mark it as successfully restored would potentially interfere with this usage pattern (you'd either have to pass --force to the restore to override the bit, or we'd have to introduce a new command that can be used to easily remove the bit to be called in tandem with rewinding disk state back to its earlier point). I guess what that means for today is that this patch series should focus just on managedsave behavior, and for save/restore should focus only on documentation. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月06日 22:25, Eric Blake 写道:
On 04/06/2011 07:11 AM, Daniel Berteaud wrote:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
I completely agree, virsh restore shouldn't remove the saved state. I often use this and restore several time the same dump, because I independently save (and restore) the VM disk.
Hmm, I should read the full thread before replying. Altering a saved image to mark it as successfully restored would potentially interfere with this usage pattern (you'd either have to pass --force to the restore to override the bit, or we'd have to introduce a new command that can be used to easily remove the bit to be called in tandem with rewinding disk state back to its earlier point).
I guess what that means for today is that this patch series should focus just on managedsave behavior, and for save/restore should focus only on documentation.
Oh, okay, I should read the full thread either. :-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
于 2011年04月06日 22:25, Eric Blake 写道:
On 04/06/2011 07:11 AM, Daniel Berteaud wrote:
The problem there is that you are changing the command behaviour. The user may snapshot the disk separately and use this to implement a simplified domain snapshot. Doing the remove may avoid troubles for those not knowing what they are doing, but also break something for those who know what they are doing.
I completely agree, virsh restore shouldn't remove the saved state. I often use this and restore several time the same dump, because I independently save (and restore) the VM disk.
Hmm, I should read the full thread before replying. Altering a saved image to mark it as successfully restored would potentially interfere with this usage pattern (you'd either have to pass --force to the restore to override the bit, or we'd have to introduce a new command that can be used to easily remove the bit to be called in tandem with rewinding disk state back to its earlier point).
I guess what that means for today is that this patch series should focus just on managedsave behavior, and for save/restore should focus only on documentation.
Oh, okay, I should read the full thread too. :)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (5)
-
Daniel Berteaud -
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Osier Yang