From 7dfb15235e702468ce0314597b3ef7b67d8b5748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Ribaud?= Date: Mon, 3 Oct 2022 13:55:06 +0200 Subject: [PATCH] Add instance.share_detach_error notification 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 --- .../instance-share_detach-error.json | 22 ++ nova/compute/manager.py | 63 ++++-- nova/notifications/objects/instance.py | 1 + nova/tests/functional/integrated_helpers.py | 5 + .../test_instance.py | 96 ++++++++- nova/tests/unit/compute/test_compute_mgr.py | 191 ++++++++++++++++-- 6 files changed, 343 insertions(+), 35 deletions(-) create mode 100644 doc/notification_samples/instance-share_detach-error.json diff --git a/doc/notification_samples/instance-share_detach-error.json b/doc/notification_samples/instance-share_detach-error.json new file mode 100644 index 000000000000..7a1b19862eae --- /dev/null +++ b/doc/notification_samples/instance-share_detach-error.json @@ -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" +} diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 668074ae3dfb..8571ffc3bb1e 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -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: diff --git a/nova/notifications/objects/instance.py b/nova/notifications/objects/instance.py index 18c895f7973a..4202242fe021 100644 --- a/nova/notifications/objects/instance.py +++ b/nova/notifications/objects/instance.py @@ -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): diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index 2cb83ef68e90..65f84226195e 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -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( diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index b0c19740c2ba..595cf08630f3 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -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 diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index f142ea8c43f5..9ce0056b1cfb 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -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, ), ])