Merge lp:~allenap/maas/pass-addresses-to-tftp-backend into lp:maas/trunk

Proposed by Gavin Panella on 2012-10-30
Status: Merged
Approved by: Gavin Panella on 2012-10-30
Approved revision: 1312
Merged at revision: 1315
Proposed branch: lp:~allenap/maas/pass-addresses-to-tftp-backend
Merge into: lp:maas/trunk
Diff against target: 313 lines (+142/-14)
6 files modified
contrib/python-tx-tftp/tftp/protocol.py (+14/-2)
contrib/python-tx-tftp/tftp/test/test_protocol.py (+73/-2)
src/maasserver/api.py (+4/-1)
src/maasserver/tests/test_api.py (+25/-7)
src/provisioningserver/tests/test_tftp.py (+20/-2)
src/provisioningserver/tftp.py (+6/-0)
To merge this branch: bzr merge lp:~allenap/maas/pass-addresses-to-tftp-backend
Reviewer Review Type Date Requested Status
Julian Edwards (community) 2012-10-30 Approve on 2012-10-30
Review via email: mp+132015@code.launchpad.net

Commit message

Use the node-facing address of the cluster controller as the iSCSI host.

The cluster and node addresses are passed over to the pxeconfig view.

To post a comment you must log in.
1312. By Gavin Panella on 2012-10-30

Don't conditionally pass the local and remote addresses to pxeconfig, always pass them.

Julian Edwards (julian-edwards) wrote :

306 + # Send the local and remote endpoint addresses.
307 + local_host, local_port = get("local", (None, None))
308 + if local_host is not None:
309 + params["local"] = local_host
310 + remote_host, remote_port = get("remote", (None, None))
311 + if remote_host is not None:
312 + params["remote"] = remote_host

As discussed, get rid of the conditionals here, just try to poke in the IP data since it's mandatory in the API. If it fails, we want to fail early.

Everything else looks tip top!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'contrib/python-tx-tftp/tftp/protocol.py'
2--- contrib/python-tx-tftp/tftp/protocol.py 2012-07-05 14:36:27 +0000
3+++ contrib/python-tx-tftp/tftp/protocol.py 2012-10-30 09:11:22 +0000
4@@ -12,6 +12,7 @@
5 from twisted.internet.defer import inlineCallbacks, returnValue
6 from twisted.internet.protocol import DatagramProtocol
7 from twisted.python import log
8+from twisted.python.context import call
9
10
11 class TFTP(DatagramProtocol):
12@@ -48,11 +49,22 @@
13
14 @inlineCallbacks
15 def _startSession(self, datagram, addr, mode):
16+ # Set up a call context so that we can pass extra arbitrary
17+ # information to interested backends without adding extra call
18+ # arguments, or switching to using a request object, for example.
19+ context = {}
20+ if self.transport is not None:
21+ # Add the local and remote addresses to the call context.
22+ local = self.transport.getHost()
23+ context["local"] = local.host, local.port
24+ context["remote"] = addr
25 try:
26 if datagram.opcode == OP_WRQ:
27- fs_interface = yield self.backend.get_writer(datagram.filename)
28+ fs_interface = yield call(
29+ context, self.backend.get_writer, datagram.filename)
30 elif datagram.opcode == OP_RRQ:
31- fs_interface = yield self.backend.get_reader(datagram.filename)
32+ fs_interface = yield call(
33+ context, self.backend.get_reader, datagram.filename)
34 except Unsupported, e:
35 self.transport.write(ERRORDatagram.from_code(ERR_ILLEGAL_OP,
36 str(e)).to_wire(), addr)
37
38=== modified file 'contrib/python-tx-tftp/tftp/test/test_protocol.py'
39--- contrib/python-tx-tftp/tftp/test/test_protocol.py 2012-07-25 19:59:37 +0000
40+++ contrib/python-tx-tftp/tftp/test/test_protocol.py 2012-10-30 09:11:22 +0000
41@@ -11,9 +11,11 @@
42 from tftp.netascii import NetasciiReceiverProxy, NetasciiSenderProxy
43 from tftp.protocol import TFTP
44 from twisted.internet import reactor
45-from twisted.internet.defer import Deferred
46+from twisted.internet.address import IPv4Address
47+from twisted.internet.defer import Deferred, inlineCallbacks
48 from twisted.internet.protocol import DatagramProtocol
49 from twisted.internet.task import Clock
50+from twisted.python import context
51 from twisted.python.filepath import FilePath
52 from twisted.test.proto_helpers import StringTransport
53 from twisted.trial import unittest
54@@ -50,7 +52,8 @@
55
56 def setUp(self):
57 self.clock = Clock()
58- self.transport = FakeTransport(hostAddress=('127.0.0.1', self.port))
59+ self.transport = FakeTransport(
60+ hostAddress=IPv4Address('UDP', '127.0.0.1', self.port))
61
62 def test_malformed_datagram(self):
63 tftp = TFTP(BackendFactory(), _clock=self.clock)
64@@ -247,3 +250,71 @@
65 self.clock.advance(1)
66 self.assertTrue(d.called)
67 self.assertTrue(IWriter.providedBy(d.result.backend))
68+
69+
70+class CapturedContext(Exception):
71+ """A donkey, to carry the call context back up the stack."""
72+
73+ def __init__(self, args, names):
74+ super(CapturedContext, self).__init__(*args)
75+ self.context = {name: context.get(name) for name in names}
76+
77+
78+class ContextCapturingBackend(object):
79+ """A fake `IBackend` that raises `CapturedContext`.
80+
81+ Calling `get_reader` or `get_writer` raises a `CapturedContext` exception,
82+ which captures the values of the call context for the given `names`.
83+ """
84+
85+ def __init__(self, *names):
86+ self.names = names
87+
88+ def get_reader(self, file_name):
89+ raise CapturedContext(("get_reader", file_name), self.names)
90+
91+ def get_writer(self, file_name):
92+ raise CapturedContext(("get_writer", file_name), self.names)
93+
94+
95+class HostTransport(object):
96+ """A fake `ITransport` that only responds to `getHost`."""
97+
98+ def __init__(self, host):
99+ self.host = host
100+
101+ def getHost(self):
102+ return IPv4Address("UDP", *self.host)
103+
104+
105+class BackendCallingContext(unittest.TestCase):
106+
107+ def setUp(self):
108+ super(BackendCallingContext, self).setUp()
109+ self.backend = ContextCapturingBackend("local", "remote")
110+ self.tftp = TFTP(self.backend)
111+ self.tftp.transport = HostTransport(("12.34.56.78", 1234))
112+
113+ @inlineCallbacks
114+ def test_context_rrq(self):
115+ rrq_datagram = RRQDatagram('nonempty', 'NetASCiI', {})
116+ rrq_addr = ('127.0.0.1', 1069)
117+ error = yield self.assertFailure(
118+ self.tftp._startSession(rrq_datagram, rrq_addr, "octet"),
119+ CapturedContext)
120+ self.assertEqual(("get_reader", rrq_datagram.filename), error.args)
121+ self.assertEqual(
122+ {"local": self.tftp.transport.host, "remote": rrq_addr},
123+ error.context)
124+
125+ @inlineCallbacks
126+ def test_context_wrq(self):
127+ wrq_datagram = WRQDatagram('nonempty', 'NetASCiI', {})
128+ wrq_addr = ('127.0.0.1', 1069)
129+ error = yield self.assertFailure(
130+ self.tftp._startSession(wrq_datagram, wrq_addr, "octet"),
131+ CapturedContext)
132+ self.assertEqual(("get_writer", wrq_datagram.filename), error.args)
133+ self.assertEqual(
134+ {"local": self.tftp.transport.host, "remote": wrq_addr},
135+ error.context)
136
137=== modified file 'src/maasserver/api.py'
138--- src/maasserver/api.py 2012-10-26 10:56:06 +0000
139+++ src/maasserver/api.py 2012-10-30 09:11:22 +0000
140@@ -1792,6 +1792,8 @@
141 :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
142 'armhf').
143 :param subarch: Subarchitecture name (in the pxelinux namespace).
144+ :param local: The IP address of the cluster controller.
145+ :param remote: The IP address of the booting node.
146 """
147 node = get_node_from_mac_string(request.GET.get('mac', None))
148
149@@ -1846,11 +1848,12 @@
150
151 purpose = get_boot_purpose(node)
152 server_address = get_maas_facing_server_address()
153+ cluster_address = get_mandatory_param(request.GET, "local")
154
155 params = KernelParameters(
156 arch=arch, subarch=subarch, release=series, purpose=purpose,
157 hostname=hostname, domain=domain, preseed_url=preseed_url,
158- log_host=server_address, fs_host=server_address)
159+ log_host=server_address, fs_host=cluster_address)
160
161 return HttpResponse(
162 json.dumps(params._asdict()),
163
164=== modified file 'src/maasserver/tests/test_api.py'
165--- src/maasserver/tests/test_api.py 2012-10-26 10:56:06 +0000
166+++ src/maasserver/tests/test_api.py 2012-10-30 09:11:22 +0000
167@@ -3202,11 +3202,16 @@
168
169 class TestPXEConfigAPI(AnonAPITestCase):
170
171+ def get_default_params(self):
172+ return {
173+ "local": factory.getRandomIPAddress(),
174+ "remote": factory.getRandomIPAddress(),
175+ }
176+
177 def get_mac_params(self):
178- return {'mac': factory.make_mac_address().mac_address}
179-
180- def get_default_params(self):
181- return dict()
182+ params = self.get_default_params()
183+ params['mac'] = factory.make_mac_address().mac_address
184+ return params
185
186 def get_pxeconfig(self, params=None):
187 """Make a request to `pxeconfig`, and return its response dict."""
188@@ -3240,7 +3245,8 @@
189 ContainsAll(KernelParameters._fields))
190
191 def test_pxeconfig_returns_data_for_known_node(self):
192- response = self.client.get(reverse('pxeconfig'), self.get_mac_params())
193+ params = self.get_mac_params()
194+ response = self.client.get(reverse('pxeconfig'), params)
195 self.assertEqual(httplib.OK, response.status_code)
196
197 def test_pxeconfig_returns_no_content_for_unknown_node(self):
198@@ -3252,6 +3258,7 @@
199 architecture = factory.getRandomEnum(ARCHITECTURE)
200 arch, subarch = architecture.split('/')
201 params = dict(
202+ self.get_default_params(),
203 mac=factory.getRandomMACAddress(delimiter=b'-'),
204 arch=arch,
205 subarch=subarch)
206@@ -3280,15 +3287,19 @@
207 full_hostname = '.'.join([host, domain])
208 node = factory.make_node(hostname=full_hostname)
209 mac = factory.make_mac_address(node=node)
210- pxe_config = self.get_pxeconfig(params={'mac': mac.mac_address})
211+ params = self.get_default_params()
212+ params['mac'] = mac.mac_address
213+ pxe_config = self.get_pxeconfig(params)
214 self.assertEqual(host, pxe_config.get('hostname'))
215 self.assertNotIn(domain, pxe_config.values())
216
217 def test_pxeconfig_uses_nodegroup_domain_for_node(self):
218 mac = factory.make_mac_address()
219+ params = self.get_default_params()
220+ params['mac'] = mac
221 self.assertEqual(
222 mac.node.nodegroup.name,
223- self.get_pxeconfig({'mac': mac.mac_address}).get('domain'))
224+ self.get_pxeconfig(params).get('domain'))
225
226 def get_without_param(self, param):
227 """Request a `pxeconfig()` response, but omit `param` from request."""
228@@ -3353,6 +3364,13 @@
229 fake_boot_purpose,
230 json.loads(response.content)["purpose"])
231
232+ def test_pxeconfig_returns_fs_host_as_cluster_controller(self):
233+ # The kernel parameter `fs_host` points to the cluster controller
234+ # address, which is passed over within the `local` parameter.
235+ params = self.get_default_params()
236+ kernel_params = KernelParameters(**self.get_pxeconfig(params))
237+ self.assertEqual(params["local"], kernel_params.fs_host)
238+
239
240 class TestNodeGroupsAPI(APIv10TestMixin, MultipleUsersScenarios, TestCase):
241 scenarios = [
242
243=== modified file 'src/provisioningserver/tests/test_tftp.py'
244--- src/provisioningserver/tests/test_tftp.py 2012-10-02 10:53:22 +0000
245+++ src/provisioningserver/tests/test_tftp.py 2012-10-30 09:11:22 +0000
246@@ -35,6 +35,7 @@
247 inlineCallbacks,
248 succeed,
249 )
250+from twisted.python import context
251 from zope.interface.verify import verifyObject
252
253
254@@ -213,6 +214,16 @@
255 mac = factory.getRandomMACAddress(b"-")
256 config_path = compose_config_path(mac)
257 backend = TFTPBackend(self.make_dir(), b"http://example.com/")
258+ # python-tx-tftp sets up call context so that backends can discover
259+ # more about the environment in which they're running.
260+ call_context = {
261+ "local": (
262+ factory.getRandomIPAddress(),
263+ factory.getRandomPort()),
264+ "remote": (
265+ factory.getRandomIPAddress(),
266+ factory.getRandomPort()),
267+ }
268
269 @partial(self.patch, backend, "get_config_reader")
270 def get_config_reader(params):
271@@ -220,9 +231,16 @@
272 params_json_reader = BytesReader(params_json)
273 return succeed(params_json_reader)
274
275- reader = yield backend.get_reader(config_path)
276+ reader = yield context.call(
277+ call_context, backend.get_reader, config_path)
278 output = reader.read(10000)
279- expected_params = dict(mac=mac)
280+ # The addresses provided by python-tx-tftp in the call context are
281+ # passed over the wire as address:port strings.
282+ expected_params = {
283+ "mac": mac,
284+ "local": call_context["local"][0], # address only.
285+ "remote": call_context["remote"][0], # address only.
286+ }
287 observed_params = json.loads(output)
288 self.assertEqual(expected_params, observed_params)
289
290
291=== modified file 'src/provisioningserver/tftp.py'
292--- src/provisioningserver/tftp.py 2012-10-03 08:50:17 +0000
293+++ src/provisioningserver/tftp.py 2012-10-30 09:11:22 +0000
294@@ -34,6 +34,7 @@
295 IReader,
296 )
297 from tftp.errors import FileNotFound
298+from twisted.python.context import get
299 from twisted.web.client import getPage
300 import twisted.web.error
301 from zope.interface import implementer
302@@ -211,6 +212,11 @@
303 for key, value in config_file_match.groupdict().items()
304 if value is not None
305 }
306+ # Send the local and remote endpoint addresses.
307+ local_host, local_port = get("local", (None, None))
308+ params["local"] = local_host
309+ remote_host, remote_port = get("remote", (None, None))
310+ params["remote"] = remote_host
311 d = self.get_config_reader(params)
312 d.addErrback(self.get_page_errback, file_name)
313 return d