Merge ~mpontillo/maas:fix-random-choice-no-choices into maas:master

Proposed by Mike Pontillo on 2017-07-19
Status: Merged
Approved by: Mike Pontillo on 2017-07-19
Approved revision: d12f855cac596173616f47bdd73a34bcf6679e57
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:fix-random-choice-no-choices
Merge into: maas:master
Diff against target: 96 lines (+32/-9)
3 files modified
requirements.txt (+1/-1)
src/maasserver/rpc/regionservice.py (+11/-7)
src/maasserver/rpc/tests/test_regionservice.py (+20/-1)
Reviewer Review Type Date Requested Status
Blake Rouse (community) 2017-07-19 Approve on 2017-07-19
Review via email: mp+327708@code.launchpad.net

Commit message

Prevent RegionService from using empty lists.

LP: #1705254

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

Looks good.

review: Approve
MAAS Lander (maas-lander) wrote :

LANDING
-b fix-random-choice-no-choices lp:~mpontillo/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/120/console

d12f855... by Mike Pontillo on 2017-07-19

Fix incorrect assumptions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/requirements.txt b/requirements.txt
2index e08077a..160c720 100644
3--- a/requirements.txt
4+++ b/requirements.txt
5@@ -13,7 +13,7 @@ petname==2.0
6 pexpect==4.2.0
7 psycopg2==2.6.2
8 pymongo==3.2
9-pyopenssl==16.0.0
10+pyopenssl==16.2.0
11 pyparsing==2.0.7
12 pyvmomi==6.0.0.2016.6
13 requests==2.11.0
14diff --git a/src/maasserver/rpc/regionservice.py b/src/maasserver/rpc/regionservice.py
15index a5790e8..02d987d 100644
16--- a/src/maasserver/rpc/regionservice.py
17+++ b/src/maasserver/rpc/regionservice.py
18@@ -992,21 +992,25 @@ class RegionService(service.Service, object):
19 def getAllClients(self):
20 """Return a list with one connection per rack controller."""
21 return [
22- common.Client(random.choice(list(conns)))
23- for conns in self.connections.values()
24+ common.Client(random.choice(list(connections)))
25+ for connections in self.connections.values()
26+ if len(connections) > 0
27 ]
28
29 @asynchronous(timeout=FOREVER)
30 def getRandomClient(self):
31- """Return a list of all connected :class:`common.Client`s."""
32- clients = list(self.connections.values())
33- if len(clients) == 0:
34+ """Return a random connected :class:`common.Client`."""
35+ connections = list(self.connections.values())
36+ if len(connections) == 0:
37 raise exceptions.NoConnectionsAvailable(
38 "Unable to connect to any rack controller; no connections "
39 "available.")
40 else:
41- conns = list(random.choice(clients))
42- return common.Client(random.choice(conns))
43+ connection = random.choice(connections)
44+ # The connection object is a set of RegionServer objects.
45+ # Make sure a sane set was returned.
46+ assert len(connection) > 0, "Connection set empty."
47+ return common.Client(random.choice(list(connection)))
48
49
50 def ignoreCancellation(failure):
51diff --git a/src/maasserver/rpc/tests/test_regionservice.py b/src/maasserver/rpc/tests/test_regionservice.py
52index e845b30..972a3d7 100644
53--- a/src/maasserver/rpc/tests/test_regionservice.py
54+++ b/src/maasserver/rpc/tests/test_regionservice.py
55@@ -80,7 +80,10 @@ from provisioningserver.rpc import (
56 common,
57 exceptions,
58 )
59-from provisioningserver.rpc.exceptions import CannotRegisterRackController
60+from provisioningserver.rpc.exceptions import (
61+ CannotRegisterRackController,
62+ NoConnectionsAvailable,
63+)
64 from provisioningserver.rpc.interfaces import IConnection
65 from provisioningserver.rpc.region import RegisterRackController
66 from provisioningserver.rpc.testing import call_responder
67@@ -91,6 +94,7 @@ from provisioningserver.utils.twisted import (
68 callInReactorWithTimeout,
69 DeferredValue,
70 )
71+from testtools import ExpectedException
72 from testtools.deferredruntest import assert_fails_with
73 from testtools.matchers import (
74 AfterPreprocessing,
75@@ -875,6 +879,21 @@ class TestRegionService(MAASTestCase):
76 self.assertThat(service.getAllClients(), Equals([]))
77
78 @wait_for_reactor
79+ def test_getAllClients_empty_connections(self):
80+ service = RegionService(sentinel.advertiser)
81+ service.connections.clear()
82+ uuid1 = factory.make_UUID()
83+ service.connections[uuid1] = set()
84+ self.assertThat(service.getAllClients(), Equals([]))
85+
86+ @wait_for_reactor
87+ def test_getRandomClient_empty_raises_NoConnectionsAvailable(self):
88+ service = RegionService(sentinel.advertiser)
89+ service.connections.clear()
90+ with ExpectedException(NoConnectionsAvailable):
91+ service.getRandomClient()
92+
93+ @wait_for_reactor
94 def test_getAllClients(self):
95 service = RegionService(sentinel.advertiser)
96 uuid1 = factory.make_UUID()

Subscribers

People subscribed via source and target branches

to all changes: