Re: [Engine-devel] Adding atomic restore snapshot command at backend

On 05/31/2012 09:04 AM, Livnat Peer wrote: > On 30/05/12 17:55, Michael Pasternak wrote: >> On 05/30/2012 05:39 PM, Omer Frenkel wrote: >>> >>> >>> ----- Original Message ----- >>>> From: "Michael Pasternak" <mpastern(a)redhat.com&gt; >>>> To: "Livnat Peer" <lpeer(a)redhat.com&gt; >>>> Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; >>>> Sent: Monday, May 28, 2012 2:37:10 PM >>>> Subject: Re: [Engine-devel] Adding atomic restore snapshot command at backend >>>> >>>> >>>> Hi Livnat, >>>> >>>> On 05/28/2012 02:07 PM, Livnat Peer wrote: >>>>> On 28/05/12 12:59, Michael Pasternak wrote: >>>>>> >>>>>> Currently 'restore snapshot' done in two steps on a client side: >>>>>> >>>>>> 1. TryBackToAllSnapshotsOfVm >>>>>> 2. RestoreAllSnapshots >>>>>> >>>>>> This implementation creates race condition on 1 and therefore >>>>>> unstable & bug prone, >>> >>> IIRC, there is an easy way to fix this, >>> by polling on engine jobs and not vdsm tasks, no? >> >> indeed, and we already doing that, but it still fails on race at >> former command. >> > > Can you give more details on that? > The Job indicates it is finished but actually it is not? Ori?

> > >>> >>>>>> i suggested refactoring 2 to include 1 as single atomic operation >>>>>> at backend. >>>>>> >>>>>> >>>>> >>>>> Hi Michael, >>>>> >>>>> The two commands above are used for functionality that is needed >>>>> regardless of the functionality you are looking for. >>>>> We want to present the user the option to preview a snapshot >>>>> without >>>>> committing to it and without loosing the current snapshot. >>>>> I think we need to model the the two options above in the REST, it >>>>> is >>>>> functionality that is used in the UI. >>> >>> +1 for that. >>> >>>> >>>> this is out of scope of this RFE. >>>> >>>>> >>>>> What you are looking for is a functionality 'restore to snapshot', >>>>> this >>>>> functionality does not exist in the engine in a single step, and I >>>>> think >>>>> that because we assumed that the user can get it in two steps It >>>>> wasn't >>>>> prioritize so far. >>>> >>>> the cons. of this approach is an async nature of the former command. >>>> >>>>> >>>>> Implementing the missing functionality with the two functions above >>>>> is >>>>> a compromise that was done in the API. >>>>> >>>>> To summarize I think the requirement you present is a missing >>>>> functionality in the engine and solving it is not about refactoring >>>>> the >>>>> two existing verbs into one. >>>> >>>> what i meant is not deprecating/refactoring old commands, but >>>> introducing >>>> new one that reusing them and expose as single (atomic) command >>>> called >>>> 'RestoreAllSnapshots', >>>> >>>> sorry if i wasn't clear. >>> >>> another suggestion is to have a way to send to the engine list of commands that will be performed. >> >> afaik both UI and API perform restore in a same manner, >> >> 1. TryBackToAllSnapshotsOfVm >> 2. polling >> 3. RestoreAllSnapshots >> >> so why not combining 1 & 3 in to single command, if from clients perspective >> it is such ... >> > > The UI is not polling and does not run the two operations above > automatically. > It is doing the TryBack and then the user can start the VM and decides > if he wants to commit to this snapshot or return to the original one - > which is the functionality the API is missing. > > >>> >>>> >>>>> >>>>> Thanks, Livnat >>>>> >>>> >>>> >>>> -- >>>> >>>> Michael Pasternak >>>> RedHat, ENG-Virtualization R&D >>>> _______________________________________________ >>>> Engine-devel mailing list >>>> Engine-devel(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel >>>> >> >> > -- Michael Pasternak RedHat, ENG-Virtualization R&D