On 05/30/2012 05:39 PM, Omer Frenkel wrote: > > > ----- Original Message ----- >> From: "Michael Pasternak" <[email protected]> >> To: "Livnat Peer" <[email protected]> >> Cc: "engine-devel" <[email protected]> >> 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 >> [email protected] >> http://lists.ovirt.org/mailman/listinfo/engine-devel >> -- Michael Pasternak RedHat, ENG-Virtualization R&D _______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
