Merge lp:~newell-jensen/maas/fix-1596046 into lp:maas/trunk

Proposed by Newell Jensen on 2016-06-29
Status: Merged
Approved by: Newell Jensen on 2016-06-30
Approved revision: 5147
Merged at revision: 5149
Proposed branch: lp:~newell-jensen/maas/fix-1596046
Merge into: lp:maas/trunk
Diff against target: 264 lines (+94/-48)
4 files modified
src/provisioningserver/boot/powernv.py (+28/-16)
src/provisioningserver/boot/tests/test_powernv.py (+29/-29)
src/provisioningserver/pserv_services/tests/test_tftp.py (+25/-0)
src/provisioningserver/pserv_services/tftp.py (+12/-3)
To merge this branch: bzr merge lp:~newell-jensen/maas/fix-1596046
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2016-06-29 Approve on 2016-06-30
Review via email: mp+298690@code.launchpad.net

Commit message

This branch fixes a regression for powerNV where the "b'ppc64el/" was getting hardcoded into the dhcpd.conf. Additionally, it adds `path` to GetBootConfig (new in MAAS 2.0) on the rack, which was left out and needed.

To post a comment you must log in.
Newell Jensen (newell-jensen) wrote :

This was tested on powerNV hardware in 1ss. The nodes are now able to enlist, commission, and deploy with this fix.

Gavin Panella (allenap) wrote :

m.rpc.boot.get_config, the eventual receiver of the added "path"
argument, doesn't actually use it.

I'm about 95% sure that it should not be passed. The arguments to
GetBootConfig are those _derived_ from the TFTP request; a raw path
should not be amongst them.

I think the bug is that PowerNVBootMethod.get_params() should not be
returning a dict with a "path" key. What do you think?

review: Needs Information
Newell Jensen (newell-jensen) wrote :

After looking at it, I agree. I didn't originally write powernv.py but
when I looke through the other boot methods, I don't see them passing
"path" to the paraemters. Will change this and test again.

On Wed, Jun 29, 2016 at 11:41 AM, Gavin Panella <<email address hidden>
> wrote:

> Review: Needs Information
>
> m.rpc.boot.get_config, the eventual receiver of the added "path"
> argument, doesn't actually use it.
>
> I'm about 95% sure that it should not be passed. The arguments to
> GetBootConfig are those _derived_ from the TFTP request; a raw path
> should not be amongst them.
>
> I think the bug is that PowerNVBootMethod.get_params() should not be
> returning a dict with a "path" key. What do you think?
>
> --
> https://code.launchpad.net/~newell-jensen/maas/fix-1596046/+merge/298690
> You are the owner of lp:~newell-jensen/maas/fix-1596046.
>

Newell Jensen (newell-jensen) wrote :

Gavin,

So powerNV needs 'path' for its get_reader method. However, I was able to strip the need of modifying the RPC calls. Have another look. Thanks.

Gavin Panella (allenap) wrote :

This looks good but I suggest solving it by selecting arguments to pass to GetBootConfig instead of killing off errant keys.

PowerNV also trims the prefix in two different places, which seems wasteful, and confusing when reading the code.

I've fixed both of these in lp:~allenap/maas/newell-jensen--fix-1596046. If you pull that into your branch you can see what I've fixed in the last couple of revisions.

lp:~newell-jensen/maas/fix-1596046 updated on 2016-06-30
5146. By Newell Jensen on 2016-06-30

Fix merge branch issues (bytes to str). Add back path_prefix as this is needed for dhcpd.conf entry which affects file path requested by the client.

Newell Jensen (newell-jensen) wrote :

Gavin,

Merged your branch, tested it on the hardware, fixed the issues, and pushed again. Have a look again when you can, thanks.

Gavin Panella (allenap) wrote :

Looks good. I'm glad you figured out why it wasn't working. One comment to check out; it won't hold you up long.

review: Approve
lp:~newell-jensen/maas/fix-1596046 updated on 2016-06-30
5147. By Newell Jensen on 2016-06-30

Update branch with some minor fixes from review.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/boot/powernv.py'
2--- src/provisioningserver/boot/powernv.py 2016-03-28 13:54:47 +0000
3+++ src/provisioningserver/boot/powernv.py 2016-06-30 19:11:09 +0000
4@@ -49,6 +49,20 @@
5 re_config_file = re_config_file.encode("ascii")
6 re_config_file = re.compile(re_config_file, re.VERBOSE)
7
8+# Due to the "ppc64el" prefix all files requested from the client using
9+# relative paths will have that prefix. Capturing the path after that prefix
10+# will give us the correct path in the local tftp root on disk.
11+re_other_file = r'''
12+ # Optional leading slash(es).
13+ ^/*
14+ ppc64el # PowerNV PXE prefix, set by dhcpd.
15+ /
16+ (?P<path>.+) # Capture path.
17+ $
18+'''
19+re_other_file = re_other_file.encode("ascii")
20+re_other_file = re.compile(re_other_file, re.VERBOSE)
21+
22
23 def format_bootif(mac):
24 """Formats a mac address into the BOOTIF format, expected by
25@@ -65,15 +79,16 @@
26 template_subdir = "pxe"
27 bootloader_path = "pxelinux.0"
28 arch_octet = "00:0E"
29- path_prefix = b"ppc64el/"
30+ path_prefix = "ppc64el/"
31
32 def get_params(self, backend, path):
33 """Gets the matching parameters from the requested path."""
34 match = re_config_file.match(path)
35 if match is not None:
36 return get_parameters(match)
37- if path.lstrip(b'/').startswith(self.path_prefix):
38- return {'path': path}
39+ match = re_other_file.match(path)
40+ if match is not None:
41+ return get_parameters(match)
42 return None
43
44 def match_path(self, backend, path):
45@@ -94,23 +109,22 @@
46 params['mac'] = mac
47 return params
48
49- def get_reader(self, backend, kernel_params, **extra):
50+ def get_reader(self, backend, kernel_params, mac=None, path=None, **extra):
51 """Render a configuration file as a unicode string.
52
53 :param backend: requesting backend
54 :param kernel_params: An instance of `KernelParameters`.
55+ :param path: Optional MAC address discovered by `match_path`.
56+ :param path: Optional path discovered by `match_path`.
57 :param extra: Allow for other arguments. This is a safety valve;
58 parameters generated in another component (for example, see
59 `TFTPBackend.get_config_reader`) won't cause this to break.
60 """
61- # Due to the path prefix, all requested files from the client will
62- # contain that prefix. Removing the prefix from the path will return
63- # the correct path in the tftp root.
64- if 'path' in extra:
65- path = extra['path']
66- if path.startswith(self.path_prefix):
67- path = path[len(self.path_prefix):]
68- target_path = backend.base.descendant(path.split(b'/'))
69+ if path is not None:
70+ # This is a request for a static file, not a configuration file.
71+ # The prefix was already trimmed by `match_path` so we need only
72+ # return a FilesystemReader for `path` beneath the backend's base.
73+ target_path = backend.base.descendant(path.split('/'))
74 return FilesystemReader(target_path)
75
76 # Return empty config for PowerNV local. PowerNV fails to
77@@ -128,10 +142,8 @@
78 # support the IPAPPEND pxelinux flag.
79 def kernel_command(params):
80 cmd_line = compose_kernel_command_line(params)
81- if 'mac' in extra:
82- mac = extra['mac']
83- mac = format_bootif(mac)
84- return '%s BOOTIF=%s' % (cmd_line, mac)
85+ if mac is not None:
86+ return '%s BOOTIF=%s' % (cmd_line, format_bootif(mac))
87 return cmd_line
88
89 namespace['kernel_command'] = kernel_command
90
91=== modified file 'src/provisioningserver/boot/tests/test_powernv.py'
92--- src/provisioningserver/boot/tests/test_powernv.py 2016-06-22 17:03:02 +0000
93+++ src/provisioningserver/boot/tests/test_powernv.py 2016-06-30 19:11:09 +0000
94@@ -5,7 +5,6 @@
95
96 __all__ = []
97
98-import os
99 import re
100 from unittest.mock import Mock
101
102@@ -114,7 +113,7 @@
103
104 def test_path_prefix(self):
105 method = PowerNVBootMethod()
106- self.assertEqual(b'ppc64el/', method.path_prefix)
107+ self.assertEqual('ppc64el/', method.path_prefix)
108
109
110 class TestPowerNVBootMethodMatchPath(MAASTestCase):
111@@ -150,7 +149,8 @@
112 expected = {
113 'arch': 'ppc64el',
114 'mac': fake_mac,
115- 'path': file_path,
116+ # The "ppc64el/" prefix has been removed from the path.
117+ 'path': file_path.decode("utf-8")[8:],
118 }
119 self.assertEqual(expected, params)
120
121@@ -242,41 +242,41 @@
122
123
124 class TestPowerNVBootMethodPathPrefix(MAASTestCase):
125- """Tests for
126- `provisioningserver.boot.powernv.PowerNVBootMethod.get_reader`.
127- """
128-
129- def test_get_reader_path_prefix(self):
130+ """Tests for `provisioningserver.boot.powernv.PowerNVBootMethod`."""
131+
132+ def test_path_prefix_removed(self):
133+ temp_dir = FilePath(self.make_dir())
134+ backend = Mock(base=temp_dir) # A `TFTPBackend`.
135+
136+ # Create a file in the backend's base directory.
137 data = factory.make_string().encode("ascii")
138- temp_file = self.make_file(name="example", contents=data)
139- temp_dir = os.path.dirname(temp_file)
140- backend = Mock(base=FilePath(temp_dir)) # A `TFTPBackend`.
141+ temp_file = temp_dir.child("example")
142+ temp_file.setContent(data)
143+
144 method = PowerNVBootMethod()
145- options = {
146- 'backend': backend,
147- 'kernel_params': make_kernel_parameters(),
148- 'path': b'ppc64el/example',
149- }
150- reader = method.get_reader(**options)
151+ params = method.get_params(backend, b"ppc64el/example")
152+ self.assertEqual({"path": "example"}, params)
153+ reader = method.get_reader(backend, make_kernel_parameters(), **params)
154 self.addCleanup(reader.finish)
155 self.assertEqual(len(data), reader.size)
156 self.assertEqual(data, reader.read(len(data)))
157 self.assertEqual(b"", reader.read(1))
158
159- def test_get_reader_path_prefix_only_removes_first_occurrence(self):
160+ def test_path_prefix_only_first_occurrence_removed(self):
161+ temp_dir = FilePath(self.make_dir())
162+ backend = Mock(base=temp_dir) # A `TFTPBackend`.
163+
164+ # Create a file nested within a "ppc64el" directory.
165 data = factory.make_string().encode("ascii")
166- temp_dir = self.make_dir()
167- temp_subdir = os.path.join(temp_dir, 'ppc64el')
168- os.mkdir(temp_subdir)
169- factory.make_file(temp_subdir, "example", data)
170- backend = Mock(base=FilePath(temp_dir)) # A `TFTPBackend`.
171+ temp_subdir = temp_dir.child("ppc64el")
172+ temp_subdir.createDirectory()
173+ temp_file = temp_subdir.child("example")
174+ temp_file.setContent(data)
175+
176 method = PowerNVBootMethod()
177- options = {
178- 'backend': backend,
179- 'kernel_params': make_kernel_parameters(),
180- 'path': b'ppc64el/ppc64el/example',
181- }
182- reader = method.get_reader(**options)
183+ params = method.get_params(backend, b"ppc64el/ppc64el/example")
184+ self.assertEqual({"path": "ppc64el/example"}, params)
185+ reader = method.get_reader(backend, make_kernel_parameters(), **params)
186 self.addCleanup(reader.finish)
187 self.assertEqual(len(data), reader.size)
188 self.assertEqual(data, reader.read(len(data)))
189
190=== modified file 'src/provisioningserver/pserv_services/tests/test_tftp.py'
191--- src/provisioningserver/pserv_services/tests/test_tftp.py 2016-05-12 19:07:37 +0000
192+++ src/provisioningserver/pserv_services/tests/test_tftp.py 2016-06-30 19:11:09 +0000
193@@ -49,6 +49,7 @@
194 UDPServer,
195 )
196 from provisioningserver.rpc.exceptions import BootConfigNoResponse
197+from provisioningserver.rpc.region import GetBootConfig
198 from provisioningserver.testing.boot_images import (
199 make_boot_image_params,
200 make_image,
201@@ -504,6 +505,30 @@
202 # ArchitectureRegistry is not stable, so we permit either here.
203 self.assertIn(observed_params["arch"], ["armhf", "arm64"])
204
205+ def test_get_kernel_params_filters_out_unnecessary_arguments(self):
206+ params_okay = {
207+ name.decode("ascii"): factory.make_name("value")
208+ for name, _ in GetBootConfig.arguments
209+ }
210+ params_other = {
211+ factory.make_name("name"): factory.make_name("value")
212+ for _ in range(3)
213+ }
214+ params_all = params_okay.copy()
215+ params_all.update(params_other)
216+
217+ client_service = Mock()
218+ client = client_service.getClient.return_value
219+ client.localIdent = params_okay["system_id"]
220+ backend = TFTPBackend(self.make_dir(), client_service)
221+ backend.fetcher = Mock()
222+
223+ backend.get_kernel_params(params_all)
224+
225+ self.assertThat(
226+ backend.fetcher, MockCalledOnceWith(
227+ client, GetBootConfig, **params_okay))
228+
229
230 class TestTFTPService(MAASTestCase):
231
232
233=== modified file 'src/provisioningserver/pserv_services/tftp.py'
234--- src/provisioningserver/pserv_services/tftp.py 2016-03-28 13:54:47 +0000
235+++ src/provisioningserver/pserv_services/tftp.py 2016-06-30 19:11:09 +0000
236@@ -204,6 +204,17 @@
237 path requested.
238 :return: A `KernelParameters` instance.
239 """
240+ # Extract from params only those arguments that GetBootConfig cares
241+ # about; params is a context-like object and other stuff (too much?)
242+ # gets in there.
243+ arguments = (
244+ name.decode("ascii")
245+ for name, _ in GetBootConfig.arguments
246+ )
247+ params = {
248+ name: params[name] for name in arguments
249+ if name in params
250+ }
251 client = self.client_service.getClient()
252 params["system_id"] = client.localIdent
253 d = self.fetcher(client, GetBootConfig, **params)
254@@ -223,9 +234,7 @@
255 return boot_method.get_reader(
256 self, kernel_params=kernel_params, **params)
257
258- d = self.get_kernel_params(params)
259- d.addCallback(generate)
260- return d
261+ return self.get_kernel_params(params).addCallback(generate)
262
263 @staticmethod
264 def no_response_errback(failure, file_name):