From 3936a3cfda406f4cea7984a307482ac67ed1a365 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 28 Apr 2017 09:38:06 -0400 Subject: [PATCH] Don't lazy-load flavor.projects during destroy() When we destroy a flavor it's happening in the API database now. Right after we delete the flavor, we send a delete notification which will attempt to lazy-load projects, but because the flavor is deleted, that will result in a FlavorNotFound from the API DB in method _get_projects_from_db. Because of our fallback code to the main cell DB we'll then call flavor_access_get_by_flavor_id which returns an empty list and that's what goes into the notification, but it's not accurate since the empty list implies there are/were no projects with access to that flavor, when in reality we don't know. This handles the delete notification case by orphaning the flavor object so that when the notification emit happens, the NotificationPayloadBase gets an OrphanedObjectError and just sets projects to None, which is a better representation in the notification payload that we don't have that data. The tests needed some work since they were not actually comparing the payload to the flavor object before, they were comparing to fields in the DB representation of the flavor, which doesn't include the projects field as it's a joined column. Change-Id: I6868efab22cf9f1eb9006589a5b62618434c3ba3 Closes-Bug: #1687012 --- doc/notification_samples/flavor-delete.json | 2 +- nova/objects/flavor.py | 7 +++- .../unit/notifications/objects/test_flavor.py | 33 +++++++++++++++++-- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/doc/notification_samples/flavor-delete.json b/doc/notification_samples/flavor-delete.json index 5b7b3e7cafcd..1cad9c1f004e 100644 --- a/doc/notification_samples/flavor-delete.json +++ b/doc/notification_samples/flavor-delete.json @@ -17,7 +17,7 @@ "vcpu_weight": 0, "flavorid": "a22d5517-147c-4147-a0d1-e698df5cd4e3", "extra_specs": null, - "projects": [] + "projects": null } }, "event_type": "flavor.delete", diff --git a/nova/objects/flavor.py b/nova/objects/flavor.py index 1d95948cf085..f8c1bb570b12 100644 --- a/nova/objects/flavor.py +++ b/nova/objects/flavor.py @@ -614,7 +614,12 @@ class Flavor(base.NovaPersistentObject, base.NovaObject, # lazy-load projects (which is a problem for instance-bound # flavors and compute-cell operations), just load them here. if 'projects' not in self: - self._load_projects() + # If the flavor is deleted we can't lazy-load projects. + # FlavorPayload will orphan the flavor which will make the + # NotificationPayloadBase set projects=None in the notification + # payload. + if action != fields.NotificationAction.DELETE: + self._load_projects() notification_type = flavor_notification.FlavorNotification payload_type = flavor_notification.FlavorPayload diff --git a/nova/tests/unit/notifications/objects/test_flavor.py b/nova/tests/unit/notifications/objects/test_flavor.py index 7fbaaa4106f0..95ee1b4a8063 100644 --- a/nova/tests/unit/notifications/objects/test_flavor.py +++ b/nova/tests/unit/notifications/objects/test_flavor.py @@ -21,6 +21,8 @@ from nova.objects import fields from nova import test from nova.tests.unit.objects.test_flavor import fake_flavor +PROJECTS_SENTINEL = object() + class TestFlavorNotification(test.TestCase): def setUp(self): @@ -29,7 +31,8 @@ class TestFlavorNotification(test.TestCase): @mock.patch('nova.notifications.objects.flavor.FlavorNotification') def _verify_notification(self, flavor_obj, flavor, action, - mock_notification, project_id=None): + mock_notification, project_id=None, + expected_projects=PROJECTS_SENTINEL): notification = mock_notification if action == "CREATE": flavor_obj.create() @@ -61,8 +64,12 @@ class TestFlavorNotification(test.TestCase): schema = flavor_notification.FlavorPayload.SCHEMA for field in schema: - if field in flavor: - self.assertEqual(flavor[field], getattr(payload, field)) + if field == 'projects' and expected_projects != PROJECTS_SENTINEL: + self.assertEqual(expected_projects, getattr(payload, field)) + elif field in flavor_obj: + self.assertEqual(flavor_obj[field], getattr(payload, field)) + else: + self.fail('Missing check for field %s in flavor_obj.' % field) @mock.patch('nova.objects.Flavor._flavor_create') def test_flavor_create_with_notification(self, mock_create): @@ -117,6 +124,26 @@ class TestFlavorNotification(test.TestCase): mock_destroy.return_value = flavor flavor_obj = objects.Flavor(context=self.ctxt, **flavor) flavor_obj.obj_reset_changes() + self.assertNotIn('projects', flavor_obj) + # We specifically expect there to not be any projects as we don't want + # to try and lazy-load them from the main database and end up with []. + self._verify_notification(flavor_obj, flavor, "DELETE", + expected_projects=None) + + @mock.patch('nova.objects.Flavor._flavor_destroy') + def test_flavor_destroy_with_notification_and_projects(self, mock_destroy): + """Tests the flavor-delete notification with flavor.projects loaded.""" + flavor = copy.deepcopy(fake_flavor) + flavorid = '1' + flavor['flavorid'] = flavorid + flavor['id'] = flavorid + mock_destroy.return_value = flavor + flavor_obj = objects.Flavor( + context=self.ctxt, projects=['foo'], **flavor) + flavor_obj.obj_reset_changes() + self.assertIn('projects', flavor_obj) + self.assertEqual(['foo'], flavor_obj.projects) + # Since projects is loaded we shouldn't try to lazy-load it. self._verify_notification(flavor_obj, flavor, "DELETE") def test_obj_make_compatible(self):