libvirt: Failure to recover from failed detach

Originally discovered here: https://bugzilla.redhat.com/show_bug.cgi?id=1669225

The above error was addressed in change I09230fc47b0950aa5a3db839a070613c9c817576 . However, the above error was generated by a nova compute containing this patch, suggesting the fix was incomplete.

The fix does contain a race which could still result in the above error being raised, though: in _do_wait_and_retry_detach get_device_conf_func() might return the present device, which is then deleted before the subsequent call to detachDeviceFlags. I've written a failing unit test to demonstrate this.

Related fix proposed to branch: master
Review: https://review.opendev.org/671023

To summarize my private comment from the BZ linked earlier, I think the logged traceback is a red herring and that there is not actually a bug in the detach_device_with_retry code.

In our code, we can see that any detach attempt that results in a "not found" exception is caught and DeviceNotFound is raised instead (which is then ignored by the driver and compute manager code):

https://github.com/openstack/nova/blob/15e082a/nova/virt/libvirt/guest.py#L405-L422

So, in the log excerpt in the bug report, we are looking at a case where we *did* correctly handle a "not found" condition and raised DeviceNotFound. We are seeing the traceback because the oslo_utils excutils.save_and_reraise_exception context manager logs the original exception when the original exception is *not* re-raised and instead a new exception is raised:

https://github.com/openstack/oslo.utils/blob/1b8bafb391b82665b99b3da8205402ec2cd495d1/oslo_utils/excutils.py#L211-L217

So that traceback is not at all related to the failure described in the BZ.

An improvement I think we could do is to suppress the logging of the dropped exception traceback by oslo_utils, to prevent being misled in the future. We could use a technique like this:

https://github.com/openstack/nova/blob/15e082a2a87c87d2d30d13936bbda06489b4edcc/nova/compute/manager.py#L8459-L8473

to conditionally tell the context manager whether to re-raise the original exception. That way it will not log a traceback when we decide to raise a new exception.

Here's a neutron bug where they were seeing similar "Original exception being dropped" messages in their logs in cases where the excutils.save_and_reraise_exception context manager was not being allowed to re-raise its original exception (because a new exception was being raised instead):

https://bugs.launchpad.net/neutron/+bug/1669244

Yep. The actual error thrown was "Unable to detach from guest transient domain.", which is now "Unable to detach the device from the live config." in master. That RetryDecorator makes this function a whole lot harder to read, but with your explanation it seems that the detach was actually timing out, which is consistent with the underlying problem we eventually discovered.

Thanks! I'll close this out.

Change abandoned by Matthew Booth (<email address hidden>) on branch: master
Review: https://review.opendev.org/671023
Reason: As discussed in the bug, this error is already correctly handled elsewhere.

Related fix proposed to branch: master
Review: https://review.opendev.org/671640

Reviewed: https://review.opendev.org/671640
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=738774b942dab21d8c03c70b4e74b6a487515ff5
Submitter: Zuul
Branch: master

commit 738774b942dab21d8c03c70b4e74b6a487515ff5
Author: melanie witt <email address hidden>
Date: Thu Jul 18 23:59:16 2019 +0000

    Avoid logging traceback when detach device not found

    We use the oslo.utils save_and_reraise_exception context manager in our
    detach device code and catch specific exceptions that mean 'not found'
    and raise DeviceNotFound instead. When we do that, the
    save_and_reraise_exception context manager logs an ERROR traceback of
    the original exception, for informational purposes. This is misleading
    when trying to debug other issues, as it makes it look like the caught
    exception caused a problem.

    This passes the reraise=False keyword arg to the context manager and
    sets the 'reraise' attribute to True only if we are not going to raise
    a different exception.

    Related-Bug: #1836212

    Change-Id: Icce1e31fe3ebcbf9e4897bbfa57b7f3d1fba67a3

Related fix proposed to branch: stable/stein
Review: https://review.opendev.org/672833

Reviewed: https://review.opendev.org/672833
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=d83342226470e1957525978d7bec7d98db9bb695
Submitter: Zuul
Branch: stable/stein

commit d83342226470e1957525978d7bec7d98db9bb695
Author: melanie witt <email address hidden>
Date: Thu Jul 18 23:59:16 2019 +0000

    Avoid logging traceback when detach device not found

    We use the oslo.utils save_and_reraise_exception context manager in our
    detach device code and catch specific exceptions that mean 'not found'
    and raise DeviceNotFound instead. When we do that, the
    save_and_reraise_exception context manager logs an ERROR traceback of
    the original exception, for informational purposes. This is misleading
    when trying to debug other issues, as it makes it look like the caught
    exception caused a problem.

    This passes the reraise=False keyword arg to the context manager and
    sets the 'reraise' attribute to True only if we are not going to raise
    a different exception.

    Related-Bug: #1836212

    Change-Id: Icce1e31fe3ebcbf9e4897bbfa57b7f3d1fba67a3
    (cherry picked from commit 738774b942dab21d8c03c70b4e74b6a487515ff5)

Related fix proposed to branch: stable/rocky
Review: https://review.opendev.org/674068

Reviewed: https://review.opendev.org/674068
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=3a1be7afd0f11a2123c59b0ddea63f08bb4436ce
Submitter: Zuul
Branch: stable/rocky

commit 3a1be7afd0f11a2123c59b0ddea63f08bb4436ce
Author: melanie witt <email address hidden>
Date: Thu Jul 18 23:59:16 2019 +0000

    Avoid logging traceback when detach device not found

    We use the oslo.utils save_and_reraise_exception context manager in our
    detach device code and catch specific exceptions that mean 'not found'
    and raise DeviceNotFound instead. When we do that, the
    save_and_reraise_exception context manager logs an ERROR traceback of
    the original exception, for informational purposes. This is misleading
    when trying to debug other issues, as it makes it look like the caught
    exception caused a problem.

    This passes the reraise=False keyword arg to the context manager and
    sets the 'reraise' attribute to True only if we are not going to raise
    a different exception.

    Related-Bug: #1836212

    Change-Id: Icce1e31fe3ebcbf9e4897bbfa57b7f3d1fba67a3
    (cherry picked from commit 738774b942dab21d8c03c70b4e74b6a487515ff5)
    (cherry picked from commit d83342226470e1957525978d7bec7d98db9bb695)