launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/cache-cleanup into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/cache-cleanup/+merge/118685

This branch fixes src/maasserver/worker_user.py: instead of using an in-process cache, use  Django's caching mechanism to cache the woker_user.

= Notes =

The main reason for that change is that I was starting to see spurious test failures (in a follow-up branch) linked to the fact that each test was not properly cleaning up the cached worker object.  Using Django's cache is:
- more flexible (we can use many backends (we're using the local memory backend at the moment), we can control the expiring time for each object)
- more test friendly: the cache gets cleared after each test.

This branch also gets rid of the NodeGroup.objects._delete_master method which was originally used to clear up the cached master nodegroup which is not cached anymore.  It was now simply removing the master nodegroup from the database and the database is wiped out between tests.

Drive-by fix: remove the unused definition of worker_user_name in src/maasserver/models/nodegroup.py.
-- 
https://code.launchpad.net/~rvb/maas/cache-cleanup/+merge/118685
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/cache-cleanup into lp:maas.

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-08-02 10:55:49 +0000
+++ src/maasserver/models/nodegroup.py	2012-08-08 08:17:19 +0000
@@ -31,9 +31,6 @@
 from provisioningserver.tasks import write_dhcp_config
 
 
-worker_user_name = 'maas-nodegroup-worker'
-
-
 class NodeGroupManager(Manager):
     """Manager for the NodeGroup class.
 
@@ -90,10 +87,6 @@
 
         return master
 
-    def _delete_master(self):
-        """For use by tests: delete the master nodegroup."""
-        self.filter(name='master').delete()
-
     def get_by_natural_key(self, name):
         """For Django, a node group's name is a natural key."""
         return self.get(name=name)

=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	2012-07-26 09:11:26 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-08-08 08:17:19 +0000
@@ -95,14 +95,12 @@
             call_command, 'config_master_dhcp', clear=True, ensure=True)
 
     def test_ensure_creates_master_nodegroup_without_dhcp_settings(self):
-        NodeGroup.objects._delete_master()
         call_command('config_master_dhcp', ensure=True)
         self.assertThat(
             NodeGroup.objects.get(name='master'),
             MatchesStructure.fromExample(make_cleared_dhcp_settings()))
 
     def test_ensure_leaves_cleared_settings_cleared(self):
-        NodeGroup.objects._delete_master()
         call_command('config_master_dhcp', clear=True)
         call_command('config_master_dhcp', ensure=True)
         self.assertThat(
@@ -110,7 +108,6 @@
             MatchesStructure.fromExample(make_cleared_dhcp_settings()))
 
     def test_ensure_leaves_dhcp_settings_intact(self):
-        NodeGroup.objects._delete_master()
         settings = make_dhcp_settings()
         call_command('config_master_dhcp', **settings)
         call_command('config_master_dhcp', ensure=True)

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-08-01 13:51:26 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-08-08 08:17:19 +0000
@@ -96,7 +96,6 @@
         self.assertEqual(key, nodegroup.dhcp_key)
 
     def test_ensure_master_creates_minimal_master_nodegroup(self):
-        NodeGroup.objects._delete_master()
         self.assertThat(
             NodeGroup.objects.ensure_master(),
             MatchesStructure.fromExample({
@@ -110,24 +109,20 @@
             }))
 
     def test_ensure_master_writes_master_nodegroup_to_database(self):
-        NodeGroup.objects._delete_master()
         master = NodeGroup.objects.ensure_master()
         self.assertEqual(
             master.id, NodeGroup.objects.get(name=master.name).id)
 
     def test_ensure_master_creates_dhcp_key(self):
-        NodeGroup.objects._delete_master()
         master = NodeGroup.objects.ensure_master()
         self.assertThat(len(master.dhcp_key), GreaterThan(20))
 
     def test_ensure_master_returns_same_nodegroup_every_time(self):
-        NodeGroup.objects._delete_master()
         self.assertEqual(
             NodeGroup.objects.ensure_master().id,
             NodeGroup.objects.ensure_master().id)
 
     def test_ensure_master_does_not_return_other_nodegroup(self):
-        NodeGroup.objects._delete_master()
         self.assertNotEqual(
             NodeGroup.objects.new(
                 factory.make_name('nodegroup'), factory.getRandomIPAddress()),

=== modified file 'src/maasserver/tests/test_worker_user.py'
--- src/maasserver/tests/test_worker_user.py	2012-07-18 04:01:06 +0000
+++ src/maasserver/tests/test_worker_user.py	2012-08-08 08:17:19 +0000
@@ -12,7 +12,9 @@
 __metaclass__ = type
 __all__ = []
 
+from django.conf import settings
 from django.contrib.auth.models import User
+from django.db import connection
 from maasserver.models import UserProfile
 from maasserver.models.user import SYSTEM_USERS
 from maasserver.testing.testcase import TestCase
@@ -37,3 +39,11 @@
         worker_user = get_worker_user()
         self.assertIn(worker_user.username, SYSTEM_USERS)
         self.assertRaises(UserProfile.DoesNotExist, worker_user.get_profile)
+
+    def test_get_worker_user_caches_user(self):
+        get_worker_user()
+        # Enable debug mode to be able to count the queries.
+        self.patch(settings, 'DEBUG', True)
+        self.patch(connection, 'queries', [])
+        get_worker_user()
+        self.assertEqual(0, len(connection.queries))

=== modified file 'src/maasserver/worker_user.py'
--- src/maasserver/worker_user.py	2012-07-25 10:20:00 +0000
+++ src/maasserver/worker_user.py	2012-08-08 08:17:19 +0000
@@ -19,19 +19,18 @@
     ]
 
 from django.contrib.auth.models import User
+from django.core.cache import cache
 
 
 user_name = 'maas-nodegroup-worker'
 
-
-# Cached, shared reference to this special user.  Keep internal to this
-# module.
-worker_user = None
+# Cache key for the worker user.
+WORKER_USER_CACHE_KEY = 'worker-user-maas-cache-key'
 
 
 def get_worker_user():
     """Get the system user representing the node-group workers."""
-    global worker_user
+    worker_user = cache.get(WORKER_USER_CACHE_KEY)
     if worker_user is None:
         worker_user, created = User.objects.get_or_create(
             username=user_name, defaults=dict(
@@ -39,4 +38,5 @@
                 last_name="Special user",
                 email="maas-nodegroup-worker@localhost",
                 is_staff=False, is_active=False, is_superuser=False))
+        cache.set(WORKER_USER_CACHE_KEY, worker_user)
     return worker_user