Merge ~larsks/cloud-init:bz/1542578 into cloud-init:master
- Git
- lp:~larsks/cloud-init
- bz/1542578
- Merge into master
Status: | Merged |
---|---|
Approved by: | Scott Moser |
Approved revision: | e65b8058aa663c096de90854b56e98f390717636 |
Merge reported by: | Scott Moser |
Merged at revision: | 529d48f69d3784b2314397f5eab9d750ab03cf6a |
Proposed branch: | ~larsks/cloud-init:bz/1542578 |
Merge into: | cloud-init:master |
Diff against target: |
263 lines (+136/-25) 3 files modified
cloudinit/config/cc_mounts.py (+34/-19) cloudinit/tests/helpers.py (+2/-2) tests/unittests/test_handler/test_handler_mounts.py (+100/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Server Team CI bot | continuous-integration | Approve | |
Scott Moser | Approve | ||
Chad Smith | Approve | ||
Review via email: mp+345113@code.launchpad.net |
Commit message
cc_mounts: Do not add devices to fstab that are already present.
Do not add new entries to /etc/fstab for devices that already have an
existing fstab entry.
Resolves: rhbz#1542578
Description of the change
We had a customer who was booting images in OpenStack that had a pre-configured swap partition. Cloud-init was adding a second entry for the same partition. This commit resolves the problem by parsing device names from /etc/fstab and declining to generate new entries for devices that already exist.
As with the existing code, entries with the "comment=
Server Team CI bot (server-team-bot) wrote : | # |
Scott Moser (smoser) wrote : | # |
Thats my first quick pass.
This code is really in need of re-factoring.
And could really use unit tests.
Lars Kellogg-Stedman (larsks) wrote : | # |
I agree re: refactoring, but I'm not sure I'm up to that right now. I'll make the changes you suggested and I'll add a test or two.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:acd63782c1c
https:/
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:/
Lars Kellogg-Stedman (larsks) wrote : | # |
I've added a few unit tests for the fstab handling code.
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:62a5986a8d8
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:74671dc8366
https:/
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:/
Lars Kellogg-Stedman (larsks) wrote : | # |
Ping @smoser; let me know if this looks okay. Thanks!
Scott Moser (smoser) wrote : | # |
Lars, thank you, and prefectly fine to poke me.
I'll try to take a look at this on monday.
one comment in line about some infrastructure that can make this easier.
Thank you for adding some tests!
Scott
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:dd61338ba19
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:68c2ca587a0
https:/
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:/
Chad Smith (chad.smith) : | # |
Lars Kellogg-Stedman (larsks) wrote : | # |
Updated to address Chad's comments.
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:759107db5ad
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:5bd9e873de6
https:/
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:/
Chad Smith (chad.smith) wrote : | # |
This looks good to me. Thanks for the changes Lars!
Scott Moser (smoser) wrote : | # |
Lars, can you rebase this ... it has conflicts on head and wont land.
Lars Kellogg-Stedman (larsks) wrote : | # |
I've rebased this on b4ae0e1fb8a48a8
Scott Moser (smoser) wrote : | # |
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:e65b8058aa6
https:/
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:/
Scott Moser (smoser) wrote : | # |
An upstream commit landed for this bug.
To view that commit see the following URL:
https:/
Preview Diff
1 | diff --git a/cloudinit/config/cc_mounts.py b/cloudinit/config/cc_mounts.py | |||
2 | index b3b25c3..eca6ea3 100644 | |||
3 | --- a/cloudinit/config/cc_mounts.py | |||
4 | +++ b/cloudinit/config/cc_mounts.py | |||
5 | @@ -76,6 +76,7 @@ DEVICE_NAME_FILTER = r"^([x]{0,1}[shv]d[a-z][0-9]*|sr[0-9]+)$" | |||
6 | 76 | DEVICE_NAME_RE = re.compile(DEVICE_NAME_FILTER) | 76 | DEVICE_NAME_RE = re.compile(DEVICE_NAME_FILTER) |
7 | 77 | WS = re.compile("[%s]+" % (whitespace)) | 77 | WS = re.compile("[%s]+" % (whitespace)) |
8 | 78 | FSTAB_PATH = "/etc/fstab" | 78 | FSTAB_PATH = "/etc/fstab" |
9 | 79 | MNT_COMMENT = "comment=cloudconfig" | ||
10 | 79 | 80 | ||
11 | 80 | LOG = logging.getLogger(__name__) | 81 | LOG = logging.getLogger(__name__) |
12 | 81 | 82 | ||
13 | @@ -327,6 +328,22 @@ def handle(_name, cfg, cloud, log, _args): | |||
14 | 327 | 328 | ||
15 | 328 | LOG.debug("mounts configuration is %s", cfgmnt) | 329 | LOG.debug("mounts configuration is %s", cfgmnt) |
16 | 329 | 330 | ||
17 | 331 | fstab_lines = [] | ||
18 | 332 | fstab_devs = {} | ||
19 | 333 | fstab_removed = [] | ||
20 | 334 | |||
21 | 335 | for line in util.load_file(FSTAB_PATH).splitlines(): | ||
22 | 336 | if MNT_COMMENT in line: | ||
23 | 337 | fstab_removed.append(line) | ||
24 | 338 | continue | ||
25 | 339 | |||
26 | 340 | try: | ||
27 | 341 | toks = WS.split(line) | ||
28 | 342 | except Exception: | ||
29 | 343 | pass | ||
30 | 344 | fstab_devs[toks[0]] = line | ||
31 | 345 | fstab_lines.append(line) | ||
32 | 346 | |||
33 | 330 | for i in range(len(cfgmnt)): | 347 | for i in range(len(cfgmnt)): |
34 | 331 | # skip something that wasn't a list | 348 | # skip something that wasn't a list |
35 | 332 | if not isinstance(cfgmnt[i], list): | 349 | if not isinstance(cfgmnt[i], list): |
36 | @@ -336,12 +353,17 @@ def handle(_name, cfg, cloud, log, _args): | |||
37 | 336 | 353 | ||
38 | 337 | start = str(cfgmnt[i][0]) | 354 | start = str(cfgmnt[i][0]) |
39 | 338 | sanitized = sanitize_devname(start, cloud.device_name_to_device, log) | 355 | sanitized = sanitize_devname(start, cloud.device_name_to_device, log) |
40 | 356 | if sanitized != start: | ||
41 | 357 | log.debug("changed %s => %s" % (start, sanitized)) | ||
42 | 358 | |||
43 | 339 | if sanitized is None: | 359 | if sanitized is None: |
44 | 340 | log.debug("Ignoring nonexistent named mount %s", start) | 360 | log.debug("Ignoring nonexistent named mount %s", start) |
45 | 341 | continue | 361 | continue |
46 | 362 | elif sanitized in fstab_devs: | ||
47 | 363 | log.info("Device %s already defined in fstab: %s", | ||
48 | 364 | sanitized, fstab_devs[sanitized]) | ||
49 | 365 | continue | ||
50 | 342 | 366 | ||
51 | 343 | if sanitized != start: | ||
52 | 344 | log.debug("changed %s => %s" % (start, sanitized)) | ||
53 | 345 | cfgmnt[i][0] = sanitized | 367 | cfgmnt[i][0] = sanitized |
54 | 346 | 368 | ||
55 | 347 | # in case the user did not quote a field (likely fs-freq, fs_passno) | 369 | # in case the user did not quote a field (likely fs-freq, fs_passno) |
56 | @@ -373,11 +395,17 @@ def handle(_name, cfg, cloud, log, _args): | |||
57 | 373 | for defmnt in defmnts: | 395 | for defmnt in defmnts: |
58 | 374 | start = defmnt[0] | 396 | start = defmnt[0] |
59 | 375 | sanitized = sanitize_devname(start, cloud.device_name_to_device, log) | 397 | sanitized = sanitize_devname(start, cloud.device_name_to_device, log) |
60 | 398 | if sanitized != start: | ||
61 | 399 | log.debug("changed default device %s => %s" % (start, sanitized)) | ||
62 | 400 | |||
63 | 376 | if sanitized is None: | 401 | if sanitized is None: |
64 | 377 | log.debug("Ignoring nonexistent default named mount %s", start) | 402 | log.debug("Ignoring nonexistent default named mount %s", start) |
65 | 378 | continue | 403 | continue |
68 | 379 | if sanitized != start: | 404 | elif sanitized in fstab_devs: |
69 | 380 | log.debug("changed default device %s => %s" % (start, sanitized)) | 405 | log.debug("Device %s already defined in fstab: %s", |
70 | 406 | sanitized, fstab_devs[sanitized]) | ||
71 | 407 | continue | ||
72 | 408 | |||
73 | 381 | defmnt[0] = sanitized | 409 | defmnt[0] = sanitized |
74 | 382 | 410 | ||
75 | 383 | cfgmnt_has = False | 411 | cfgmnt_has = False |
76 | @@ -409,31 +437,18 @@ def handle(_name, cfg, cloud, log, _args): | |||
77 | 409 | log.debug("No modifications to fstab needed") | 437 | log.debug("No modifications to fstab needed") |
78 | 410 | return | 438 | return |
79 | 411 | 439 | ||
80 | 412 | comment = "comment=cloudconfig" | ||
81 | 413 | cc_lines = [] | 440 | cc_lines = [] |
82 | 414 | needswap = False | 441 | needswap = False |
83 | 415 | dirs = [] | 442 | dirs = [] |
84 | 416 | for line in actlist: | 443 | for line in actlist: |
85 | 417 | # write 'comment' in the fs_mntops, entry, claiming this | 444 | # write 'comment' in the fs_mntops, entry, claiming this |
87 | 418 | line[3] = "%s,%s" % (line[3], comment) | 445 | line[3] = "%s,%s" % (line[3], MNT_COMMENT) |
88 | 419 | if line[2] == "swap": | 446 | if line[2] == "swap": |
89 | 420 | needswap = True | 447 | needswap = True |
90 | 421 | if line[1].startswith("/"): | 448 | if line[1].startswith("/"): |
91 | 422 | dirs.append(line[1]) | 449 | dirs.append(line[1]) |
92 | 423 | cc_lines.append('\t'.join(line)) | 450 | cc_lines.append('\t'.join(line)) |
93 | 424 | 451 | ||
94 | 425 | fstab_lines = [] | ||
95 | 426 | removed = [] | ||
96 | 427 | for line in util.load_file(FSTAB_PATH).splitlines(): | ||
97 | 428 | try: | ||
98 | 429 | toks = WS.split(line) | ||
99 | 430 | if toks[3].find(comment) != -1: | ||
100 | 431 | removed.append(line) | ||
101 | 432 | continue | ||
102 | 433 | except Exception: | ||
103 | 434 | pass | ||
104 | 435 | fstab_lines.append(line) | ||
105 | 436 | |||
106 | 437 | for d in dirs: | 452 | for d in dirs: |
107 | 438 | try: | 453 | try: |
108 | 439 | util.ensure_dir(d) | 454 | util.ensure_dir(d) |
109 | @@ -441,7 +456,7 @@ def handle(_name, cfg, cloud, log, _args): | |||
110 | 441 | util.logexc(log, "Failed to make '%s' config-mount", d) | 456 | util.logexc(log, "Failed to make '%s' config-mount", d) |
111 | 442 | 457 | ||
112 | 443 | sadds = [WS.sub(" ", n) for n in cc_lines] | 458 | sadds = [WS.sub(" ", n) for n in cc_lines] |
114 | 444 | sdrops = [WS.sub(" ", n) for n in removed] | 459 | sdrops = [WS.sub(" ", n) for n in fstab_removed] |
115 | 445 | 460 | ||
116 | 446 | sops = (["- " + drop for drop in sdrops if drop not in sadds] + | 461 | sops = (["- " + drop for drop in sdrops if drop not in sadds] + |
117 | 447 | ["+ " + add for add in sadds if add not in sdrops]) | 462 | ["+ " + add for add in sadds if add not in sdrops]) |
118 | diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py | |||
119 | index 117a9cf..07059fd 100644 | |||
120 | --- a/cloudinit/tests/helpers.py | |||
121 | +++ b/cloudinit/tests/helpers.py | |||
122 | @@ -111,12 +111,12 @@ class TestCase(unittest2.TestCase): | |||
123 | 111 | super(TestCase, self).setUp() | 111 | super(TestCase, self).setUp() |
124 | 112 | self.reset_global_state() | 112 | self.reset_global_state() |
125 | 113 | 113 | ||
127 | 114 | def add_patch(self, target, attr, **kwargs): | 114 | def add_patch(self, target, attr, *args, **kwargs): |
128 | 115 | """Patches specified target object and sets it as attr on test | 115 | """Patches specified target object and sets it as attr on test |
129 | 116 | instance also schedules cleanup""" | 116 | instance also schedules cleanup""" |
130 | 117 | if 'autospec' not in kwargs: | 117 | if 'autospec' not in kwargs: |
131 | 118 | kwargs['autospec'] = True | 118 | kwargs['autospec'] = True |
133 | 119 | m = mock.patch(target, **kwargs) | 119 | m = mock.patch(target, *args, **kwargs) |
134 | 120 | p = m.start() | 120 | p = m.start() |
135 | 121 | self.addCleanup(m.stop) | 121 | self.addCleanup(m.stop) |
136 | 122 | setattr(self, attr, p) | 122 | setattr(self, attr, p) |
137 | diff --git a/tests/unittests/test_handler/test_handler_mounts.py b/tests/unittests/test_handler/test_handler_mounts.py | |||
138 | index fe492d4..8fea6c2 100644 | |||
139 | --- a/tests/unittests/test_handler/test_handler_mounts.py | |||
140 | +++ b/tests/unittests/test_handler/test_handler_mounts.py | |||
141 | @@ -1,8 +1,6 @@ | |||
142 | 1 | # This file is part of cloud-init. See LICENSE file for license information. | 1 | # This file is part of cloud-init. See LICENSE file for license information. |
143 | 2 | 2 | ||
144 | 3 | import os.path | 3 | import os.path |
145 | 4 | import shutil | ||
146 | 5 | import tempfile | ||
147 | 6 | 4 | ||
148 | 7 | from cloudinit.config import cc_mounts | 5 | from cloudinit.config import cc_mounts |
149 | 8 | 6 | ||
150 | @@ -18,8 +16,7 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase): | |||
151 | 18 | 16 | ||
152 | 19 | def setUp(self): | 17 | def setUp(self): |
153 | 20 | super(TestSanitizeDevname, self).setUp() | 18 | super(TestSanitizeDevname, self).setUp() |
156 | 21 | self.new_root = tempfile.mkdtemp() | 19 | self.new_root = self.tmp_dir() |
155 | 22 | self.addCleanup(shutil.rmtree, self.new_root) | ||
157 | 23 | self.patchOS(self.new_root) | 20 | self.patchOS(self.new_root) |
158 | 24 | 21 | ||
159 | 25 | def _touch(self, path): | 22 | def _touch(self, path): |
160 | @@ -134,4 +131,103 @@ class TestSanitizeDevname(test_helpers.FilesystemMockingTestCase): | |||
161 | 134 | cc_mounts.sanitize_devname( | 131 | cc_mounts.sanitize_devname( |
162 | 135 | 'ephemeral0.1', lambda x: disk_path, mock.Mock())) | 132 | 'ephemeral0.1', lambda x: disk_path, mock.Mock())) |
163 | 136 | 133 | ||
164 | 134 | |||
165 | 135 | class TestFstabHandling(test_helpers.FilesystemMockingTestCase): | ||
166 | 136 | |||
167 | 137 | swap_path = '/dev/sdb1' | ||
168 | 138 | |||
169 | 139 | def setUp(self): | ||
170 | 140 | super(TestFstabHandling, self).setUp() | ||
171 | 141 | self.new_root = self.tmp_dir() | ||
172 | 142 | self.patchOS(self.new_root) | ||
173 | 143 | |||
174 | 144 | self.fstab_path = os.path.join(self.new_root, 'etc/fstab') | ||
175 | 145 | self._makedirs('/etc') | ||
176 | 146 | |||
177 | 147 | self.add_patch('cloudinit.config.cc_mounts.FSTAB_PATH', | ||
178 | 148 | 'mock_fstab_path', | ||
179 | 149 | self.fstab_path, | ||
180 | 150 | autospec=False) | ||
181 | 151 | |||
182 | 152 | self.add_patch('cloudinit.config.cc_mounts._is_block_device', | ||
183 | 153 | 'mock_is_block_device', | ||
184 | 154 | return_value=True) | ||
185 | 155 | |||
186 | 156 | self.add_patch('cloudinit.config.cc_mounts.util.subp', | ||
187 | 157 | 'mock_util_subp') | ||
188 | 158 | |||
189 | 159 | self.mock_cloud = mock.Mock() | ||
190 | 160 | self.mock_log = mock.Mock() | ||
191 | 161 | self.mock_cloud.device_name_to_device = self.device_name_to_device | ||
192 | 162 | |||
193 | 163 | def _makedirs(self, directory): | ||
194 | 164 | directory = os.path.join(self.new_root, directory.lstrip('/')) | ||
195 | 165 | if not os.path.exists(directory): | ||
196 | 166 | os.makedirs(directory) | ||
197 | 167 | |||
198 | 168 | def device_name_to_device(self, path): | ||
199 | 169 | if path == 'swap': | ||
200 | 170 | return self.swap_path | ||
201 | 171 | else: | ||
202 | 172 | dev = None | ||
203 | 173 | |||
204 | 174 | return dev | ||
205 | 175 | |||
206 | 176 | def test_fstab_no_swap_device(self): | ||
207 | 177 | '''Ensure that cloud-init adds a discovered swap partition | ||
208 | 178 | to /etc/fstab.''' | ||
209 | 179 | |||
210 | 180 | fstab_original_content = '' | ||
211 | 181 | fstab_expected_content = ( | ||
212 | 182 | '%s\tnone\tswap\tsw,comment=cloudconfig\t' | ||
213 | 183 | '0\t0\n' % (self.swap_path,) | ||
214 | 184 | ) | ||
215 | 185 | |||
216 | 186 | with open(cc_mounts.FSTAB_PATH, 'w') as fd: | ||
217 | 187 | fd.write(fstab_original_content) | ||
218 | 188 | |||
219 | 189 | cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) | ||
220 | 190 | |||
221 | 191 | with open(cc_mounts.FSTAB_PATH, 'r') as fd: | ||
222 | 192 | fstab_new_content = fd.read() | ||
223 | 193 | self.assertEqual(fstab_expected_content, fstab_new_content) | ||
224 | 194 | |||
225 | 195 | def test_fstab_same_swap_device_already_configured(self): | ||
226 | 196 | '''Ensure that cloud-init will not add a swap device if the same | ||
227 | 197 | device already exists in /etc/fstab.''' | ||
228 | 198 | |||
229 | 199 | fstab_original_content = '%s swap swap defaults 0 0\n' % ( | ||
230 | 200 | self.swap_path,) | ||
231 | 201 | fstab_expected_content = fstab_original_content | ||
232 | 202 | |||
233 | 203 | with open(cc_mounts.FSTAB_PATH, 'w') as fd: | ||
234 | 204 | fd.write(fstab_original_content) | ||
235 | 205 | |||
236 | 206 | cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) | ||
237 | 207 | |||
238 | 208 | with open(cc_mounts.FSTAB_PATH, 'r') as fd: | ||
239 | 209 | fstab_new_content = fd.read() | ||
240 | 210 | self.assertEqual(fstab_expected_content, fstab_new_content) | ||
241 | 211 | |||
242 | 212 | def test_fstab_alternate_swap_device_already_configured(self): | ||
243 | 213 | '''Ensure that cloud-init will add a discovered swap device to | ||
244 | 214 | /etc/fstab even when there exists a swap definition on another | ||
245 | 215 | device.''' | ||
246 | 216 | |||
247 | 217 | fstab_original_content = '/dev/sdc1 swap swap defaults 0 0\n' | ||
248 | 218 | fstab_expected_content = ( | ||
249 | 219 | fstab_original_content + | ||
250 | 220 | '%s\tnone\tswap\tsw,comment=cloudconfig\t' | ||
251 | 221 | '0\t0\n' % (self.swap_path,) | ||
252 | 222 | ) | ||
253 | 223 | |||
254 | 224 | with open(cc_mounts.FSTAB_PATH, 'w') as fd: | ||
255 | 225 | fd.write(fstab_original_content) | ||
256 | 226 | |||
257 | 227 | cc_mounts.handle(None, {}, self.mock_cloud, self.mock_log, []) | ||
258 | 228 | |||
259 | 229 | with open(cc_mounts.FSTAB_PATH, 'r') as fd: | ||
260 | 230 | fstab_new_content = fd.read() | ||
261 | 231 | self.assertEqual(fstab_expected_content, fstab_new_content) | ||
262 | 232 | |||
263 | 137 | # vi: ts=4 expandtab | 233 | # vi: ts=4 expandtab |
PASSED: Continuous integration, rev:288fbfe09ef 5846547e9621225 2c7e8cb8d53fd7 /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 1102/
https:/
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: /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 1102/rebuild
https:/