Re: [Qemu-devel] ping RE: question: I found a qemu crash about migration

[Dr. David Alan Gilbert]

* Kevin Wolf (address@hidden) wrote:
> Am 29.09.2017 um 21:06 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (address@hidden) wrote:
> > > Am 28.09.2017 um 19:01 hat Dr. David Alan Gilbert geschrieben:
> > > > Hi,
> > > >   This is a 'fun' bug;  I had a good chat to kwolf about it earlier.
> > > > A proper fix really needs to be done together with libvirt so that we
> > > > can sequence:
> > > >    a) The stopping of the CPU on the source
> > > >    b) The termination of the mirroring block job
> > > >    c) The inactivation of the block devices on the source
> > > >        (bdrv_inactivate_all)
> > > >    d) The activation of the block devices on the destination
> > > >        (bdrv_invalidate_cache_all)
> > > >    e) The start of the CPU on the destination
> > > > 
> > > > It looks like you're hitting a race between b/c;  we've had races
> > > > between c/d in the past and moved the bdrv_inactivate_all.
> > > > 
> > > > During the discussion we ended up with two proposed solutions;
> > > > both of them require one extra command and one extra migration
> > > > capability.
> > > > 
> > > > The block way
> > > > -------------
> > > >    1) Add a new migration capability pause-at-complete
> > > >    2) Add a new migration state almost-complete
> > > >    3) After saving devices, if pause-at-complete is set,
> > > >       transition to almost-complete
> > > >    4) Add a new command (migration-continue) that 
> > > >       causes the migration to inactivate the devices (c)
> > > >       and send the final EOF to the destination.
> > > > 
> > > > You set pause-at-complete, wait until migrate hits almost-complete;
> > > > cleanup the mirror job, and then do migration-continue.  When it
> > > > completes do 'cont' on the destination.
> > > 
> > > Especially since you're concerned about the exact position of the pause,
> > > the following would be a little safer (using the opportunity to suggest
> > > slightly different names,too):
> > > 
> > > 1) Add a new migration capability pause-at-complete
> > > 2) Add a new migration state ready
> > > 3) After migration has converged (i.e. the background phase of the
> > >    migration has completed) and we are ready to actually switch to the
> > >    destination, stop the VM, transition to 'ready' and emit a QMP event
> > > 4) Continue processing QMP commands in the 'ready' phase. This is where
> > >    libvirt is supposed to tear down any running non-VM activities like
> > >    block jobs, NBD servers, etc.
> > > 5) Add a new command (migration-complete) that does the final part of
> > >    migration, including sending the remaining dirty memory, device state
> > >    and the final EOF that signals the destination to resume the VM if -S
> > >    isn't given.
> > 
> > What worries me here is whether some other subsection is going to say
> > that this pause must happen after the device state save, e.g. if the
> > device state save causes them to push out the last block/packet/etc -
> > and I've got no real way to say whether that point is any better or
> > worse than the point I was suggestion.
> > And the position in the cycle when the pause happens is part of the API.
> 
> The important point here is that the VM is stopped. If device emulation
> is doing anything by itself while the VM is stopped, that is a bug.
> In other words, that last block/packet/etc. must already have been
> pushed out when we stopped the VM, so there is nothing left to push out
> when we save the state.
> 
> With correctly written device code, there is no difference whether we
> save the device state first or last, without external influence it will
> be the same.
> 
> The obvious external thing that I can see is monitor commands, e.g. the
> monitor allows to access I/O ports, which can change the device state.
> However, monitor commands are completely under the control of the user,
> so they can always choose the order that they need.
> 
> In any case, saving the device state only immediately before doing the
> switch makes sure that we consider any changes that have still been
> made.
> 
> > > The main reason why I advocate this "block way" is that it looks a lot
> > > less hacky, but more future-proof to me. Instead of adding a one-off
> > > hack for the problem at hand, it introduces a phase where libvirt can
> > > cleanly tear down any activity it has still running on the source qemu.
> > 
> > Yes, if we get that phase right.
> > 
> > > We got away without doing this because we never did a clean handover of
> > > resources from the source to the destination. From the perspective of a
> > > management tool, we had these distinct phases during migration:
> > > 
> > > a) 'migrate' command:
> > >    Source VM is still running and in control of all resources (like disk
> > >    images), migrating some state in the background
> > > 
> > > b) Migration converges:
> > >    Both VMs are stopped (assuming -S is given on the destination,
> > >    otherwise this phase is skipped), the destination is in control of
> > >    the resources
> > > 
> > > c) 'cont' command:
> > >    Destination VM is running and in control of the resources
> > > 
> > > There is an asymmetry here because there is no phase where the VMs are
> > > stopped, but the source is in control of the resources (this is the
> > > additional state that my proposal introduces).
> > > 
> > > As long as we didn't really manage control of the resources with locks,
> > > we could abuse b) for things where the source still accessed the
> > > resources if we knew that the destination wouldn't use them yet (it's
> > > not an assumption that I'm willing to trust too much) and it would still
> > > kind of work in the common cases. But it's not really the correct thing
> > > to do, it has probably always caused bugs in corner cases, and the
> > > locking mechanisms are pointing that out now.
> > > 
> > > We're now noticing this with block device because we're using locking,
> > > but I wouldn't be surprised if we found sooner or later that there are
> > > more resources that should really be handed over in a cleanly designed
> > > way and that work only by accident as long as you don't do that. So I
> > > don't think a local hack for mirroring (or even all block jobs) is
> > > sufficient to be future-proof.
> > 
> > Without the block-job we were actually pretty safe - up until the EOF
> > we knew the destination wouldn't start, and similarly we wouldn't start
> > using a device on the destination before the source had saved that
> > device.
> 
> s/Without the block-job/With only the guest device/
> 
> As long as only a guest device is using a resource, we're pretty safe,
> yes. This is because the VM is stopped when we actually migrate, so the
> user is already shut down without management tool interaction and the
> resource isn't actually in use at this point.
> 
> Anything that keeps using a resource while the VM is stopped is a
> potential problem. Apart from block jobs, this includes at least the
> built-in NBD server, which also needs to be shut down before we can
> complete migration. And probably other things I'm forgetting right now.
> 
> > My worry is where the edge of 'source is in control of the device is' -
> > is that upto the point at which the end of the device save? Before that?
> > After that? What?  It seems to be pretty arbitrary.  Unless we can
> > understand why it's where it is, then I'm not sure if it's a 'clean
> > migration phase model'.
> 
> I can tell you where in the phase model it is: The source gives up
> control of the image during 'migration-complete', i.e. at the transition
> between the 'ready' and the 'completed' phase on the source.
> 
> Within this transition, it's less clear where it has to be, but that's
> the same with its current place. I expect all of this code to stay
> mostly unchanged, but just called a bit later, so the place where we
> inactivate images today sounds like a good candidate. It's important
> that it's done before sending the EOF packet that triggers the
> destination to try and take control of the images.
> 
> > > > The migration way
> > > > -----------------
> > > >    1) Stop doing (d) when the destination is started with -S
> > > >       since it happens anyway when 'cont' is issued
> > > >    2) Add a new migration capability ext-manage-storage
> > > >    3) When 'ext-manage-storage' is set, we don't bother doing (c)
> > > >    4) Add a new command 'block-inactivate' on the source
> > > > 
> > > > You set ext-manage-storage, do the migrate and when it's finished
> > > > clean up the block job, block-inactivate on the source, and
> > > > then cont on the destination.
> > > 
> > > You would have to inactivate every single block node, which is not
> > > a thing libvirt can currently do (they don't even really manage
> > > individual nodes yet). Apart from that I believe it is much too
> > > low-level and users shouldn't have to worry about what implications
> > > migration has to block devices. A clean migration phase model should be
> > > much easier to understand.
> > 
> > Yep, I see that.
> > (Having said that, this extra round-trip with management is adding extra
> > downtime, and we'll have to work out things like what happens if you
> > cancel or fail in that time).
> 
> libvirt can opt in to the additional phase only when it actually needs
> it.
> 
> I'm not sure if the requirement can change while migration is already
> running. I don't think so currently, but if yes, we might need a way for
> management tools to change the capability setting after the fact.

OK, I'm going to look at doing this in the next few days.

Dave

> Kevin
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK