launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/pre-virsh-power into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/pre-virsh-power/+merge/107963

This rearranges the way we compose power parameters a bit, so that it can fit the virsh use-case without special-purpose code.  The virsh power type needs to know the system_id, because it does not identify a node by its MAC.  (I know because I have that branch already underway; this work has been discussed with Julian).

The change means that sometimes we'll look up a node's primary MAC when it's not needed.  I don't think it's a very expensive operation; there's things we could do to avoid the database access, at the cost of some special-casing, but that's micro-optimization.

I'm not entirely convinced that the defaulting behaviour I chose is what we want, but I just kept the existing behaviour and created a place to encapsulate it.  We can change it later as insight progresses.

You'll also see a new Node method to obtain the node's primary MAC address.  Something in the back of my head says we probably had code elsewhere that could benefit from reusing the new method, but I was unable to find it.  Maybe it was in the provisioning server, in which case there's not much point trying to share the same code — one has access to the ORM and the other does not.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/pre-virsh-power/+merge/107963
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/pre-virsh-power into lp:maas.

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-05-28 14:28:46 +0000
+++ src/maasserver/models/__init__.py	2012-05-30 12:37:23 +0000
@@ -340,35 +340,24 @@
         :return: Those Nodes for which power-on was actually requested.
         :rtype: list
         """
-        # TODO: File structure needs sorting out to avoid this circular
-        # import dance.
+        # Avoid circular imports.
         from metadataserver.models import NodeUserData
+
         nodes = self.get_nodes(by_user, NODE_PERMISSION.EDIT, ids=ids)
+        for node in nodes:
+            NodeUserData.objects.set_user_data(node, user_data)
         processed_nodes = []
         for node in nodes:
-            NodeUserData.objects.set_user_data(node, user_data)
-            # Wake on LAN is a special case, deal with it first.
+            power_params = node.get_effective_power_parameters()
             node_power_type = node.get_effective_power_type()
             if node_power_type == POWER_TYPE.WAKE_ON_LAN:
-                # If power_parameters is set, use it.  Otherwise, use the
-                # first registered MAC address.
-                mac = None
-                if node.power_parameters:
-                    mac = node.power_parameters.get("mac", None)
-                else:
-                    try:
-                        macaddress = node.macaddress_set.order_by('created')[0]
-                    except IndexError:
-                        pass  # No MAC recorded for this node.
-                    else:
-                        mac = macaddress.mac_address
-                if mac is not None and mac != "":
-                    power_on.delay(node_power_type, mac=mac)
-                    processed_nodes.append(node)
+                mac = power_params.get('mac')
+                do_start = (mac != '' and mac is not None)
             else:
-                if node.power_parameters:
-                    power_on.delay(node_power_type, **node.power_parameters)
-                    processed_nodes.append(node)
+                do_start = True
+            if do_start:
+                power_on.delay(node_power_type, **power_params)
+                processed_nodes.append(node)
         return processed_nodes
 
 
@@ -596,6 +585,31 @@
             power_type = self.power_type
         return power_type
 
+    def get_primary_mac(self):
+        """Return the primary :class:`MACAddress` for this node."""
+        macs = self.macaddress_set.order_by('created')[:1]
+        if len(macs) > 0:
+            return macs[0]
+        else:
+            return None
+
+    def get_effective_power_parameters(self):
+        """Return effective power parameters, including any defaults."""
+        if self.power_parameters:
+            power_params = self.power_parameters.copy()
+        else:
+            # An empty power_parameters comes out as an empty unicode string!
+            power_params = {}
+
+        power_params.setdefault('system_id', self.system_id)
+        # The "mac" parameter defaults to the node's primary MAC
+        # address, but only if no power parameters were set at all.
+        if not self.power_parameters:
+            primary_mac = self.get_primary_mac()
+            if primary_mac is not None:
+                power_params['mac'] = primary_mac.mac_address
+        return power_params
+
     def acquire(self, user, token=None):
         """Mark commissioned node as acquired by the given user and token."""
         assert self.owner is None

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-05-30 01:41:24 +0000
+++ src/maasserver/tests/test_models.py	2012-05-30 12:37:23 +0000
@@ -12,7 +12,10 @@
 __metaclass__ = type
 __all__ = []
 
-from datetime import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 
 from django.conf import settings
 from django.contrib.auth.models import User
@@ -187,6 +190,27 @@
             node=node, mac_address='AA:BB:CC:DD:EE:FF').count()
         self.assertEqual(0, macs)
 
+    def test_get_primary_mac_returns_mac_address(self):
+        node = factory.make_node()
+        mac = factory.getRandomMACAddress()
+        node.add_mac_address(mac)
+        self.assertEqual(mac, node.get_primary_mac().mac_address)
+
+    def test_get_primary_mac_returns_None_if_node_has_no_mac(self):
+        node = factory.make_node()
+        self.assertIsNone(node.get_primary_mac())
+
+    def test_get_primary_mac_returns_oldest_mac(self):
+        node = factory.make_node()
+        macs = [factory.getRandomMACAddress() for counter in range(3)]
+        offset = timedelta(0)
+        for mac in macs:
+            mac_address = node.add_mac_address(mac)
+            mac_address.created += offset
+            mac_address.save()
+            offset += timedelta(1)
+        self.assertEqual(macs[0], node.get_primary_mac().mac_address)
+
     def test_delete_node_deletes_related_mac(self):
         node = factory.make_node()
         mac = node.add_mac_address('AA:BB:CC:DD:EE:FF')
@@ -267,6 +291,31 @@
         node = factory.make_node(power_type=POWER_TYPE.DEFAULT)
         self.assertEqual("", node.power_parameters)
 
+    def test_get_effective_power_parameters_returns_power_parameters(self):
+        params = {'test_parameter': factory.getRandomString()}
+        node = factory.make_node(power_parameters=params)
+        self.assertEqual(
+            params['test_parameter'],
+            node.get_effective_power_parameters()['test_parameter'])
+
+    def test_get_effective_power_parameters_adds_system_id(self):
+        node = factory.make_node()
+        self.assertEqual(
+            node.system_id,
+            node.get_effective_power_parameters()['system_id'])
+
+    def test_get_effective_power_parameters_adds_mac_if_no_params_set(self):
+        node = factory.make_node()
+        mac = factory.getRandomMACAddress()
+        node.add_mac_address(mac)
+        self.assertEqual(mac, node.get_effective_power_parameters()['mac'])
+
+    def test_get_effective_power_parameters_adds_no_mac_if_params_set(self):
+        node = factory.make_node(power_parameters={'foo': 'bar'})
+        mac = factory.getRandomMACAddress()
+        node.add_mac_address(mac)
+        self.assertNotIn('mac', node.get_effective_power_parameters())
+
     def test_acquire(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()