On Tue, Apr 20, 2010 at 11:42:15AM +0200, Iustin Pop wrote: > On Tue, Apr 20, 2010 at 10:03:19AM +0100, Guido Trotter wrote: > > On Tue, Apr 20, 2010 at 9:31 AM, Iustin Pop <[email protected]> wrote: > > > On Mon, Apr 19, 2010 at 04:54:37PM +0100, Guido Trotter wrote: > > >> Currently some hypervisors (namely kvm) need to do some cleanup after > > >> making sure an instance is stopped. With the moving of the retry cycle > > >> in backend those cleanups were never done. In order to solve this we add > > >> a new optional hypervisor function, CleanupInstance, which gets called > > >> at the end of the shutdown procedure, and which interested hypervisors > > >> can implement to be sure not to miss cleanup operations. > > >> > > >> Signed-off-by: Guido Trotter <[email protected]> > > > > > > NAK. > > > > > > See my patch from yesterday which fixes this by removing the retry cycle > > > from backend. > > > > > > The rationale for not adding a separate cleanup step (which I thought > > > about) is that: > > > > > > - you cannot know just from instance list whether to run or not the > > > Cleanup > > > - the existence of a new state (shutdown but not cleaned up) should be > > > reflected all across to the master and command line tools, and we > > > don't want that > > > - it is cleaner to just make sure StopInstance does all the neeed > > > cleanup, rather than introduce a new step > > > > > > Let's discuss! > > > > > > > Ok, that solution works well for me! :) > > Heh, I wanted to discuss this, as it's not quite trivial. > > I finally got to test my patch with burnin, and at least for Xen, the > removal of the Retry from backend breaks things as the shutdown is not > immediate. > > So: > > - we do need somewhere a retry logic for actually killing the VM (but > not necessarily other cleanup steps) > - we do need a cleanup step > > I think that both of these should be in the hypervisor code, since > backend doesn't know about each hypervisor's needs. > > So we can either: > - go with your patch as is, but we might need to add the Cleanup phase > to the retry cycle > - move the retry cycle inside hv_base, and abstract the actual "is instance > running" and "terminate instance" in a generic way
I just realized: most, if not all, hypervisors have the following things: - is instance running? - cleanly stop instance - forcefully stop instance So even if we just abstract these checks separately, we have a nicer code. Right now, the StopInstance code is somewhat unclean. This is the "more work" option though. iustin -- Subscription settings: http://groups.google.com/group/ganeti-devel/subscribe?hl=en
