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
1=== modified file 'nova/api/ec2/__init__.py'
2--- nova/api/ec2/__init__.py 2011-03-24 18:17:40 +0000
3+++ nova/api/ec2/__init__.py 2011-04-22 06:45:54 +0000
4@@ -46,8 +46,6 @@
5 'Number of minutes to lockout if triggered.')
6 flags.DEFINE_integer('lockout_window', 15,
7 'Number of minutes for lockout window.')
8-flags.DEFINE_list('lockout_memcached_servers', None,
9- 'Memcached servers or None for in process cache.')
10
11
12 class RequestLogging(wsgi.Middleware):
13@@ -107,11 +105,11 @@
14
15 def __init__(self, application):
16 """middleware can use fake for testing."""
17- if FLAGS.lockout_memcached_servers:
18+ if FLAGS.memcached_servers:
19 import memcache
20 else:
21 from nova import fakememcache as memcache
22- self.mc = memcache.Client(FLAGS.lockout_memcached_servers,
23+ self.mc = memcache.Client(FLAGS.memcached_servers,
24 debug=0)
25 super(Lockout, self).__init__(application)
26
27
28=== modified file 'nova/auth/manager.py'
29--- nova/auth/manager.py 2011-04-18 20:40:16 +0000
30+++ nova/auth/manager.py 2011-04-22 06:45:54 +0000
31@@ -223,6 +223,13 @@
32 if driver or not getattr(self, 'driver', None):
33 self.driver = utils.import_class(driver or FLAGS.auth_driver)
34
35+ if FLAGS.memcached_servers:
36+ import memcache
37+ else:
38+ from nova import fakememcache as memcache
39+ self.mc = memcache.Client(FLAGS.memcached_servers,
40+ debug=0)
41+
42 def authenticate(self, access, signature, params, verb='GET',
43 server_string='127.0.0.1:8773', path='/',
44 check_type='ec2', headers=None):
45@@ -360,6 +367,27 @@
46 if self.has_role(user, role):
47 return True
48
49+ def _build_mc_key(self, user, role, project=None):
50+ key_parts = ['rolecache', User.safe_id(user), str(role)]
51+ if project:
52+ key_parts.append(Project.safe_id(project))
53+ return '-'.join(key_parts)
54+
55+ def _clear_mc_key(self, user, role, project=None):
56+ # NOTE(anthony): it would be better to delete the key
57+ self.mc.set(self._build_mc_key(user, role, project), None)
58+
59+ def _has_role(self, user, role, project=None):
60+ mc_key = self._build_mc_key(user, role, project)
61+ rslt = self.mc.get(mc_key)
62+ if rslt is None:
63+ with self.driver() as drv:
64+ rslt = drv.has_role(user, role, project)
65+ self.mc.set(mc_key, rslt)
66+ return rslt
67+ else:
68+ return rslt
69+
70 def has_role(self, user, role, project=None):
71 """Checks existence of role for user
72
73@@ -383,24 +411,24 @@
74 @rtype: bool
75 @return: True if the user has the role.
76 """
77- with self.driver() as drv:
78- if role == 'projectmanager':
79- if not project:
80- raise exception.Error(_("Must specify project"))
81- return self.is_project_manager(user, project)
82-
83- global_role = drv.has_role(User.safe_id(user),
84- role,
85- None)
86- if not global_role:
87- return global_role
88-
89- if not project or role in FLAGS.global_roles:
90- return global_role
91-
92- return drv.has_role(User.safe_id(user),
93- role,
94- Project.safe_id(project))
95+ if role == 'projectmanager':
96+ if not project:
97+ raise exception.Error(_("Must specify project"))
98+ return self.is_project_manager(user, project)
99+
100+ global_role = self._has_role(User.safe_id(user),
101+ role,
102+ None)
103+
104+ if not global_role:
105+ return global_role
106+
107+ if not project or role in FLAGS.global_roles:
108+ return global_role
109+
110+ return self._has_role(User.safe_id(user),
111+ role,
112+ Project.safe_id(project))
113
114 def add_role(self, user, role, project=None):
115 """Adds role for user
116@@ -432,6 +460,7 @@
117 LOG.audit(_("Adding sitewide role %(role)s to user %(uid)s")
118 % locals())
119 with self.driver() as drv:
120+ self._clear_mc_key(uid, role, pid)
121 drv.add_role(uid, role, pid)
122
123 def remove_role(self, user, role, project=None):
124@@ -460,6 +489,7 @@
125 LOG.audit(_("Removing sitewide role %(role)s"
126 " from user %(uid)s") % locals())
127 with self.driver() as drv:
128+ self._clear_mc_key(uid, role, pid)
129 drv.remove_role(uid, role, pid)
130
131 @staticmethod
132
133=== modified file 'nova/flags.py'
134--- nova/flags.py 2011-04-21 23:01:38 +0000
135+++ nova/flags.py 2011-04-22 06:45:54 +0000
136@@ -369,6 +369,9 @@
137 DEFINE_string('node_availability_zone', 'nova',
138 'availability zone of this node')
139
140+DEFINE_list('memcached_servers', None,
141+ 'Memcached servers or None for in process cache.')
142+
143 DEFINE_string('zone_name', 'nova', 'name of this zone')
144 DEFINE_list('zone_capabilities',
145 ['hypervisor=xenserver;kvm', 'os=linux;windows'],