From 13d45b6d375d8ba2f36483bdf4bbebac259b6d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Ribaud?= Date: Fri, 22 Jul 2022 16:32:37 +0200 Subject: [PATCH] Add instance.share_detach notification This patch add a notification when a share is detached from an instance. 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: I203b71a39878c3929b22b53220f58c12c0a4c0b9 --- .../instance-share_detach-end.json | 8 + .../instance-share_detach-start.json | 8 + nova/compute/manager.py | 18 ++ nova/notifications/objects/instance.py | 2 + .../test_instance.py | 33 ++++ nova/tests/unit/compute/test_compute_mgr.py | 157 ++++++++++++++++-- .../share-notifications-e9f096aa2a302c57.yaml | 9 + 7 files changed, 225 insertions(+), 10 deletions(-) create mode 100644 doc/notification_samples/instance-share_detach-end.json create mode 100644 doc/notification_samples/instance-share_detach-start.json create mode 100644 releasenotes/notes/share-notifications-e9f096aa2a302c57.yaml diff --git a/doc/notification_samples/instance-share_detach-end.json b/doc/notification_samples/instance-share_detach-end.json new file mode 100644 index 0000000000..c92411d323 --- /dev/null +++ b/doc/notification_samples/instance-share_detach-end.json @@ -0,0 +1,8 @@ +{ + "event_type": "instance.share_detach.end", + "payload": { + "$ref": "common_payloads/InstanceActionSharePayload.json#" + }, + "priority": "INFO", + "publisher_id": "nova-api:compute" +} diff --git a/doc/notification_samples/instance-share_detach-start.json b/doc/notification_samples/instance-share_detach-start.json new file mode 100644 index 0000000000..04576c0829 --- /dev/null +++ b/doc/notification_samples/instance-share_detach-start.json @@ -0,0 +1,8 @@ +{ + "event_type": "instance.share_detach.start", + "payload": { + "$ref": "common_payloads/InstanceActionSharePayload.json#" + }, + "priority": "INFO", + "publisher_id": "nova-api:compute" +} diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 66bf9e35c9..b1344eaba6 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4831,8 +4831,26 @@ class ComputeManager(manager.Manager): # 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 + ) + _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 36f28c2c84..1f8370b4d3 100644 --- a/nova/notifications/objects/instance.py +++ b/nova/notifications/objects/instance.py @@ -626,6 +626,8 @@ class InstanceActionVolumeNotification(base.NotificationBase): @base.notification_sample('instance-share_attach-start.json') @base.notification_sample('instance-share_attach-end.json') +@base.notification_sample('instance-share_detach-start.json') +@base.notification_sample('instance-share_detach-end.json') @nova_base.NovaObjectRegistry.register_notification class InstanceActionShareNotification(base.NotificationBase): # Version 1.0: Initial version diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index bdaab010eb..7852b4455f 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -390,6 +390,7 @@ class TestInstanceNotificationSample( self._test_lock_unlock_instance, self._test_lock_unlock_instance_with_reason, self._test_share_attach, + self._test_share_detach, ] for action in actions: @@ -1732,6 +1733,38 @@ class TestInstanceNotificationSample( self.api.post_server_action(server['id'], {'os-start': {}}) self._wait_for_state_change(server, expected_status='ACTIVE') + def _test_share_detach(self, server): + self.api.post_server_action(server['id'], {'os-stop': {}}) + self._wait_for_state_change(server, expected_status='SHUTOFF') + self.notifier.reset() + + # Detach share + self._detach_share(server, 'e8debdc0-447a-4376-a10a-4cd9122d7986') + + self.assertEqual(2, len(self.notifier.versioned_notifications), + self.notifier.versioned_notifications) + self._verify_notification( + 'instance-share_detach-start', + replacements={ + 'reservation_id': server['reservation_id'], + 'uuid': server['id'], + 'state': 'stopped', + 'power_state': 'shutdown'}, + actual=self.notifier.versioned_notifications[0]) + self._verify_notification( + 'instance-share_detach-end', + replacements={ + 'reservation_id': server['reservation_id'], + 'uuid': server['id'], + 'state': 'stopped', + 'power_state': 'shutdown', + }, + actual=self.notifier.versioned_notifications[1]) + + # Restart server + self.api.post_server_action(server['id'], {'os-start': {}}) + self._wait_for_state_change(server, expected_status='ACTIVE') + 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 8b70b71e2a..46a3d7a5d7 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -2704,12 +2704,17 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock_allow.assert_called_once_with( mock.ANY, share_mapping.share_id, 'ip', compute_ip, 'rw') + @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') def test_deny_share( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we can deny the instance share. """ @@ -2735,13 +2740,36 @@ 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', + 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') def test_deny_share_in_use( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we cannot deny a share used by an instance. """ @@ -2760,13 +2788,36 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, self.compute.deny_share(self.context, instance, share_mapping) mock_deny.assert_not_called() 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', + 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') def test_deny_share_in_error( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we can deny a share in error on the instance detaching the share. @@ -2794,13 +2845,36 @@ 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', + 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') def test_deny_share_access_not_found_in_manila( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we can deny a share even if access is not found in manila. """ @@ -2825,12 +2899,17 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock.ANY, share_mapping.share_id, 'ip', compute_ip) mock_db_delete.assert_called_once() + @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') def test_deny_share_not_found_in_manila( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we can deny a share even if the share is not found in manila. """ @@ -2855,6 +2934,10 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock.ANY, share_mapping.share_id, 'ip', compute_ip) mock_db_delete.assert_called_once() + @mock.patch( + 'nova.compute.utils.notify_about_share_attach_detach', + return_value=None + ) @mock.patch('nova.objects.share_mapping.ShareMapping.save') @mock.patch('nova.objects.share_mapping.ShareMapping.delete') @mock.patch('nova.share.manila.API.deny') @@ -2862,7 +2945,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, @mock.patch('nova.objects.share_mapping.ShareMappingList.get_by_share_id') def test_deny_share_fails_access_removal_error( self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, - mock_db_save + mock_db_save, mock_notifications ): """Ensure we have an exception if the access cannot be removed by manila. @@ -2893,6 +2976,10 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock_db_delete.assert_not_called() self.assertEqual(share_mapping.status, 'error') + @mock.patch( + 'nova.compute.utils.notify_about_share_attach_detach', + return_value=None + ) @mock.patch('nova.objects.share_mapping.ShareMapping.save') @mock.patch('nova.objects.share_mapping.ShareMapping.delete') @mock.patch('nova.share.manila.API.deny') @@ -2900,7 +2987,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, @mock.patch('nova.objects.share_mapping.ShareMappingList.get_by_share_id') def test_deny_share_fails_keystone_unauthorized( self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, - mock_db_save + mock_db_save, mock_notifications ): """Ensure we have an exception if the access cannot be removed by manila. @@ -2930,6 +3017,10 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock_db_delete.assert_not_called() self.assertEqual(share_mapping.status, 'error') + @mock.patch( + 'nova.compute.utils.notify_about_share_attach_detach', + return_value=None + ) @mock.patch('nova.objects.share_mapping.ShareMapping.save') @mock.patch('nova.objects.share_mapping.ShareMapping.delete') @mock.patch('nova.share.manila.API.deny') @@ -2937,7 +3028,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, @mock.patch('nova.objects.share_mapping.ShareMappingList.get_by_share_id') def test_deny_share_fails_protocol_not_supported( self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, - mock_db_save + mock_db_save, mock_notifications ): """Ensure we have an exception if the access cannot be removed by manila. @@ -2967,12 +3058,17 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock_db_delete.assert_not_called() self.assertEqual(share_mapping.status, 'error') + @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') def test_deny_share_in_use_by_another_instance( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we do not deny a share used by another instance. """ @@ -2993,13 +3089,36 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, self.compute.deny_share(self.context, instance, share_mapping1) mock_deny.assert_not_called() 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_mapping1.share_id + ), + mock.call( + mock.ANY, + instance, + "fake-host", + action=fields.NotificationAction.SHARE_DETACH, + phase=fields.NotificationPhase.END, + share_id=share_mapping1.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') def test_deny_share_in_error_on_another_instance( - self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete + self, mock_db_get_share, mock_get_access, mock_deny, mock_db_delete, + mock_notifications ): """Ensure we cannot deny a share in error state on another instance. If the other instance is hard rebooted, it might need the share. @@ -3022,6 +3141,24 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, self.compute.deny_share(self.context, instance, share_mapping1) mock_deny.assert_not_called() 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_mapping1.share_id + ), + mock.call( + mock.ANY, + instance, + "fake-host", + action=fields.NotificationAction.SHARE_DETACH, + phase=fields.NotificationPhase.END, + share_id=share_mapping1.share_id + ), + ]) @mock.patch('nova.objects.share_mapping.ShareMapping.save') @mock.patch('nova.virt.fake.FakeDriver.mount_share') diff --git a/releasenotes/notes/share-notifications-e9f096aa2a302c57.yaml b/releasenotes/notes/share-notifications-e9f096aa2a302c57.yaml new file mode 100644 index 0000000000..575436c24d --- /dev/null +++ b/releasenotes/notes/share-notifications-e9f096aa2a302c57.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + The following share attach and share detach versioned notifications have + been added to the nova-compute service: + * instance.share_attach.start + * instance.share_attach.end + * instance.share_detach.start + * instance.share_detach.end