Fix missing instance.delete notification

The I8742071b55f018f864f5a382de20075a5b444a79 introduced cases when an
instance object is destroyed without the instance.delete notification
being emitted.

This patch adds the necessary notification to restore legacy
behaviour.

Closes-Bug: #1665263
Change-Id: I55ce90ca34c927c5dcd340fa5bdb0607a4ad4971
This commit is contained in:
Balazs Gibizer 2017-03-09 17:28:02 +01:00 committed by Matt Riedemann
parent 54629ff944
commit f9ac9531fb
8 changed files with 245 additions and 15 deletions

View File

@ -1761,7 +1761,9 @@ class API(base.Base):
# If instance is None it has already been deleted.
if cell:
with nova_context.target_cell(context, cell):
instance.destroy()
with compute_utils.notify_about_instance_delete(
self.notifier, context, instance):
instance.destroy()
else:
instance.destroy()
except exception.InstanceNotFound:
@ -1808,7 +1810,9 @@ class API(base.Base):
cell, instance = self._lookup_instance(context, instance.uuid)
if cell and instance:
with nova_context.target_cell(context, cell):
instance.destroy()
with compute_utils.notify_about_instance_delete(
self.notifier, context, instance):
instance.destroy()
return
if not instance:
# Instance is already deleted.

View File

@ -14,6 +14,7 @@
"""Compute-related Utilities and helpers."""
import contextlib
import functools
import inspect
import itertools
@ -700,3 +701,18 @@ class UnlimitedSemaphore(object):
@property
def balance(self):
return 0
@contextlib.contextmanager
def notify_about_instance_delete(notifier, context, instance):
# Pre-load system_metadata because if this context is around an
# instance.destroy(), lazy-loading it later would result in an
# InstanceNotFound error.
system_metadata = instance.system_metadata
try:
notify_about_instance_usage(notifier, context, instance,
"delete.start")
yield
finally:
notify_about_instance_usage(notifier, context, instance, "delete.end",
system_metadata=system_metadata)

View File

@ -970,7 +970,7 @@ class ComputeTaskManager(base.Base):
inst_mapping.save()
if not self._delete_build_request(
build_request, instance, cell, instance_bdms):
context, build_request, instance, cell, instance_bdms):
# The build request was deleted before/during scheduling so
# the instance is gone and we don't have anything to build for
# this one.
@ -995,13 +995,15 @@ class ComputeTaskManager(base.Base):
host=host['host'], node=host['nodename'],
limits=host['limits'])
def _delete_build_request(self, build_request, instance, cell,
def _delete_build_request(self, context, build_request, instance, cell,
instance_bdms):
"""Delete a build request after creating the instance in the cell.
This method handles cleaning up the instance in case the build request
is already deleted by the time we try to delete it.
:param context: the context of the request being handled
:type context: nova.context.RequestContext'
:param build_request: the build request to delete
:type build_request: nova.objects.BuildRequest
:param instance: the instance created from the build_request
@ -1020,18 +1022,20 @@ class ComputeTaskManager(base.Base):
# processed, and the build should halt here. Clean up the
# bdm and instance record.
with obj_target_cell(instance, cell):
try:
instance.destroy()
except exception.InstanceNotFound:
pass
except exception.ObjectActionError:
# NOTE(melwitt): Instance became scheduled during
# the destroy, "host changed". Refresh and re-destroy.
with compute_utils.notify_about_instance_delete(
self.notifier, context, instance):
try:
instance.refresh()
instance.destroy()
except exception.InstanceNotFound:
pass
except exception.ObjectActionError:
# NOTE(melwitt): Instance became scheduled during
# the destroy, "host changed". Refresh and re-destroy.
try:
instance.refresh()
instance.destroy()
except exception.InstanceNotFound:
pass
for bdm in instance_bdms:
with obj_target_cell(bdm, cell):
try:

View File

@ -7823,6 +7823,100 @@ class ComputeTestCase(BaseTestCase):
block_device_mapping=[])
self.assertEqual('Preserve this', instance.fault.message)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.context.target_cell')
@mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
@mock.patch('nova.objects.Service.get_minimum_version')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.objects.BuildRequest.get_by_instance_uuid')
def test_delete_while_booting_instance_not_in_cell_db_cellsv2(
self, br_get_by_instance, notify, minimum_server_version,
im_get_by_instance, target_cell, instance_destroy):
minimum_server_version.return_value = 15
im_get_by_instance.return_value = mock.Mock()
instance = self._create_fake_instance_obj()
instance.host = None
instance.save()
self.compute_api._delete_instance(self.context, instance)
instance_destroy.assert_called_once_with()
# the instance is updated during the delete so we only match by uuid
test_utils.assert_instance_delete_notification_by_uuid(
notify, instance.uuid, self.compute_api.notifier, self.context)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.objects.Service.get_minimum_version')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.objects.BuildRequest.get_by_instance_uuid')
def test_delete_while_booting_instance_not_in_cell_db_cellsv1(
self, br_get_by_instance, notify, minimum_server_version,
instance_destroy):
minimum_server_version.return_value = 14
instance = self._create_fake_instance_obj()
instance.host = None
instance.save()
self.compute_api._delete_instance(self.context, instance)
test_utils.assert_instance_delete_notification_by_uuid(
notify, instance.uuid, self.compute_api.notifier, self.context)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.objects.BuildRequest.get_by_instance_uuid')
def test_delete_while_booting_instance_not_scheduled_cellv1(
self, br_get_by_instance, notify, im_get_by_instance,
instance_destroy):
instance = self._create_fake_instance_obj()
instance.host = None
instance.save()
# This means compute api looks for an instance to destroy
br_get_by_instance.side_effect = exception.BuildRequestNotFound(
uuid=instance.uuid)
# no mapping means cellv1
im_get_by_instance.return_value = None
self.compute_api._delete_instance(self.context, instance)
test_utils.assert_instance_delete_notification_by_uuid(
notify, instance.uuid, self.compute_api.notifier, self.context)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.context.target_cell')
@mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.objects.BuildRequest.get_by_instance_uuid')
def test_delete_while_booting_instance_not_scheduled_cellv2(
self, br_get_by_instance, notify, im_get_by_instance, target_cell,
instance_destroy):
instance = self._create_fake_instance_obj()
instance.host = None
instance.save()
# This means compute api looks for an instance to destroy
br_get_by_instance.side_effect = exception.BuildRequestNotFound(
uuid=instance.uuid)
# having a mapping means cellsv2
im_get_by_instance.return_value = mock.Mock()
self.compute_api._delete_instance(self.context, instance)
instance_destroy.assert_called_once_with()
test_utils.assert_instance_delete_notification_by_uuid(
notify, instance.uuid, self.compute_api.notifier, self.context)
class ComputeAPITestCase(BaseTestCase):
def setUp(self):

View File

@ -1209,8 +1209,7 @@ class _ComputeAPIUnitTestMixIn(object):
constraint='constraint').AndReturn(fake_inst)
compute_utils.notify_about_instance_usage(
self.compute_api.notifier, self.context,
inst, 'delete.end',
system_metadata=inst.system_metadata)
inst, 'delete.end', system_metadata={})
self.mox.ReplayAll()

View File

@ -835,6 +835,23 @@ class ComputeUtilsTestCase(test.NoDBTestCase):
self.assertEqual([], addresses)
mock_ifaddresses.assert_called_once_with(iface)
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.objects.Instance.destroy')
def test_notify_about_instance_delete(self, mock_instance_destroy,
mock_notify_usage):
instance = fake_instance.fake_instance_obj(
self.context, expected_attrs=('system_metadata',))
with compute_utils.notify_about_instance_delete(
mock.sentinel.notifier, self.context, instance):
instance.destroy()
expected_notify_calls = [
mock.call(mock.sentinel.notifier, self.context, instance,
'delete.start'),
mock.call(mock.sentinel.notifier, self.context, instance,
'delete.end', system_metadata=instance.system_metadata)
]
mock_notify_usage.assert_has_calls(expected_notify_calls)
class ComputeUtilsQuotaDeltaTestCase(test.TestCase):
def setUp(self):

View File

@ -55,6 +55,7 @@ from nova.tests.unit import fake_instance
from nova.tests.unit import fake_notifier
from nova.tests.unit import fake_request_spec
from nova.tests.unit import fake_server_actions
from nova.tests.unit import utils as test_utils
from nova.tests import uuidsentinel as uuids
from nova import utils
@ -1554,6 +1555,7 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
self.assertEqual('error', instance.vm_state)
self.assertIsNone(None, instance.task_state)
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance')
@mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations')
@mock.patch('nova.objects.BuildRequest.destroy')
@ -1561,7 +1563,9 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
def test_schedule_and_build_delete_during_scheduling(self, bury,
br_destroy,
select_destinations,
build_and_run):
build_and_run,
notify):
br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo')
self.start_service('compute', host='fake-host')
select_destinations.return_value = [{'host': 'fake-host',
@ -1572,6 +1576,69 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
self.assertFalse(bury.called)
self.assertTrue(br_destroy.called)
test_utils.assert_instance_delete_notification_by_uuid(
notify, self.params['build_requests'][0].instance_uuid,
self.conductor.notifier, self.ctxt)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance')
@mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations')
@mock.patch('nova.objects.BuildRequest.destroy')
@mock.patch('nova.conductor.manager.ComputeTaskManager._bury_in_cell0')
def test_schedule_and_build_delete_during_scheduling_host_changed(
self, bury, br_destroy, select_destinations, build_and_run,
notify, instance_destroy):
br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo')
instance_destroy.side_effect = [
exc.ObjectActionError(action='destroy',
reason='host changed'),
None,
]
self.start_service('compute', host='fake-host')
select_destinations.return_value = [{'host': 'fake-host',
'nodename': 'nodesarestupid',
'limits': None}]
self.conductor.schedule_and_build_instances(**self.params)
self.assertFalse(build_and_run.called)
self.assertFalse(bury.called)
self.assertTrue(br_destroy.called)
self.assertEqual(2, instance_destroy.call_count)
test_utils.assert_instance_delete_notification_by_uuid(
notify, self.params['build_requests'][0].instance_uuid,
self.conductor.notifier, self.ctxt)
@mock.patch('nova.objects.Instance.destroy')
@mock.patch('nova.compute.utils.notify_about_instance_usage')
@mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance')
@mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations')
@mock.patch('nova.objects.BuildRequest.destroy')
@mock.patch('nova.conductor.manager.ComputeTaskManager._bury_in_cell0')
def test_schedule_and_build_delete_during_scheduling_instance_not_found(
self, bury, br_destroy, select_destinations, build_and_run,
notify, instance_destroy):
br_destroy.side_effect = exc.BuildRequestNotFound(uuid='foo')
instance_destroy.side_effect = [
exc.InstanceNotFound(instance_id='fake'),
None,
]
self.start_service('compute', host='fake-host')
select_destinations.return_value = [{'host': 'fake-host',
'nodename': 'nodesarestupid',
'limits': None}]
self.conductor.schedule_and_build_instances(**self.params)
self.assertFalse(build_and_run.called)
self.assertFalse(bury.called)
self.assertTrue(br_destroy.called)
self.assertEqual(1, instance_destroy.call_count)
test_utils.assert_instance_delete_notification_by_uuid(
notify, self.params['build_requests'][0].instance_uuid,
self.conductor.notifier, self.ctxt)
@mock.patch('nova.compute.rpcapi.ComputeAPI.build_and_run_instance')
@mock.patch('nova.scheduler.rpcapi.SchedulerAPI.select_destinations')
@mock.patch('nova.objects.BuildRequest.get_by_instance_uuid')

View File

@ -17,6 +17,7 @@ import platform
import socket
import sys
import mock
from six.moves import range
from nova.compute import flavors
@ -291,3 +292,31 @@ def obj_called_with(the_mock, *args, **kwargs):
def obj_called_once_with(the_mock, *args, **kwargs):
return _obj_called_with(the_mock, *args, **kwargs) == 1
class CustomMockCallMatcher(object):
def __init__(self, comparator):
self.comparator = comparator
def __eq__(self, other):
return self.comparator(other)
def assert_instance_delete_notification_by_uuid(
mock_notify, expected_instance_uuid, expected_notifier,
expected_context):
match_by_instance_uuid = CustomMockCallMatcher(
lambda instance:
instance.uuid == expected_instance_uuid)
mock_notify.assert_has_calls([
mock.call(expected_notifier,
expected_context,
match_by_instance_uuid,
'delete.start'),
mock.call(expected_notifier,
expected_context,
match_by_instance_uuid,
'delete.end',
system_metadata={})])