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, i suggested refactoring 2 to include 1 as single atomic operation at backend.

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,

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

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.

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

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.

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

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

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