launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/prefetch-bug-1084443 into lp:maas.

Commit message:
Use selected_related to work around bug 1084443.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1084443 in MAAS: "Node listing page errors when nodes from different cluster controllers are displayed."
  https://bugs.launchpad.net/maas/+bug/1084443

For more details, see:
https://code.launchpad.net/~rvb/maas/prefetch-bug-1084443/+merge/136910

Ok, this a tricky one: I think there is a bug in 'prefetch_related' in Django.

I could not find the exact source of the bug but it happens when 'prefetch_related' is used to traverse many-to-many relations.  I don't really have time to investigate more and I found a cheap solution:  instead of using:

nodes = prefetch_related('nodegroup')
nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')

I changed it to:

nodes = nodes.select_related('nodegroup')
nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')

This fixes the problem (see the test) plus it saves one query because using 'select_related' makes the pre-fetching happen inside the main query.

See https://docs.djangoproject.com/en/dev/ref/models/querysets/#django.db.models.query.QuerySet.prefetch_related for details.

I've also fixed the other calls to prefetch_related with many-to-many relashionship.
-- 
https://code.launchpad.net/~rvb/maas/prefetch-bug-1084443/+merge/136910
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/prefetch-bug-1084443 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-27 16:01:37 +0000
+++ src/maasserver/api.py	2012-11-29 11:34:20 +0000
@@ -664,7 +664,7 @@
         # related interfaces).
         nodes = nodes.prefetch_related('macaddress_set__node')
         nodes = nodes.prefetch_related('tags')
-        nodes = nodes.prefetch_related('nodegroup')
+        nodes = nodes.select_related('nodegroup')
         nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')
         return nodes.order_by('id')
 

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-11-26 12:55:17 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-11-29 11:34:20 +0000
@@ -96,6 +96,16 @@
         self.assertIn(sort_hostname, get_content_links(response))
         self.assertIn(sort_status, get_content_links(response))
 
+    def test_node_list_lists_nodes_from_different_nodegroups(self):
+        # Bug 1084443.
+        nodegroup1 = factory.make_node_group()
+        nodegroup2 = factory.make_node_group()
+        factory.make_node(nodegroup=nodegroup1)
+        factory.make_node(nodegroup=nodegroup2)
+        factory.make_node(nodegroup=nodegroup2)
+        response = self.client.get(reverse('node-list'))
+        self.assertEqual(httplib.OK, response.status_code)
+
     def test_node_list_sorts_by_hostname(self):
         names = ['zero', 'one', 'five']
         nodes = [factory.make_node(hostname=n) for n in names]

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-11-26 12:55:17 +0000
+++ src/maasserver/views/nodes.py	2012-11-29 11:34:20 +0000
@@ -137,7 +137,7 @@
             except InvalidConstraint as e:
                 self.query_error = e
                 return Node.objects.none()
-        nodes = nodes.prefetch_related('nodegroup')
+        nodes = nodes.select_related('nodegroup')
         nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')
         return nodes
 

=== modified file 'src/maasserver/views/tags.py'
--- src/maasserver/views/tags.py	2012-11-08 12:58:48 +0000
+++ src/maasserver/views/tags.py	2012-11-29 11:34:20 +0000
@@ -36,7 +36,7 @@
         nodes = Tag.objects.get_nodes(
             self.tag, user=self.request.user, prefetch_mac=True,
             ).order_by('-created')
-        nodes = nodes.prefetch_related('nodegroup')
+        nodes = nodes.select_related('nodegroup')
         nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')
         return nodes