Merge ~smoser/cloud-init:bug/noise-on-cmdline-url-fail into cloud-init:master
- Git
- lp:~smoser/cloud-init
- bug/noise-on-cmdline-url-fail
- Merge into master
Proposed by
Scott Moser
Status: | Merged |
---|---|
Merged at revision: | a1b185d0cce5064e9b36b4db7b55564e2ab1d7a8 |
Proposed branch: | ~smoser/cloud-init:bug/noise-on-cmdline-url-fail |
Merge into: | cloud-init:master |
Diff against target: |
377 lines (+173/-97) 4 files modified
cloudinit/cmd/main.py (+103/-15) cloudinit/util.py (+0/-44) tests/unittests/helpers.py (+11/-5) tests/unittests/test__init__.py (+59/-33) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
cloud-init Commiters | Pending | ||
Review via email: mp+311549@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) : | # |
Revision history for this message
Scott Moser (smoser) wrote : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py |
2 | index c83496c..65b15ed 100644 |
3 | --- a/cloudinit/cmd/main.py |
4 | +++ b/cloudinit/cmd/main.py |
5 | @@ -26,6 +26,7 @@ from cloudinit import signal_handler |
6 | from cloudinit import sources |
7 | from cloudinit import stages |
8 | from cloudinit import templater |
9 | +from cloudinit import url_helper |
10 | from cloudinit import util |
11 | from cloudinit import version |
12 | |
13 | @@ -129,23 +130,104 @@ def apply_reporting_cfg(cfg): |
14 | reporting.update_configuration(cfg.get('reporting')) |
15 | |
16 | |
17 | +def parse_cmdline_url(cmdline, names=('cloud-config-url', 'url')): |
18 | + data = util.keyval_str_to_dict(cmdline) |
19 | + for key in names: |
20 | + if key in data: |
21 | + return key, data[key] |
22 | + raise KeyError("No keys (%s) found in string '%s'" % |
23 | + (cmdline, names)) |
24 | + |
25 | + |
26 | +def attempt_cmdline_url(path, network=True, cmdline=None): |
27 | + """Write data from url referenced in command line to path. |
28 | + |
29 | + path: a file to write content to if downloaded. |
30 | + network: should network access be assumed. |
31 | + cmdline: the cmdline to parse for cloud-config-url. |
32 | + |
33 | + This is used in MAAS datasource, in "ephemeral" (read-only root) |
34 | + environment where the instance netboots to iscsi ro root. |
35 | + and the entity that controls the pxe config has to configure |
36 | + the maas datasource. |
37 | + |
38 | + An attempt is made on network urls even in local datasource |
39 | + for case of network set up in initramfs. |
40 | + |
41 | + Return value is a tuple of a logger function (logging.DEBUG) |
42 | + and a message indicating what happened. |
43 | + """ |
44 | + |
45 | + if cmdline is None: |
46 | + cmdline = util.get_cmdline() |
47 | + |
48 | + try: |
49 | + cmdline_name, url = parse_cmdline_url(cmdline) |
50 | + except KeyError: |
51 | + return (logging.DEBUG, "No kernel command line url found.") |
52 | + |
53 | + path_is_local = url.startswith("file://") or url.startswith("/") |
54 | + |
55 | + if path_is_local and os.path.exists(path): |
56 | + if network: |
57 | + m = ("file '%s' existed, possibly from local stage download" |
58 | + " of command line url '%s'. Not re-writing." % (path, url)) |
59 | + level = logging.INFO |
60 | + if path_is_local: |
61 | + level = logging.DEBUG |
62 | + else: |
63 | + m = ("file '%s' existed, possibly from previous boot download" |
64 | + " of command line url '%s'. Not re-writing." % (path, url)) |
65 | + level = logging.WARN |
66 | + |
67 | + return (level, m) |
68 | + |
69 | + kwargs = {'url': url, 'timeout': 10, 'retries': 2} |
70 | + if network or path_is_local: |
71 | + level = logging.WARN |
72 | + kwargs['sec_between'] = 1 |
73 | + else: |
74 | + level = logging.DEBUG |
75 | + kwargs['sec_between'] = .1 |
76 | + |
77 | + data = None |
78 | + header = b'#cloud-config' |
79 | + try: |
80 | + resp = util.read_file_or_url(**kwargs) |
81 | + if resp.ok(): |
82 | + data = resp.contents |
83 | + if not resp.contents.startswith(header): |
84 | + if cmdline_name == 'cloud-config-url': |
85 | + level = logging.WARN |
86 | + else: |
87 | + level = logging.INFO |
88 | + return ( |
89 | + level, |
90 | + "contents of '%s' did not start with %s" % (url, header)) |
91 | + else: |
92 | + return (level, |
93 | + "url '%s' returned code %s. Ignoring." % (url, resp.code)) |
94 | + |
95 | + except url_helper.UrlError as e: |
96 | + return (level, "retrieving url '%s' failed: %s" % (url, e)) |
97 | + |
98 | + util.write_file(path, data, mode=0o600) |
99 | + return (logging.INFO, |
100 | + "wrote cloud-config data from %s='%s' to %s" % |
101 | + (cmdline_name, url, path)) |
102 | + |
103 | + |
104 | def main_init(name, args): |
105 | deps = [sources.DEP_FILESYSTEM, sources.DEP_NETWORK] |
106 | if args.local: |
107 | deps = [sources.DEP_FILESYSTEM] |
108 | |
109 | - if not args.local: |
110 | - # See doc/kernel-cmdline.txt |
111 | - # |
112 | - # This is used in maas datasource, in "ephemeral" (read-only root) |
113 | - # environment where the instance netboots to iscsi ro root. |
114 | - # and the entity that controls the pxe config has to configure |
115 | - # the maas datasource. |
116 | - # |
117 | - # Could be used elsewhere, only works on network based (not local). |
118 | - root_name = "%s.d" % (CLOUD_CONFIG) |
119 | - target_fn = os.path.join(root_name, "91_kernel_cmdline_url.cfg") |
120 | - util.read_write_cmdline_url(target_fn) |
121 | + early_logs = [] |
122 | + early_logs.append( |
123 | + attempt_cmdline_url( |
124 | + path=os.path.join("%s.d" % CLOUD_CONFIG, |
125 | + "91_kernel_cmdline_url.cfg"), |
126 | + network=not args.local)) |
127 | |
128 | # Cloud-init 'init' stage is broken up into the following sub-stages |
129 | # 1. Ensure that the init object fetches its config without errors |
130 | @@ -171,12 +253,14 @@ def main_init(name, args): |
131 | outfmt = None |
132 | errfmt = None |
133 | try: |
134 | - LOG.debug("Closing stdin") |
135 | + early_logs.append((logging.DEBUG, "Closing stdin.")) |
136 | util.close_stdin() |
137 | (outfmt, errfmt) = util.fixup_output(init.cfg, name) |
138 | except Exception: |
139 | - util.logexc(LOG, "Failed to setup output redirection!") |
140 | - print_exc("Failed to setup output redirection!") |
141 | + msg = "Failed to setup output redirection!" |
142 | + util.logexc(LOG, msg) |
143 | + print_exc(msg) |
144 | + early_logs.append((logging.WARN, msg)) |
145 | if args.debug: |
146 | # Reset so that all the debug handlers are closed out |
147 | LOG.debug(("Logging being reset, this logger may no" |
148 | @@ -190,6 +274,10 @@ def main_init(name, args): |
149 | # been redirected and log now configured. |
150 | welcome(name, msg=w_msg) |
151 | |
152 | + # re-play early log messages before logging was setup |
153 | + for lvl, msg in early_logs: |
154 | + LOG.log(lvl, msg) |
155 | + |
156 | # Stage 3 |
157 | try: |
158 | init.initialize() |
159 | diff --git a/cloudinit/util.py b/cloudinit/util.py |
160 | index 5725129..7196a7c 100644 |
161 | --- a/cloudinit/util.py |
162 | +++ b/cloudinit/util.py |
163 | @@ -1089,31 +1089,6 @@ def get_fqdn_from_hosts(hostname, filename="/etc/hosts"): |
164 | return fqdn |
165 | |
166 | |
167 | -def get_cmdline_url(names=('cloud-config-url', 'url'), |
168 | - starts=b"#cloud-config", cmdline=None): |
169 | - if cmdline is None: |
170 | - cmdline = get_cmdline() |
171 | - |
172 | - data = keyval_str_to_dict(cmdline) |
173 | - url = None |
174 | - key = None |
175 | - for key in names: |
176 | - if key in data: |
177 | - url = data[key] |
178 | - break |
179 | - |
180 | - if not url: |
181 | - return (None, None, None) |
182 | - |
183 | - resp = read_file_or_url(url) |
184 | - # allow callers to pass starts as text when comparing to bytes contents |
185 | - starts = encode_text(starts) |
186 | - if resp.ok() and resp.contents.startswith(starts): |
187 | - return (key, url, resp.contents) |
188 | - |
189 | - return (key, url, None) |
190 | - |
191 | - |
192 | def is_resolvable(name): |
193 | """determine if a url is resolvable, return a boolean |
194 | This also attempts to be resilent against dns redirection. |
195 | @@ -1475,25 +1450,6 @@ def ensure_dirs(dirlist, mode=0o755): |
196 | ensure_dir(d, mode) |
197 | |
198 | |
199 | -def read_write_cmdline_url(target_fn): |
200 | - if not os.path.exists(target_fn): |
201 | - try: |
202 | - (key, url, content) = get_cmdline_url() |
203 | - except Exception: |
204 | - logexc(LOG, "Failed fetching command line url") |
205 | - return |
206 | - try: |
207 | - if key and content: |
208 | - write_file(target_fn, content, mode=0o600) |
209 | - LOG.debug(("Wrote to %s with contents of command line" |
210 | - " url %s (len=%s)"), target_fn, url, len(content)) |
211 | - elif key and not content: |
212 | - LOG.debug(("Command line key %s with url" |
213 | - " %s had no contents"), key, url) |
214 | - except Exception: |
215 | - logexc(LOG, "Failed writing url content to %s", target_fn) |
216 | - |
217 | - |
218 | def yaml_dumps(obj, explicit_start=True, explicit_end=True): |
219 | return yaml.safe_dump(obj, |
220 | line_break="\n", |
221 | diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py |
222 | index cf3b46d..64e56d9 100644 |
223 | --- a/tests/unittests/helpers.py |
224 | +++ b/tests/unittests/helpers.py |
225 | @@ -264,16 +264,22 @@ class HttprettyTestCase(TestCase): |
226 | |
227 | class TempDirTestCase(TestCase): |
228 | # provide a tempdir per class, not per test. |
229 | - def setUp(self): |
230 | - super(TempDirTestCase, self).setUp() |
231 | - self.tmp = tempfile.mkdtemp() |
232 | - self.addCleanup(shutil.rmtree, self.tmp) |
233 | + @classmethod |
234 | + def setUpClass(cls): |
235 | + cls.tmpd = tempfile.mkdtemp(prefix="ci-%s." % cls.__name__) |
236 | + return TestCase.setUpClass() |
237 | + |
238 | + @classmethod |
239 | + def tearDownClass(cls): |
240 | + shutil.rmtree(cls.tmpd) |
241 | + return TestCase.tearDownClass() |
242 | |
243 | def tmp_path(self, path): |
244 | + # if absolute path (starts with /), then make ./path |
245 | if path.startswith(os.path.sep): |
246 | path = "." + path |
247 | |
248 | - return os.path.normpath(os.path.join(self.tmp, path)) |
249 | + return os.path.normpath(os.path.join(self.tmpd, path)) |
250 | |
251 | |
252 | def populate_dir(path, files): |
253 | diff --git a/tests/unittests/test__init__.py b/tests/unittests/test__init__.py |
254 | index 7b6f8c4..e6f4c31 100644 |
255 | --- a/tests/unittests/test__init__.py |
256 | +++ b/tests/unittests/test__init__.py |
257 | @@ -1,16 +1,18 @@ |
258 | # This file is part of cloud-init. See LICENSE file for license information. |
259 | |
260 | +import logging |
261 | import os |
262 | import shutil |
263 | import tempfile |
264 | |
265 | +from cloudinit.cmd import main |
266 | from cloudinit import handlers |
267 | from cloudinit import helpers |
268 | from cloudinit import settings |
269 | from cloudinit import url_helper |
270 | from cloudinit import util |
271 | |
272 | -from .helpers import TestCase, ExitStack, mock |
273 | +from .helpers import TestCase, TempDirTestCase, ExitStack, mock |
274 | |
275 | |
276 | class FakeModule(handlers.Handler): |
277 | @@ -170,44 +172,68 @@ class TestHandlerHandlePart(TestCase): |
278 | self.data, self.ctype, self.filename, self.payload) |
279 | |
280 | |
281 | -class TestCmdlineUrl(TestCase): |
282 | - def test_invalid_content(self): |
283 | - url = "http://example.com/foo" |
284 | - key = "mykey" |
285 | - payload = b"0" |
286 | - cmdline = "ro %s=%s bar=1" % (key, url) |
287 | +class TestCmdlineUrl(TempDirTestCase): |
288 | + def test_parse_cmdline_url_nokey_raises_keyerror(self): |
289 | + self.assertRaises( |
290 | + KeyError, main.parse_cmdline_url, 'root=foo bar single') |
291 | |
292 | - with mock.patch('cloudinit.url_helper.readurl', |
293 | - return_value=url_helper.StringResponse(payload)): |
294 | - self.assertEqual( |
295 | - util.get_cmdline_url(names=[key], starts="xxxxxx", |
296 | - cmdline=cmdline), |
297 | - (key, url, None)) |
298 | + def test_parse_cmdline_url_found(self): |
299 | + cmdline = 'root=foo bar single url=http://example.com arg1 -v' |
300 | + self.assertEqual( |
301 | + ('url', 'http://example.com'), main.parse_cmdline_url(cmdline)) |
302 | |
303 | - def test_valid_content(self): |
304 | - url = "http://example.com/foo" |
305 | - key = "mykey" |
306 | - payload = b"xcloud-config\nmydata: foo\nbar: wark\n" |
307 | + @mock.patch('cloudinit.cmd.main.util.read_file_or_url') |
308 | + def test_invalid_content(self, m_read): |
309 | + key = "cloud-config-url" |
310 | + url = 'http://example.com/foo' |
311 | cmdline = "ro %s=%s bar=1" % (key, url) |
312 | + m_read.return_value = url_helper.StringResponse(b"unexpected blob") |
313 | |
314 | - with mock.patch('cloudinit.url_helper.readurl', |
315 | - return_value=url_helper.StringResponse(payload)): |
316 | - self.assertEqual( |
317 | - util.get_cmdline_url(names=[key], starts=b"xcloud-config", |
318 | - cmdline=cmdline), |
319 | - (key, url, payload)) |
320 | + fpath = self.tmp_path("test_valid") |
321 | + lvl, msg = main.attempt_cmdline_url( |
322 | + fpath, network=True, cmdline=cmdline) |
323 | + self.assertEqual(logging.WARN, lvl) |
324 | + self.assertIn(url, msg) |
325 | + self.assertFalse(os.path.exists(fpath)) |
326 | |
327 | - def test_no_key_found(self): |
328 | + @mock.patch('cloudinit.cmd.main.util.read_file_or_url') |
329 | + def test_valid_content(self, m_read): |
330 | url = "http://example.com/foo" |
331 | - key = "mykey" |
332 | - cmdline = "ro %s=%s bar=1" % (key, url) |
333 | - |
334 | - with mock.patch('cloudinit.url_helper.readurl', |
335 | - return_value=url_helper.StringResponse(b'')): |
336 | - self.assertEqual( |
337 | - util.get_cmdline_url(names=["does-not-appear"], |
338 | - starts="#cloud-config", cmdline=cmdline), |
339 | - (None, None, None)) |
340 | + payload = b"#cloud-config\nmydata: foo\nbar: wark\n" |
341 | + cmdline = "ro %s=%s bar=1" % ('cloud-config-url', url) |
342 | + |
343 | + m_read.return_value = url_helper.StringResponse(payload) |
344 | + fpath = self.tmp_path("test_valid") |
345 | + lvl, msg = main.attempt_cmdline_url( |
346 | + fpath, network=True, cmdline=cmdline) |
347 | + self.assertEqual(util.load_file(fpath, decode=False), payload) |
348 | + self.assertEqual(logging.INFO, lvl) |
349 | + self.assertIn(url, msg) |
350 | + |
351 | + @mock.patch('cloudinit.cmd.main.util.read_file_or_url') |
352 | + def test_no_key_found(self, m_read): |
353 | + cmdline = "ro mykey=http://example.com/foo root=foo" |
354 | + fpath = self.tmp_path("test_no_key_found") |
355 | + lvl, msg = main.attempt_cmdline_url( |
356 | + fpath, network=True, cmdline=cmdline) |
357 | + |
358 | + m_read.assert_not_called() |
359 | + self.assertFalse(os.path.exists(fpath)) |
360 | + self.assertEqual(logging.DEBUG, lvl) |
361 | + |
362 | + @mock.patch('cloudinit.cmd.main.util.read_file_or_url') |
363 | + def test_exception_warns(self, m_read): |
364 | + url = "http://example.com/foo" |
365 | + cmdline = "ro cloud-config-url=%s root=LABEL=bar" % url |
366 | + fpath = self.tmp_path("test_no_key_found") |
367 | + m_read.side_effect = url_helper.UrlError( |
368 | + cause="Unexpected Error", url="http://example.com/foo") |
369 | + |
370 | + lvl, msg = main.attempt_cmdline_url( |
371 | + fpath, network=True, cmdline=cmdline) |
372 | + self.assertEqual(logging.WARN, lvl) |
373 | + self.assertIn(url, msg) |
374 | + self.assertFalse(os.path.exists(fpath)) |
375 | |
376 | |
377 | # vi: ts=4 expandtab |
Thanks for the review, your comments seem sane.
I really do want to get this in, as currently this is a common path for failure in maas (node can't reach region controller) and there isn't much path as to what is going on there.