[libvirt] [PATCH] qemu: fix migration with local and VIR_STORAGE_TYPE_NETWORK disks
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
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 :|
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
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.
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 :|
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
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.
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 :|
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
participants (3)
-
Chris Friesen -
Daniel P. Berrangé -
Peter Krempa