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

Proposed by Mike Pontillo on 2017-07-19
Status: Merged
Approved by: Mike Pontillo on 2017-07-19
Approved revision: 462a87f5e22fbd7707da860f1c47473eb879f864
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:fix-random-choice-no-choices--2.2
Merge into: maas:2.2
Diff against target: 117 lines (+36/-11)
4 files modified
docs/changelog.rst (+3/-1)
requirements.txt (+2/-2)
src/maasserver/rpc/regionservice.py (+11/-7)
src/maasserver/rpc/tests/test_regionservice.py (+20/-1)
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve on 2017-07-19
Review via email: mp+327716@code.launchpad.net

Commit message

Prevent RegionService from using empty lists.

LP: #1705254
Backports: 3dd8eedfba921760f98ea54805d05c86e63266ec

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

Self-approve backport.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/docs/changelog.rst b/docs/changelog.rst
2index 7eaaeb7..abea670 100644
3--- a/docs/changelog.rst
4+++ b/docs/changelog.rst
5@@ -28,7 +28,9 @@ LP: #1703845 Set the re-check interval for rack to region RPC connections to
6
7 LP: #1703403 Lower the maximum number of connections per worker to 9 connections is allowed as 1 is reserved.
8
9-LP: #1704489 [2.2] Duplicate communication is occurring to the same rack controller
10+LP: #1704489 Duplicate communication is occurring to the same rack controller
11+
12+LP: #1705254 RegionService can raise unexpected exceptions for an empty connection list
13
14
15 2.2.1
16diff --git a/requirements.txt b/requirements.txt
17index ae87b46..098e026 100644
18--- a/requirements.txt
19+++ b/requirements.txt
20@@ -1,4 +1,3 @@
21-bson==0.4.3
22 convoy==0.4.4
23 crochet==1.5.0
24 django==1.8
25@@ -13,7 +12,8 @@ paramiko==1.16.3
26 petname==2.0
27 pexpect==4.2.0
28 psycopg2==2.6.2
29-pyopenssl==16.0.0
30+pymongo==3.2
31+pyopenssl==16.2.0
32 pyparsing==2.0.7
33 pyvmomi==6.0.0.2016.6
34 requests==2.11.0
35diff --git a/src/maasserver/rpc/regionservice.py b/src/maasserver/rpc/regionservice.py
36index a68b038..4d962e8 100644
37--- a/src/maasserver/rpc/regionservice.py
38+++ b/src/maasserver/rpc/regionservice.py
39@@ -967,21 +967,25 @@ class RegionService(service.Service, object):
40 def getAllClients(self):
41 """Return a list with one connection per rack controller."""
42 return [
43- common.Client(random.choice(list(conns)))
44- for conns in self.connections.values()
45+ common.Client(random.choice(list(connections)))
46+ for connections in self.connections.values()
47+ if len(connections) > 0
48 ]
49
50 @asynchronous(timeout=FOREVER)
51 def getRandomClient(self):
52- """Return a list of all connected :class:`common.Client`s."""
53- clients = list(self.connections.values())
54- if len(clients) == 0:
55+ """Return a random connected :class:`common.Client`."""
56+ connections = list(self.connections.values())
57+ if len(connections) == 0:
58 raise exceptions.NoConnectionsAvailable(
59 "Unable to connect to any rack controller; no connections "
60 "available.")
61 else:
62- conns = list(random.choice(clients))
63- return common.Client(random.choice(conns))
64+ connection = random.choice(connections)
65+ # The connection object is a set of RegionServer objects.
66+ # Make sure a sane set was returned.
67+ assert len(connection) > 0, "Connection set empty."
68+ return common.Client(random.choice(list(connection)))
69
70
71 def ignoreCancellation(failure):
72diff --git a/src/maasserver/rpc/tests/test_regionservice.py b/src/maasserver/rpc/tests/test_regionservice.py
73index e845b30..972a3d7 100644
74--- a/src/maasserver/rpc/tests/test_regionservice.py
75+++ b/src/maasserver/rpc/tests/test_regionservice.py
76@@ -80,7 +80,10 @@ from provisioningserver.rpc import (
77 common,
78 exceptions,
79 )
80-from provisioningserver.rpc.exceptions import CannotRegisterRackController
81+from provisioningserver.rpc.exceptions import (
82+ CannotRegisterRackController,
83+ NoConnectionsAvailable,
84+)
85 from provisioningserver.rpc.interfaces import IConnection
86 from provisioningserver.rpc.region import RegisterRackController
87 from provisioningserver.rpc.testing import call_responder
88@@ -91,6 +94,7 @@ from provisioningserver.utils.twisted import (
89 callInReactorWithTimeout,
90 DeferredValue,
91 )
92+from testtools import ExpectedException
93 from testtools.deferredruntest import assert_fails_with
94 from testtools.matchers import (
95 AfterPreprocessing,
96@@ -875,6 +879,21 @@ class TestRegionService(MAASTestCase):
97 self.assertThat(service.getAllClients(), Equals([]))
98
99 @wait_for_reactor
100+ def test_getAllClients_empty_connections(self):
101+ service = RegionService(sentinel.advertiser)
102+ service.connections.clear()
103+ uuid1 = factory.make_UUID()
104+ service.connections[uuid1] = set()
105+ self.assertThat(service.getAllClients(), Equals([]))
106+
107+ @wait_for_reactor
108+ def test_getRandomClient_empty_raises_NoConnectionsAvailable(self):
109+ service = RegionService(sentinel.advertiser)
110+ service.connections.clear()
111+ with ExpectedException(NoConnectionsAvailable):
112+ service.getRandomClient()
113+
114+ @wait_for_reactor
115 def test_getAllClients(self):
116 service = RegionService(sentinel.advertiser)
117 uuid1 = factory.make_UUID()

Subscribers

People subscribed via source and target branches

to all changes: