Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > Am 03.04.2018 um 22:52 hat Dr. David Alan Gilbert geschrieben:
> > > > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > > > Am 28.03.2018 um 19:02 hat Dr. David Alan Gilbert (git) geschrieben:
> > > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > > 
> > > > > > Activating the block devices causes the locks to be taken on
> > > > > > the backing file.  If we're running with -S and the destination libvirt
> > > > > > hasn't started the destination with 'cont', it's expecting the locks are
> > > > > > still untaken.
> > > > > > 
> > > > > > Don't activate the block devices if we're not going to autostart the VM;
> > > > > > 'cont' already will do that anyway.
> > > > > > 
> > > > > > bz: https://bugzilla.redhat.com/show_bug.cgi?id=1560854
> > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > 
> > > > > I'm not sure that this is a good idea. Going back to my old writeup of
> > > > > the migration phases...
> > > > > 
> > > > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg07917.html
> > > > > 
> > > > > ...the phase between migration completion and 'cont' is described like
> > > > > this:
> > > > > 
> > > > >     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
> > > > > 
> > > > > This patch changes the definition of the phase so that neither side is
> > > > > in control of the resources. We lose the phase where the destination is
> > > > > in control, but the VM isn't running yet. This feels like a problem to
> > > > > me.
> > > > 
> > > > But see Jiri's writeup on that bz;  libvirt is hitting the opposite
> > > > problem;   in this corner case they can't have the destination taking
> > > > control yet.
> > > 
> > > I wonder if they can't already grant the destination QEMU the necessary
> > > permission in the pre-switchover phase. Just a thought, I don't know how
> > > this works in detail, so it might not possible after all.
> > 
> > It's a fairly hairy failure case they had; if I remember correctly it's:
> >   a) Start migration
> >   b) Migration gets to completion point
> >   c) Destination is still paused
> >   d) Libvirt is restarted on the source
> >   e) Since libvirt was restarted it fails the migration (and hence knows
> >      the destination won't be started)
> >   f) It now tries to resume the qemu on the source
> > 
> > (f) fails because (b) caused the locks to be taken on the destination;
> > hence this patch stops doing that.  It's a case we don't really think
> > about - i.e. that the migration has actually completed and all the data
> > is on the destination, but libvirt decides for some other reason to
> > abandon migration.
> 
> If you do remember correctly, that scenario doesn't feel tricky at all.
> libvirt needs to quit the destination qemu, which will inactivate the
> images on the destination and release the lock, and then it can continue
> the source.
> 
> In fact, this is so straightforward that I wonder what else libvirt is
> doing. Is the destination qemu only shut down after trying to continue
> the source? That would be libvirt using the wrong order of steps.

I'll leave Jiri to reply to this; I think this is a case of the source
realising libvirt has restarted, then trying to recover all of it's VMs
without being in the position of being able to check on the destination.

> > > > > Consider a case where the management tool keeps a mirror job with
> > > > > sync=none running to expose all I/O requests to some external process.
> > > > > It needs to shut down the old block job on the source in the
> > > > > 'pre-switchover' state, and start a new block job on the destination
> > > > > when the destination controls the images, but the VM doesn't run yet (so
> > > > > that it doesn't miss an I/O request). This patch removes the migration
> > > > > phase that the management tool needs to implement this correctly.
> > > > > 
> > > > > If we need a "neither side has control" phase, we might need to
> > > > > introduce it in addition to the existing phases rather than replacing a
> > > > > phase that is still needed in other cases.
> > > > 
> > > > This is yet another phase to be added.
> > > > IMHO this needs the managment tool to explicitly take control in the
> > > > case you're talking about.
> > > 
> > > What kind of mechanism do you have in mind there?
> > > 
> > > Maybe what could work would be separate QMP commands to inactivate (and
> > > possibly for symmetry activate) all block nodes. Then the management
> > > tool could use the pre-switchover phase to shut down its block jobs
> > > etc., inactivate all block nodes, transfer its own locks and then call
> > > migrate-continue.
> > 
> > Yes it was a 'block-activate' that I'd wondered about.  One complication
> > is that if this now under the control of the management layer then we
> > should stop asserting when the block devices aren't in the expected
> > state and just cleanly fail the command instead.
> 
> Requiring an explicit 'block-activate' on the destination would be an
> incompatible change, so you would have to introduce a new option for
> that. 'block-inactivate' on the source feels a bit simpler.

I'd only want the 'block-activate' in the case of this new block-job
you're suggesting; not in the case of normal migrates - they'd still get
it when they do 'cont' - so the change in behaviour is only with that
block-job case that must start before the end of migrate.

> And yes, you're probably right that we would have to be more careful to
> catch inactive images without crashing. On the other hand, it would
> become a state that is easier to test because it can be directly
> influenced via QMP rather than being only a side effect of migration.

Yes; but crashing is really bad, so we should really really stopping
asserting all over.

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK