[libvirt] [PATCH] qemu: fix migration with local and VIR_STORAGE_TYPE_NETWORK disks

[Peter Krempa]

On Thu, Feb 08, 2018 at 13:24:58 -0600, Chris Friesen wrote:
> On 02/08/2018 03:07 AM, Daniel P. Berrangé wrote:
> > On Wed, Feb 07, 2018 at 01:11:33PM -0600, Chris Friesen wrote:
> > > Are you okay with the other change?
> > 
> > That part of the code was intended to be funtionally identical to what
> > QEMU's previous built-in storage migration code would do. I don't want
> > to see the semantics of that change, because it makes libvirt behaviour
> > vary depending on which QEMU version you are using.
> > 
> > If that logic is not right for a particular usage scenario, applications
> > are expected to provide the "migrate_disks" parameter.
> 
> My coworker has pointed out another related issue.
> 
> In tools/virsh-domain.c, doMigrate(), if we specify "migrate-disks" with an
> empty list, the behaviour is the same as if it is not specified at all.
> That is, the fact that it was specified but empty is lost.

That is a quirk of the shell->API translation. In the API it's not
possible to set an empty list, but rather you don't specify any disks to
migrate 

> 
> > > Our original problem scenario was where the root disk is rbd and there is a
> > > read-only ISO config-drive, and "nmigrate_disks" is zero.  What we see in
> > > this case is that qemuMigrateDisk() returns "true" for the rbd disk, which
> > > then causes qemuMigrationPrecreateStorage() to fail with "pre-creation of
> > > storage targets for incremental storage migration is not supported".
> > 
> > So you want zero disks migrated. Simply don't ask for storage migration in
> > the first place if you don't have any disks to migrate.
> 
> In this case yes, but now we're talking about duplicating the libvirt logic
> around which disks to migrate in the code that calls libvirt.

Not really. The logic behind specifying disks to migrate from the
external application should also include the information whether a given
disk image is present on the remote host or not.

The API is designed to allow migrating a subset of the disks in cases
where some images are actually accessible and some are not. See below
for further explanation.

> There is a comment in the OpenStack nova code that looks like this:
> 
> # Due to a quirk in the libvirt python bindings,
> # VIR_MIGRATE_NON_SHARED_INC with an empty migrate_disks is
> # interpreted as "block migrate all writable disks" rather than
> # "don't block migrate any disks". This includes attached
> # volumes, which will potentially corrupt data on those
> # volumes. Consequently we need to explicitly unset
> # VIR_MIGRATE_NON_SHARED_INC if there are no disks to be block
> # migrated.
> 
> It sounds like it's not just a quirk, but rather design intent?

Exactly. And the above comment seems like a misunderstanding of the
meaning of the flag in the documentation:

VIR_MIGRATE_NON_SHARED_DISK 	= 	64 	

Migrate full disk images in addition to domain's memory. By default only
non-shared non-readonly disk images are transferred. The
VIR_MIGRATE_PARAM_MIGRATE_DISKS parameter can be used to specify which
disks should be migrated. This flag and VIR_MIGRATE_NON_SHARED_INC are
mutually exclusive.

VIR_MIGRATE_NON_SHARED_INC 	= 	128 	

Migrate disk images in addition to domain's memory. This is similar to
VIR_MIGRATE_NON_SHARED_DISK, but only the top level of each disk's
backing chain is copied. That is, the rest of the backing chain is
expected to be present on the destination and to be exactly the same as
on the source host. This flag and VIR_MIGRATE_NON_SHARED_DISK are
mutually exclusive.

> Given your comment above about "I don't want to see the semantics of that
> change", it sounds like you're suggesting:
> 
> 1) If there are any non-shared non-readonly network drives then the user

Note that the 'shared' word here implies disks declared with the
<shareable/> keyword. This means that the disk is written to by multiple
VMs (use of filesystem supporting this is required).

> can't rely on the default behaviour of VIR_MIGRATE_NON_SHARED_INC to do the
> right thing and therefore must explicitly specify the list of drives to
> migrate

Exactly. Libvirt can't really assume which disks the user wishes to
migrate (which are not accessible on the destination). The defaults
assume that no storage is common between the hosts.

Readonly disks are not copied since qemu refuses to write to them, so
they would need to be instantiated as read-write, but that would
violate the configuration.

> 2) If there are no drives to migrate, then it is not valid to specify
> VIR_MIGRATE_NON_SHARED_INC with an empty "migrate_disks", but instead the
> caller should ensure that VIR_MIGRATE_NON_SHARED_INC is not set.

As said above. The API does not have a notion of empty "migrate_diks",
since it's using the virTypedParameter generic interface.

This means that you actually can't specify the "migrate_disks" parameter
as empty. You can only omit it completely, which then translates to the
default behavior.

This means that if an APP is constructing the "migrate_disks" parameter
and the result is empty it shall not include VIR_MIGRATE_NON_SHARED_INC.

> Is that a fair summation?  If so, I'd suggest that this is non-intuitive
> from a user's perspective and at a minimum should be more explicitly
> documented.

I think the documentation is clear, but feel free to suggest a better
wording. Note that libvirt by default assumes that the images are
accessible on the destination directly (or in a different path if a
modified XML is provided). So by default no disk image data is copied at
all.

Also note that the behavior of virTypedParams can't really be changed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180209/35b30792/attachment-0001.sig>