Merge lp:~gmb/maas/dont-powercheck-broken-nodes-bug-1402243 into lp:maas/trunk

Proposed by Graham Binns on 2014-12-15
Status: Merged
Approved by: Graham Binns on 2014-12-15
Approved revision: 3431
Merged at revision: 3428
Proposed branch: lp:~gmb/maas/dont-powercheck-broken-nodes-bug-1402243
Merge into: lp:maas/trunk
Diff against target: 69 lines (+29/-1)
2 files modified
src/maasserver/rpc/nodes.py (+2/-1)
src/maasserver/rpc/tests/test_nodes.py (+27/-0)
To merge this branch: bzr merge lp:~gmb/maas/dont-powercheck-broken-nodes-bug-1402243
Reviewer Review Type Date Requested Status
Raphaël Badin (community) 2014-12-15 Approve on 2014-12-15
Review via email: mp+244704@code.launchpad.net

Commit message

Ensure that BROKEN nodes are not checked by the power query service. Previously, nodes marked as BROKEN -- which could be marked as such due to BMC issues -- were continually checked by the power service, even though there could be no way to query them.

To post a comment you must log in.
3430. By Graham Binns on 2014-12-15

Damn you, formatter.

Raphaël Badin (rvb) wrote :

Looks good, just one tiny remark about the test.

review: Approve
Graham Binns (gmb) wrote :

On 15 December 2014 at 10:21, Raphaël Badin <email address hidden> wrote:
> These checks are a bit weird: they check that the working node is the only one there which, of course, implies that the broken node is not; wouldn't it be simpler and clearer to actually check that the system_id of the broken node *isn't* there?
>
> i.e. something like:
>
> self.assertThat(
> [power_parameter['system_id'] for power_parameter in power_parameters],
> Not(Contains(broken_node.system_id))
> )

Yeah, it would certainly be more cromulent for the test, wouldn't it?

3431. By Graham Binns on 2014-12-15

Review changes for Raphaël.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/rpc/nodes.py'
2--- src/maasserver/rpc/nodes.py 2014-10-23 21:06:40 +0000
3+++ src/maasserver/rpc/nodes.py 2014-12-15 10:56:48 +0000
4@@ -22,6 +22,7 @@
5 from django.core.exceptions import ValidationError
6 from maasserver import exceptions
7 from maasserver.api.utils import get_overridden_query_dict
8+from maasserver.enum import NODE_STATUS
9 from maasserver.forms import AdminNodeWithMACAddressesForm
10 from maasserver.models import (
11 MACAddress,
12@@ -71,7 +72,7 @@
13 else:
14 power_info_by_node = (
15 (node, node.get_effective_power_info())
16- for node in nodegroup.node_set.all()
17+ for node in nodegroup.node_set.exclude(status=NODE_STATUS.BROKEN)
18 )
19 return [
20 {
21
22=== modified file 'src/maasserver/rpc/tests/test_nodes.py'
23--- src/maasserver/rpc/tests/test_nodes.py 2014-11-25 13:06:34 +0000
24+++ src/maasserver/rpc/tests/test_nodes.py 2014-12-15 10:56:48 +0000
25@@ -20,6 +20,7 @@
26 from maasserver.enum import NODE_STATUS
27 from maasserver.rpc.nodes import (
28 create_node,
29+ list_cluster_nodes_power_parameters,
30 mark_node_failed,
31 request_node_info_by_mac_address,
32 )
33@@ -41,6 +42,10 @@
34 )
35 from provisioningserver.rpc.testing import always_succeed_with
36 from simplejson import dumps
37+from testtools.matchers import (
38+ Contains,
39+ Not,
40+ )
41
42
43 class TestCreateNode(MAASServerTestCase):
44@@ -191,3 +196,25 @@
45 node, boot_purpose = request_node_info_by_mac_address(
46 mac_address.mac_address.get_raw())
47 self.assertEqual(node, mac_address.node)
48+
49+
50+class TestListClusterNodesPowerParameters(MAASServerTestCase):
51+ """Tests for the `list_cluster_nodes_power_parameters()` function."""
52+
53+ # Note that there are other, one-level-removed tests for this
54+ # function in the TestRegionProtocol_ListNodePowerParameters
55+ # testcase in maasserver.rpc.tests.test_regionservice.
56+ # Those tests have been left there for now because they also check
57+ # that the return values are being formatted correctly for RPC.
58+
59+ def test_does_not_return_power_info_for_broken_nodes(self):
60+ cluster = factory.make_NodeGroup()
61+ broken_node = factory.make_Node(
62+ nodegroup=cluster, status=NODE_STATUS.BROKEN)
63+
64+ power_parameters = list_cluster_nodes_power_parameters(cluster.uuid)
65+ returned_system_ids = [
66+ power_params['system_id'] for power_params in power_parameters]
67+
68+ self.assertThat(
69+ returned_system_ids, Not(Contains(broken_node.system_id)))