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

[Ryan Harper]

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.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:521
https://jenkins.ubuntu.com/server/job/curtin-ci/593/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/593
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/593
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/593
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/593
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/593/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/593/rebuild

review: Needs Fixing (continuous-integration)

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

522. By Ryan Harper on 2017-08-10

Handle empty mount option string

[Ryan Harper (raharper)]

vmtest run here:

https://jenkins.ubuntu.com/server/job/curtin-vmtest-devel-debug/18/

[Ryan Harper (raharper)]

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

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:522
https://jenkins.ubuntu.com/server/job/curtin-ci/594/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/594
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/594
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/594
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/594
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/594/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/594/rebuild

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

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

[Ryan Harper (raharper)]

Thanks for the review, will fix and update.

[Chad Smith (chad.smith)]

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

review: Approve

[Ryan Harper (raharper)]

Will fix up a few more things suggested.

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

523. By Ryan Harper on 2017-08-10

Address MP feedback

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:523
https://jenkins.ubuntu.com/server/job/curtin-ci/596/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/596
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/596
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/596
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/596

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/596/rebuild

review: Approve (continuous-integration)

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

524. By Ryan Harper on 2017-08-15

merge from trunk

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:524
https://jenkins.ubuntu.com/server/job/curtin-ci/597/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/597
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/597
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/597
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/597

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/597/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

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.

[Scott Moser (smoser)]

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)

[Ryan Harper (raharper)]

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. ...

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

525. By Ryan Harper on 2017-08-29

attempt to mount storage with options applied to catch possible failures

526. By Ryan Harper on 2017-08-29

Add options via extend

527. By Ryan Harper on 2017-09-01

merge from trunk

[Scott Moser (smoser)]

2 comments inline, then I think I approve.

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

528. By Ryan Harper on 2017-11-27

merge from trunk

529. By Ryan Harper on 2017-11-27

Log mount exception as string, add warning to documentation

[Ryan Harper (raharper)]

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

[Scott Moser (smoser)]

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

[lp:~raharper/curtin/trunk.lp1709284-mount-options]

530. By Ryan Harper on 2017-11-27

doc: replace period with colon because punctuation.

531. By Ryan Harper on 2017-11-27

merge from trunk

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2017-10-27 16:29:07 +0000
+++ curtin/commands/block_meta.py	2017-11-27 19:41:30 +0000
@@ -620,9 +620,27 @@
 
 
 def mount_handler(info, storage_config):
+    """ Handle storage config type: mount
+
+    info = {
+        'id': 'rootfs_mount',
+        'type': 'mount',
+        'path': '/',
+        'options': 'defaults,errors=remount-ro',
+        'device': 'rootfs',
+    }
+
+    Mount specified device under target at 'path' and generate
+    fstab entry.
+    """
     state = util.load_command_environment()
     path = info.get('path')
     filesystem = storage_config.get(info.get('device'))
+    mount_options = info.get('options')
+    # handle unset, or empty('') strings
+    if not mount_options:
+        mount_options = 'defaults'
+
     if not path and filesystem.get('fstype') != "swap":
         raise ValueError("path to mountpoint must be specified")
     volume = storage_config.get(filesystem.get('volume'))
@@ -638,41 +656,50 @@
         mount_point = os.path.sep.join([state['target'], path])
         mount_point = os.path.normpath(mount_point)
 
-        # Create mount point if does not exist
-        util.ensure_dir(mount_point)
-
-        # Mount volume
-        util.subp(['mount', volume_path, mount_point])
-
-        path = "/%s" % path
-
-        options = ["defaults"]
+        options = mount_options.split(",")
         # If the volume_path's kname is backed by iSCSI or (in the case of
         # LVM/DM) if any of its slaves are backed by iSCSI, then we need to
         # append _netdev to the fstab line
         if iscsi.volpath_is_iscsi(volume_path):
             LOG.debug("Marking volume_path:%s as '_netdev'", volume_path)
             options.append("_netdev")
+
+        # Create mount point if does not exist
+        util.ensure_dir(mount_point)
+
+        # Mount volume, with options
+        try:
+            opts = ['-o', ','.join(options)]
+            util.subp(['mount', volume_path, mount_point] + opts, capture=True)
+        except util.ProcessExecutionError as e:
+            LOG.exception(e)
+            msg = ('Mount failed: %s @ %s with options %s' % (volume_path,
+                                                              mount_point,
+                                                              ",".join(opts)))
+            LOG.error(msg)
+            raise RuntimeError(msg)
+
+        # set path
+        path = "/%s" % path
+
     else:
         path = "none"
         options = ["sw"]
 
     # 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,
+                                             ",".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-09-29 16:38:59 +0000
+++ doc/topics/storage.rst	2017-11-27 19:41:30 +0000
@@ -366,12 +366,30 @@
   fstab entry will contain ``_netdev`` to indicate networking is
   required to mount this filesystem.
 
+**options**: *<mount(8) comma-separated options string>*
+
+The ``options`` key will replace the default options value of ``defaults``.
+
+.. warning:: 
+  The kernel and user-space utilities may differ between the install
+  environment and the runtime environment.  Not all kernels and user-space
+  combinations will support all options.  Providing options for a mount point
+  will have both of the following effects:
+
+  - ``curtin`` will mount the filesystems with the provided options during the installation.
+
+  - ``curtin`` will ensure the target OS uses the provided mount options by updating the target OS (/etc/fstab).
+
+  If either of the environments (install or target) do not have support for
+  the provided options, the behavior is undefined.
+
 **Config Example**::
 
  - id: disk0-part1-fs1-mount0
    type: mount
    path: /home
    device: disk0-part1-fs1
+   options: 'noatime,errors=remount-ro'
 
 Lvm Volgroup Command
 ~~~~~~~~~~~~~~~~~~~~

=== modified file 'examples/tests/basic.yaml'
--- examples/tests/basic.yaml	2017-10-11 14:47:43 +0000
+++ examples/tests/basic.yaml	2017-11-27 19:41:30 +0000
@@ -59,6 +59,7 @@
       - id: btrfs_disk_mnt_id
         type: mount
         path: /btrfs
+        options: 'defaults,noatime'
         device: btrfs_disk_fmt_id
       - id: pnum_disk
         type: disk

=== modified file 'examples/tests/basic_scsi.yaml'
--- examples/tests/basic_scsi.yaml	2016-07-30 22:42:26 +0000
+++ examples/tests/basic_scsi.yaml	2017-11-27 19:41:30 +0000
@@ -53,6 +53,7 @@
       - id: btrfs_disk_mnt_id
         type: mount
         path: /btrfs
+        options: 'defaults,noatime'
         device: btrfs_disk_fmt_id
       - id: pnum_disk
         type: disk

=== modified file 'tests/unittests/test_commands_block_meta.py'
--- tests/unittests/test_commands_block_meta.py	2017-10-27 16:29:07 +0000
+++ tests/unittests/test_commands_block_meta.py	2017-11-27 19:41:30 +0000
@@ -115,7 +115,10 @@
         basepath = 'curtin.commands.block_meta.'
         self.add_patch(basepath + 'get_path_to_storage_volume', 'mock_getpath')
         self.add_patch(basepath + 'make_dname', 'mock_make_dname')
+        self.add_patch('curtin.util.load_command_environment',
+                       'mock_load_env')
         self.add_patch('curtin.util.subp', 'mock_subp')
+        self.add_patch('curtin.util.ensure_dir', 'mock_ensure_dir')
         self.add_patch('curtin.block.get_part_table_type',
                        'mock_block_get_part_table_type')
         self.add_patch('curtin.block.wipe_volume',
@@ -130,6 +133,10 @@
                        'mock_clear_holders')
         self.add_patch('curtin.block.clear_holders.assert_clear',
                        'mock_assert_clear')
+        self.add_patch('curtin.block.iscsi.volpath_is_iscsi',
+                       'mock_volpath_is_iscsi')
+        self.add_patch('curtin.block.get_volume_uuid',
+                       'mock_block_get_volume_uuid')
         self.add_patch('curtin.block.zero_file_at_offsets',
                        'mock_block_zero_file')
 
@@ -154,7 +161,20 @@
                      'size': '511705088B',
                      'type': 'partition',
                      'uuid': 'fc7ab24c-b6bf-460f-8446-d3ac362c0625',
-                     'wipe': 'superblock'}
+                     'wipe': 'superblock'},
+                    {'id': 'sda1-root',
+                     'type': 'format',
+                     'fstype': 'xfs',
+                     'volume': 'sda-part1'},
+                    {'id': 'sda-part1-mnt-root',
+                     'type': 'mount',
+                     'path': '/',
+                     'device': 'sda1-root'},
+                    {'id': 'sda-part1-mnt-root-ro',
+                     'type': 'mount',
+                     'path': '/readonly',
+                     'options': 'ro',
+                     'device': 'sda1-root'}
                 ],
             }
         }
@@ -195,10 +215,106 @@
         self.mock_block_sys_block_path.return_value = '/sys/class/block/xxx'
 
         block_meta.partition_handler(part_info, self.storage_config)
-
         part_offset = 2048 * 512
         self.mock_block_zero_file.assert_called_with(disk_kname, [part_offset],
                                                      exclusive=False)
         self.mock_subp.assert_called_with(['parted', disk_kname, '--script',
                                            'mkpart', 'primary', '2048s',
                                            '1001471s'], capture=True)
+
+    @patch('curtin.util.write_file')
+    def test_mount_handler_defaults(self, mock_write_file):
+        """Test mount_handler has defaults to 'defaults' for mount options"""
+        fstab = self.tmp_path('fstab')
+        self.mock_load_env.return_value = {'fstab': fstab,
+                                           'target': self.target}
+        disk_info = self.storage_config.get('sda')
+        fs_info = self.storage_config.get('sda1-root')
+        mount_info = self.storage_config.get('sda-part1-mnt-root')
+
+        self.mock_getpath.return_value = '/wark/xxx'
+        self.mock_volpath_is_iscsi.return_value = False
+        self.mock_block_get_volume_uuid.return_value = None
+
+        block_meta.mount_handler(mount_info, self.storage_config)
+        options = 'defaults'
+        expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
+                                          mount_info['path'],
+                                          fs_info['fstype'], options)
+
+        mock_write_file.assert_called_with(fstab, expected, omode='a')
+
+    @patch('curtin.util.write_file')
+    def test_mount_handler_uses_mount_options(self, mock_write_file):
+        """Test mount_handler 'options' string is present in fstab entry"""
+        fstab = self.tmp_path('fstab')
+        self.mock_load_env.return_value = {'fstab': fstab,
+                                           'target': self.target}
+        disk_info = self.storage_config.get('sda')
+        fs_info = self.storage_config.get('sda1-root')
+        mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
+
+        self.mock_getpath.return_value = '/wark/xxx'
+        self.mock_volpath_is_iscsi.return_value = False
+        self.mock_block_get_volume_uuid.return_value = None
+
+        block_meta.mount_handler(mount_info, self.storage_config)
+        options = 'ro'
+        expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
+                                          mount_info['path'],
+                                          fs_info['fstype'], options)
+
+        mock_write_file.assert_called_with(fstab, expected, omode='a')
+
+    @patch('curtin.util.write_file')
+    def test_mount_handler_empty_options_string(self, mock_write_file):
+        """Test mount_handler with empty 'options' string, selects defaults"""
+        fstab = self.tmp_path('fstab')
+        self.mock_load_env.return_value = {'fstab': fstab,
+                                           'target': self.target}
+        disk_info = self.storage_config.get('sda')
+        fs_info = self.storage_config.get('sda1-root')
+        mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
+        mount_info['options'] = ''
+
+        self.mock_getpath.return_value = '/wark/xxx'
+        self.mock_volpath_is_iscsi.return_value = False
+        self.mock_block_get_volume_uuid.return_value = None
+
+        block_meta.mount_handler(mount_info, self.storage_config)
+        options = 'defaults'
+        expected = "%s %s %s %s 0 0\n" % (disk_info['path'],
+                                          mount_info['path'],
+                                          fs_info['fstype'], options)
+
+        mock_write_file.assert_called_with(fstab, expected, omode='a')
+
+    def test_mount_handler_appends_to_fstab(self):
+        """Test mount_handler appends fstab lines to existing file"""
+        fstab = self.tmp_path('fstab')
+        with open(fstab, 'w') as fh:
+            fh.write("#curtin-test\n")
+
+        self.mock_load_env.return_value = {'fstab': fstab,
+                                           'target': self.target}
+        disk_info = self.storage_config.get('sda')
+        fs_info = self.storage_config.get('sda1-root')
+        mount_info = self.storage_config.get('sda-part1-mnt-root-ro')
+        mount_info['options'] = ''
+
+        self.mock_getpath.return_value = '/wark/xxx'
+        self.mock_volpath_is_iscsi.return_value = False
+        self.mock_block_get_volume_uuid.return_value = None
+
+        block_meta.mount_handler(mount_info, self.storage_config)
+        options = 'defaults'
+        expected = "#curtin-test\n%s %s %s %s 0 0\n" % (disk_info['path'],
+                                                        mount_info['path'],
+                                                        fs_info['fstype'],
+                                                        options)
+
+        with open(fstab, 'r') as fh:
+            rendered_fstab = fh.read()
+
+        print(rendered_fstab)
+        self.assertEqual(rendered_fstab, expected)

=== modified file 'tests/vmtests/test_basic.py'
--- tests/vmtests/test_basic.py	2017-11-03 21:27:17 +0000
+++ tests/vmtests/test_basic.py	2017-11-27 19:41:30 +0000
@@ -104,6 +104,7 @@
                 break
         self.assertIsNotNone(fstab_entry)
         self.assertEqual(fstab_entry.split(' ')[1], "/btrfs")
+        self.assertEqual(fstab_entry.split(' ')[3], "defaults,noatime")
 
     def test_whole_disk_format(self):
         # confirm the whole disk format is the expected device
@@ -237,7 +238,7 @@
         self.assertIsNotNone(fstab_entry)
         self.assertEqual(fstab_entry.split(' ')[1], "/home")
 
-        # Test whole disk sdc is mounted at /btrfs
+        # Test whole disk sdc is mounted at /btrfs, and uses defaults,noatime
         uuid = self._kname_to_uuid('sdc')
         fstab_entry = None
         for line in fstab_lines:
@@ -246,6 +247,7 @@
                 break
         self.assertIsNotNone(fstab_entry)
         self.assertEqual(fstab_entry.split(' ')[1], "/btrfs")
+        self.assertEqual(fstab_entry.split(' ')[3], "defaults,noatime")
 
     def test_whole_disk_format(self):
         # confirm the whole disk format is the expected device