openstack/nova
Code Issues Proposed changes

Add instance.share_detach_error notification

Browse Source 
This patch add a notification when a share can not be unmounted on the
compute host.

Manila is the OpenStack Shared Filesystems service.
These series of patches implement changes required in Nova to allow the shares
provided by Manila to be associated with and attached to instances using
virtiofs.

Implements: blueprint libvirt-virtiofs-attach-manila-shares
Change-Id: Ic1aeb2308af5bf645fa67fe1f47e5a076efd6071
This commit is contained in:
René Ribaud 2022-10-03 13:55:06 +02:00
parent 9f8b05fd90
commit 7dfb15235e
6 changed files with 343 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
doc/notification_samples/instance-share_detach-error.json Normal file
View File

@ -0,0 +1,22 @@
{
"event_type": "instance.share_detach.error",
"payload": {
"$ref": "common_payloads/InstanceActionSharePayload.json#",
"nova_object.data": {
"fault": {
"nova_object.data": {
"exception": "ShareAccessRemovalError",
"exception_message": "Share access could not be removed from share id 8db0037b-e98f-4bde-ae71-f96a077c19a4.\nReason: Connection timed out.",
"function_name": "_execute_mock_call",
"module_name": "unittest.mock",
"traceback": "Traceback (most recent call last):\n File \"nova/compute/manager.py\", line ..."
},
"nova_object.name": "ExceptionPayload",
"nova_object.namespace": "nova",
"nova_object.version": "1.1"
}
}
},
"priority": "ERROR",
"publisher_id": "nova-api:compute"
}

63
nova/compute/manager.py
View File

@ -4783,6 +4783,7 @@ class ComputeManager(manager.Manager):
@utils.synchronized(share_mapping.share_id)
def _deny_share(context, instance, share_mapping):
def check_share_usage(context, instance_uuid):
share_mappings_used_by_share = (
objects.share_mapping.ShareMappingList.get_by_share_id(
@ -4819,6 +4820,15 @@ class ComputeManager(manager.Manager):
)
try:
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
)
still_used = check_share_usage(context, instance.uuid)
share_mapping.set_access_according_to_protocol()
@ -4837,6 +4847,15 @@ class ComputeManager(manager.Manager):
share_mapping.delete()
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id,
)
except (
exception.ShareAccessRemovalError,
exception.ShareProtocolNotSupported,
@ -4844,39 +4863,47 @@ class ComputeManager(manager.Manager):
self._set_share_mapping_status(
share_mapping, fields.ShareMappingStatus.ERROR
)
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=e
)
LOG.error(e.format_message())
raise
except keystone_exception.http.Unauthorized as e:
self._set_share_mapping_status(
share_mapping, fields.ShareMappingStatus.ERROR
)
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=e
)
LOG.error(e)
raise
except (exception.ShareNotFound, exception.ShareAccessNotFound):
# Ignore the error if for any reason there is nothing to
# remove from manila, so we can still detach the share.
share_mapping.delete()
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id
)
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id,
)
_deny_share(context, instance, share_mapping)
compute_utils.notify_about_share_attach_detach(
context,
instance,
instance.host,
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id
)
@wrap_exception()
def _mount_all_shares(self, context, instance, share_info):
for share_mapping in share_info:

1
nova/notifications/objects/instance.py
View File

@ -686,6 +686,7 @@ class InstanceActionVolumeNotification(base.NotificationBase):
@base.notification_sample('instance-share_attach-error.json')
@base.notification_sample('instance-share_attach-end.json')
@base.notification_sample('instance-share_detach-start.json')
@base.notification_sample('instance-share_detach-error.json')
@base.notification_sample('instance-share_detach-end.json')
@nova_base.NovaObjectRegistry.register_notification
class InstanceActionShareNotification(base.NotificationBase):

5
nova/tests/functional/integrated_helpers.py
View File

@ -576,6 +576,11 @@ class InstanceHelperMixin:
self.notifier.wait_for_versioned_notifications(
'instance.share_detach.end')
def _detach_share_with_error(self, server, share_id):
self.api.delete_server_share(server['id'], share_id)
self.notifier.wait_for_versioned_notifications(
'instance.share_detach.error')
def _rebuild_server(self, server, image_uuid, expected_state='ACTIVE'):
"""Rebuild a server."""
self.api.post_server_action(

96
nova/tests/functional/notification_sample_tests/test_instance.py
View File

@ -393,7 +393,7 @@ class TestInstanceNotificationSample(
self._test_lock_unlock_instance,
self._test_lock_unlock_instance_with_reason,
self._test_share_attach_detach,
self._test_share_attach_error,
self._test_share_attach_detach_error,
]
for action in actions:
@ -1844,7 +1844,7 @@ class TestInstanceNotificationSample(
},
actual=self.notifier.versioned_notifications[1])
# Restart server
# Start server
self.notifier.reset()
self.api.post_server_action(server['id'], {'os-start': {}})
self._wait_for_state_change(server, expected_status='ACTIVE')
@ -1869,7 +1869,7 @@ class TestInstanceNotificationSample(
},
actual=self.notifier.versioned_notifications[1])
def _test_share_attach_error(self, server):
def _test_share_attach_detach_error(self, server):
expected_shares = [
{
@ -1906,6 +1906,7 @@ class TestInstanceNotificationSample(
):
# Simulate an error attaching the share.
original_side_effect = self.manila_fixture.mock_allow.side_effect
self.manila_fixture.mock_allow.side_effect = (
exception.ShareAccessGrantError(
share_id="8db0037b-e98f-4bde-ae71-f96a077c19a4",
@ -2004,6 +2005,95 @@ class TestInstanceNotificationSample(
self._wait_for_notification('instance.reboot.start')
self._wait_for_notification('instance.reboot.end')
# Attach a share again in order to simulate a detach error
self.manila_fixture.mock_allow.side_effect = original_side_effect
self.api.post_server_action(server['id'], {'os-stop': {}})
self._wait_for_state_change(server, expected_status='SHUTOFF')
self.notifier.reset()
self._attach_share(server, "e8debdc0-447a-4376-a10a-4cd9122d7986")
share_info = self._get_share(
server, "e8debdc0-447a-4376-a10a-4cd9122d7986", admin=True
)
expected_shares[0]["nova_object.data"][
"share_mapping_uuid"
] = share_info["uuid"]
self.assertEqual(2, len(self.notifier.versioned_notifications),
self.notifier.versioned_notifications)
expected_shares[0]['nova_object.data']['status'] = 'attaching'
self._verify_notification(
'instance-share_attach-start',
replacements={
'reservation_id': server['reservation_id'],
'uuid': server['id'],
'state': 'stopped',
'power_state': 'shutdown',
'shares': expected_shares
},
actual=self.notifier.versioned_notifications[0])
expected_shares[0]['nova_object.data']['status'] = 'inactive'
self._verify_notification(
'instance-share_attach-end',
replacements={
'reservation_id': server['reservation_id'],
'uuid': server['id'],
'state': 'stopped',
'power_state': 'shutdown',
'shares': expected_shares
},
actual=self.notifier.versioned_notifications[1])
# Simulate an error detaching the share.
self.manila_fixture.mock_deny.side_effect = (
exception.ShareAccessRemovalError(
share_id="8db0037b-e98f-4bde-ae71-f96a077c19a4",
reason="Connection timed out"
)
)
self.notifier.reset()
self._detach_share_with_error(
server, "e8debdc0-447a-4376-a10a-4cd9122d7986")
# 0: instance-share_detach-start
# 1: instance-share_detach-error
# 2: compute.exception
self.assertEqual(3, len(self.notifier.versioned_notifications),
self.notifier.versioned_notifications)
expected_shares[0]["nova_object.data"]["status"] = "detaching"
self._verify_notification(
'instance-share_detach-start',
replacements={
'reservation_id': server['reservation_id'],
'uuid': server['id'],
'state': 'stopped',
'power_state': 'shutdown',
'shares': expected_shares,
},
actual=self.notifier.versioned_notifications[0])
expected_shares[0]["nova_object.data"]["status"] = "error"
self._verify_notification(
'instance-share_detach-error',
replacements={
'reservation_id': server['reservation_id'],
'uuid': server['id'],
'state': 'stopped',
'power_state': 'shutdown',
'shares': expected_shares,
'fault.traceback': self.ANY
},
actual=self.notifier.versioned_notifications[1])
self._wait_for_notification('compute.exception')
# Reboot server
self.notifier.reset()
post = {'reboot': {'type': 'HARD'}}
self.api.post_server_action(server['id'], post)
self._wait_for_notification('instance.reboot.start')
self._wait_for_notification('instance.reboot.end')
def _test_rescue_unrescue_server(self, server):
# Both "rescue" and "unrescue" notification asserts are made here
# rescue notification asserts

191
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -2813,7 +2813,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
@ -2821,7 +2821,77 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
return_value=None
)
@mock.patch('nova.objects.share_mapping.ShareMapping.delete')
@mock.patch('nova.share.manila.API.deny')
@mock.patch('nova.share.manila.API.get_access')
@mock.patch('nova.objects.share_mapping.ShareMappingList.get_by_share_id')
@mock.patch('nova.objects.share_mapping.ShareMapping.save')
def test_deny_share_fails_access_removal(
self, mock_db, mock_db_get_share, mock_get_access, mock_deny,
mock_db_delete, mock_notifications
):
"""Make sure we can remove a share even if we have an error with
the access or the access is not existing anymore for any reason.
"""
self.flags(shutdown_retry_interval=20, group='compute')
instance = fake_instance.fake_instance_obj(
self.context,
uuid=uuids.instance,
vm_state=vm_states.ACTIVE,
task_state=task_states.POWERING_OFF)
mock_db_get_share.return_value = (
objects.share_mapping.ShareMappingList()
)
share_mapping = self.get_fake_share_mapping()
mock_db_get_share.return_value.objects.append(share_mapping)
# Ensure CONF.my_shared_fs_storage_ip default is my_ip
self.flags(my_ip="10.0.0.2")
self.assertEqual(CONF.my_shared_fs_storage_ip, '10.0.0.2')
# Set CONF.my_shared_fs_storage_ip to ensure it is used by the code
self.flags(my_shared_fs_storage_ip="192.168.0.1")
compute_ip = CONF.my_shared_fs_storage_ip
self.assertEqual(compute_ip, '192.168.0.1')
exc = exception.ShareAccessRemovalError(
share_id=share_mapping.share_id,
reason="fake_reason"
)
mock_deny.side_effect = exc
self.assertRaises(
exception.ShareAccessRemovalError,
self.compute.deny_share,
self.context,
instance,
share_mapping,
)
mock_deny.assert_called_once_with(
mock.ANY, share_mapping.share_id, 'ip', compute_ip)
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=exc
),
])
@ -2861,7 +2931,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
@ -2869,7 +2939,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
])
@ -2918,7 +2988,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
@ -2926,7 +2996,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id
share_id=share_mapping.share_id,
),
])
@ -2964,6 +3034,24 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
mock_deny.assert_called_once_with(
mock.ANY, share_mapping.share_id, 'ip', compute_ip)
mock_db_delete.assert_called_once()
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
@ -2999,6 +3087,24 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
mock_deny.assert_called_once_with(
mock.ANY, share_mapping.share_id, 'ip', compute_ip)
mock_db_delete.assert_called_once()
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping.share_id,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
@ -3032,15 +3138,34 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
share_id=share_mapping.share_id,
reason="fake_reason"
)
self.assertRaises(
exc = self.assertRaises(
exception.ShareAccessRemovalError,
self.compute.deny_share,
self.context,
instance,
share_mapping
share_mapping,
)
mock_db_delete.assert_not_called()
self.assertEqual(share_mapping.status, 'error')
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=exc,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
@ -3073,7 +3198,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
mock_deny.side_effect = keystone_exception.http.Unauthorized(
message="Unauthorized"
)
self.assertRaises(
exc = self.assertRaises(
keystone_exception.http.Unauthorized,
self.compute.deny_share,
self.context,
@ -3082,6 +3207,25 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
)
mock_db_delete.assert_not_called()
self.assertEqual(share_mapping.status, 'error')
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=exc,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
@ -3114,7 +3258,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
mock_deny.side_effect = exception.ShareProtocolNotSupported(
share_proto=share_mapping.share_proto
)
self.assertRaises(
exc = self.assertRaises(
exception.ShareProtocolNotSupported,
self.compute.deny_share,
self.context,
@ -3123,6 +3267,25 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
)
mock_db_delete.assert_not_called()
self.assertEqual(share_mapping.status, 'error')
mock_notifications.assert_has_calls([
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping.share_id,
),
mock.call(
mock.ANY,
instance,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.ERROR,
share_id=share_mapping.share_id,
exception=exc,
),
])
@mock.patch(
'nova.compute.utils.notify_about_share_attach_detach',
@ -3162,7 +3325,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping1.share_id
share_id=share_mapping1.share_id,
),
mock.call(
mock.ANY,
@ -3170,7 +3333,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping1.share_id
share_id=share_mapping1.share_id,
),
])
@ -3214,7 +3377,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.START,
share_id=share_mapping1.share_id
share_id=share_mapping1.share_id,
),
mock.call(
mock.ANY,
@ -3222,7 +3385,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
"fake-host",
action=fields.NotificationAction.SHARE_DETACH,
phase=fields.NotificationPhase.END,
share_id=share_mapping1.share_id
share_id=share_mapping1.share_id,
),
])