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.

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.

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

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.

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.

Loading then looks for that bit, and fails to restore an image with the bit set unless you pass a --force option.

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.

> 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.

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.

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list