Merge lp:~allenap/maas/fewer-power-params-per-batch into lp:~maas-committers/maas/trunk

[Gavin Panella]

Limit batches of node power parameters to 10 for the periodic power checking service.

This is intended to reduce the likelihood of transactional conflicts.

When each batch of node power parameters is fetched from the region by a rack's power monitor service there is a sting in the tail: every row in maasserver_node that corresponds to those parameters is updated. This appears to be increasing transactional conflicts in the database to the point where it exceeds 10 retries and the transaction is fully rejected.

A better way to fix this would be to not update those rows, or to have a separate table to record last-queried times, but that's a lot more work and there isn't time right now, so instead we limit the batch size to 10.

This also fixes a long-standing bug in Factory.make_Node where the given power parameters were being ignored when also using bmc_connected_to. This fix improves the speed of one particular test, taking it from ~15 seconds to <1 second.

[Andres Rodriguez (andreserl)]

lgtm! Should this be attached to the bug report ?

review: Approve

=== modified file 'src/maasserver/rpc/nodes.py'
--- src/maasserver/rpc/nodes.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/rpc/nodes.py	2017-04-06 08:23:19 +0000
@@ -11,7 +11,10 @@
 ]
 
 from datetime import timedelta
-from itertools import chain
+from itertools import (
+    chain,
+    islice,
+)
 import json
 
 from django.contrib.auth.models import User
@@ -135,11 +138,15 @@
 
 @synchronous
 @transactional
-def list_cluster_nodes_power_parameters(system_id):
+def list_cluster_nodes_power_parameters(system_id, limit=10):
     """Return power parameters that a rack controller should power check,
     in priority order.
 
     For :py:class:`~provisioningserver.rpc.region.ListNodePowerParameters`.
+
+    :param limit: Limit the number of nodes for which to return power
+        parameters. Pass `None` to remove this numerical limit; there is still
+        a limit on the quantity of power information that will be returned.
     """
     try:
         rack = RackController.objects.get(system_id=system_id)
@@ -149,6 +156,7 @@
     # Generate all the the power queries that will fit into the response.
     nodes = rack.get_bmc_accessible_nodes()
     details = _gen_cluster_nodes_power_parameters(nodes)
+    details = islice(details, limit)  # ... but never more than `limit`.
     details = _gen_up_to_json_limit(details, 60 * (2 ** 10))  # 60kiB
     details = list(details)
 
 
=== modified file 'src/maasserver/rpc/tests/test_nodes.py'
--- src/maasserver/rpc/tests/test_nodes.py	2017-03-31 22:57:48 +0000
+++ src/maasserver/rpc/tests/test_nodes.py	2017-04-06 08:23:19 +0000
@@ -61,6 +61,7 @@
 from testtools.matchers import (
     Equals,
     GreaterThan,
+    HasLength,
     Is,
     LessThan,
     Not,
@@ -469,7 +470,7 @@
             system_ids)
 
     def test__returns_at_most_60kiB_of_JSON(self):
-        # Configure the rack controller subnt to be very large so it
+        # Configure the rack controller subnet to be very large so it
         # can hold that many BMC connected to the interface for the rack
         # controller.
         rack = factory.make_RackController(power_type='')
@@ -482,14 +483,15 @@
 
         # Ensure that there are at least 64kiB of power parameters (when
         # converted to JSON) in the database.
-        example_parameters = {"key%d" % i: "value%d" % i for i in range(100)}
+        example_parameters = {"key%d" % i: "value%d" % i for i in range(250)}
         remaining = 2 ** 16
         while remaining > 0:
             node = self.make_Node(
                 bmc_connected_to=rack, power_parameters=example_parameters)
             remaining -= len(json.dumps(node.get_effective_power_parameters()))
 
-        nodes = list_cluster_nodes_power_parameters(rack.system_id)
+        nodes = list_cluster_nodes_power_parameters(
+            rack.system_id, limit=None)  # Remove numeric limit.
 
         # The total size of the JSON is less than 60kiB, but only a bit.
         nodes_json = map(json.dumps, nodes)
@@ -500,6 +502,25 @@
         expected_minimum = 50 * (2 ** 10)  # 50kiB
         self.expectThat(nodes_json_length, GreaterThan(expected_minimum - 1))
 
+    def test__limited_to_10_nodes_at_a_time_by_default(self):
+        # Configure the rack controller subnet to be large enough.
+        rack = factory.make_RackController(power_type='')
+        rack_interface = rack.get_boot_interface()
+        subnet = factory.make_Subnet(
+            cidr=str(factory.make_ipv6_network(slash=8)))
+        factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet), subnet=subnet,
+            interface=rack_interface)
+
+        # Create at least 11 nodes connected to the rack.
+        for _ in range(11):
+            self.make_Node(bmc_connected_to=rack)
+
+        # Only 10 nodes' power parameters are returned.
+        self.assertThat(
+            list_cluster_nodes_power_parameters(rack.system_id),
+            HasLength(10))
+
 
 class TestUpdateNodePowerState(MAASServerTestCase):
 
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2017-04-03 19:46:59 +0000
+++ src/maasserver/testing/factory.py	2017-04-06 08:23:19 +0000
@@ -456,6 +456,7 @@
                 ip_address = existing_static_ips[0]
             bmc_ip_address = self.pick_ip_in_Subnet(ip_address.subnet)
             node.power_parameters = {
+                **node.power_parameters,
                 "power_address": "qemu+ssh://user@%s/system" % (
                     factory.ip_to_url_format(bmc_ip_address)),
                 "power_id": factory.make_name("power_id"),
 
=== modified file 'src/maasserver/testing/tests/test_factory.py'
--- src/maasserver/testing/tests/test_factory.py	2016-09-22 02:53:33 +0000
+++ src/maasserver/testing/tests/test_factory.py	2017-04-06 08:23:19 +0000
@@ -12,6 +12,7 @@
 from maasserver.utils.orm import reload_object
 from testtools.matchers import (
     Contains,
+    ContainsDict,
     Equals,
 )
 
@@ -99,3 +100,18 @@
         self.assertThat(
             {iface.vlan for iface in sip.interface_set.all()},
             Contains(iface.vlan))
+
+
+class TestFactoryForNodes(MAASServerTestCase):
+
+    def test_make_Node_uses_power_parameters_when_connecting_to_BMC(self):
+        p_key = factory.make_name("key")
+        p_value = factory.make_name("value")
+        node = factory.make_Node(
+            bmc_connected_to=factory.make_RackController(),
+            power_parameters={p_key: p_value},
+        )
+        self.assertThat(
+            node.get_effective_power_parameters(),
+            ContainsDict({p_key: Equals(p_value)}),
+        )