Merge lp:~sleepsonthefloor/nova/709057 into lp:~hudson-openstack/nova/trunk

[Anthony Young]

Fixes bug #709057

* Mark security_group_instance_associations as deleted when instance deleted
* Mark security_group_instance_associations as deleted when security_group deleted
* Make SecurityGroup.instances mapping properly ignore deleted security_group_instance_associations
* Add tests

IMO, this is important to merge. Nebula has monitoring projects that continually launch and destroy instances, and I imagine other real-world deployments may have similar health checkers. After a few weeks such health checkers choke due to the inefficient queries related to #709057.

[Vish Ishaya (vishvananda)]

I think this is a good candidate for an exception. It is a pretty annoying performance error for large deployments, and it is limited to the db layer.

[Devin Carlen (devcamcar)]

5 with session.begin():
6 instance_ref = instance_get(context, instance_id, session=session)
7 instance_ref.delete(session=session)
8 + session.execute('update security_group_instance_association'
9 + ' set deleted=1 where instance_id=:id',
10 + {'id': instance_id})

Would prefer to not see mixing of the sqlalchemy orm and raw sql code in a single operation. Eventually we should replace the raw sql with the sqlalchemy query language (which is essentially in between the orm and raw sql, a rather happy medium).

I propose making lines 6 and 7 match this style:

16 session.execute('update security_groups set deleted=1 where id=:id',
17 {'id': security_group_id})

This also has the advantage of not eating an extra query looking up the instance_ref, which instance_ref.delete currently does.

review: Needs Fixing

[Thierry Carrez (ttx)]

I agree this sounds like a good candidate. I'll wait for the final diff before accepting it though.

[Rick Harris (rconradharris)]

> + self.assert_(len(group.instances) == 0)

Micro-nit: This is usually better written with self.assertEqual; not only is it more specific, it allows the unittest library to better pretty-print an assertion error since it has access to both the LHS and RHS of the expression.

Overall this patch lgtm, though this problem feels like it would be more elegantly solved using SQLAlchemy's cascading delete feature [1].

[1] http://www.sqlalchemy.org/docs/05/ormtutorial.html - search for cascade

review: Approve

[Devin Carlen (devcamcar)]

We can't use casading deletes because in this case we don't want to actually destroy the rows. We are using paranoid deletions so everything needs to manually set the deleted flag.

lgtm

review: Approve

[Thierry Carrez (ttx)]

Exception granted if it can get another core review asap.

review: Approve (gfe)

[Soren Hansen (soren)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~sleepsonthefloor/nova/709057 into lp:nova failed. Below is the output from the failed tests.

AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ok
    test_create_instance ok
    test_delete_backup_schedules ok
    test_delete_server_instance ok
    test_get_all_server_details ok
    ...

[lp:~sleepsonthefloor/nova/709057]

640. By Anthony Young on 2011-02-01

fix pep8 error :/

641. By Anthony Young on 2011-02-01

merge trunk

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-01-27 18:48:00 +0000
+++ nova/db/sqlalchemy/api.py	2011-02-01 17:51:07 +0000
@@ -19,6 +19,7 @@
 Implementation of SQLAlchemy backend.
 """
 
+import datetime
 import warnings
 
 from nova import db
@@ -670,8 +671,14 @@
 def instance_destroy(context, instance_id):
     session = get_session()
     with session.begin():
-        instance_ref = instance_get(context, instance_id, session=session)
-        instance_ref.delete(session=session)
+        session.execute('update instances set deleted=1,'
+                        'deleted_at=:at where id=:id',
+                        {'id': instance_id,
+                         'at': datetime.datetime.utcnow()})
+        session.execute('update security_group_instance_association '
+                        'set deleted=1,deleted_at=:at where instance_id=:id',
+                        {'id': instance_id,
+                         'at': datetime.datetime.utcnow()})
 
 
 @require_context
@@ -1583,6 +1590,11 @@
         # TODO(vish): do we have to use sql here?
         session.execute('update security_groups set deleted=1 where id=:id',
                         {'id': security_group_id})
+        session.execute('update security_group_instance_association '
+                        'set deleted=1,deleted_at=:at '
+                        'where security_group_id=:id',
+                        {'id': security_group_id,
+                         'at': datetime.datetime.utcnow()})
         session.execute('update security_group_rules set deleted=1 '
                         'where group_id=:id',
                         {'id': security_group_id})
 
=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-01-18 23:51:13 +0000
+++ nova/db/sqlalchemy/models.py	2011-02-01 17:51:07 +0000
@@ -311,10 +311,14 @@
                              secondary="security_group_instance_association",
                              primaryjoin='and_('
         'SecurityGroup.id == '
-            'SecurityGroupInstanceAssociation.security_group_id,'
+        'SecurityGroupInstanceAssociation.security_group_id,'
+        'SecurityGroupInstanceAssociation.deleted == False,'
         'SecurityGroup.deleted == False)',
                              secondaryjoin='and_('
         'SecurityGroupInstanceAssociation.instance_id == Instance.id,'
+        # (anthony) the condition below shouldn't be necessary now that the
+        # association is being marked as deleted.  However, removing this
+        # may cause existing deployments to choke, so I'm leaving it
         'Instance.deleted == False)',
                              backref='security_groups')
 
 
=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2011-01-17 18:05:26 +0000
+++ nova/tests/test_compute.py	2011-02-01 17:51:07 +0000
@@ -49,7 +49,7 @@
         self.manager = manager.AuthManager()
         self.user = self.manager.create_user('fake', 'fake', 'fake')
         self.project = self.manager.create_project('fake', 'fake', 'fake')
-        self.context = context.get_admin_context()
+        self.context = context.RequestContext('fake', 'fake', False)
 
     def tearDown(self):
         self.manager.delete_user(self.user)
@@ -69,6 +69,13 @@
         inst['ami_launch_index'] = 0
         return db.instance_create(self.context, inst)['id']
 
+    def _create_group(self):
+        values = {'name': 'testgroup',
+                  'description': 'testgroup',
+                  'user_id': self.user.id,
+                  'project_id': self.project.id}
+        return db.security_group_create(self.context, values)
+
     def test_create_instance_defaults_display_name(self):
         """Verify that an instance cannot be created without a display_name."""
         cases = [dict(), dict(display_name=None)]
@@ -82,21 +89,53 @@
 
     def test_create_instance_associates_security_groups(self):
         """Make sure create associates security groups"""
-        values = {'name': 'default',
-                  'description': 'default',
-                  'user_id': self.user.id,
-                  'project_id': self.project.id}
-        group = db.security_group_create(self.context, values)
+        group = self._create_group()
         ref = self.compute_api.create(
                 self.context,
                 instance_type=FLAGS.default_instance_type,
                 image_id=None,
-                security_group=['default'])
+                security_group=['testgroup'])
         try:
             self.assertEqual(len(db.security_group_get_by_instance(
-                self.context, ref[0]['id'])), 1)
-        finally:
-            db.security_group_destroy(self.context, group['id'])
+                             self.context, ref[0]['id'])), 1)
+            group = db.security_group_get(self.context, group['id'])
+            self.assert_(len(group.instances) == 1)
+        finally:
+            db.security_group_destroy(self.context, group['id'])
+            db.instance_destroy(self.context, ref[0]['id'])
+
+    def test_destroy_instance_disassociates_security_groups(self):
+        """Make sure destroying disassociates security groups"""
+        group = self._create_group()
+
+        ref = self.compute_api.create(
+                self.context,
+                instance_type=FLAGS.default_instance_type,
+                image_id=None,
+                security_group=['testgroup'])
+        try:
+            db.instance_destroy(self.context, ref[0]['id'])
+            group = db.security_group_get(self.context, group['id'])
+            self.assert_(len(group.instances) == 0)
+        finally:
+            db.security_group_destroy(self.context, group['id'])
+
+    def test_destroy_security_group_disassociates_instances(self):
+        """Make sure destroying security groups disassociates instances"""
+        group = self._create_group()
+
+        ref = self.compute_api.create(
+                self.context,
+                instance_type=FLAGS.default_instance_type,
+                image_id=None,
+                security_group=['testgroup'])
+
+        try:
+            db.security_group_destroy(self.context, group['id'])
+            group = db.security_group_get(context.get_admin_context(
+                                          read_deleted=True), group['id'])
+            self.assert_(len(group.instances) == 0)
+        finally:
             db.instance_destroy(self.context, ref[0]['id'])
 
     def test_run_terminate(self):