Merge "objects: Stop fetching from security_groups table"

Author: Zuul

nova/objects/instance.py

@@ -429,7 +429,7 @@ def _from_db_object(context, instance, db_inst, expected_attrs=None):
         if 'security_groups' in expected_attrs:
             sec_groups = base.obj_make_list(
                     context, objects.SecurityGroupList(context),
-                    objects.SecurityGroup, db_inst.get('security_groups', []))
+                    objects.SecurityGroup, [])
             instance['security_groups'] = sec_groups
 
         if 'tags' in expected_attrs:
@@ -525,7 +525,7 @@ def _db_instance_get_by_uuid(context, uuid, columns_to_join,
     @base.remotable_classmethod
     def get_by_uuid(cls, context, uuid, expected_attrs=None, use_slave=False):
         if expected_attrs is None:
-            expected_attrs = ['info_cache', 'security_groups']
+            expected_attrs = ['info_cache']
         columns_to_join = _expected_cols(expected_attrs)
         db_inst = cls._db_instance_get_by_uuid(context, uuid, columns_to_join,
                                                use_slave=use_slave)
@@ -535,7 +535,7 @@ def get_by_uuid(cls, context, uuid, expected_attrs=None, use_slave=False):
     @base.remotable_classmethod
     def get_by_id(cls, context, inst_id, expected_attrs=None):
         if expected_attrs is None:
-            expected_attrs = ['info_cache', 'security_groups']
+            expected_attrs = ['info_cache']
         columns_to_join = _expected_cols(expected_attrs)
         db_inst = db.instance_get(context, inst_id,
                                   columns_to_join=columns_to_join)
@@ -576,10 +576,6 @@ def create(self):
         expected_attrs = [attr for attr in INSTANCE_DEFAULT_FIELDS
                           if attr in updates]
 
-        # TODO(stephenfin): Remove this as it's related to nova-network
-        if 'security_groups' in updates:
-            updates['security_groups'] = [x.name for x in
-                                          updates['security_groups']]
         if 'info_cache' in updates:
             updates['info_cache'] = {
                 'network_info': updates['info_cache'].network_info.json()
@@ -699,11 +695,9 @@ def _save_info_cache(self, context):
 
     # TODO(stephenfin): Remove this as it's related to nova-network
     def _save_security_groups(self, context):
-        security_groups = self.security_groups or []
-        for secgroup in security_groups:
-            with secgroup.obj_alternate_context(context):
-                secgroup.save()
-        self.security_groups.obj_reset_changes()
+        # NOTE(stephenfin): We no longer bother saving these since they
+        # shouldn't be created in the first place
+        pass
 
     def _save_fault(self, context):
         # NOTE(danms): I don't think we need to worry about this, do we?
@@ -1007,11 +1001,6 @@ def _load_vcpu_model(self, db_vcpu_model=_NO_DATA_SENTINEL):
     def _load_ec2_ids(self):
         self.ec2_ids = objects.EC2Ids.get_by_instance(self._context, self)
 
-    # TODO(stephenfin): Remove this as it's related to nova-network
-    def _load_security_groups(self):
-        self.security_groups = objects.SecurityGroupList.get_by_instance(
-            self._context, self)
-
     def _load_pci_devices(self):
         self.pci_devices = objects.PciDeviceList.get_by_instance_uuid(
             self._context, self.uuid)
@@ -1198,7 +1187,7 @@ def _obj_load_attr(self, attrname):
         elif attrname == 'resources':
             return self._load_resources()
         elif attrname == 'security_groups':
-            self._load_security_groups()
+            self.security_groups = objects.SecurityGroupList()
         elif attrname == 'pci_devices':
             self._load_pci_devices()
         elif 'flavor' in attrname:
@@ -1560,17 +1549,12 @@ def get_active_by_window_joined(cls, context, begin, end=None,
     # TODO(stephenfin): Remove this as it's related to nova-network
     @base.remotable_classmethod
     def get_by_security_group_id(cls, context, security_group_id):
-        db_secgroup = db.security_group_get(
-            context, security_group_id,
-            columns_to_join=['instances.info_cache',
-                             'instances.system_metadata'])
-        return _make_instance_list(context, cls(), db_secgroup['instances'],
-                                   ['info_cache', 'system_metadata'])
+        raise NotImplementedError()
 
     # TODO(stephenfin): Remove this as it's related to nova-network
     @classmethod
     def get_by_security_group(cls, context, security_group):
-        return cls.get_by_security_group_id(context, security_group.id)
+        raise NotImplementedError()
 
     # TODO(stephenfin): Remove this as it's related to nova-network
     @base.remotable_classmethod

nova/tests/unit/objects/test_instance.py

@@ -36,7 +36,6 @@
 from nova.objects import instance
 from nova.objects import instance_info_cache
 from nova.objects import pci_device
-from nova.objects import security_group
 from nova import test
 from nova.tests.unit import fake_instance
 from nova.tests.unit.objects import test_instance_device_metadata
@@ -46,7 +45,6 @@
 from nova.tests.unit.objects import test_instance_pci_requests
 from nova.tests.unit.objects import test_migration_context as test_mig_ctxt
 from nova.tests.unit.objects import test_objects
-from nova.tests.unit.objects import test_security_group
 from nova.tests.unit.objects import test_vcpu_model
 from nova import utils
 
@@ -66,7 +64,6 @@ def fake_instance(self):
         db_inst['launched_at'] = datetime.datetime(1955, 11, 12,
                                                    22, 4, 0)
         db_inst['deleted'] = False
-        db_inst['security_groups'] = []
         db_inst['pci_devices'] = []
         db_inst['user_id'] = self.context.user_id
         db_inst['project_id'] = self.context.project_id
@@ -380,7 +377,7 @@ def test_get_by_id(self, mock_get):
         inst = objects.Instance.get_by_id(self.context, 'instid')
         self.assertEqual(self.fake_instance['uuid'], inst.uuid)
         mock_get.assert_called_once_with(self.context, 'instid',
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(db, 'instance_get_by_uuid')
     def test_load(self, mock_get):
@@ -399,8 +396,7 @@ def test_load(self, mock_get):
         self.assertEqual({'foo': 'bar'}, meta2)
 
         call_list = [mock.call(self.context, fake_uuid,
-                               columns_to_join=['info_cache',
-                                                'security_groups']),
+                               columns_to_join=['info_cache']),
                      mock.call(self.context, fake_uuid,
                                columns_to_join=['metadata']),
                      ]
@@ -475,7 +471,7 @@ def test_get_remote(self, mock_get):
                          str(inst.access_ip_v6))
 
         mock_get.assert_called_once_with(self.context, uuids.instance,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(instance_info_cache.InstanceInfoCache, 'refresh')
     @mock.patch.object(db, 'instance_get_by_uuid')
@@ -492,11 +488,9 @@ def test_refresh(self, mock_get, mock_refresh):
         self.assertEqual(set([]), inst.obj_what_changed())
 
         get_call_list = [mock.call(self.context, fake_uuid,
-                                   columns_to_join=['info_cache',
-                                                    'security_groups']),
+                                   columns_to_join=['info_cache']),
                          mock.call(self.context, fake_uuid,
-                                   columns_to_join=['info_cache',
-                                                    'security_groups']),
+                                   columns_to_join=['info_cache']),
                          ]
         mock_get.assert_has_calls(get_call_list, any_order=False)
         mock_refresh.assert_called_once_with()
@@ -566,12 +560,10 @@ def _save_test_helper(self, save_kwargs,
         # NOTE(danms): Ignore flavor migrations for the moment
         self.assertEqual(set([]), inst.obj_what_changed() - set(['flavor']))
         mock_db_instance_get_by_uuid.assert_called_once_with(
-            self.context, fake_uuid, columns_to_join=['info_cache',
-                                                      'security_groups'])
+            self.context, fake_uuid, columns_to_join=['info_cache'])
         mock_db_instance_update_and_get_original.assert_called_once_with(
             self.context, fake_uuid, expected_updates,
-            columns_to_join=['info_cache', 'security_groups',
-                             'system_metadata']
+            columns_to_join=['info_cache', 'system_metadata']
         )
         mock_notifications_send_update.assert_called_with(self.context,
                                                           mock.ANY,
@@ -609,10 +601,10 @@ def test_save_rename_sends_notification(self, mock_send, mock_get,
         self.assertEqual(set([]), inst.obj_what_changed() - set(['flavor']))
 
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
         mock_update_and_get.assert_called_once_with(self.context, fake_uuid,
-            expected_updates, columns_to_join=['info_cache', 'security_groups',
-            'system_metadata'])
+            expected_updates, columns_to_join=['info_cache', 'system_metadata']
+        )
         mock_send.assert_called_once_with(self.context, mock.ANY, mock.ANY)
 
     @mock.patch('nova.db.main.api.instance_extra_update_by_uuid')
@@ -794,7 +786,7 @@ def test_get_deleted(self, mock_get):
         # NOTE(danms): Make sure it's actually a bool
         self.assertTrue(inst.deleted)
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(db, 'instance_get_by_uuid')
     def test_get_not_cleaned(self, mock_get):
@@ -806,7 +798,7 @@ def test_get_not_cleaned(self, mock_get):
         # NOTE(mikal): Make sure it's actually a bool
         self.assertFalse(inst.cleaned)
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(db, 'instance_get_by_uuid')
     def test_get_cleaned(self, mock_get):
@@ -818,7 +810,7 @@ def test_get_cleaned(self, mock_get):
         # NOTE(mikal): Make sure it's actually a bool
         self.assertTrue(inst.cleaned)
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(db, 'instance_update_and_get_original')
     @mock.patch.object(db, 'instance_info_cache_update')
@@ -846,7 +838,7 @@ def test_with_info_cache(self, mock_get, mock_upd_cache, mock_upd_and_get):
         inst.save()
 
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
         mock_upd_cache.assert_called_once_with(self.context, fake_uuid,
             {'network_info': nwinfo2_json})
         self.assertFalse(mock_upd_and_get.called)
@@ -900,22 +892,15 @@ def test_with_security_groups(self, mock_get, mock_upd_and_get,
         mock_upd_secgrp.return_value = fake_inst['security_groups'][0]
 
         inst = objects.Instance.get_by_uuid(self.context, fake_uuid)
-        self.assertEqual(2, len(inst.security_groups))
-        for index, group in enumerate(fake_inst['security_groups']):
-            for key in group:
-                self.assertEqual(group[key],
-                                 getattr(inst.security_groups[index], key))
-                self.assertIsInstance(inst.security_groups[index],
-                                      security_group.SecurityGroup)
+        # we no longer actually save these, so this should return 0
+        self.assertEqual(0, len(inst.security_groups))
         self.assertEqual(set(), inst.security_groups.obj_what_changed())
-        inst.security_groups[0].description = 'changed'
         inst.save()
         self.assertEqual(set(), inst.security_groups.obj_what_changed())
 
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
-        mock_upd_secgrp.assert_called_once_with(self.context, 1,
-            {'description': 'changed'})
+            columns_to_join=['info_cache'])
+        mock_upd_secgrp.assert_not_called()
         self.assertFalse(mock_upd_and_get.called)
 
     @mock.patch.object(db, 'instance_get_by_uuid')
@@ -927,7 +912,7 @@ def test_with_empty_security_groups(self, mock_get):
         inst = objects.Instance.get_by_uuid(self.context, fake_uuid)
         self.assertEqual(0, len(inst.security_groups))
         mock_get.assert_called_once_with(self.context, fake_uuid,
-            columns_to_join=['info_cache', 'security_groups'])
+            columns_to_join=['info_cache'])
 
     @mock.patch.object(db, 'instance_get_by_uuid')
     def test_with_empty_pci_devices(self, mock_get):
@@ -1201,23 +1186,16 @@ def test_create_with_special_things(self, mock_create):
         fake_inst = fake_instance.fake_db_instance()
         mock_create.return_value = fake_inst
 
-        secgroups = security_group.SecurityGroupList()
-        secgroups.objects = []
-        for name in ('foo', 'bar'):
-            secgroup = security_group.SecurityGroup()
-            secgroup.name = name
-            secgroups.objects.append(secgroup)
         info_cache = instance_info_cache.InstanceInfoCache()
         info_cache.network_info = network_model.NetworkInfo()
         inst = objects.Instance(context=self.context,
-                                host='foo-host', security_groups=secgroups,
+                                host='foo-host',
                                 info_cache=info_cache)
         inst.create()
 
         mock_create.assert_called_once_with(self.context,
                            {'host': 'foo-host',
                             'deleted': 0,
-                            'security_groups': ['foo', 'bar'],
                             'info_cache': {'network_info': '[]'},
                             'extra': {
                                 'vcpu_model': None,
@@ -1340,6 +1318,7 @@ def test_from_db_object_info_cache_not_set(self):
         self.assertIsNone(inst.info_cache)
 
     def test_from_db_object_security_groups_net_set(self):
+        # this is the default now since we no longer set these things
         inst = instance.Instance(context=self.context,
                                  info_cache=None)
         db_inst = fake_instance.fake_db_instance()
@@ -1601,20 +1580,6 @@ def test_load_ec2_ids(self, mock_get):
         mock_get.assert_called_once_with(self.context, inst)
         self.assertEqual(fake_ec2_ids, ec2_ids)
 
-    @mock.patch('nova.objects.SecurityGroupList.get_by_instance')
-    def test_load_security_groups(self, mock_get):
-        secgroups = []
-        for name in ('foo', 'bar'):
-            secgroup = security_group.SecurityGroup()
-            secgroup.name = name
-            secgroups.append(secgroup)
-        fake_secgroups = security_group.SecurityGroupList(objects=secgroups)
-        mock_get.return_value = fake_secgroups
-        inst = objects.Instance(context=self.context, uuid=uuids.instance)
-        secgroups = inst.security_groups
-        mock_get.assert_called_once_with(self.context, inst)
-        self.assertEqual(fake_secgroups, secgroups)
-
     @mock.patch('nova.objects.PciDeviceList.get_by_instance_uuid')
     def test_load_pci_devices(self, mock_get):
         fake_pci_devices = pci_device.PciDeviceList()
@@ -1744,7 +1709,6 @@ def fake_instance(self, id, updates=None):
         db_inst['updated_at'] = None
         db_inst['launched_at'] = datetime.datetime(1955, 11, 12,
                                                    22, 4, 0)
-        db_inst['security_groups'] = []
         db_inst['deleted'] = 0
 
         db_inst['info_cache'] = dict(test_instance_info_cache.fake_info_cache,
@@ -2010,49 +1974,6 @@ def test_fill_faults(self, mock_fault_get):
                                                [x.uuid for x in insts],
                                                latest=True)
 
-    @mock.patch('nova.objects.instance.Instance.obj_make_compatible')
-    def test_get_by_security_group(self, mock_compat):
-        fake_secgroup = dict(test_security_group.fake_secgroup)
-        fake_secgroup['instances'] = [
-            fake_instance.fake_db_instance(id=1,
-                                           system_metadata={'foo': 'bar'}),
-            fake_instance.fake_db_instance(id=2),
-            ]
-
-        with mock.patch.object(db, 'security_group_get') as sgg:
-            sgg.return_value = fake_secgroup
-            secgroup = security_group.SecurityGroup()
-            secgroup.id = fake_secgroup['id']
-            instances = instance.InstanceList.get_by_security_group(
-                self.context, secgroup)
-
-        self.assertEqual(2, len(instances))
-        self.assertEqual([1, 2], [x.id for x in instances])
-        self.assertTrue(instances[0].obj_attr_is_set('system_metadata'))
-        self.assertEqual({'foo': 'bar'}, instances[0].system_metadata)
-
-    def test_get_by_security_group_after_destroy(self):
-        db_sg = db.security_group_create(
-            self.context,
-            {'name': 'foo',
-             'description': 'test group',
-             'user_id': self.context.user_id,
-             'project_id': self.context.project_id})
-        self.assertFalse(db.security_group_in_use(self.context, db_sg.id))
-        inst = objects.Instance(
-            context=self.context,
-            user_id=self.context.user_id,
-            project_id=self.context.project_id)
-        inst.create()
-
-        db.instance_add_security_group(self.context,
-                                       inst.uuid,
-                                       db_sg.id)
-
-        self.assertTrue(db.security_group_in_use(self.context, db_sg.id))
-        inst.destroy()
-        self.assertFalse(db.security_group_in_use(self.context, db_sg.id))
-
     @mock.patch('nova.db.main.api.instance_get_all_uuids_by_hosts')
     def test_get_uuids_by_host_no_match(self, mock_get_all):
         mock_get_all.return_value = collections.defaultdict(list)
@@ -2159,14 +2080,14 @@ def test_expected_cols_extra(self):
 
     def test_expected_cols_no_duplicates(self):
         expected_attr = ['metadata', 'system_metadata', 'info_cache',
-                         'security_groups', 'info_cache', 'metadata',
+                         'info_cache', 'metadata',
                          'pci_devices', 'tags', 'extra', 'flavor']
 
         result_list = instance._expected_cols(expected_attr)
 
         self.assertEqual(len(result_list), len(set(expected_attr)))
         self.assertEqual(['metadata', 'system_metadata', 'info_cache',
-                         'security_groups', 'pci_devices', 'tags', 'extra',
+                         'pci_devices', 'tags', 'extra',
                          'extra.flavor'], result_list)