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

Proposed by Vish Ishaya on 2011-04-05
Status: Merged
Approved by: Rick Harris on 2011-04-29
Approved revision: 745
Merged at revision: 1037
Proposed branch: lp:~vishvananda/nova/has_role_cache
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 145 lines (+53/-22)
3 files modified
nova/api/ec2/__init__.py (+2/-4)
nova/auth/manager.py (+48/-18)
nova/flags.py (+3/-0)
To merge this branch: bzr merge lp:~vishvananda/nova/has_role_cache
Reviewer Review Type Date Requested Status
Rick Harris (community) Approve on 2011-04-29
Brian Waldon (community) Approve on 2011-04-28
Devin Carlen (community) 2011-04-05 Approve on 2011-04-05
Review via email: mp+56449@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
lp:~vishvananda/nova/has_role_cache updated on 2011-04-05
742. By Vish Ishaya on 2011-04-05

merged trunk

743. By Vish Ishaya on 2011-04-05

fixed comment

Devin Carlen (devcamcar) wrote :

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) wrote :

Devin: good point, fixed.

lp:~vishvananda/nova/has_role_cache updated on 2011-04-05
744. By Vish Ishaya on 2011-04-05

remove -None for user roles

Devin Carlen (devcamcar) wrote :

lgtm

review: Approve
Rick Harris (rconradharris) wrote :

> 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) wrote :

good fixes Rick, changed.

lp:~vishvananda/nova/has_role_cache updated on 2011-04-22
745. By Vish Ishaya on 2011-04-22

Fixes per review

Brian Waldon (bcwaldon) wrote :

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) wrote :

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) wrote :

Great work.

review: Approve
Rick Harris (rconradharris) wrote :

Looks great, nice job Vish.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
46 'Number of minutes to lockout if triggered.')46 'Number of minutes to lockout if triggered.')
47flags.DEFINE_integer('lockout_window', 15,47flags.DEFINE_integer('lockout_window', 15,
48 'Number of minutes for lockout window.')48 'Number of minutes for lockout window.')
49flags.DEFINE_list('lockout_memcached_servers', None,
50 'Memcached servers or None for in process cache.')
5149
5250
53class RequestLogging(wsgi.Middleware):51class RequestLogging(wsgi.Middleware):
@@ -107,11 +105,11 @@
107105
108 def __init__(self, application):106 def __init__(self, application):
109 """middleware can use fake for testing."""107 """middleware can use fake for testing."""
110 if FLAGS.lockout_memcached_servers:108 if FLAGS.memcached_servers:
111 import memcache109 import memcache
112 else:110 else:
113 from nova import fakememcache as memcache111 from nova import fakememcache as memcache
114 self.mc = memcache.Client(FLAGS.lockout_memcached_servers,112 self.mc = memcache.Client(FLAGS.memcached_servers,
115 debug=0)113 debug=0)
116 super(Lockout, self).__init__(application)114 super(Lockout, self).__init__(application)
117115
118116
=== 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 @@
223 if driver or not getattr(self, 'driver', None):223 if driver or not getattr(self, 'driver', None):
224 self.driver = utils.import_class(driver or FLAGS.auth_driver)224 self.driver = utils.import_class(driver or FLAGS.auth_driver)
225225
226 if FLAGS.memcached_servers:
227 import memcache
228 else:
229 from nova import fakememcache as memcache
230 self.mc = memcache.Client(FLAGS.memcached_servers,
231 debug=0)
232
226 def authenticate(self, access, signature, params, verb='GET',233 def authenticate(self, access, signature, params, verb='GET',
227 server_string='127.0.0.1:8773', path='/',234 server_string='127.0.0.1:8773', path='/',
228 check_type='ec2', headers=None):235 check_type='ec2', headers=None):
@@ -360,6 +367,27 @@
360 if self.has_role(user, role):367 if self.has_role(user, role):
361 return True368 return True
362369
370 def _build_mc_key(self, user, role, project=None):
371 key_parts = ['rolecache', User.safe_id(user), str(role)]
372 if project:
373 key_parts.append(Project.safe_id(project))
374 return '-'.join(key_parts)
375
376 def _clear_mc_key(self, user, role, project=None):
377 # NOTE(anthony): it would be better to delete the key
378 self.mc.set(self._build_mc_key(user, role, project), None)
379
380 def _has_role(self, user, role, project=None):
381 mc_key = self._build_mc_key(user, role, project)
382 rslt = self.mc.get(mc_key)
383 if rslt is None:
384 with self.driver() as drv:
385 rslt = drv.has_role(user, role, project)
386 self.mc.set(mc_key, rslt)
387 return rslt
388 else:
389 return rslt
390
363 def has_role(self, user, role, project=None):391 def has_role(self, user, role, project=None):
364 """Checks existence of role for user392 """Checks existence of role for user
365393
@@ -383,24 +411,24 @@
383 @rtype: bool411 @rtype: bool
384 @return: True if the user has the role.412 @return: True if the user has the role.
385 """413 """
386 with self.driver() as drv:414 if role == 'projectmanager':
387 if role == 'projectmanager':415 if not project:
388 if not project:416 raise exception.Error(_("Must specify project"))
389 raise exception.Error(_("Must specify project"))417 return self.is_project_manager(user, project)
390 return self.is_project_manager(user, project)418
391419 global_role = self._has_role(User.safe_id(user),
392 global_role = drv.has_role(User.safe_id(user),420 role,
393 role,421 None)
394 None)422
395 if not global_role:423 if not global_role:
396 return global_role424 return global_role
397425
398 if not project or role in FLAGS.global_roles:426 if not project or role in FLAGS.global_roles:
399 return global_role427 return global_role
400428
401 return drv.has_role(User.safe_id(user),429 return self._has_role(User.safe_id(user),
402 role,430 role,
403 Project.safe_id(project))431 Project.safe_id(project))
404432
405 def add_role(self, user, role, project=None):433 def add_role(self, user, role, project=None):
406 """Adds role for user434 """Adds role for user
@@ -432,6 +460,7 @@
432 LOG.audit(_("Adding sitewide role %(role)s to user %(uid)s")460 LOG.audit(_("Adding sitewide role %(role)s to user %(uid)s")
433 % locals())461 % locals())
434 with self.driver() as drv:462 with self.driver() as drv:
463 self._clear_mc_key(uid, role, pid)
435 drv.add_role(uid, role, pid)464 drv.add_role(uid, role, pid)
436465
437 def remove_role(self, user, role, project=None):466 def remove_role(self, user, role, project=None):
@@ -460,6 +489,7 @@
460 LOG.audit(_("Removing sitewide role %(role)s"489 LOG.audit(_("Removing sitewide role %(role)s"
461 " from user %(uid)s") % locals())490 " from user %(uid)s") % locals())
462 with self.driver() as drv:491 with self.driver() as drv:
492 self._clear_mc_key(uid, role, pid)
463 drv.remove_role(uid, role, pid)493 drv.remove_role(uid, role, pid)
464494
465 @staticmethod495 @staticmethod
466496
=== 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 @@
369DEFINE_string('node_availability_zone', 'nova',369DEFINE_string('node_availability_zone', 'nova',
370 'availability zone of this node')370 'availability zone of this node')
371371
372DEFINE_list('memcached_servers', None,
373 'Memcached servers or None for in process cache.')
374
372DEFINE_string('zone_name', 'nova', 'name of this zone')375DEFINE_string('zone_name', 'nova', 'name of this zone')
373DEFINE_list('zone_capabilities',376DEFINE_list('zone_capabilities',
374 ['hypervisor=xenserver;kvm', 'os=linux;windows'],377 ['hypervisor=xenserver;kvm', 'os=linux;windows'],