From 1590f18c633d076fc0b2a49ef24e3227c07d76b3 Mon Sep 17 00:00:00 2001 From: Kevin_Zheng Date: Wed, 26 Apr 2017 10:56:00 +0800 Subject: [PATCH] Send out notifications when instance tags changed This patch adds notifications when instance tags changed, as tag is commonly used for searching and filtering, and projects like searchlight depends on the notifications. To avoid unnecessarily building a notification payload when notifications are disabled, this change also adds the if_notifications_enabled decorator to send_instance_update_notification. Change-Id: I03c8e8225e51fd80580772752c0b292987e34218 Implements: bp send-tag-notification --- .../instance-update-tags-action.json | 90 +++++++++++++++++++ nova/api/openstack/compute/server_tags.py | 36 ++++++-- nova/notifications/base.py | 7 +- .../test_instance.py | 41 +++++---- .../api/openstack/compute/test_server_tags.py | 22 +++-- nova/tests/unit/notifications/test_base.py | 18 ++++ nova/tests/unit/test_notifications.py | 8 +- ...instance-tag-changes-67c08000b6e0cd2a.yaml | 4 + 8 files changed, 191 insertions(+), 35 deletions(-) create mode 100644 doc/notification_samples/instance-update-tags-action.json create mode 100644 releasenotes/notes/send-notification-when-instance-tag-changes-67c08000b6e0cd2a.yaml diff --git a/doc/notification_samples/instance-update-tags-action.json b/doc/notification_samples/instance-update-tags-action.json new file mode 100644 index 000000000000..4e704f86320b --- /dev/null +++ b/doc/notification_samples/instance-update-tags-action.json @@ -0,0 +1,90 @@ +{ + "event_type": "instance.update", + "payload": { + "nova_object.data": { + "architecture": "x86_64", + "audit_period": { + "nova_object.data": { + "audit_period_beginning": "2012-10-01T00:00:00Z", + "audit_period_ending": "2012-10-29T13:42:11Z"}, + "nova_object.name": "AuditPeriodPayload", + "nova_object.namespace": "nova", + "nova_object.version": "1.0"}, + "auto_disk_config": "MANUAL", + "availability_zone": "nova", + "bandwidth": [], + "created_at": "2012-10-29T13:42:11Z", + "deleted_at": null, + "display_description": "some-server", + "display_name": "some-server", + "flavor": { + "nova_object.data": { + "disabled": false, + "ephemeral_gb": 0, + "extra_specs": { + "hw:watchdog_action": "disabled" + }, + "flavorid": "a22d5517-147c-4147-a0d1-e698df5cd4e3", + "is_public": true, + "memory_mb": 512, + "name": "test_flavor", + "projects": null, + "root_gb": 1, + "rxtx_factor": 1.0, + "swap": 0, + "vcpu_weight": 0, + "vcpus": 1}, + "nova_object.name": "FlavorPayload", + "nova_object.namespace": "nova", + "nova_object.version": "1.3"}, + "host": "compute", + "host_name": "some-server", + "image_uuid": "155d900f-4e14-4e4c-a73d-069cbf4541e6", + "ip_addresses": [ + { + "nova_object.data": { + "address": "192.168.1.3", + "device_name": "tapce531f90-19", + "label": "private-network", + "mac": "fa:16:3e:4c:2c:30", + "meta": {}, + "port_uuid": "ce531f90-199f-48c0-816c-13e38010b442", + "version": 4}, + "nova_object.name": "IpPayload", + "nova_object.namespace": "nova", + "nova_object.version": "1.0" + } + ], + "kernel_id": "", + "launched_at": "2012-10-29T13:42:11Z", + "locked": false, + "metadata": {}, + "node": "fake-mini", + "old_display_name": null, + "os_type": null, + "power_state": "running", + "progress": 0, + "ramdisk_id": "", + "reservation_id": "r-53ad660s", + "state": "active", + "state_update": { + "nova_object.data": { + "new_task_state": null, + "old_state": "active", + "old_task_state": null, + "state": "active"}, + "nova_object.name": "InstanceStateUpdatePayload", + "nova_object.namespace": "nova", + "nova_object.version": "1.0"}, + "tags": ["tag1"], + "task_state": null, + "tenant_id": "6f70656e737461636b20342065766572", + "terminated_at": null, + "user_id": "fake", + "uuid": "d5ca7280-1f12-4238-a86f-378bb5d93c38"}, + "nova_object.name": "InstanceUpdatePayload", + "nova_object.namespace": "nova", + "nova_object.version": "1.3"}, + "priority": "INFO", + "publisher_id": "nova-api:fake-mini" +} diff --git a/nova/api/openstack/compute/server_tags.py b/nova/api/openstack/compute/server_tags.py index 5ee6a780fcd0..25413b6b3ebc 100644 --- a/nova/api/openstack/compute/server_tags.py +++ b/nova/api/openstack/compute/server_tags.py @@ -25,6 +25,7 @@ from nova.compute import vm_states from nova import context as nova_context from nova import exception from nova.i18n import _ +from nova.notifications import base as notifications_base from nova import objects from nova.policies import server_tags as st_policies @@ -58,6 +59,7 @@ class ServerTagsController(wsgi.Controller): method=action) common.raise_http_conflict_for_instance_invalid_state(exc, action, server_id) + return instance @wsgi.Controller.api_version("2.26") @wsgi.response(204) @@ -106,8 +108,8 @@ class ServerTagsController(wsgi.Controller): im = _get_instance_mapping(context, server_id) with nova_context.target_cell(context, im.cell_mapping) as cctxt: - self._check_instance_in_valid_state(cctxt, server_id, - 'update tag') + instance = self._check_instance_in_valid_state( + cctxt, server_id, 'update tag') try: jsonschema.validate(id, parameter_types.tag) @@ -136,9 +138,14 @@ class ServerTagsController(wsgi.Controller): with nova_context.target_cell(context, im.cell_mapping) as cctxt: tag = objects.Tag(context=cctxt, resource_id=server_id, tag=id) tag.create() + instance.tags = objects.TagList.get_by_resource_id(cctxt, + server_id) except exception.InstanceNotFound as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) + notifications_base.send_instance_update_notification( + context, instance, service="nova-api") + response = webob.Response(status_int=201) response.headers['Location'] = self._view_builder.get_location( req, server_id, id) @@ -153,15 +160,19 @@ class ServerTagsController(wsgi.Controller): im = _get_instance_mapping(context, server_id) with nova_context.target_cell(context, im.cell_mapping) as cctxt: - self._check_instance_in_valid_state(cctxt, server_id, - 'update tags') + instance = self._check_instance_in_valid_state( + cctxt, server_id, 'update tags') try: with nova_context.target_cell(context, im.cell_mapping) as cctxt: tags = objects.TagList.create(cctxt, server_id, body['tags']) + instance.tags = tags except exception.InstanceNotFound as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) + notifications_base.send_instance_update_notification( + context, instance, service="nova-api") + return {'tags': _get_tags_names(tags)} @wsgi.Controller.api_version("2.26") @@ -173,16 +184,21 @@ class ServerTagsController(wsgi.Controller): im = _get_instance_mapping(context, server_id) with nova_context.target_cell(context, im.cell_mapping) as cctxt: - self._check_instance_in_valid_state(cctxt, server_id, - 'delete tag') + instance = self._check_instance_in_valid_state( + cctxt, server_id, 'delete tag') try: with nova_context.target_cell(context, im.cell_mapping) as cctxt: objects.Tag.destroy(cctxt, server_id, id) + instance.tags = objects.TagList.get_by_resource_id(cctxt, + server_id) except (exception.InstanceTagNotFound, exception.InstanceNotFound) as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) + notifications_base.send_instance_update_notification( + context, instance, service="nova-api") + @wsgi.Controller.api_version("2.26") @wsgi.response(204) @extensions.expected_errors((404, 409)) @@ -192,11 +208,15 @@ class ServerTagsController(wsgi.Controller): im = _get_instance_mapping(context, server_id) with nova_context.target_cell(context, im.cell_mapping) as cctxt: - self._check_instance_in_valid_state(cctxt, server_id, - 'delete tags') + instance = self._check_instance_in_valid_state( + cctxt, server_id, 'delete tags') try: with nova_context.target_cell(context, im.cell_mapping) as cctxt: objects.TagList.destroy(cctxt, server_id) + instance.tags = objects.TagList() except exception.InstanceNotFound as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) + + notifications_base.send_instance_update_notification( + context, instance, service="nova-api") diff --git a/nova/notifications/base.py b/nova/notifications/base.py index 2478dc0c39c4..05808ceea286 100644 --- a/nova/notifications/base.py +++ b/nova/notifications/base.py @@ -134,7 +134,7 @@ def send_update(context, old_instance, new_instance, service="compute", old_display_name = None if new_instance["display_name"] != old_instance["display_name"]: old_display_name = old_instance["display_name"] - _send_instance_update_notification(context, new_instance, + send_instance_update_notification(context, new_instance, service=service, host=host, old_display_name=old_display_name) except exception.InstanceNotFound: @@ -176,7 +176,7 @@ def send_update_with_states(context, instance, old_vm_state, new_vm_state, if fire_update: # send either a state change or a regular notification try: - _send_instance_update_notification(context, instance, + send_instance_update_notification(context, instance, old_vm_state=old_vm_state, old_task_state=old_task_state, new_vm_state=new_vm_state, new_task_state=new_task_state, service=service, host=host) @@ -217,7 +217,8 @@ def _compute_states_payload(instance, old_vm_state=None, return states_payload -def _send_instance_update_notification(context, instance, old_vm_state=None, +@rpc.if_notifications_enabled +def send_instance_update_notification(context, instance, old_vm_state=None, old_task_state=None, new_vm_state=None, new_task_state=None, service="compute", host=None, old_display_name=None): """Send 'compute.instance.update' notification to inform observers diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index 38037d4f3ab5..f658e461db18 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -97,12 +97,13 @@ class TestInstanceNotificationSample( extra_params={'networks': [{'port': self.neutron.port_1['id']}]}) self.api.delete_server(server['id']) self._wait_until_deleted(server) - self.assertEqual(6, len(fake_notifier.VERSIONED_NOTIFICATIONS)) + self.assertEqual(7, len(fake_notifier.VERSIONED_NOTIFICATIONS)) # This list needs to be in order. expected_notifications = [ 'instance-create-start', 'instance-create-end', + 'instance-update-tags-action', 'instance-delete-start', 'instance-shutdown-start', 'instance-shutdown-end', @@ -166,15 +167,19 @@ class TestInstanceNotificationSample( server = self._boot_a_server( extra_params={'networks': [{'port': self.neutron.port_1['id']}]}) - instance_updates = self._wait_for_notifications('instance.update', 7) + instance_updates = self._wait_for_notifications('instance.update', 8) - # The first notification comes from the nova-conductor the + # The first notification comes from the nova-conductor, the + # eighth notification comes from nova-api the # rest is from the nova-compute. To keep the test simpler # assert this fact and then modify the publisher_id of the - # first notification to match the template + # first and eighth notification to match the template self.assertEqual('conductor:fake-mini', instance_updates[0]['publisher_id']) + self.assertEqual('nova-api:fake-mini', + instance_updates[7]['publisher_id']) instance_updates[0]['publisher_id'] = 'nova-compute:fake-mini' + instance_updates[7]['publisher_id'] = 'nova-compute:fake-mini' create_steps = [ # nothing -> scheduling @@ -237,6 +242,11 @@ class TestInstanceNotificationSample( 'state': 'active', 'task_state': None, 'power_state': 'running'}, + + # tag added + {'state_update.old_task_state': None, + 'state_update.old_state': 'active', + 'tags': ['tag1']}, ] replacements = self._verify_instance_update_steps( @@ -708,19 +718,19 @@ class TestInstanceNotificationSample( self.cinder.SWAP_NEW_VOL) self._wait_until_swap_volume(server, self.cinder.SWAP_NEW_VOL) - self.assertEqual(6, len(fake_notifier.VERSIONED_NOTIFICATIONS)) + self.assertEqual(7, len(fake_notifier.VERSIONED_NOTIFICATIONS)) self._verify_notification( 'instance-volume_swap-start', replacements={ 'reservation_id': server['reservation_id'], 'uuid': server['id']}, - actual=fake_notifier.VERSIONED_NOTIFICATIONS[4]) + actual=fake_notifier.VERSIONED_NOTIFICATIONS[5]) self._verify_notification( 'instance-volume_swap-end', replacements={ 'reservation_id': server['reservation_id'], 'uuid': server['id']}, - actual=fake_notifier.VERSIONED_NOTIFICATIONS[5]) + actual=fake_notifier.VERSIONED_NOTIFICATIONS[6]) def test_volume_swap_server_with_error(self): server = self._boot_a_server( @@ -740,12 +750,13 @@ class TestInstanceNotificationSample( # which generates the last notification (compute.exception). # 0. instance-create-start # 1. instance-create-end - # 2. instance-volume_attach-start - # 3. instance-volume_attach-end - # 4. instance-volume_swap-start - # 5. instance-volume_swap-error - # 6. compute.exception - self.assertTrue(len(fake_notifier.VERSIONED_NOTIFICATIONS) >= 6, + # 2. instance-update + # 3. instance-volume_attach-start + # 4. instance-volume_attach-end + # 5. instance-volume_swap-start + # 6. instance-volume_swap-error + # 7. compute.exception + self.assertTrue(len(fake_notifier.VERSIONED_NOTIFICATIONS) >= 7, 'Unexpected number of versioned notifications. ' 'Expected at least 6, got: %s' % len(fake_notifier.VERSIONED_NOTIFICATIONS)) @@ -756,13 +767,13 @@ class TestInstanceNotificationSample( 'old_volume_id': self.cinder.SWAP_ERR_OLD_VOL, 'reservation_id': server['reservation_id'], 'uuid': server['id']}, - actual=fake_notifier.VERSIONED_NOTIFICATIONS[4]) + actual=fake_notifier.VERSIONED_NOTIFICATIONS[5]) self._verify_notification( 'instance-volume_swap-error', replacements={ 'reservation_id': server['reservation_id'], 'uuid': server['id']}, - actual=fake_notifier.VERSIONED_NOTIFICATIONS[5]) + actual=fake_notifier.VERSIONED_NOTIFICATIONS[6]) def _test_revert_server(self, server): pass diff --git a/nova/tests/unit/api/openstack/compute/test_server_tags.py b/nova/tests/unit/api/openstack/compute/test_server_tags.py index 7545eb8e82e3..5c53a4f60ff7 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_tags.py +++ b/nova/tests/unit/api/openstack/compute/test_server_tags.py @@ -88,8 +88,9 @@ class ServerTagsTest(test.TestCase): self.assertEqual(TAGS, res.get('tags')) mock_db_get_inst_tags.assert_called_once_with(mock.ANY, UUID) + @mock.patch('nova.notifications.base.send_instance_update_notification') @mock.patch('nova.db.instance_tag_set') - def test_update_all(self, mock_db_set_inst_tags): + def test_update_all(self, mock_db_set_inst_tags, mock_notify): self.stub_out('nova.api.openstack.common.get_instance', return_server) fake_tags = [self._get_tag(tag) for tag in TAGS] mock_db_set_inst_tags.return_value = fake_tags @@ -99,6 +100,7 @@ class ServerTagsTest(test.TestCase): self.assertEqual(TAGS, res['tags']) mock_db_set_inst_tags.assert_called_once_with(mock.ANY, UUID, TAGS) + self.assertEqual(1, mock_notify.call_count) def test_update_all_too_many_tags(self): self.stub_out('nova.api.openstack.common.get_instance', return_server) @@ -160,9 +162,11 @@ class ServerTagsTest(test.TestCase): self.assertRaises(exc.HTTPNotFound, self.controller.show, req, UUID, TAG1) + @mock.patch('nova.notifications.base.send_instance_update_notification') @mock.patch('nova.db.instance_tag_add') @mock.patch('nova.db.instance_tag_get_by_instance_uuid') - def test_update(self, mock_db_get_inst_tags, mock_db_add_inst_tags): + def test_update(self, mock_db_get_inst_tags, mock_db_add_inst_tags, + mock_notify): self.stub_out('nova.api.openstack.common.get_instance', return_server) mock_db_get_inst_tags.return_value = [self._get_tag(TAG1)] mock_db_add_inst_tags.return_value = self._get_tag(TAG2) @@ -176,7 +180,8 @@ class ServerTagsTest(test.TestCase): self.assertEqual(0, len(res.body)) self.assertEqual(location, res.headers['Location']) mock_db_add_inst_tags.assert_called_once_with(mock.ANY, UUID, TAG2) - mock_db_get_inst_tags.assert_called_once_with(mock.ANY, UUID) + self.assertEqual(2, mock_db_get_inst_tags.call_count) + self.assertEqual(1, mock_notify.call_count) @mock.patch('nova.db.instance_tag_get_by_instance_uuid') def test_update_existing_tag(self, mock_db_get_inst_tags): @@ -232,13 +237,18 @@ class ServerTagsTest(test.TestCase): self.assertRaises(exc.HTTPConflict, self.controller.update, req, UUID, TAG1, body=None) + @mock.patch('nova.db.instance_tag_get_by_instance_uuid') + @mock.patch('nova.notifications.base.send_instance_update_notification') @mock.patch('nova.db.instance_tag_delete') - def test_delete(self, mock_db_delete_inst_tags): + def test_delete(self, mock_db_delete_inst_tags, mock_notify, + mock_db_get_inst_tags): self.stub_out('nova.api.openstack.common.get_instance', return_server) req = self._get_request( '/v2/fake/servers/%s/tags/%s' % (UUID, TAG2), 'DELETE') self.controller.delete(req, UUID, TAG2) mock_db_delete_inst_tags.assert_called_once_with(mock.ANY, UUID, TAG2) + mock_db_get_inst_tags.assert_called_once_with(mock.ANY, UUID) + self.assertEqual(1, mock_notify.call_count) @mock.patch('nova.db.instance_tag_delete') def test_delete_non_existing_tag(self, mock_db_delete_inst_tags): @@ -263,12 +273,14 @@ class ServerTagsTest(test.TestCase): self.assertRaises(exc.HTTPConflict, self.controller.delete, req, UUID, TAG1) + @mock.patch('nova.notifications.base.send_instance_update_notification') @mock.patch('nova.db.instance_tag_delete_all') - def test_delete_all(self, mock_db_delete_inst_tags): + def test_delete_all(self, mock_db_delete_inst_tags, mock_notify): self.stub_out('nova.api.openstack.common.get_instance', return_server) req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'DELETE') self.controller.delete_all(req, UUID) mock_db_delete_inst_tags.assert_called_once_with(mock.ANY, UUID) + self.assertEqual(1, mock_notify.call_count) def test_delete_all_invalid_instance_state(self): self.stub_out('nova.api.openstack.common.get_instance', diff --git a/nova/tests/unit/notifications/test_base.py b/nova/tests/unit/notifications/test_base.py index b5b622ad0c2e..2136c489261a 100644 --- a/nova/tests/unit/notifications/test_base.py +++ b/nova/tests/unit/notifications/test_base.py @@ -14,6 +14,8 @@ # under the License. import datetime +import mock + from nova.notifications import base from nova import test from nova import utils @@ -44,3 +46,19 @@ class TestNullSafeUtils(test.NoDBTestCase): self.assertEqual('', base.null_safe_int(number)) number = 10 self.assertEqual(number, base.null_safe_int(number)) + + +class TestSendInstanceUpdateNotification(test.NoDBTestCase): + + @mock.patch.object(base, 'info_from_instance', + new_callable=mock.NonCallableMock) # asserts not called + # TODO(mriedem): Rather than mock is_enabled, it would be better to + # configure oslo_messaging_notifications.driver=['noop'] + @mock.patch('nova.rpc.NOTIFIER.is_enabled', return_value=False) + def test_send_instance_update_notification_disabled(self, mock_enabled, + mock_info): + """Tests the case that notifications are disabled which makes + send_instance_update_notification a noop. + """ + base.send_instance_update_notification(mock.sentinel.ctxt, + mock.sentinel.instance) diff --git a/nova/tests/unit/test_notifications.py b/nova/tests/unit/test_notifications.py index 7b0b6d78c5a7..d94b54b36426 100644 --- a/nova/tests/unit/test_notifications.py +++ b/nova/tests/unit/test_notifications.py @@ -551,7 +551,7 @@ class NotificationsTestCase(test.TestCase): def sending_no_state_change(context, instance, **kwargs): called[0] = True self.stub_out('nova.notifications.base.' - '_send_instance_update_notification', + 'send_instance_update_notification', sending_no_state_change) notifications.send_update(self.context, self.instance, self.instance) self.assertTrue(called[0]) @@ -560,7 +560,7 @@ class NotificationsTestCase(test.TestCase): def fail_sending(context, instance, **kwargs): raise Exception('failed to notify') self.stub_out('nova.notifications.base.' - '_send_instance_update_notification', + 'send_instance_update_notification', fail_sending) notifications.send_update(self.context, self.instance, self.instance) @@ -572,7 +572,7 @@ class NotificationsTestCase(test.TestCase): # not logged as an error. notfound = exception.InstanceNotFound(instance_id=self.instance.uuid) with mock.patch.object(notifications, - '_send_instance_update_notification', + 'send_instance_update_notification', side_effect=notfound): notifications.send_update( self.context, self.instance, self.instance) @@ -586,7 +586,7 @@ class NotificationsTestCase(test.TestCase): # not logged as an error. notfound = exception.InstanceNotFound(instance_id=self.instance.uuid) with mock.patch.object(notifications, - '_send_instance_update_notification', + 'send_instance_update_notification', side_effect=notfound): notifications.send_update_with_states( self.context, self.instance, diff --git a/releasenotes/notes/send-notification-when-instance-tag-changes-67c08000b6e0cd2a.yaml b/releasenotes/notes/send-notification-when-instance-tag-changes-67c08000b6e0cd2a.yaml new file mode 100644 index 000000000000..2873a2db8067 --- /dev/null +++ b/releasenotes/notes/send-notification-when-instance-tag-changes-67c08000b6e0cd2a.yaml @@ -0,0 +1,4 @@ +--- +features: + - Versioned instance.update notification will be sent + when server's tags field is updated.