Merge lp:~raharper/curtin/trunk.lp1709284-mount-options into lp:~curtin-dev/curtin/trunk

Proposed by Ryan Harper
Status: Merged
Merged at revision: 544
Proposed branch: lp:~raharper/curtin/trunk.lp1709284-mount-options
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 349 lines (+191/-26)
6 files modified
curtin/commands/block_meta.py (+50/-23)
doc/topics/storage.rst (+18/-0)
examples/tests/basic.yaml (+1/-0)
examples/tests/basic_scsi.yaml (+1/-0)
tests/unittests/test_commands_block_meta.py (+118/-2)
tests/vmtests/test_basic.py (+3/-1)
To merge this branch: bzr merge lp:~raharper/curtin/trunk.lp1709284-mount-options
Reviewer Review Type Date Requested Status
Scott Moser Approve
Server Team CI bot continuous-integration Approve
Chad Smith Approve
Review via email: mp+328814@code.launchpad.net

Description of the change

storage: add 'options' key to the mount storage type to specify mount parameters in fstab

Add support, documentation, unittests and vmtests to deliver a new parameter to the mount configuration dictionary. This parameter if set will modify the the mount options field in the rendered /etc/fstab.

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
522. By Ryan Harper

Handle empty mount option string

Revision history for this message
Ryan Harper (raharper) wrote :
Revision history for this message
Ryan Harper (raharper) wrote :

The CI failure looks to be related to infrastructure, not the branch:

+ tox
/tmp/hudson2703813631282842425.sh: line 9: tox: command not found
Build step 'Execute shell' marked build as failure

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Chad Smith (chad.smith) wrote :

looks pretty good, checking a few other things but here are some inline comments.

Revision history for this message
Ryan Harper (raharper) wrote :

Thanks for the review, will fix and update.

Revision history for this message
Chad Smith (chad.smith) wrote :

additional unittest/doc tweaks inline and we are a +1.

review: Approve
Revision history for this message
Ryan Harper (raharper) wrote :

Will fix up a few more things suggested.

523. By Ryan Harper

Address MP feedback

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
524. By Ryan Harper

merge from trunk

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

generally fine with this, but i would like some warnings on this section.
its basically impossible to do this without somewhat deep knowledge of your target OS.

Also, I suppose that curtin probably *should* mount the filesystem with the options that are provided when it mounts the filesystem. Other wise, it would mount the filesystem during install with one set of options and then the boot would mount with a different set. That could potentially be hazardous.

Revision history for this message
Scott Moser (smoser) wrote :

Note, that if curtin did mount with the options provided, then it would require the ephemeral' environment's kernel to support these options which could fail. (in maas, any custom OS such as centos will be installed with the ephemeral environment of Ubuntu)

Revision history for this message
Ryan Harper (raharper) wrote :
Download full text (3.5 KiB)

On Fri, Aug 18, 2017 at 3:21 PM, Scott Moser <email address hidden> wrote:

> generally fine with this, but i would like some warnings on this section.
>

Something generic along mount options might be incompatible between OS
releases
or more specific about certain ones?

> its basically impossible to do this without somewhat deep knowledge of
> your target OS.
>

Indeed.

>
> Also, I suppose that curtin probably *should* mount the filesystem with
> the options that are provided when it mounts the filesystem. Other wise,
> it would mount the filesystem during install with one set of options and
> then the boot would mount with a different set. That could potentially be
> hazardous.
>

That's a really good point.

>
>
> Diff comments:
>
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py 2017-05-26 14:07:29 +0000
> > +++ curtin/commands/block_meta.py 2017-08-15 19:45:43 +0000
> > @@ -656,20 +674,18 @@
> >
> > # Add volume to fstab
> > if state['fstab']:
> > - with open(state['fstab'], "a") as fp:
> > - location = get_path_to_storage_volume(volume.get('id'),
> > - storage_config)
> > - uuid = block.get_volume_uuid(volume_path)
> > - if len(uuid) > 0:
> > - location = "UUID=%s" % uuid
> > -
> > - if filesystem.get('fstype') in ["fat", "fat12", "fat16",
> "fat32",
> > - "fat64"]:
> > - fstype = "vfat"
> > - else:
> > - fstype = filesystem.get('fstype')
> > - fp.write("%s %s %s %s 0 0\n" % (location, path, fstype,
> > - ",".join(options)))
> > + uuid = block.get_volume_uuid(volume_path)
> > + location = ("UUID=%s" % uuid) if uuid else (
> > + get_path_to_storage_volume(volume.get('id'),
> > + storage_config))
> > +
> > + fstype = filesystem.get('fstype')
> > + if fstype in ["fat", "fat12", "fat16", "fat32", "fat64"]:
> > + fstype = "vfat"
> > +
> > + fstab_entry = "%s %s %s %s 0 0\n" % (location, path, fstype,
>
> seems better as ' '.join([location, path, fstype, ",".join(options), "0",
> "0")
>
> then you could also '\t'.join()
>
> > + ",".join(options))
> > + util.write_file(state['fstab'], fstab_entry, omode='a')
> > else:
> > LOG.info("fstab not in environment, so not writing")
> >
> >
> > === modified file 'doc/topics/storage.rst'
> > --- doc/topics/storage.rst 2017-04-21 17:36:27 +0000
> > +++ doc/topics/storage.rst 2017-08-15 19:45:43 +0000
> > @@ -358,6 +358,10 @@
> > config. The target device must already contain a valid filesystem and be
> > accessible.
> >
> > +**options**: *<mount(8) comma-separated options string>*
> > +
> > +The ``options`` key will replace the default options value of
> ``defaults``.
> > +
>
> lets mention here that options are often times OS or OS version specific,
> and behavior of non-implemented options is not well defined. ...

Read more...

525. By Ryan Harper

attempt to mount storage with options applied to catch possible failures

526. By Ryan Harper

Add options via extend

527. By Ryan Harper

merge from trunk

Revision history for this message
Scott Moser (smoser) wrote :

2 comments inline, then I think I approve.

528. By Ryan Harper

merge from trunk

529. By Ryan Harper

Log mount exception as string, add warning to documentation

Revision history for this message
Ryan Harper (raharper) wrote :

Ran a vmtest over test_basic which now checks/validates mount options.

Ran 96 tests in 303.208s

OK
Mon, 27 Nov 2017 18:14:53 +0000: vmtest end [0] in 305s

https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-debug/22/console

Revision history for this message
Scott Moser (smoser) wrote :

please fix one typo-ish
 - will have both of the following effects.
 + will have both of the following effects:

other than that approve.

review: Approve
530. By Ryan Harper

doc: replace period with colon because punctuation.

531. By Ryan Harper

merge from trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'curtin/commands/block_meta.py'
2--- curtin/commands/block_meta.py 2017-10-27 16:29:07 +0000
3+++ curtin/commands/block_meta.py 2017-11-27 19:41:30 +0000
4@@ -620,9 +620,27 @@
5
6
7 def mount_handler(info, storage_config):
8+ """ Handle storage config type: mount
9+
10+ info = {
11+ 'id': 'rootfs_mount',
12+ 'type': 'mount',
13+ 'path': '/',
14+ 'options': 'defaults,errors=remount-ro',
15+ 'device': 'rootfs',
16+ }
17+
18+ Mount specified device under target at 'path' and generate
19+ fstab entry.
20+ """
21 state = util.load_command_environment()
22 path = info.get('path')
23 filesystem = storage_config.get(info.get('device'))
24+ mount_options = info.get('options')
25+ # handle unset, or empty('') strings
26+ if not mount_options:
27+ mount_options = 'defaults'
28+
29 if not path and filesystem.get('fstype') != "swap":
30 raise ValueError("path to mountpoint must be specified")
31 volume = storage_config.get(filesystem.get('volume'))
32@@ -638,41 +656,50 @@
33 mount_point = os.path.sep.join([state['target'], path])
34 mount_point = os.path.normpath(mount_point)
35
36- # Create mount point if does not exist
37- util.ensure_dir(mount_point)
38-
39- # Mount volume
40- util.subp(['mount', volume_path, mount_point])
41-
42- path = "/%s" % path
43-
44- options = ["defaults"]
45+ options = mount_options.split(",")
46 # If the volume_path's kname is backed by iSCSI or (in the case of
47 # LVM/DM) if any of its slaves are backed by iSCSI, then we need to
48 # append _netdev to the fstab line
49 if iscsi.volpath_is_iscsi(volume_path):
50 LOG.debug("Marking volume_path:%s as '_netdev'", volume_path)
51 options.append("_netdev")
52+
53+ # Create mount point if does not exist
54+ util.ensure_dir(mount_point)
55+
56+ # Mount volume, with options
57+ try:
58+ opts = ['-o', ','.join(options)]
59+ util.subp(['mount', volume_path, mount_point] + opts, capture=True)
60+ except util.ProcessExecutionError as e:
61+ LOG.exception(e)
62+ msg = ('Mount failed: %s @ %s with options %s' % (volume_path,
63+ mount_point,
64+ ",".join(opts)))
65+ LOG.error(msg)
66+ raise RuntimeError(msg)
67+
68+ # set path
69+ path = "/%s" % path
70+
71 else:
72 path = "none"
73 options = ["sw"]
74
75 # Add volume to fstab
76 if state['fstab']:
77- with open(state['fstab'], "a") as fp:
78- location = get_path_to_storage_volume(volume.get('id'),
79- storage_config)
80- uuid = block.get_volume_uuid(volume_path)
81- if len(uuid) > 0:
82- location = "UUID=%s" % uuid
83-
84- if filesystem.get('fstype') in ["fat", "fat12", "fat16", "fat32",
85- "fat64"]:
86- fstype = "vfat"
87- else:
88- fstype = filesystem.get('fstype')
89- fp.write("%s %s %s %s 0 0\n" % (location, path, fstype,
90- ",".join(options)))
91+ uuid = block.get_volume_uuid(volume_path)
92+ location = ("UUID=%s" % uuid) if uuid else (
93+ get_path_to_storage_volume(volume.get('id'),
94+ storage_config))
95+
96+ fstype = filesystem.get('fstype')
97+ if fstype in ["fat", "fat12", "fat16", "fat32", "fat64"]:
98+ fstype = "vfat"
99+
100+ fstab_entry = "%s %s %s %s 0 0\n" % (location, path, fstype,
101+ ",".join(options))
102+ util.write_file(state['fstab'], fstab_entry, omode='a')
103 else:
104 LOG.info("fstab not in environment, so not writing")
105
106
107=== modified file 'doc/topics/storage.rst'
108--- doc/topics/storage.rst 2017-09-29 16:38:59 +0000
109+++ doc/topics/storage.rst 2017-11-27 19:41:30 +0000
110@@ -366,12 +366,30 @@
111 fstab entry will contain ``_netdev`` to indicate networking is
112 required to mount this filesystem.
113
114+**options**: *<mount(8) comma-separated options string>*
115+
116+The ``options`` key will replace the default options value of ``defaults``.
117+
118+.. warning::
119+ The kernel and user-space utilities may differ between the install
120+ environment and the runtime environment. Not all kernels and user-space
121+ combinations will support all options. Providing options for a mount point
122+ will have both of the following effects:
123+
124+ - ``curtin`` will mount the filesystems with the provided options during the installation.
125+
126+ - ``curtin`` will ensure the target OS uses the provided mount options by updating the target OS (/etc/fstab).
127+
128+ If either of the environments (install or target) do not have support for
129+ the provided options, the behavior is undefined.
130+
131 **Config Example**::
132
133 - id: disk0-part1-fs1-mount0
134 type: mount
135 path: /home
136 device: disk0-part1-fs1
137+ options: 'noatime,errors=remount-ro'
138
139 Lvm Volgroup Command
140 ~~~~~~~~~~~~~~~~~~~~
141
142=== modified file 'examples/tests/basic.yaml'
143--- examples/tests/basic.yaml 2017-10-11 14:47:43 +0000
144+++ examples/tests/basic.yaml 2017-11-27 19:41:30 +0000
145@@ -59,6 +59,7 @@
146 - id: btrfs_disk_mnt_id
147 type: mount
148 path: /btrfs
149+ options: 'defaults,noatime'
150 device: btrfs_disk_fmt_id
151 - id: pnum_disk
152 type: disk
153
154=== modified file 'examples/tests/basic_scsi.yaml'
155--- examples/tests/basic_scsi.yaml 2016-07-30 22:42:26 +0000
156+++ examples/tests/basic_scsi.yaml 2017-11-27 19:41:30 +0000
157@@ -53,6 +53,7 @@
158 - id: btrfs_disk_mnt_id
159 type: mount
160 path: /btrfs
161+ options: 'defaults,noatime'
162 device: btrfs_disk_fmt_id
163 - id: pnum_disk
164 type: disk
165
166=== modified file 'tests/unittests/test_commands_block_meta.py'
167--- tests/unittests/test_commands_block_meta.py 2017-10-27 16:29:07 +0000
168+++ tests/unittests/test_commands_block_meta.py 2017-11-27 19:41:30 +0000
169@@ -115,7 +115,10 @@
170 basepath = 'curtin.commands.block_meta.'
171 self.add_patch(basepath + 'get_path_to_storage_volume', 'mock_getpath')
172 self.add_patch(basepath + 'make_dname', 'mock_make_dname')
173+ self.add_patch('curtin.util.load_command_environment',
174+ 'mock_load_env')
175 self.add_patch('curtin.util.subp', 'mock_subp')
176+ self.add_patch('curtin.util.ensure_dir', 'mock_ensure_dir')
177 self.add_patch('curtin.block.get_part_table_type',
178 'mock_block_get_part_table_type')
179 self.add_patch('curtin.block.wipe_volume',
180@@ -130,6 +133,10 @@
181 'mock_clear_holders')
182 self.add_patch('curtin.block.clear_holders.assert_clear',
183 'mock_assert_clear')
184+ self.add_patch('curtin.block.iscsi.volpath_is_iscsi',
185+ 'mock_volpath_is_iscsi')
186+ self.add_patch('curtin.block.get_volume_uuid',
187+ 'mock_block_get_volume_uuid')
188 self.add_patch('curtin.block.zero_file_at_offsets',
189 'mock_block_zero_file')
190
191@@ -154,7 +161,20 @@
192 'size': '511705088B',
193 'type': 'partition',
194 'uuid': 'fc7ab24c-b6bf-460f-8446-d3ac362c0625',
195- 'wipe': 'superblock'}
196+ 'wipe': 'superblock'},
197+ {'id': 'sda1-root',
198+ 'type': 'format',
199+ 'fstype': 'xfs',
200+ 'volume': 'sda-part1'},
201+ {'id': 'sda-part1-mnt-root',
202+ 'type': 'mount',
203+ 'path': '/',
204+ 'device': 'sda1-root'},
205+ {'id': 'sda-part1-mnt-root-ro',
206+ 'type': 'mount',
207+ 'path': '/readonly',
208+ 'options': 'ro',
209+ 'device': 'sda1-root'}
210 ],
211 }
212 }
213@@ -195,10 +215,106 @@
214 self.mock_block_sys_block_path.return_value = '/sys/class/block/xxx'
215
216 block_meta.partition_handler(part_info, self.storage_config)
217-
218 part_offset = 2048 * 512
219 self.mock_block_zero_file.assert_called_with(disk_kname, [part_offset],
220 exclusive=False)
221 self.mock_subp.assert_called_with(['parted', disk_kname, '--script',
222 'mkpart', 'primary', '2048s',
223 '1001471s'], capture=True)
224+
225+ @patch('curtin.util.write_file')
226+ def test_mount_handler_defaults(self, mock_write_file):
227+ """Test mount_handler has defaults to 'defaults' for mount options"""
228+ fstab = self.tmp_path('fstab')
229+ self.mock_load_env.return_value = {'fstab': fstab,
230+ 'target': self.target}
231+ disk_info = self.storage_config.get('sda')
232+ fs_info = self.storage_config.get('sda1-root')
233+ mount_info = self.storage_config.get('sda-part1-mnt-root')
234+
235+ self.mock_getpath.return_value = '/wark/xxx'
236+ self.mock_volpath_is_iscsi.return_value = False
237+ self.mock_block_get_volume_uuid.return_value = None
238+
239+ block_meta.mount_handler(mount_info, self.storage_config)
240+ options = 'defaults'
241+ expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
242+ mount_info['path'],
243+ fs_info['fstype'], options)
244+
245+ mock_write_file.assert_called_with(fstab, expected, omode='a')
246+
247+ @patch('curtin.util.write_file')
248+ def test_mount_handler_uses_mount_options(self, mock_write_file):
249+ """Test mount_handler 'options' string is present in fstab entry"""
250+ fstab = self.tmp_path('fstab')
251+ self.mock_load_env.return_value = {'fstab': fstab,
252+ 'target': self.target}
253+ disk_info = self.storage_config.get('sda')
254+ fs_info = self.storage_config.get('sda1-root')
255+ mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
256+
257+ self.mock_getpath.return_value = '/wark/xxx'
258+ self.mock_volpath_is_iscsi.return_value = False
259+ self.mock_block_get_volume_uuid.return_value = None
260+
261+ block_meta.mount_handler(mount_info, self.storage_config)
262+ options = 'ro'
263+ expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
264+ mount_info['path'],
265+ fs_info['fstype'], options)
266+
267+ mock_write_file.assert_called_with(fstab, expected, omode='a')
268+
269+ @patch('curtin.util.write_file')
270+ def test_mount_handler_empty_options_string(self, mock_write_file):
271+ """Test mount_handler with empty 'options' string, selects defaults"""
272+ fstab = self.tmp_path('fstab')
273+ self.mock_load_env.return_value = {'fstab': fstab,
274+ 'target': self.target}
275+ disk_info = self.storage_config.get('sda')
276+ fs_info = self.storage_config.get('sda1-root')
277+ mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
278+ mount_info['options'] = ''
279+
280+ self.mock_getpath.return_value = '/wark/xxx'
281+ self.mock_volpath_is_iscsi.return_value = False
282+ self.mock_block_get_volume_uuid.return_value = None
283+
284+ block_meta.mount_handler(mount_info, self.storage_config)
285+ options = 'defaults'
286+ expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
287+ mount_info['path'],
288+ fs_info['fstype'], options)
289+
290+ mock_write_file.assert_called_with(fstab, expected, omode='a')
291+
292+ def test_mount_handler_appends_to_fstab(self):
293+ """Test mount_handler appends fstab lines to existing file"""
294+ fstab = self.tmp_path('fstab')
295+ with open(fstab, 'w') as fh:
296+ fh.write("#curtin-test\n")
297+
298+ self.mock_load_env.return_value = {'fstab': fstab,
299+ 'target': self.target}
300+ disk_info = self.storage_config.get('sda')
301+ fs_info = self.storage_config.get('sda1-root')
302+ mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
303+ mount_info['options'] = ''
304+
305+ self.mock_getpath.return_value = '/wark/xxx'
306+ self.mock_volpath_is_iscsi.return_value = False
307+ self.mock_block_get_volume_uuid.return_value = None
308+
309+ block_meta.mount_handler(mount_info, self.storage_config)
310+ options = 'defaults'
311+ expected = "#curtin-test\n%s %s %s %s 0 0\n" % (disk_info['path'],
312+ mount_info['path'],
313+ fs_info['fstype'],
314+ options)
315+
316+ with open(fstab, 'r') as fh:
317+ rendered_fstab = fh.read()
318+
319+ print(rendered_fstab)
320+ self.assertEqual(rendered_fstab, expected)
321
322=== modified file 'tests/vmtests/test_basic.py'
323--- tests/vmtests/test_basic.py 2017-11-03 21:27:17 +0000
324+++ tests/vmtests/test_basic.py 2017-11-27 19:41:30 +0000
325@@ -104,6 +104,7 @@
326 break
327 self.assertIsNotNone(fstab_entry)
328 self.assertEqual(fstab_entry.split(' ')[1], "/btrfs")
329+ self.assertEqual(fstab_entry.split(' ')[3], "defaults,noatime")
330
331 def test_whole_disk_format(self):
332 # confirm the whole disk format is the expected device
333@@ -237,7 +238,7 @@
334 self.assertIsNotNone(fstab_entry)
335 self.assertEqual(fstab_entry.split(' ')[1], "/home")
336
337- # Test whole disk sdc is mounted at /btrfs
338+ # Test whole disk sdc is mounted at /btrfs, and uses defaults,noatime
339 uuid = self._kname_to_uuid('sdc')
340 fstab_entry = None
341 for line in fstab_lines:
342@@ -246,6 +247,7 @@
343 break
344 self.assertIsNotNone(fstab_entry)
345 self.assertEqual(fstab_entry.split(' ')[1], "/btrfs")
346+ self.assertEqual(fstab_entry.split(' ')[3], "defaults,noatime")
347
348 def test_whole_disk_format(self):
349 # confirm the whole disk format is the expected device

Subscribers

People subscribed via source and target branches