In the current implementation of qemuMigrateDisk() the value of the
"nmigrate_disks" parameter wrongly impacts the decision whether or not
to migrate a disk that is not a member of "migrate_disks":

1) If "nmigrate_disks" is zero, "disk" is migrated if it's non-shared
non-readonly with source.

2) If "nmigrate_disks" is non-zero and "disk" is not a member of
"migrate_disks" then "disk" is not migrated.  This should instead proceed
with checking conditions as per 1) and allow migration of non-shared
non-readonly disks with source.

Fixing 2) breaks migration of VMs with a mix of rbd and local
disks because now libvirt tries to migrate the rbd root disk
and it fails.

This new problem is solved by updating 1) to factor in disk source type
and migrate only 'local' non-shared non-readonly disks with source.

The end result is that disks not in "migrate_disks" are treated
uniformly regardless of the value of "nmigrate_disks".

Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
---
 src/qemu/qemu_migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 5ee9e5c..77fafc6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -409,12 +409,12 @@ qemuMigrateDisk(virDomainDiskDef const *disk,
             if (STREQ(disk->dst, migrate_disks[i]))
                 return true;
         }
-        return false;
     }
 
-    /* Default is to migrate only non-shared non-readonly disks
+    /* Default is to migrate only non-shared non-readonly local disks
      * with source */
     return !disk->src->shared && !disk->src->readonly &&
+           (disk->src->type != VIR_STORAGE_TYPE_NETWORK) &&
            !virStorageSourceIsEmpty(disk->src);
 }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Feb 07, 2018 at 11:57:19AM -0600, Chris Friesen wrote:
> In the current implementation of qemuMigrateDisk() the value of the
> "nmigrate_disks" parameter wrongly impacts the decision whether or not
> to migrate a disk that is not a member of "migrate_disks":
> 
> 1) If "nmigrate_disks" is zero, "disk" is migrated if it's non-shared
> non-readonly with source.
> 
> 2) If "nmigrate_disks" is non-zero and "disk" is not a member of
> "migrate_disks" then "disk" is not migrated.  This should instead proceed
> with checking conditions as per 1) and allow migration of non-shared
> non-readonly disks with source.

Huh, this doesn't make sense. If an app has passed a list of disks
in migrate_disks, we must *never* touch any disk that is not present
in this list. If the app wanted the other disk(s) migrated, it would
have included it in the list of disks it passed in.

> 
> Fixing 2) breaks migration of VMs with a mix of rbd and local
> disks because now libvirt tries to migrate the rbd root disk
> and it fails.
> 
> This new problem is solved by updating 1) to factor in disk source type
> and migrate only 'local' non-shared non-readonly disks with source.
> 
> The end result is that disks not in "migrate_disks" are treated
> uniformly regardless of the value of "nmigrate_disks".
> 
> Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
> ---
>  src/qemu/qemu_migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 5ee9e5c..77fafc6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -409,12 +409,12 @@ qemuMigrateDisk(virDomainDiskDef const *disk,
>              if (STREQ(disk->dst, migrate_disks[i]))
>                  return true;
>          }
> -        return false;
>      }
>  
> -    /* Default is to migrate only non-shared non-readonly disks
> +    /* Default is to migrate only non-shared non-readonly local disks
>       * with source */
>      return !disk->src->shared && !disk->src->readonly &&
> +           (disk->src->type != VIR_STORAGE_TYPE_NETWORK) &&
>             !virStorageSourceIsEmpty(disk->src);
>  }
>  
> -- 
> 1.8.3.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/07/2018 12:05 PM, Daniel P. Berrangé wrote:
> On Wed, Feb 07, 2018 at 11:57:19AM -0600, Chris Friesen wrote:
>> In the current implementation of qemuMigrateDisk() the value of the
>> "nmigrate_disks" parameter wrongly impacts the decision whether or not
>> to migrate a disk that is not a member of "migrate_disks":
>>
>> 1) If "nmigrate_disks" is zero, "disk" is migrated if it's non-shared
>> non-readonly with source.
>>
>> 2) If "nmigrate_disks" is non-zero and "disk" is not a member of
>> "migrate_disks" then "disk" is not migrated.  This should instead proceed
>> with checking conditions as per 1) and allow migration of non-shared
>> non-readonly disks with source.
>
> Huh, this doesn't make sense. If an app has passed a list of disks
> in migrate_disks, we must *never* touch any disk that is not present
> in this list. If the app wanted the other disk(s) migrated, it would
> have included it in the list of disks it passed in.

Okay, that makes sense.  I can restore the "return false" here.

Are you okay with the other change?

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".

Chris

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Feb 07, 2018 at 13:11:33 -0600, Chris Friesen wrote:
> On 02/07/2018 12:05 PM, Daniel P. Berrangé wrote:
> > On Wed, Feb 07, 2018 at 11:57:19AM -0600, Chris Friesen wrote:
> > > In the current implementation of qemuMigrateDisk() the value of the
> > > "nmigrate_disks" parameter wrongly impacts the decision whether or not
> > > to migrate a disk that is not a member of "migrate_disks":
> > > 
> > > 1) If "nmigrate_disks" is zero, "disk" is migrated if it's non-shared
> > > non-readonly with source.
> > > 
> > > 2) If "nmigrate_disks" is non-zero and "disk" is not a member of
> > > "migrate_disks" then "disk" is not migrated.  This should instead proceed
> > > with checking conditions as per 1) and allow migration of non-shared
> > > non-readonly disks with source.
> > 
> > Huh, this doesn't make sense. If an app has passed a list of disks
> > in migrate_disks, we must *never* touch any disk that is not present
> > in this list. If the app wanted the other disk(s) migrated, it would
> > have included it in the list of disks it passed in.
> 
> Okay, that makes sense.  I can restore the "return false" here.
> 
> Are you okay with the other change?
> 
> 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 why do you use storage migration if your storage is shared or would
not be migrated as in the case of the config cdrom?

Said this, by default the storage migration probably should not try to
migrate network disks unless explicitly asked to do so. The check should
use virStorageSourceIsLocalStorage though, since blindly checking
NETWORK is not okay as you need to check the actual storage type.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Feb 07, 2018 at 01:11:33PM -0600, Chris Friesen wrote:
> On 02/07/2018 12:05 PM, Daniel P. Berrangé wrote:
> > On Wed, Feb 07, 2018 at 11:57:19AM -0600, Chris Friesen wrote:
> > > In the current implementation of qemuMigrateDisk() the value of the
> > > "nmigrate_disks" parameter wrongly impacts the decision whether or not
> > > to migrate a disk that is not a member of "migrate_disks":
> > > 
> > > 1) If "nmigrate_disks" is zero, "disk" is migrated if it's non-shared
> > > non-readonly with source.
> > > 
> > > 2) If "nmigrate_disks" is non-zero and "disk" is not a member of
> > > "migrate_disks" then "disk" is not migrated.  This should instead proceed
> > > with checking conditions as per 1) and allow migration of non-shared
> > > non-readonly disks with source.
> > 
> > Huh, this doesn't make sense. If an app has passed a list of disks
> > in migrate_disks, we must *never* touch any disk that is not present
> > in this list. If the app wanted the other disk(s) migrated, it would
> > have included it in the list of disks it passed in.
> 
> Okay, that makes sense.  I can restore the "return false" here.
> 
> 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.

> 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.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

>> 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.

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?



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

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.

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.

Chris

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/09/2018 04:15 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 08, 2018 at 01:24:58PM -0600, Chris Friesen wrote:

>> 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.

Thanks for the clarifications.

Chris

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list