qemu_migration: check for interface type 'hostdev'

[PATCH] qemu_migration: check for interface type 'hostdev'

Posted by Kristina Hanicova 2 years, 8 months ago

When we try to migrate vm, we check if it contains only devices
that are able to migrate. If a hostdev device is not able to
migrate we raise an error with <hostdev/>, but it can actually be
<interface/>, so we need to check if hostdev device was created
by us from interface and show the right error message.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942315

Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
 src/qemu/qemu_migration.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4d651aeb1a..34eee9c8b6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1272,9 +1272,15 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
                 }
 
                 /* all other PCI hostdevs can't be migrated */
-                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-                               _("cannot migrate a domain with <hostdev mode='subsystem' type='%s'>"),
-                               virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
+                if (hostdev->parentnet) {
+                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                                   _("cannot migrate a domain with <interface type='%s'>"),
+                                   virDomainNetTypeToString(hostdev->parentnet->type));
+                } else {
+                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                                   _("cannot migrate a domain with <hostdev mode='subsystem' type='%s'>"),
+                                   virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
+                }
                 return false;
 
             case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
-- 
2.31.1

Re: [PATCH] qemu_migration: check for interface type 'hostdev'

Posted by Michal Prívozník 2 years, 8 months ago

On 7/28/21 6:17 PM, Kristina Hanicova wrote:
> When we try to migrate vm, we check if it contains only devices
> that are able to migrate. If a hostdev device is not able to
> migrate we raise an error with <hostdev/>, but it can actually be
> <interface/>, so we need to check if hostdev device was created
> by us from interface and show the right error message.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942315
> 
> Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 4d651aeb1a..34eee9c8b6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1272,9 +1272,15 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
>                  }
>  
>                  /* all other PCI hostdevs can't be migrated */
> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> -                               _("cannot migrate a domain with <hostdev mode='subsystem' type='%s'>"),
> -                               virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
> +                if (hostdev->parentnet) {
> +                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                                   _("cannot migrate a domain with <interface type='%s'>"),
> +                                   virDomainNetTypeToString(hostdev->parentnet->type));

Small nit, I wonder whether we should report actual type here. Looking
into virDomainNetDefFormat() it looks like for a running VM we do report
actual type (unless inactive or migratable XML was requested). Thus I
think the error message should follow that logic. Otherwise we might
report "cannot migrate a domain with interface type=network" while in
fact in 'virsh dumpxml' there is just interface type='hostdev' (the
type='network' is in inactive XML).

Laine, what's your opinion?

Michal

Re: [PATCH] qemu_migration: check for interface type 'hostdev'

Posted by Laine Stump 2 years, 8 months ago

On 7/29/21 5:42 AM, Michal Prívozník wrote:
> On 7/28/21 6:17 PM, Kristina Hanicova wrote:
>> When we try to migrate vm, we check if it contains only devices
>> that are able to migrate. If a hostdev device is not able to
>> migrate we raise an error with <hostdev/>, but it can actually be
>> <interface/>, so we need to check if hostdev device was created
>> by us from interface and show the right error message.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942315
>>
>> Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
>> ---
>>   src/qemu/qemu_migration.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 4d651aeb1a..34eee9c8b6 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1272,9 +1272,15 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def)
>>                   }
>>   
>>                   /* all other PCI hostdevs can't be migrated */
>> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> -                               _("cannot migrate a domain with <hostdev mode='subsystem' type='%s'>"),
>> -                               virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
>> +                if (hostdev->parentnet) {
>> +                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> +                                   _("cannot migrate a domain with <interface type='%s'>"),
>> +                                   virDomainNetTypeToString(hostdev->parentnet->type));
> 
> Small nit, I wonder whether we should report actual type here. Looking
> into virDomainNetDefFormat() it looks like for a running VM we do report
> actual type (unless inactive or migratable XML was requested). Thus I
> think the error message should follow that logic. Otherwise we might
> report "cannot migrate a domain with interface type=network" while in
> fact in 'virsh dumpxml' there is just interface type='hostdev' (the
> type='network' is in inactive XML).
> 
> Laine, what's your opinion?


Well.... neither is ideal :-/. If the log message says "interface 
type='network'" then that could mislead the user into thinking that no 
type='network' interfaces would be supported. But if the log message 
says "interface type='hostdev'" then the user will look in their XML, 
not find any type='hostdev', and then be confused about what the message 
is referencing.

Ideally the error message should make it simple to find the exact 
element causing the problem while also being precise in explaining what 
is problematic. Since neither of the choices here satisfy both, we just 
have to pick the 'least bad' option. I do think in this case that the 
least bad option is to display tha actualtype as you suggest (which, 
BTW, in this case will always be "hostdev").

Re: [PATCH] qemu_migration: check for interface type 'hostdev'

Posted by Michal Prívozník 2 years, 8 months ago

On 8/3/21 3:59 PM, Laine Stump wrote:
> On 7/29/21 5:42 AM, Michal Prívozník wrote:
>> On 7/28/21 6:17 PM, Kristina Hanicova wrote:
>>> When we try to migrate vm, we check if it contains only devices
>>> that are able to migrate. If a hostdev device is not able to
>>> migrate we raise an error with <hostdev/>, but it can actually be
>>> <interface/>, so we need to check if hostdev device was created
>>> by us from interface and show the right error message.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942315
>>>
>>> Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
>>> ---
>>>   src/qemu/qemu_migration.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index 4d651aeb1a..34eee9c8b6 100644
>>> --- a/src/qemu/qemu_migration.c
>>> +++ b/src/qemu/qemu_migration.c
>>> @@ -1272,9 +1272,15 @@ qemuMigrationSrcIsAllowedHostdev(const
>>> virDomainDef *def)
>>>                   }
>>>                     /* all other PCI hostdevs can't be migrated */
>>> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> -                               _("cannot migrate a domain with
>>> <hostdev mode='subsystem' type='%s'>"),
>>> -                              
>>> virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type));
>>> +                if (hostdev->parentnet) {
>>> +                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> +                                   _("cannot migrate a domain with
>>> <interface type='%s'>"),
>>> +                                  
>>> virDomainNetTypeToString(hostdev->parentnet->type));
>>
>> Small nit, I wonder whether we should report actual type here. Looking
>> into virDomainNetDefFormat() it looks like for a running VM we do report
>> actual type (unless inactive or migratable XML was requested). Thus I
>> think the error message should follow that logic. Otherwise we might
>> report "cannot migrate a domain with interface type=network" while in
>> fact in 'virsh dumpxml' there is just interface type='hostdev' (the
>> type='network' is in inactive XML).
>>
>> Laine, what's your opinion?
> 
> 
> Well.... neither is ideal :-/. If the log message says "interface
> type='network'" then that could mislead the user into thinking that no
> type='network' interfaces would be supported. But if the log message
> says "interface type='hostdev'" then the user will look in their XML,
> not find any type='hostdev', and then be confused about what the message
> is referencing.

They would find type='hostdev' in the live XML and type='network' in
inactive one.

> 
> Ideally the error message should make it simple to find the exact
> element causing the problem while also being precise in explaining what
> is problematic. Since neither of the choices here satisfy both, we just
> have to pick the 'least bad' option. I do think in this case that the
> least bad option is to display tha actualtype as you suggest (which,
> BTW, in this case will always be "hostdev").
> 

Agreed. Fixed and pushed.

Michal