Merge lp:~vishvananda/nova/has_role_cache into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

Uses memcached to cache roles so that ldap is actually usable.

Uses memcached to cache roles so that ldap is actually usable.

This lowers response time in our test environment from almost two seconds to .8 seconds for actions by non-admin users when using ldap.

This is sleepsonthefloor's code. I just merged trunk and fixed conflicts and formatting because he won't be able to update it in time for Gamma Freeze.

[lp:~vishvananda/nova/has_role_cache]

742. By Vish Ishaya on 2011-04-05

merged trunk

743. By Vish Ishaya on 2011-04-05

fixed comment

[Devin Carlen (devcamcar)]

49 + def _build_mc_key(self, user, role, project=None):
50 + return str("rolecache-%s-%s-%s" % (User.safe_id(user), role,
51 + (Project.safe_id(project) if project else 'None')))

Nitpicky, but it's more accurate to use rolecache-user-role for global roles, as opposed to rolecache-user-role-None.

review: Needs Fixing

[Vish Ishaya (vishvananda)]

Devin: good point, fixed.

[lp:~vishvananda/nova/has_role_cache]

744. By Vish Ishaya on 2011-04-05

remove -None for user roles

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Rick Harris (rconradharris)]

> 62 + if rslt == None:

Minor nit, but better as, `if rslt is None`.

> 50 + role_key = str("rolecache-%s-%s" % (User.safe_id(user), role))
> 51 + if project:
> 52 + return "%s-%s" % (role_key, Project.safe_id(project))
> 53 + return role_key

Nothing wrong with the code above, but, it looks like this case is calling out for a `join`:

key_parts = ['rolecache', User.safe_id(user), str(role)]
if project:
  key_parts.append(Project.safe_id(project))
return '-'.join(key_parts)

review: Needs Fixing

[Vish Ishaya (vishvananda)]

good fixes Rick, changed.

[lp:~vishvananda/nova/has_role_cache]

745. By Vish Ishaya on 2011-04-22

Fixes per review

[Brian Waldon (bcwaldon)]

97: Can you replace exception.Error with an exception that makes more sense? nova/exception.py has been refactored with a ton of specific exception classes and there may be one that fits better in this case. If not, it would be great if you would add one for this case.

review: Needs Fixing

[Vish Ishaya (vishvananda)]

Brian: Made the fix you requested. Thanks for the exception refactor.

On Apr 28, 2011, at 9:45 AM, Brian Waldon wrote:

> Review: Needs Fixing
> 97: Can you replace exception.Error with an exception that makes more sense? nova/exception.py has been refactored with a ton of specific exception classes and there may be one that fits better in this case. If not, it would be great if you would add one for this case.
> --
> https://code.launchpad.net/~vishvananda/nova/has_role_cache/+merge/56449
> You are the owner of lp:~vishvananda/nova/has_role_cache.

[Brian Waldon (bcwaldon)]

Great work.

review: Approve

[Rick Harris (rconradharris)]

Looks great, nice job Vish.

review: Approve

=== modified file 'nova/api/ec2/__init__.py'
--- nova/api/ec2/__init__.py	2011-03-24 18:17:40 +0000
+++ nova/api/ec2/__init__.py	2011-04-22 06:45:54 +0000
@@ -46,8 +46,6 @@
                      'Number of minutes to lockout if triggered.')
 flags.DEFINE_integer('lockout_window', 15,
                      'Number of minutes for lockout window.')
-flags.DEFINE_list('lockout_memcached_servers', None,
-                  'Memcached servers or None for in process cache.')
 
 
 class RequestLogging(wsgi.Middleware):
@@ -107,11 +105,11 @@
 
     def __init__(self, application):
         """middleware can use fake for testing."""
-        if FLAGS.lockout_memcached_servers:
+        if FLAGS.memcached_servers:
             import memcache
         else:
             from nova import fakememcache as memcache
-        self.mc = memcache.Client(FLAGS.lockout_memcached_servers,
+        self.mc = memcache.Client(FLAGS.memcached_servers,
                                   debug=0)
         super(Lockout, self).__init__(application)
 
 
=== modified file 'nova/auth/manager.py'
--- nova/auth/manager.py	2011-04-18 20:40:16 +0000
+++ nova/auth/manager.py	2011-04-22 06:45:54 +0000
@@ -223,6 +223,13 @@
         if driver or not getattr(self, 'driver', None):
             self.driver = utils.import_class(driver or FLAGS.auth_driver)
 
+        if FLAGS.memcached_servers:
+            import memcache
+        else:
+            from nova import fakememcache as memcache
+        self.mc = memcache.Client(FLAGS.memcached_servers,
+                                  debug=0)
+
     def authenticate(self, access, signature, params, verb='GET',
                      server_string='127.0.0.1:8773', path='/',
                      check_type='ec2', headers=None):
@@ -360,6 +367,27 @@
             if self.has_role(user, role):
                 return True
 
+    def _build_mc_key(self, user, role, project=None):
+        key_parts = ['rolecache', User.safe_id(user), str(role)]
+        if project:
+            key_parts.append(Project.safe_id(project))
+        return '-'.join(key_parts)
+
+    def _clear_mc_key(self, user, role, project=None):
+        # NOTE(anthony): it would be better to delete the key
+        self.mc.set(self._build_mc_key(user, role, project), None)
+
+    def _has_role(self, user, role, project=None):
+        mc_key = self._build_mc_key(user, role, project)
+        rslt = self.mc.get(mc_key)
+        if rslt is None:
+            with self.driver() as drv:
+                rslt = drv.has_role(user, role, project)
+                self.mc.set(mc_key, rslt)
+                return rslt
+        else:
+            return rslt
+
     def has_role(self, user, role, project=None):
         """Checks existence of role for user
 
@@ -383,24 +411,24 @@
         @rtype: bool
         @return: True if the user has the role.
         """
-        with self.driver() as drv:
-            if role == 'projectmanager':
-                if not project:
-                    raise exception.Error(_("Must specify project"))
-                return self.is_project_manager(user, project)
-
-            global_role = drv.has_role(User.safe_id(user),
-                                        role,
-                                        None)
-            if not global_role:
-                return global_role
-
-            if not project or role in FLAGS.global_roles:
-                return global_role
-
-            return drv.has_role(User.safe_id(user),
-                                 role,
-                                 Project.safe_id(project))
+        if role == 'projectmanager':
+            if not project:
+                raise exception.Error(_("Must specify project"))
+            return self.is_project_manager(user, project)
+
+        global_role = self._has_role(User.safe_id(user),
+                                     role,
+                                     None)
+
+        if not global_role:
+            return global_role
+
+        if not project or role in FLAGS.global_roles:
+            return global_role
+
+        return self._has_role(User.safe_id(user),
+                              role,
+                              Project.safe_id(project))
 
     def add_role(self, user, role, project=None):
         """Adds role for user
@@ -432,6 +460,7 @@
             LOG.audit(_("Adding sitewide role %(role)s to user %(uid)s")
                     % locals())
         with self.driver() as drv:
+            self._clear_mc_key(uid, role, pid)
             drv.add_role(uid, role, pid)
 
     def remove_role(self, user, role, project=None):
@@ -460,6 +489,7 @@
             LOG.audit(_("Removing sitewide role %(role)s"
                     " from user %(uid)s") % locals())
         with self.driver() as drv:
+            self._clear_mc_key(uid, role, pid)
             drv.remove_role(uid, role, pid)
 
     @staticmethod
 
=== modified file 'nova/flags.py'
--- nova/flags.py	2011-04-21 23:01:38 +0000
+++ nova/flags.py	2011-04-22 06:45:54 +0000
@@ -369,6 +369,9 @@
 DEFINE_string('node_availability_zone', 'nova',
               'availability zone of this node')
 
+DEFINE_list('memcached_servers', None,
+            'Memcached servers or None for in process cache.')
+
 DEFINE_string('zone_name', 'nova', 'name of this zone')
 DEFINE_list('zone_capabilities',
                 ['hypervisor=xenserver;kvm', 'os=linux;windows'],