On Thu, Feb 08, 2018 at 01:24:58PM -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. Yes, that is an unfortunately unfixable artifact of the way we encode the parameter list in the API and on the wire. We simply have an array of virTypedParameter elements, and so given that it is impossible to distinguish between not specified, and specified but empty. THis is obvious at the C level, but surprising at the Python level because of the way its mapped to Python. > 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. > > 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? I wouldn't say "intent" - it is essentially the only choice we had at the python level, given the C API. > 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 > 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 I would not make that conditional. Just always specific the list of disk to migrate, if you're using new enough libvirt. > 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. Yes, don't ask for shared storage migration if there's no storage to migrate. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
