[PATCH] qemu_migration: check for interface type 'hostdev'
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
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
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").
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
participants (3)
-
Kristina Hanicova -
Laine Stump -
Michal Prívozník