Merge ~chad.smith/cloud-init:fix-device-path-from-cmdline-regression into cloud-init:master

Proposed by Scott Moser on 2017-10-23
Status: Merged
Approved by: Scott Moser on 2017-10-23
Approved revision: 00fdcd665a11980b69b5c66c7e369e1df2630d1d
Merged at revision: 17a15f9e0ae78e4fc4e24fab0caebdf78f06ef66
Proposed branch: ~chad.smith/cloud-init:fix-device-path-from-cmdline-regression
Merge into: cloud-init:master
Diff against target: 302 lines (+70/-64)
2 files modified
cloudinit/config/cc_resizefs.py (+13/-30)
tests/unittests/test_handler/test_handler_resizefs.py (+57/-34)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Needs Fixing on 2017-10-25
Scott Moser Approve on 2017-10-23
Review via email: mp+332634@code.launchpad.net

Commit message

resizefs: Fix regression when system booted with root=PARTUUID=

A recent cleanup of the resizefs module broke resizing when
a system was booted with root=PARTUUID=<uuid> and the device /dev/root
does not exist. This path is exposed with the Ubuntu 16.04 but
not with Ubuntu 17.10. A recreate exists under bug 1684869.

LP: #1725067

To post a comment you must log in.
Scott Moser (smoser) wrote :

Please go ahead and delete the 'rootdev_from_cmdline' function in cc_rezisefs.py
it is not used anywhere and is just confusing. the one in util is preferred.

PASSED: Continuous integration, rev:129beef23adedcba82135c4dc8e6ba4aa2fe7c04
https://jenkins.ubuntu.com/server/job/cloud-init-ci/428/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/428/rebuild

review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

I tried the suggestion of using FileSystemMockingTestCase and mocking os.stat
 http://paste.ubuntu.com/25801791/

Where the idea became problematic was there is no simple way to make /dev/disk/by-partuuid be a block device, and the code specifically checks for that.

so at this point, it does seem like we should still patch os.stat , but doing so is
not related to your MP without further work.

6d58f22... by Chad Smith on 2017-10-23

drop rootdev_from_cmdline duplicate definition from cc_resizefs as we use util.rootdev_from_cmdline. Adjust unit tests to test util.rootdev_from_cmdline instead of cc_resizefs.rootdev_from_cmdline. Replace jsonschema in requirements.txt

00fdcd6... by Chad Smith on 2017-10-23

add comment about using FilesystemMocking instead of os.stat mocks

Chad Smith (chad.smith) wrote :

@smoser review comments addressed

FAILED: Continuous integration, rev:445ee93379afefb00f0768dbb964de24190b4297
https://jenkins.ubuntu.com/server/job/cloud-init-ci/429/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    FAILED: MAAS Compatability Testing

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/429/rebuild

review: Needs Fixing (continuous-integration)
Scott Moser (smoser) :
review: Approve

FAILED: Continuous integration, rev:445ee93379afefb00f0768dbb964de24190b4297
https://jenkins.ubuntu.com/server/job/cloud-init-ci/430/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    FAILED: MAAS Compatability Testing

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/430/rebuild

review: Needs Fixing (continuous-integration)

PASSED: Continuous integration, rev:00fdcd665a11980b69b5c66c7e369e1df2630d1d
https://jenkins.ubuntu.com/server/job/cloud-init-ci/433/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/433/rebuild

review: Approve (continuous-integration)

FAILED: Continuous integration, rev:445ee93379afefb00f0768dbb964de24190b4297
https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
2index f774baa..0d282e6 100644
3--- a/cloudinit/config/cc_resizefs.py
4+++ b/cloudinit/config/cc_resizefs.py
5@@ -145,25 +145,6 @@ RESIZE_FS_PRECHECK_CMDS = {
6 }
7
8
9-def rootdev_from_cmdline(cmdline):
10- found = None
11- for tok in cmdline.split():
12- if tok.startswith("root="):
13- found = tok[5:]
14- break
15- if found is None:
16- return None
17-
18- if found.startswith("/dev/"):
19- return found
20- if found.startswith("LABEL="):
21- return "/dev/disk/by-label/" + found[len("LABEL="):]
22- if found.startswith("UUID="):
23- return "/dev/disk/by-uuid/" + found[len("UUID="):]
24-
25- return "/dev/" + found
26-
27-
28 def can_skip_resize(fs_type, resize_what, devpth):
29 fstype_lc = fs_type.lower()
30 for i, func in RESIZE_FS_PRECHECK_CMDS.items():
31@@ -172,14 +153,15 @@ def can_skip_resize(fs_type, resize_what, devpth):
32 return False
33
34
35-def is_device_path_writable_block(devpath, info, log):
36- """Return True if devpath is a writable block device.
37+def maybe_get_writable_device_path(devpath, info, log):
38+ """Return updated devpath if the devpath is a writable block device.
39
40- @param devpath: Path to the root device we want to resize.
41+ @param devpath: Requested path to the root device we want to resize.
42 @param info: String representing information about the requested device.
43 @param log: Logger to which logs will be added upon error.
44
45- @returns Boolean True if block device is writable
46+ @returns devpath or updated devpath per kernel commandline if the device
47+ path is a writable block device, returns None otherwise.
48 """
49 container = util.is_container()
50
51@@ -189,12 +171,12 @@ def is_device_path_writable_block(devpath, info, log):
52 devpath = util.rootdev_from_cmdline(util.get_cmdline())
53 if devpath is None:
54 log.warn("Unable to find device '/dev/root'")
55- return False
56+ return None
57 log.debug("Converted /dev/root to '%s' per kernel cmdline", devpath)
58
59 if devpath == 'overlayroot':
60 log.debug("Not attempting to resize devpath '%s': %s", devpath, info)
61- return False
62+ return None
63
64 try:
65 statret = os.stat(devpath)
66@@ -207,7 +189,7 @@ def is_device_path_writable_block(devpath, info, log):
67 devpath, info)
68 else:
69 raise exc
70- return False
71+ return None
72
73 if not stat.S_ISBLK(statret.st_mode) and not stat.S_ISCHR(statret.st_mode):
74 if container:
75@@ -216,8 +198,8 @@ def is_device_path_writable_block(devpath, info, log):
76 else:
77 log.warn("device '%s' not a block device. cannot resize: %s" %
78 (devpath, info))
79- return False
80- return True
81+ return None
82+ return devpath # The writable block devpath
83
84
85 def handle(name, cfg, _cloud, log, args):
86@@ -242,8 +224,9 @@ def handle(name, cfg, _cloud, log, args):
87 info = "dev=%s mnt_point=%s path=%s" % (devpth, mount_point, resize_what)
88 log.debug("resize_info: %s" % info)
89
90- if not is_device_path_writable_block(devpth, info, log):
91- return
92+ devpth = maybe_get_writable_device_path(devpth, info, log)
93+ if not devpth:
94+ return # devpath was not a writable block device
95
96 resizer = None
97 if can_skip_resize(fs_type, resize_what, devpth):
98diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
99index 3e5d436..29d5574 100644
100--- a/tests/unittests/test_handler/test_handler_resizefs.py
101+++ b/tests/unittests/test_handler/test_handler_resizefs.py
102@@ -1,9 +1,9 @@
103 # This file is part of cloud-init. See LICENSE file for license information.
104
105 from cloudinit.config.cc_resizefs import (
106- can_skip_resize, handle, is_device_path_writable_block,
107- rootdev_from_cmdline)
108+ can_skip_resize, handle, maybe_get_writable_device_path)
109
110+from collections import namedtuple
111 import logging
112 import textwrap
113
114@@ -138,47 +138,48 @@ class TestRootDevFromCmdline(CiTestCase):
115 invalid_cases = [
116 'BOOT_IMAGE=/adsf asdfa werasef root adf', 'BOOT_IMAGE=/adsf', '']
117 for case in invalid_cases:
118- self.assertIsNone(rootdev_from_cmdline(case))
119+ self.assertIsNone(util.rootdev_from_cmdline(case))
120
121 def test_rootdev_from_cmdline_with_root_startswith_dev(self):
122 """Return the cmdline root when the path starts with /dev."""
123 self.assertEqual(
124- '/dev/this', rootdev_from_cmdline('asdf root=/dev/this'))
125+ '/dev/this', util.rootdev_from_cmdline('asdf root=/dev/this'))
126
127 def test_rootdev_from_cmdline_with_root_without_dev_prefix(self):
128 """Add /dev prefix to cmdline root when the path lacks the prefix."""
129- self.assertEqual('/dev/this', rootdev_from_cmdline('asdf root=this'))
130+ self.assertEqual(
131+ '/dev/this', util.rootdev_from_cmdline('asdf root=this'))
132
133 def test_rootdev_from_cmdline_with_root_with_label(self):
134 """When cmdline root contains a LABEL, our root is disk/by-label."""
135 self.assertEqual(
136 '/dev/disk/by-label/unique',
137- rootdev_from_cmdline('asdf root=LABEL=unique'))
138+ util.rootdev_from_cmdline('asdf root=LABEL=unique'))
139
140 def test_rootdev_from_cmdline_with_root_with_uuid(self):
141 """When cmdline root contains a UUID, our root is disk/by-uuid."""
142 self.assertEqual(
143 '/dev/disk/by-uuid/adsfdsaf-adsf',
144- rootdev_from_cmdline('asdf root=UUID=adsfdsaf-adsf'))
145+ util.rootdev_from_cmdline('asdf root=UUID=adsfdsaf-adsf'))
146
147
148-class TestIsDevicePathWritableBlock(CiTestCase):
149+class TestMaybeGetDevicePathAsWritableBlock(CiTestCase):
150
151 with_logs = True
152
153- def test_is_device_path_writable_block_false_on_overlayroot(self):
154+ def test_maybe_get_writable_device_path_none_on_overlayroot(self):
155 """When devpath is overlayroot (on MAAS), is_dev_writable is False."""
156 info = 'does not matter'
157- is_writable = wrap_and_call(
158+ devpath = wrap_and_call(
159 'cloudinit.config.cc_resizefs.util',
160 {'is_container': {'return_value': False}},
161- is_device_path_writable_block, 'overlayroot', info, LOG)
162- self.assertFalse(is_writable)
163+ maybe_get_writable_device_path, 'overlayroot', info, LOG)
164+ self.assertIsNone(devpath)
165 self.assertIn(
166 "Not attempting to resize devpath 'overlayroot'",
167 self.logs.getvalue())
168
169- def test_is_device_path_writable_block_warns_missing_cmdline_root(self):
170+ def test_maybe_get_writable_device_path_warns_missing_cmdline_root(self):
171 """When root does not exist isn't in the cmdline, log warning."""
172 info = 'does not matter'
173
174@@ -190,43 +191,43 @@ class TestIsDevicePathWritableBlock(CiTestCase):
175 exists_mock_path = 'cloudinit.config.cc_resizefs.os.path.exists'
176 with mock.patch(exists_mock_path) as m_exists:
177 m_exists.return_value = False
178- is_writable = wrap_and_call(
179+ devpath = wrap_and_call(
180 'cloudinit.config.cc_resizefs.util',
181 {'is_container': {'return_value': False},
182 'get_mount_info': {'side_effect': fake_mount_info},
183 'get_cmdline': {'return_value': 'BOOT_IMAGE=/vmlinuz.efi'}},
184- is_device_path_writable_block, '/dev/root', info, LOG)
185- self.assertFalse(is_writable)
186+ maybe_get_writable_device_path, '/dev/root', info, LOG)
187+ self.assertIsNone(devpath)
188 logs = self.logs.getvalue()
189 self.assertIn("WARNING: Unable to find device '/dev/root'", logs)
190
191- def test_is_device_path_writable_block_does_not_exist(self):
192+ def test_maybe_get_writable_device_path_does_not_exist(self):
193 """When devpath does not exist, a warning is logged."""
194 info = 'dev=/I/dont/exist mnt_point=/ path=/dev/none'
195- is_writable = wrap_and_call(
196+ devpath = wrap_and_call(
197 'cloudinit.config.cc_resizefs.util',
198 {'is_container': {'return_value': False}},
199- is_device_path_writable_block, '/I/dont/exist', info, LOG)
200- self.assertFalse(is_writable)
201+ maybe_get_writable_device_path, '/I/dont/exist', info, LOG)
202+ self.assertIsNone(devpath)
203 self.assertIn(
204 "WARNING: Device '/I/dont/exist' did not exist."
205 ' cannot resize: %s' % info,
206 self.logs.getvalue())
207
208- def test_is_device_path_writable_block_does_not_exist_in_container(self):
209+ def test_maybe_get_writable_device_path_does_not_exist_in_container(self):
210 """When devpath does not exist in a container, log a debug message."""
211 info = 'dev=/I/dont/exist mnt_point=/ path=/dev/none'
212- is_writable = wrap_and_call(
213+ devpath = wrap_and_call(
214 'cloudinit.config.cc_resizefs.util',
215 {'is_container': {'return_value': True}},
216- is_device_path_writable_block, '/I/dont/exist', info, LOG)
217- self.assertFalse(is_writable)
218+ maybe_get_writable_device_path, '/I/dont/exist', info, LOG)
219+ self.assertIsNone(devpath)
220 self.assertIn(
221 "DEBUG: Device '/I/dont/exist' did not exist in container."
222 ' cannot resize: %s' % info,
223 self.logs.getvalue())
224
225- def test_is_device_path_writable_block_raises_oserror(self):
226+ def test_maybe_get_writable_device_path_raises_oserror(self):
227 """When unexpected OSError is raises by os.stat it is reraised."""
228 info = 'dev=/I/dont/exist mnt_point=/ path=/dev/none'
229 with self.assertRaises(OSError) as context_manager:
230@@ -234,41 +235,63 @@ class TestIsDevicePathWritableBlock(CiTestCase):
231 'cloudinit.config.cc_resizefs',
232 {'util.is_container': {'return_value': True},
233 'os.stat': {'side_effect': OSError('Something unexpected')}},
234- is_device_path_writable_block, '/I/dont/exist', info, LOG)
235+ maybe_get_writable_device_path, '/I/dont/exist', info, LOG)
236 self.assertEqual(
237 'Something unexpected', str(context_manager.exception))
238
239- def test_is_device_path_writable_block_non_block(self):
240+ def test_maybe_get_writable_device_path_non_block(self):
241 """When device is not a block device, emit warning return False."""
242 fake_devpath = self.tmp_path('dev/readwrite')
243 util.write_file(fake_devpath, '', mode=0o600) # read-write
244 info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
245
246- is_writable = wrap_and_call(
247+ devpath = wrap_and_call(
248 'cloudinit.config.cc_resizefs.util',
249 {'is_container': {'return_value': False}},
250- is_device_path_writable_block, fake_devpath, info, LOG)
251- self.assertFalse(is_writable)
252+ maybe_get_writable_device_path, fake_devpath, info, LOG)
253+ self.assertIsNone(devpath)
254 self.assertIn(
255 "WARNING: device '{0}' not a block device. cannot resize".format(
256 fake_devpath),
257 self.logs.getvalue())
258
259- def test_is_device_path_writable_block_non_block_on_container(self):
260+ def test_maybe_get_writable_device_path_non_block_on_container(self):
261 """When device is non-block device in container, emit debug log."""
262 fake_devpath = self.tmp_path('dev/readwrite')
263 util.write_file(fake_devpath, '', mode=0o600) # read-write
264 info = 'dev=/dev/root mnt_point=/ path={0}'.format(fake_devpath)
265
266- is_writable = wrap_and_call(
267+ devpath = wrap_and_call(
268 'cloudinit.config.cc_resizefs.util',
269 {'is_container': {'return_value': True}},
270- is_device_path_writable_block, fake_devpath, info, LOG)
271- self.assertFalse(is_writable)
272+ maybe_get_writable_device_path, fake_devpath, info, LOG)
273+ self.assertIsNone(devpath)
274 self.assertIn(
275 "DEBUG: device '{0}' not a block device in container."
276 ' cannot resize'.format(fake_devpath),
277 self.logs.getvalue())
278
279+ def test_maybe_get_writable_device_path_returns_cmdline_root(self):
280+ """When root device is UUID in kernel commandline, update devpath."""
281+ # XXX Long-term we want to use FilesystemMocking test to avoid
282+ # touching os.stat.
283+ FakeStat = namedtuple(
284+ 'FakeStat', ['st_mode', 'st_size', 'st_mtime']) # minimal def.
285+ info = 'dev=/dev/root mnt_point=/ path=/does/not/matter'
286+ devpath = wrap_and_call(
287+ 'cloudinit.config.cc_resizefs',
288+ {'util.get_cmdline': {'return_value': 'asdf root=UUID=my-uuid'},
289+ 'util.is_container': False,
290+ 'os.path.exists': False, # /dev/root doesn't exist
291+ 'os.stat': {
292+ 'return_value': FakeStat(25008, 0, 1)} # char block device
293+ },
294+ maybe_get_writable_device_path, '/dev/root', info, LOG)
295+ self.assertEqual('/dev/disk/by-uuid/my-uuid', devpath)
296+ self.assertIn(
297+ "DEBUG: Converted /dev/root to '/dev/disk/by-uuid/my-uuid'"
298+ " per kernel cmdline",
299+ self.logs.getvalue())
300+
301
302 # vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches