Merge lp:~tr3buchet/nova/lock into lp:~hudson-openstack/nova/trunk

[Trey Morris]

This branch implements lock functionality. The lock is stored in the compute worker database. Decorators have been added to the openstack API actions which alter instances in any way.

[Trey Morris (tr3buchet)]

also, not shown is me modifying the users and instances tables in another window to set the state of is_admin and locked attributes which are commented about above.

[lp:~tr3buchet/nova/lock]

507. By Trey Morris on 2010-12-29

removed some code i didn't end up using

[Trey Morris (tr3buchet)]

also, I apologize for launchpad mangling my commit message

[Rick Harris (rconradharris)]

Overall lgtm. A few small comments:

> + if(admin or not locked):

No parens needed :-)

> + @checks_lock

I'm wondering if we should give this particular lock a memorable/unambiguous name-- there will be various kinds of locks, mutexs, etc throughout the system, so down the road, check_lock might not be as clear as it is today.

Maybe, @check_instance_lock, or @check_state_lock. Just a thought.

[Trey Morris (tr3buchet)]

i like that
i'll fix up both

[lp:~tr3buchet/nova/lock]

508. By Trey Morris on 2010-12-29

removed () from if (can't believe i did that) and renamed checks_lock decorator

[Trey Morris (tr3buchet)]

fixed.

also moved my testing notes from above to this comment so it isn't in the commit message

Notes: unittests passed locally. pep8 returned not errors locally. My integration test with output is as follows (remember that the openstack API maps state "pause" to "error"):

(trey|3nova)~/openstack/n3vascript> n3va.sh teardown
(trey|3nova)~/openstack/n3vascript> n3va.sh run

(screen window)
(trey|3nova)~/openstack/n3vascript> . /home/trey/openstack/n3vascript/novarc
(trey|3nova)~/openstack/n3vascript> euca-add-keypair test > test.pem
(trey|3nova)~/openstack/n3vascript> export CLOUD_SERVERS_USERNAME=admin
(trey|3nova)~/openstack/n3vascript> export CLOUD_SERVERS_API_KEY=admin
(trey|3nova)~/openstack/n3vascript> export CLOUD_SERVERS_URL=http://localhost:8774/v1.0/
(trey|3nova)~/openstack/n3vascript> euca-run-instances -k test -t m1.tiny ami-tiny
RESERVATION r-27hxqjra admin
INSTANCE i-qssosb ami-tiny scheduling test (admin, None) 0 m1.tiny 2010-12-29 20:54:38.391163
(trey|3nova)~/openstack/n3vascript>
(trey|3nova)~/openstack/n3vascript> # unlocked + admin
(trey|3nova)~/openstack/n3vascript> cloudservers list
+------------+-------------------+--------+-----------+------------+
| ID | Name | Status | Public IP | Private IP |
+------------+-------------------+--------+-----------+------------+
| 1620488315 | Server 1620488315 | active | | |
+------------+-------------------+--------+-----------+------------+
(trey|3nova)~/openstack/n3vascript> cloudservers pause 1620488315
(trey|3nova)~/openstack/n3vascript> cloudservers list
+------------+-------------------+--------+-----------+------------+
| ID | Name | Status | Public IP | Private IP |
+------------+-------------------+--------+-----------+------------+
| 1620488315 | Server 1620488315 | error | | |
+------------+-------------------+--------+-----------+------------+
(trey|3nova)~/openstack/n3vascript> cloudservers unpause 1620488315
(trey|3nova)~/openstack/n3vascript> cloudservers list
+------------+-------------------+--------+-----------+------------+
| ID | Name | Status | Public IP | Private IP |
+------------+-------------------+--------+-----------+------------+
| 1620488315 | Server 1620488315 | active | | |
+------------+-------------------+--------+-----------+------------+
(trey|3nova)~/openstack/n3vascript>
(trey|3nova)~/openstack/n3vascript> # locked + admin
(trey|3nova)~/openstack/n3vascript> cloudservers list
+------------+-------------------+--------+-----------+------------+
| ID | Name | Status | Public IP | Private IP |
+------------+-------------------+--------+-----------+------------+
| 1620488315 | Server 1620488315 | active | | |
+------------+-------------------+--------+-----------+------------+
(trey|3nova)~/openstack/n3vascript> cloudservers pause 1620488315
(trey|3nova)~/openstack/n3vascript> cloudservers list
+------------+-------------------+--------+-----------+------------+
| ID | Name | Status | Public ...

[Trey Morris (tr3buchet)]

oooh less mangling this way too :D

[Eric Day (eday)]

I think the lock check should be happening on the compute worker node, not within the API server (and if it were happening in the API server, it should be in compute.api, not in the openstack API specific code). We need to start planning for race conditions in a distributed system, and this means handling operations at the canonical source.

review: Needs Fixing

[Matt Dietz (cerberus)]

I'm not sure I agree with the idea of a locked instance returning not found. I think it delivers an idea inconsistent with the intent of the locking mechanism. I would say HTTP 405 (MethodNotAllowed) or something like that instead, as a 404 wouldn't let the end user know that it needs to be unlocked. Also, I don't think we want to lock the show action, as the lock is supposed to represent immutability, not visibility.

review: Needs Fixing

[lp:~tr3buchet/nova/lock]

509. By Trey Morris on 2010-12-29

removed lock check from show and changed returning 404 to 405

[Trey Morris (tr3buchet)]

re matthew: fixed.
re eric: yikes, good point.

[Trey Morris (tr3buchet)]

also re eric: you devil...

[lp:~tr3buchet/nova/lock]

510. By Trey Morris on 2010-12-30

moved check lock decorator to compute api level. altered openstack.test_servers according and wrote test for lock in tests.test_compute

511. By Trey Morris on 2010-12-30

fixd variables being out of scope in lock decorator

512. By Trey Morris on 2010-12-30

altered error exception/logging

513. By Trey Morris on 2010-12-30

altered error exception/logging

514. By Trey Morris on 2010-12-30

typo, trying to hurry.. look where that got me

515. By Trey Morris on 2010-12-30

added some logging

516. By Trey Morris on 2010-12-30

removed db.set_lock, using update_instance instead

517. By Trey Morris on 2010-12-30

moved check lock decorator from the compute api to the come manager... when it rains it pours

518. By Trey Morris on 2010-12-30

syntax error

519. By Trey Morris on 2010-12-30

fixed up the compute lock test, was failing because the context was always admin

520. By Trey Morris on 2010-12-30

removed tests.api.openstack.test_servers test_lock, to hell with it. i'm not even sure if testing lock needs to be at this level

[Trey Morris (tr3buchet)]

re eric:
ok moved the lock check business to the compute api. Tests spent forever fixing the unittests, gave up. Moved the lock check business to the compute manager. Tests are passing. Testing shows it works. Attempting to perform a lock checking action on a locked instance when not an admin will throw an exception which is currently uncaught. Also I removed the test for lock in the openstack api tests because I'm not sure a lock test should go there.

[lp:~tr3buchet/nova/lock]

521. By Trey Morris on 2010-12-30

altered the compute lock test

522. By Trey Morris on 2010-12-30

fixed the compute lock test

[Trey Morris (tr3buchet)]

alright here we go. Now it works, passes unittests and when you attempt to do something to a locked instance and are not an admin, nothing happens and this shows up in the logs (for example):

ERROR:root:Instance |1| is locked, cannot execute |<function pause_instance at 0x2d910c8>|

[Devin Carlen (devcamcar)]

good work, lock name is clear and code is in compute api now.

review: Approve

[Eric Day (eday)]

140: I would simplify this and assume (self, context, instance_id) for args[0/1/2] and not looks in kwargs. In fact, on 129, I would instead do:

+ def decorated_function(self, context, instance_id, *args, **kwargs):

and remove the try block altogether.

[lp:~tr3buchet/nova/lock]

523. By Trey Morris on 2011-01-06

altered argument handling

524. By Trey Morris on 2011-01-06

pep8

525. By Trey Morris on 2011-01-06

merged trunk

526. By Trey Morris on 2011-01-06

accidentally left unlocked in there, it should have been locked

527. By Trey Morris on 2011-01-06

passing the correct parameters to decorated function

528. By Trey Morris on 2011-01-06

refers to instance_id instead of instance_ref[instance_id]

529. By Trey Morris on 2011-01-06

typo

[Trey Morris (tr3buchet)]

alright eric day, i've got your changes implemented. locally unittests pass, pep8 returns nothing and the lock works.

[Eric Day (eday)]

lgtm

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-01-05 17:50:19 +0000
+++ nova/api/openstack/servers.py	2011-01-07 00:00:12 +0000
@@ -170,6 +170,50 @@
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()
 
+    def lock(self, req, id):
+        """
+        lock the instance with id
+        admin only operation
+
+        """
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.lock(context, id)
+        except:
+            readable = traceback.format_exc()
+            logging.error(_("Compute.api::lock %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
+    def unlock(self, req, id):
+        """
+        unlock the instance with id
+        admin only operation
+
+        """
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.unlock(context, id)
+        except:
+            readable = traceback.format_exc()
+            logging.error(_("Compute.api::unlock %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
+    def get_lock(self, req, id):
+        """
+        return the boolean state of (instance with id)'s lock
+
+        """
+        context = req.environ['nova.context']
+        try:
+            self.compute_api.get_lock(context, id)
+        except:
+            readable = traceback.format_exc()
+            logging.error(_("Compute.api::get_lock %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
     def pause(self, req, id):
         """ Permit Admins to Pause the server. """
         ctxt = req.environ['nova.context']
 
=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-01-05 17:50:19 +0000
+++ nova/compute/api.py	2011-01-07 00:00:12 +0000
@@ -147,6 +147,7 @@
             'user_data': user_data or '',
             'key_name': key_name,
             'key_data': key_data,
+            'locked': False,
             'availability_zone': availability_zone}
 
         elevated = context.elevated()
@@ -354,6 +355,38 @@
                  {"method": "unrescue_instance",
                   "args": {"instance_id": instance_id}})
 
+    def lock(self, context, instance_id):
+        """
+        lock the instance with instance_id
+
+        """
+        instance = self.get_instance(context, instance_id)
+        host = instance['host']
+        rpc.cast(context,
+                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                 {"method": "lock_instance",
+                  "args": {"instance_id": instance['id']}})
+
+    def unlock(self, context, instance_id):
+        """
+        unlock the instance with instance_id
+
+        """
+        instance = self.get_instance(context, instance_id)
+        host = instance['host']
+        rpc.cast(context,
+                 self.db.queue_get_for(context, FLAGS.compute_topic, host),
+                 {"method": "unlock_instance",
+                  "args": {"instance_id": instance['id']}})
+
+    def get_lock(self, context, instance_id):
+        """
+        return the boolean state of (instance with instance_id)'s lock
+
+        """
+        instance = self.get_instance(context, instance_id)
+        return instance['locked']
+
     def attach_volume(self, context, instance_id, volume_id, device):
         if not re.match("^/dev/[a-z]d[a-z]+$", device):
             raise exception.ApiError(_("Invalid device specified: %s. "
 
=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-01-06 13:12:57 +0000
+++ nova/compute/manager.py	2011-01-07 00:00:12 +0000
@@ -36,6 +36,7 @@
 
 import datetime
 import logging
+import functools
 
 from nova import exception
 from nova import flags
@@ -53,6 +54,38 @@
                     'Stub network related code')
 
 
+def checks_instance_lock(function):
+    """
+    decorator used for preventing action against locked instances
+    unless, of course, you happen to be admin
+
+    """
+
+    @functools.wraps(function)
+    def decorated_function(self, context, instance_id, *args, **kwargs):
+
+        logging.info(_("check_instance_lock: decorating: |%s|"), function)
+        logging.info(_("check_instance_lock: arguments: |%s| |%s| |%s|"),
+                                                                   self,
+                                                                   context,
+                                                                   instance_id)
+        locked = self.get_lock(context, instance_id)
+        admin = context.is_admin
+        logging.info(_("check_instance_lock: locked: |%s|"), locked)
+        logging.info(_("check_instance_lock: admin: |%s|"), admin)
+
+        # if admin or unlocked call function otherwise log error
+        if admin or not locked:
+            logging.info(_("check_instance_lock: executing: |%s|"), function)
+            function(self, context, instance_id, *args, **kwargs)
+        else:
+            logging.error(_("check_instance_lock: not executing |%s|"),
+                                                              function)
+            return False
+
+    return decorated_function
+
+
 class ComputeManager(manager.Manager):
 
     """Manages the running instances from creation to destruction."""
@@ -158,6 +191,7 @@
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def terminate_instance(self, context, instance_id):
         """Terminate an instance on this machine."""
         context = context.elevated()
@@ -202,6 +236,7 @@
         self.db.instance_destroy(context, instance_id)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def reboot_instance(self, context, instance_id):
         """Reboot an instance on this server."""
         context = context.elevated()
@@ -246,6 +281,7 @@
         self.driver.snapshot(instance_ref, name)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def rescue_instance(self, context, instance_id):
         """Rescue an instance on this server."""
         context = context.elevated()
@@ -261,6 +297,7 @@
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def unrescue_instance(self, context, instance_id):
         """Rescue an instance on this server."""
         context = context.elevated()
@@ -280,6 +317,7 @@
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def pause_instance(self, context, instance_id):
         """Pause an instance on this server."""
         context = context.elevated()
@@ -297,6 +335,7 @@
                                                        result))
 
     @exception.wrap_exception
+    @checks_instance_lock
     def unpause_instance(self, context, instance_id):
         """Unpause a paused instance on this server."""
         context = context.elevated()
@@ -324,8 +363,12 @@
             return self.driver.get_diagnostics(instance_ref)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def suspend_instance(self, context, instance_id):
-        """suspend the instance with instance_id"""
+        """
+        suspend the instance with instance_id
+
+        """
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
 
@@ -340,8 +383,12 @@
                                                        result))
 
     @exception.wrap_exception
+    @checks_instance_lock
     def resume_instance(self, context, instance_id):
-        """resume the suspended instance with instance_id"""
+        """
+        resume the suspended instance with instance_id
+
+        """
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
 
@@ -356,6 +403,41 @@
                                                        result))
 
     @exception.wrap_exception
+    def lock_instance(self, context, instance_id):
+        """
+        lock the instance with instance_id
+
+        """
+        context = context.elevated()
+        instance_ref = self.db.instance_get(context, instance_id)
+
+        logging.debug(_('instance %s: locking'), instance_id)
+        self.db.instance_update(context, instance_id, {'locked': True})
+
+    @exception.wrap_exception
+    def unlock_instance(self, context, instance_id):
+        """
+        unlock the instance with instance_id
+
+        """
+        context = context.elevated()
+        instance_ref = self.db.instance_get(context, instance_id)
+
+        logging.debug(_('instance %s: unlocking'), instance_id)
+        self.db.instance_update(context, instance_id, {'locked': False})
+
+    @exception.wrap_exception
+    def get_lock(self, context, instance_id):
+        """
+        return the boolean state of (instance with instance_id)'s lock
+
+        """
+        context = context.elevated()
+        logging.debug(_('instance %s: getting locked state'), instance_id)
+        instance_ref = self.db.instance_get(context, instance_id)
+        return instance_ref['locked']
+
+    @exception.wrap_exception
     def get_console_output(self, context, instance_id):
         """Send the console output for an instance."""
         context = context.elevated()
@@ -365,6 +447,7 @@
         return self.driver.get_console_output(instance_ref)
 
     @exception.wrap_exception
+    @checks_instance_lock
     def attach_volume(self, context, instance_id, volume_id, mountpoint):
         """Attach a volume to an instance."""
         context = context.elevated()
@@ -394,6 +477,7 @@
         return True
 
     @exception.wrap_exception
+    @checks_instance_lock
     def detach_volume(self, context, instance_id, volume_id):
         """Detach a volume from an instance."""
         context = context.elevated()
 
=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-01-05 22:59:41 +0000
+++ nova/db/sqlalchemy/models.py	2011-01-07 00:00:12 +0000
@@ -224,6 +224,8 @@
     display_name = Column(String(255))
     display_description = Column(String(255))
 
+    locked = Column(Boolean)
+
     # TODO(vish): see Ewan's email about state improvements, probably
     #             should be in a driver base class or some such
     # vmstate_state = running, halted, suspended, paused
 
=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2010-12-31 03:55:00 +0000
+++ nova/tests/test_compute.py	2011-01-07 00:00:12 +0000
@@ -178,3 +178,22 @@
                           self.context,
                           instance_id)
         self.compute.terminate_instance(self.context, instance_id)
+
+    def test_lock(self):
+        """ensure locked instance cannot be changed"""
+        instance_id = self._create_instance()
+        self.compute.run_instance(self.context, instance_id)
+
+        non_admin_context = context.RequestContext(None, None, False, False)
+
+        # decorator should return False (fail) with locked nonadmin context
+        self.compute.lock_instance(self.context, instance_id)
+        ret_val = self.compute.reboot_instance(non_admin_context, instance_id)
+        self.assertEqual(ret_val, False)
+
+        # decorator should return None (success) with unlocked nonadmin context
+        self.compute.unlock_instance(self.context, instance_id)
+        ret_val = self.compute.reboot_instance(non_admin_context, instance_id)
+        self.assertEqual(ret_val, None)
+
+        self.compute.terminate_instance(self.context, instance_id)