Merge lp:~blake-rouse/maas/fix-1401983 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Previously every PXE request would update the nodes pxe_mac link even if it was already that value. This make sure that it is only update when the mac address that was PXE booted changes.

[Newell Jensen (newell-jensen)]

LGTM.

review: Approve

[Jason Hobbs (jason-hobbs)]

approve

review: Approve

[Andres Rodriguez (andreserl)]

Great! I think we need a backport to 1.7
On Dec 12, 2014 2:24 PM, "Blake Rouse" <email address hidden> wrote:

> Blake Rouse has proposed merging lp:~blake-rouse/maas/fix-1401983 into
> lp:maas.
>
> Commit message:
> Previously every PXE request would update the nodes pxe_mac link even if
> it was already that value. This make sure that it is only update when the
> mac address that was PXE booted changes.
>
> Requested reviews:
> MAAS Maintainers (maas-maintainers)
> Related bugs:
> Bug #1401983 in MAAS: "Exception: deadlock detected"
> https://bugs.launchpad.net/maas/+bug/1401983
>
> For more details, see:
> https://code.launchpad.net/~blake-rouse/maas/fix-1401983/+merge/244646
> --
> You are subscribed to branch lp:maas.
>
> === modified file 'src/maasserver/api/pxeconfig.py'
> --- src/maasserver/api/pxeconfig.py 2014-12-09 15:51:13 +0000
> +++ src/maasserver/api/pxeconfig.py 2014-12-12 19:23:43 +0000
> @@ -168,10 +168,15 @@
> node = get_node_from_mac_string(request_mac)
>
> if node is not None:
> - # Update the record of which MAC address and cluster interface
> - # this node uses to PXE boot.
> - node.pxe_mac = MACAddress.objects.get(mac_address=request_mac)
> - node.save()
> + # Only update the PXE booting interface for the node if it has
> + # changed.
> + if (node.pxe_mac is None or
> + node.pxe_mac.mac_address != request_mac):
> + node.pxe_mac = MACAddress.objects.get(mac_address=request_mac)
> + node.save()
> +
> + # Update the record of the cluster interface this node uses
> + # to PXE boot.
> update_mac_cluster_interfaces(request_ip, request_mac,
> node.nodegroup)
>
> if node is None or node.get_boot_purpose() == "commissioning":
>
> === modified file 'src/maasserver/api/tests/test_pxeconfig.py'
> --- src/maasserver/api/tests/test_pxeconfig.py 2014-12-09 15:51:13 +0000
> +++ src/maasserver/api/tests/test_pxeconfig.py 2014-12-12 19:23:43 +0000
> @@ -40,6 +40,7 @@
> Config,
> Event,
> MACAddress,
> + Node,
> )
> from maasserver.preseed import (
> compose_enlistment_preseed_url,
> @@ -501,9 +502,19 @@
> description,
> Event.objects.get(node=node).description)
>
> - def test_pxeconfig_updates_pxe_mac_for_existing_node(self):
> + def test_pxeconfig_sets_pxe_mac_when_empty(self):
> + node = factory.make_Node(mac=True)
> + mac = node.macaddress_set.first()
> + params = self.get_default_params()
> + params['mac'] = mac.mac_address
> + self.client.get(reverse('pxeconfig'), params)
> + node = reload_object(node)
> + self.assertEqual(mac, node.pxe_mac)
> +
> + def test_pxeconfig_updates_pxe_mac_when_changed(self):
> node = factory.make_Node()
> node.pxe_mac = factory.make_MACAddress(node=node)
> + node.save()
> mac = factory.make_MACAddress(node=node)
> params = self.get_default_params()
> params['mac'] = mac.mac_address
> @@ -511,6 +522,16 @@
> node = reload_object(node)
> self.assertEqual(mac, node.pxe_mac)
>
>...

[Jason Hobbs (jason-hobbs)]

This also seems to help the performance issue in oil.

=== modified file 'src/maasserver/api/pxeconfig.py'
--- src/maasserver/api/pxeconfig.py	2014-12-09 15:51:13 +0000
+++ src/maasserver/api/pxeconfig.py	2014-12-12 19:23:43 +0000
@@ -168,10 +168,15 @@
     node = get_node_from_mac_string(request_mac)
 
     if node is not None:
-        # Update the record of which MAC address and cluster interface
-        # this node uses to PXE boot.
-        node.pxe_mac = MACAddress.objects.get(mac_address=request_mac)
-        node.save()
+        # Only update the PXE booting interface for the node if it has
+        # changed.
+        if (node.pxe_mac is None or
+                node.pxe_mac.mac_address != request_mac):
+            node.pxe_mac = MACAddress.objects.get(mac_address=request_mac)
+            node.save()
+
+        # Update the record of the cluster interface this node uses
+        # to PXE boot.
         update_mac_cluster_interfaces(request_ip, request_mac, node.nodegroup)
 
     if node is None or node.get_boot_purpose() == "commissioning":

=== modified file 'src/maasserver/api/tests/test_pxeconfig.py'
--- src/maasserver/api/tests/test_pxeconfig.py	2014-12-09 15:51:13 +0000
+++ src/maasserver/api/tests/test_pxeconfig.py	2014-12-12 19:23:43 +0000
@@ -40,6 +40,7 @@
     Config,
     Event,
     MACAddress,
+    Node,
     )
 from maasserver.preseed import (
     compose_enlistment_preseed_url,
@@ -501,9 +502,19 @@
                 description,
                 Event.objects.get(node=node).description)
 
-    def test_pxeconfig_updates_pxe_mac_for_existing_node(self):
+    def test_pxeconfig_sets_pxe_mac_when_empty(self):
+        node = factory.make_Node(mac=True)
+        mac = node.macaddress_set.first()
+        params = self.get_default_params()
+        params['mac'] = mac.mac_address
+        self.client.get(reverse('pxeconfig'), params)
+        node = reload_object(node)
+        self.assertEqual(mac, node.pxe_mac)
+
+    def test_pxeconfig_updates_pxe_mac_when_changed(self):
         node = factory.make_Node()
         node.pxe_mac = factory.make_MACAddress(node=node)
+        node.save()
         mac = factory.make_MACAddress(node=node)
         params = self.get_default_params()
         params['mac'] = mac.mac_address
@@ -511,6 +522,16 @@
         node = reload_object(node)
         self.assertEqual(mac, node.pxe_mac)
 
+    def test_pxeconfig_doesnt_update_pxe_mac_when_same(self):
+        node = factory.make_Node()
+        node.pxe_mac = factory.make_MACAddress(node=node)
+        node.save()
+        params = self.get_default_params()
+        params['mac'] = node.pxe_mac.mac_address
+        mock_save = self.patch(Node, 'save')
+        self.client.get(reverse('pxeconfig'), params)
+        self.assertThat(mock_save, MockNotCalled())
+
     def test_pxeconfig_returns_commissioning_os_series_for_other_oses(self):
         osystem = Config.objects.get_config('default_osystem')
         release = Config.objects.get_config('default_distro_series')