9:07 a.m.
qemuDomainDetachNetDevice should not treat a timeout in a call to
qemuDomainWaitForDeviceRemoval as success. Instead, it should treat it
as failure -- this is the intention behind having a timeout.
If qemuDomainWaitForDeviceRemoval times out, it returns 0. In this
instance, if qemuDomainDetachNetDevice returns 0, virsh detach-interface
reports:
Interface detached successfully
with a zero exit status.
But with a negative return value from qemuDomainDetachNetDevice, virsh
detach-interface reports:
error: Failed to detach interface
error: An error occurred, but the cause is unknown
with a non-zero exit status, which is more appropriate.
Signed-off-by: Jonathan Davies <jonathan.davies(a)nutanix.com>
---
src/qemu/qemu_hotplug.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4be0f546c..d9a2f2d4d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5082,8 +5082,13 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ ret = qemuDomainWaitForDeviceRemoval(vm);
+ if (ret == 1) {
ret = qemuDomainRemoveNetDevice(driver, vm, detach);
+ } else if (ret == 0) {
+ VIR_WARN("Detach of device %s timed out; treating as a failure",
detach->ifname);
+ ret = -1;
+ }
cleanup:
qemuDomainResetDeviceRemoval(vm);
--
2.14.1
9:19 a.m.
On Tue, Mar 13, 2018 at 14:07:08 +0000, Jonathan Davies wrote:
qemuDomainDetachNetDevice should not treat a timeout in a call to
qemuDomainWaitForDeviceRemoval as success. Instead, it should treat it
as failure -- this is the intention behind having a timeout.
If qemuDomainWaitForDeviceRemoval times out, it returns 0. In this
instance, if qemuDomainDetachNetDevice returns 0, virsh detach-interface
reports:
Interface detached successfully
with a zero exit status.
But with a negative return value from qemuDomainDetachNetDevice, virsh
detach-interface reports:
error: Failed to detach interface
error: An error occurred, but the cause is unknown
with a non-zero exit status, which is more appropriate.
Signed-off-by: Jonathan Davies <jonathan.davies(a)nutanix.com>
---
This is unfortunately pre-existing behavior that we can't change. Some
versions of qemu don't support reporting of successful detach using the
event infrastructure, thus we can't ever know when that was successful.
Applications which need to be sure that the device was detached need to
register handlers for the libvirt device detached event.
NACK, this is broken behavior, but can't be changed.
9:23 a.m.
On Tue, Mar 13, 2018 at 02:07:08PM +0000, Jonathan Davies wrote:
qemuDomainDetachNetDevice should not treat a timeout in a call to
qemuDomainWaitForDeviceRemoval as success. Instead, it should treat it
as failure -- this is the intention behind having a timeout.
Actually this is intentional current behaviour. See the API docs for
virDomainDetachDeviceFlags:
* Beware that depending on the hypervisor and device type, detaching a device
* from a running domain may be asynchronous. That is, calling
* virDomainDetachDeviceFlags may just request device removal while the device
* is actually removed later (in cooperation with a guest OS). Previously,
* this fact was ignored and the device could have been removed from domain
* configuration before it was actually removed by the hypervisor causing
* various failures on subsequent operations. To check whether the device was
* successfully removed, either recheck domain configuration using
* virDomainGetXMLDesc() or add a handler for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
* event. In case the device is already gone when virDomainDetachDeviceFlags
* returns, the event is delivered before this API call ends. To help existing
* clients work better in most cases, this API will try to transform an
* asynchronous device removal that finishes shortly after the request into
* a synchronous removal. In other words, this API may wait a bit for the
* removal to complete in case it was not synchronous.
If qemuDomainWaitForDeviceRemoval times out, it returns 0. In this
instance, if qemuDomainDetachNetDevice returns 0, virsh detach-interface
reports:
Interface detached successfully
If anything needs changing it is this message from virsh.
Virsh should not assume return value of 0 means the device has been
detached. Virsh should do what the API docs suggest and check the
XML config to see if the asynchronous detach requets has completed
or not. IOW, it should report either
Interface detached successfully
or
Interface detach request still pending
with a zero exit status.
But with a negative return value from qemuDomainDetachNetDevice, virsh
detach-interface reports:
error: Failed to detach interface
error: An error occurred, but the cause is unknown
with a non-zero exit status, which is more appropriate.
Signed-off-by: Jonathan Davies <jonathan.davies(a)nutanix.com>
---
src/qemu/qemu_hotplug.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4be0f546c..d9a2f2d4d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5082,8 +5082,13 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ ret = qemuDomainWaitForDeviceRemoval(vm);
+ if (ret == 1) {
ret = qemuDomainRemoveNetDevice(driver, vm, detach);
+ } else if (ret == 0) {
+ VIR_WARN("Detach of device %s timed out; treating as a failure",
detach->ifname);
+ ret = -1;
+ }
cleanup:
qemuDomainResetDeviceRemoval(vm);
--
2.14.1
--
libvir-list mailing list
libvir-list(a)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 :|
9:42 a.m.
On Tue, Mar 13, 2018 at 14:23:22 +0000, Daniel P. Berrangé wrote:
On Tue, Mar 13, 2018 at 02:07:08PM +0000, Jonathan Davies wrote:
> If qemuDomainWaitForDeviceRemoval times out, it returns 0. In this
> instance, if qemuDomainDetachNetDevice returns 0, virsh detach-interface
> reports:
>
> Interface detached successfully
If anything needs changing it is this message from virsh.
Virsh should not assume return value of 0 means the device has been
detached. Virsh should do what the API docs suggest and check the
XML config to see if the asynchronous detach requets has completed
or not. IOW, it should report either
Interface detached successfully
or
Interface detach request still pending
Yeah, it's been on my nice-to-have todo list for a long time now.
Unfortunately, virsh doesn't know what device (i.e., its alias) is going
to be removed based on the XML passed by a user. I was thinking about
adding a new public API which would accept a device XML and return the
matching full device XML from domain definition to help virsh with this
task, but I was too lazy^Wbusy.
Jirka
9:43 a.m.
On Tue, Mar 13, 2018 at 03:42:10PM +0100, Jiri Denemark wrote:
On Tue, Mar 13, 2018 at 14:23:22 +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 13, 2018 at 02:07:08PM +0000, Jonathan Davies wrote:
> > If qemuDomainWaitForDeviceRemoval times out, it returns 0. In this
> > instance, if qemuDomainDetachNetDevice returns 0, virsh detach-interface
> > reports:
> >
> > Interface detached successfully
>
> If anything needs changing it is this message from virsh.
>
> Virsh should not assume return value of 0 means the device has been
> detached. Virsh should do what the API docs suggest and check the
> XML config to see if the asynchronous detach requets has completed
> or not. IOW, it should report either
>
> Interface detached successfully
>
> or
>
> Interface detach request still pending
Yeah, it's been on my nice-to-have todo list for a long time now.
Unfortunately, virsh doesn't know what device (i.e., its alias) is going
to be removed based on the XML passed by a user. I was thinking about
adding a new public API which would accept a device XML and return the
matching full device XML from domain definition to help virsh with this
task, but I was too lazy^Wbusy.
Then, we should at least make virsh say
Interface detach request accepted
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 :|
1:13 p.m.
On 13/03/18 14:23, Daniel P. Berrangé wrote:
On Tue, Mar 13, 2018 at 02:07:08PM +0000, Jonathan Davies wrote:
> qemuDomainDetachNetDevice should not treat a timeout in a call to
> qemuDomainWaitForDeviceRemoval as success. Instead, it should treat it
> as failure -- this is the intention behind having a timeout.
Actually this is intentional current behaviour. See the API docs for
virDomainDetachDeviceFlags:
* Beware that depending on the hypervisor and device type, detaching a device
* from a running domain may be asynchronous. That is, calling
* virDomainDetachDeviceFlags may just request device removal while the device
* is actually removed later (in cooperation with a guest OS). Previously,
* this fact was ignored and the device could have been removed from domain
* configuration before it was actually removed by the hypervisor causing
* various failures on subsequent operations. To check whether the device was
* successfully removed, either recheck domain configuration using
* virDomainGetXMLDesc() or add a handler for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
* event. In case the device is already gone when virDomainDetachDeviceFlags
* returns, the event is delivered before this API call ends. To help existing
* clients work better in most cases, this API will try to transform an
* asynchronous device removal that finishes shortly after the request into
* a synchronous removal. In other words, this API may wait a bit for the
* removal to complete in case it was not synchronous.
Thanks all the helpful replies.
Just to check I've understood: The design is that the call is
synchronous but may become asynchronous if it times out. The purpose of
the timeout is just to limit the window in which it might be synchronous.
But there's no way for the client to know whether it was synchronous or
asynchronous since the return value is the same in both cases.
Therefore a well-written client would always need to check whether the
operation completed successfully or not.
In which case, why bother with the synchronous mode and the server-side
timeout? Wouldn't it be simpler for it to be always asynchronous,
allowing the client to wait for as long as it likes before giving up?
Please correct me if I've misunderstood!
Thanks,
Jonathan
1:20 p.m.
On Tue, Mar 13, 2018 at 06:13:46PM +0000, Jonathan Davies wrote:
On 13/03/18 14:23, Daniel P. Berrangé wrote:
> On Tue, Mar 13, 2018 at 02:07:08PM +0000, Jonathan Davies wrote:
> > qemuDomainDetachNetDevice should not treat a timeout in a call to
> > qemuDomainWaitForDeviceRemoval as success. Instead, it should treat it
> > as failure -- this is the intention behind having a timeout.
>
> Actually this is intentional current behaviour. See the API docs for
> virDomainDetachDeviceFlags:
>
> * Beware that depending on the hypervisor and device type, detaching a device
> * from a running domain may be asynchronous. That is, calling
> * virDomainDetachDeviceFlags may just request device removal while the device
> * is actually removed later (in cooperation with a guest OS). Previously,
> * this fact was ignored and the device could have been removed from domain
> * configuration before it was actually removed by the hypervisor causing
> * various failures on subsequent operations. To check whether the device was
> * successfully removed, either recheck domain configuration using
> * virDomainGetXMLDesc() or add a handler for the
VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
> * event. In case the device is already gone when virDomainDetachDeviceFlags
> * returns, the event is delivered before this API call ends. To help existing
> * clients work better in most cases, this API will try to transform an
> * asynchronous device removal that finishes shortly after the request into
> * a synchronous removal. In other words, this API may wait a bit for the
> * removal to complete in case it was not synchronous.
Thanks all the helpful replies.
Just to check I've understood: The design is that the call is synchronous
but may become asynchronous if it times out. The purpose of the timeout is
just to limit the window in which it might be synchronous.
I'd say the opposite - the API is designed to be asynchronous, but you
might get lucky and have it complete before it returns.
But there's no way for the client to know whether it was
synchronous or
asynchronous since the return value is the same in both cases.
Therefore a well-written client would always need to check whether the
operation completed successfully or not.
In which case, why bother with the synchronous mode and the server-side
timeout? Wouldn't it be simpler for it to be always asynchronous, allowing
the client to wait for as long as it likes before giving up?
Yes Well written applications should always wait and check for actual
removal. Adding the server side wait was just a slightly gross
hack to make it slightly more reliable with badly written apps that
blindly assume completion
Please correct me if I've misunderstood!
You're right, and yes this is an unpleasnt API design. If we could go
back and do it again we'd do things differently, but we're fairly stuck
with it now.
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 :|
2:36 p.m.
On Tue, Mar 13, 2018 at 18:20:51 +0000, Daniel P. Berrangé wrote:
On Tue, Mar 13, 2018 at 06:13:46PM +0000, Jonathan Davies wrote:
> Thanks all the helpful replies.
>
> Just to check I've understood: The design is that the call is synchronous
> but may become asynchronous if it times out. The purpose of the timeout is
> just to limit the window in which it might be synchronous.
I'd say the opposite - the API is designed to be asynchronous, but you
might get lucky and have it complete before it returns.
It's both actually :-) The API was originally designed as synchronous,
although it was implemented asynchronously for most cases. Mainly
because the action requires a guest OS to cooperate and it may not even
allow or support detaching some devices at all and the request will be
completely ignored. On the other hand, detaching USB devices in QEMU is
a rare example of implementation which is always synchronous. And if a
guest OS cooperates, the process may be very fast so in reality users
were seeing this API as synchronous because in "normal" cases the
devices were already removed at the time the API returned success (or
when they immediately checked the device after the API finished).
When we fixed the API documentation to be explicit about the
asynchronous behavior, we wanted to make sure existing users still see
it as synchronous in normal cases and the short timeout was introduced.
At the same time we could not change the return value to distinguish
between finished and requested detach.
This is quite a mess, but it was still considered better than
introducing a new detach API which could have better semantics, but all
apps using libvirt would need to adapt and possibly be able to work with
both versions anyway.
Jirka
2446
days inactive
2446
days old
7 comments
5 participants
participants (5)
-
Daniel P. Berrangé -
Jiri Denemark -
Jonathan Davies -
Jonathan Davies -
Peter Krempa