Merge ~d-info-e/cloud-init:resizefs-on-zfs-root into cloud-init:master

[do3meli]

FreeBSD: resizefs module now able to handle zfs/zpool.

Previously there was no support at all for zfs file system. With this
change it is now possible to use the resizefs module to grow a zpool to
its maximum partition size on FreeBSD.

LP: #1721243

this change adds the possibility to use resizefs module to extend a zfs file system on a corresponding zpool.

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

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/800/
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/800/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Hi,
I'd like to see some unit tests added on this.

Definitely for the newly added path, but also for the old path.
I know that its painful to be asked to add unit tests for
code that you didnt' write, but the fact is I'm afraid that
merging these changes will break non-zfs without tests to show.

review: Needs Fixing

[do3meli (d-info-e)]

hi scott,
i have re factored the original merge request and added some tests. can you have a look at it again and trigger a jenkins build?
cheers dom

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

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/928/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

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

review: Needs Fixing (continuous-integration)

[do3meli (d-info-e)]

i have rebased my branch - please re-run

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

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/929/
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/929/rebuild

review: Approve (continuous-integration)

[do3meli (d-info-e)]

any other comments? if not i will do some "manual" integration test against a FreeBSD to see if that really works or not.

[Scott Moser (smoser)]

lets change the commit message to indicate this is only adding
support FreeBSD zpool/zfs. As at best other distro are untested.

In curtin, we recently added zfs root support, so
we need to make sure that this doesnt raise error with zfs on zpool
at least on Ubuntu. while this is 'Experiemental', I dont want
cloud-init to show it failed on such a deployment.

 https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-amd64/345/artifact/output/BionicTestZfsRoot/collect/

^ has some files collected form a zfs root.
most useful likely is /proc/mounts (http://paste.ubuntu.com/p/JnBBhmFgt9/)
 https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-amd64/345/artifact/output/BionicTestZfsRoot/collect/proc_mounts/*view*/

I'm pretty sure it should not regress linux, as cc_resize_fs calls
'get_mount_info'. On linux that will take the 'parse_mount_info' path in,
and you have not modified that path here.

So I'm mostly OK with this as it seems confined to zfs on FreeBSD.

[Scott Moser (smoser)]

review: Approve

[do3meli (d-info-e)]

the commit message has been adjusted by myself and Scott.

my "manual" cloud-init test on FreeBSD in my environment did NOT run good with this branch. i got the following log:

2018-03-26 15:41:16,159 - util.py[WARNING]: Running module resizefs (<module 'cloudinit.config.cc_resizefs' from '/usr/local/lib/python2.7/site-packages/cloud_init-18.1-py2.7.egg/cloudinit/config/cc_resizefs.pyc'>) failed

will investigate this further tomorrow.

i am still trying to think through the curtin stuff you mentioned. will get back on this tomorrow.

7986954... by do3meli on 2018-03-27

parse_mount() now using correct if/else for FreeBSD with zpools

[do3meli (d-info-e)]

the last commit fixes the problem mentioned above in my environment. i haven't run into this problem with unit tests as i mocked the return value and did not follow the logic in the code. another zfs exclusion in the if/else construct fixed it now.

regarding the curtin stuff. i see it the same way as you:
the cc_resizefs module calls 'get_mount_info' which then on linux will call the 'parse_mount_info' function. as i haven't adjusted the get_mount_info nor the parse_mount_info functions it will at least not break any existing system. on the other hand as you mentioned as well it is not guaranteed to be working for zfs/zpool on ubuntu for example.

so i am good to go ahead with this now. please re-trigger a CI run.

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

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/933/
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/933/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Thank you for your merge proposal.

Your branch has been set to 'Work in progress'.
Please set the branch back to 'Needs Review' after resolving the issues below.

Thanks again,
Your friendly neighborhood cloud-init robot.

Please fix the following issues:
------------------------------
Commit message lints:
 - Line #2 has 106 too many characters. Line starts with: "Previously there was"...
------------------------------

For more information, see commit message guidelines at
https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-for-each-feature-or-bug

review: Needs Fixing

[Scott Moser (smoser)]

review: Approve

[Scott Moser (smoser)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=20e3ddab

diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
index cec22bb..c8e1752 100644
--- a/cloudinit/config/cc_resizefs.py
+++ b/cloudinit/config/cc_resizefs.py
@@ -84,6 +84,10 @@ def _resize_ufs(mount_point, devpth):
     return ('growfs', devpth)
 
 
+def _resize_zfs(mount_point, devpth):
+    return ('zpool', 'online', '-e', mount_point, devpth)
+
+
 def _get_dumpfs_output(mount_point):
     dumpfs_res, err = util.subp(['dumpfs', '-m', mount_point])
     return dumpfs_res
@@ -148,6 +152,7 @@ RESIZE_FS_PREFIXES_CMDS = [
     ('ext', _resize_ext),
     ('xfs', _resize_xfs),
     ('ufs', _resize_ufs),
+    ('zfs', _resize_zfs),
 ]
 
 RESIZE_FS_PRECHECK_CMDS = {
@@ -188,6 +193,13 @@ def maybe_get_writable_device_path(devpath, info, log):
         log.debug("Not attempting to resize devpath '%s': %s", devpath, info)
         return None
 
+    # FreeBSD zpool can also just use gpt/<label>
+    # with that in mind we can not do an os.stat on "gpt/whatever"
+    # therefore return the devpath already here.
+    if devpath.startswith('gpt/'):
+        log.debug('We have a gpt label - just go ahead')
+        return devpath
+
     try:
         statret = os.stat(devpath)
     except OSError as exc:
@@ -231,6 +243,16 @@ def handle(name, cfg, _cloud, log, args):
 
     (devpth, fs_type, mount_point) = result
 
+    # if we have a zfs then our device path at this point
+    # is the zfs label. For example: vmzroot/ROOT/freebsd
+    # we will have to get the zpool name out of this
+    # and set the resize_what variable to the zpool
+    # so the _resize_zfs function gets the right attribute.
+    if fs_type == 'zfs':
+        zpool = devpth.split('/')[0]
+        devpth = util.get_device_info_from_zpool(zpool)
+        resize_what = zpool
+
     info = "dev=%s mnt_point=%s path=%s" % (devpth, mount_point, resize_what)
     log.debug("resize_info: %s" % info)
 
diff --git a/cloudinit/util.py b/cloudinit/util.py
index fb4ee5f..0ab2c48 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2234,7 +2234,7 @@ def get_path_dev_freebsd(path, mnt_list):
     return path_found
 
 
-def get_mount_info_freebsd(path, log=LOG):
+def get_mount_info_freebsd(path):
     (result, err) = subp(['mount', '-p', path], rcs=[0, 1])
     if len(err):
         # find a path if the input is not a mounting point
@@ -2248,23 +2248,49 @@ def get_mount_info_freebsd(path, log=LOG):
     return "/dev/" + label_part, ret[2], ret[1]
 
 
+def get_device_info_from_zpool(zpool):
+    (zpoolstatus, err) = subp(['zpool', 'status', zpool])
+    if len(err):
+        return None
+    r = r'.*(ONLINE).*'
+    for line in zpoolstatus.split("\n"):
+        if re.search(r, line) and zpool not in line and "state" not in line:
+            disk = line.split()[0]
+            LOG.debug('found zpool "%s" on disk %s', zpool, disk)
+            return disk
+
+
 def parse_mount(path):
-    (mountoutput, _err) = subp("mount")
+    (mountoutput, _err) = subp(['mount'])
     mount_locs = mountoutput.splitlines()
+    # there are 2 types of mount outputs we have to parse therefore
+    # the regex is a bit complex. to better understand this regex see:
+    # https://regex101.com/r/2F6c1k/1
+    # https://regex101.com/r/T2en7a/1
+    regex = r'^(/dev/[\S]+|.*zroot\S*?) on (/[\S]*) ' + \
+            '(?=(?:type)[\s]+([\S]+)|\(([^,]*))'
     for line in mount_locs:
-        m = re.search(r'^(/dev/[\S]+) on (/.*) \((.+), .+, (.+)\)$', line)
+        m = re.search(regex, line)
         if not m:
             continue
+        devpth = m.group(1)
+        mount_point = m.group(2)
+        # above regex will either fill the fs_type in group(3)
+        # or group(4) depending on the format we have.
+        fs_type = m.group(3)
+        if fs_type is None:
+            fs_type = m.group(4)
+        LOG.debug('found line in mount -> devpth: %s, mount_point: %s, '
+                  'fs_type: %s', devpth, mount_point, fs_type)
         # check whether the dev refers to a label on FreeBSD
         # for example, if dev is '/dev/label/rootfs', we should
         # continue finding the real device like '/dev/da0'.
-        devm = re.search('^(/dev/.+)p([0-9])$', m.group(1))
-        if (not devm and is_FreeBSD()):
+        # this is only valid for non zfs file systems as a zpool
+        # can have gpt labels as disk.
+        devm = re.search('^(/dev/.+)p([0-9])$', devpth)
+        if not devm and is_FreeBSD() and fs_type != 'zfs':
             return get_mount_info_freebsd(path)
-        devpth = m.group(1)
-        mount_point = m.group(2)
-        fs_type = m.group(3)
-        if mount_point == path:
+        elif mount_point == path:
             return devpth, fs_type, mount_point
     return None
 
diff --git a/tests/data/mount_parse_ext.txt b/tests/data/mount_parse_ext.txt
new file mode 100644
index 0000000..da0c870
--- /dev/null
+++ b/tests/data/mount_parse_ext.txt
@@ -0,0 +1,19 @@
+/dev/mapper/vg00-lv_root on / type ext4 (rw,errors=remount-ro)
+proc on /proc type proc (rw,noexec,nosuid,nodev)
+sysfs on /sys type sysfs (rw,noexec,nosuid,nodev)
+none on /sys/fs/cgroup type tmpfs (rw)
+none on /sys/fs/fuse/connections type fusectl (rw)
+none on /sys/kernel/debug type debugfs (rw)
+none on /sys/kernel/security type securityfs (rw)
+udev on /dev type devtmpfs (rw,mode=0755)
+devpts on /dev/pts type devpts (rw,noexec,nosuid,gid=5,mode=0620)
+none on /tmp type tmpfs (rw)
+tmpfs on /run type tmpfs (rw,noexec,nosuid,size=10%,mode=0755)
+none on /run/lock type tmpfs (rw,noexec,nosuid,nodev,size=5242880)
+none on /run/shm type tmpfs (rw,nosuid,nodev)
+none on /run/user type tmpfs (rw,noexec,nosuid,nodev,size=104857600,mode=0755)
+none on /sys/fs/pstore type pstore (rw)
+/dev/mapper/vg00-lv_var on /var type ext4 (rw)
+rpc_pipefs on /run/rpc_pipefs type rpc_pipefs (rw)
+systemd on /sys/fs/cgroup/systemd type cgroup (rw,noexec,nosuid,nodev,none,name=systemd)
+10.0.1.1:/backup on /backup type nfs (rw,noexec,nosuid,nodev,bg,nolock,tcp,nfsvers=3,hard,addr=10.0.1.1)
\ No newline at end of file
diff --git a/tests/data/mount_parse_zfs.txt b/tests/data/mount_parse_zfs.txt
new file mode 100644
index 0000000..08af04f
--- /dev/null
+++ b/tests/data/mount_parse_zfs.txt
@@ -0,0 +1,21 @@
+vmzroot/ROOT/freebsd on / (zfs, local, nfsv4acls)
+devfs on /dev (devfs, local, multilabel)
+fdescfs on /dev/fd (fdescfs)
+vmzroot/root on /root (zfs, local, nfsv4acls)
+vmzroot/tmp on /tmp (zfs, local, nosuid, nfsv4acls)
+vmzroot/ROOT/freebsd/usr on /usr (zfs, local, nfsv4acls)
+vmzroot/ROOT/freebsd/usr/local on /usr/local (zfs, local, nfsv4acls)
+vmzroot/ROOT/freebsd/var on /var (zfs, local, nfsv4acls)
+vmzroot/ROOT/freebsd/var/cache on /var/cache (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/ROOT/freebsd/var/crash on /var/crash (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/var/cron on /var/cron (zfs, local, nosuid, nfsv4acls)
+vmzroot/ROOT/freebsd/var/db on /var/db (zfs, local, noatime, noexec, nosuid, nfsv4acls)
+vmzroot/ROOT/freebsd/var/empty on /var/empty (zfs, local, noexec, nosuid, read-only, nfsv4acls)
+vmzroot/var/log on /var/log (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/var/log/pf on /var/log/pf (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/var/mail on /var/mail (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/ROOT/freebsd/var/run on /var/run (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/var/spool on /var/spool (zfs, local, noexec, nosuid, nfsv4acls)
+vmzroot/var/tmp on /var/tmp (zfs, local, nosuid, nfsv4acls)
+10.0.0.1:/vol/test on /mnt/test (nfs, read-only)
+10.0.0.2:/vol/tes2 on /mnt/test2 (nfs, nosuid)
\ No newline at end of file
diff --git a/tests/data/zpool_status_simple.txt b/tests/data/zpool_status_simple.txt
new file mode 100644
index 0000000..a2c573a
--- /dev/null
+++ b/tests/data/zpool_status_simple.txt
@@ -0,0 +1,10 @@
+  pool: vmzroot
+ state: ONLINE
+  scan: none requested
+config:
+
+	NAME          STATE     READ WRITE CKSUM
+	vmzroot       ONLINE       0     0     0
+	  gpt/system  ONLINE       0     0     0
+
+errors: No known data errors
\ No newline at end of file
diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
index c2a7f9f..7a7ba1f 100644
--- a/tests/unittests/test_handler/test_handler_resizefs.py
+++ b/tests/unittests/test_handler/test_handler_resizefs.py
@@ -1,7 +1,8 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 from cloudinit.config.cc_resizefs import (
-    can_skip_resize, handle, maybe_get_writable_device_path, _resize_btrfs)
+    can_skip_resize, handle, maybe_get_writable_device_path, _resize_btrfs,
+    _resize_zfs, _resize_xfs, _resize_ext, _resize_ufs)
 
 from collections import namedtuple
 import logging
@@ -60,6 +61,9 @@ class TestResizefs(CiTestCase):
         res = can_skip_resize(fs_type, resize_what, devpth)
         self.assertTrue(res)
 
+    def test_can_skip_resize_ext(self):
+        self.assertFalse(can_skip_resize('ext', '/', '/dev/sda1'))
+
     def test_handle_noops_on_disabled(self):
         """The handle function logs when the configuration disables resize."""
         cfg = {'resize_rootfs': False}
@@ -122,6 +126,51 @@ class TestResizefs(CiTestCase):
         logs = self.logs.getvalue()
         self.assertIn("WARNING: Unable to find device '/dev/root'", logs)
 
+    def test_resize_zfs_cmd_return(self):
+        zpool = 'zroot'
+        devpth = 'gpt/system'
+        self.assertEqual(('zpool', 'online', '-e', zpool, devpth),
+                         _resize_zfs(zpool, devpth))
+
+    def test_resize_xfs_cmd_return(self):
+        mount_point = '/mnt/test'
+        devpth = '/dev/sda1'
+        self.assertEqual(('xfs_growfs', mount_point),
+                         _resize_xfs(mount_point, devpth))
+
+    def test_resize_ext_cmd_return(self):
+        mount_point = '/'
+        devpth = '/dev/sdb1'
+        self.assertEqual(('resize2fs', devpth),
+                         _resize_ext(mount_point, devpth))
+
+    def test_resize_ufs_cmd_return(self):
+        mount_point = '/'
+        devpth = '/dev/sda2'
+        self.assertEqual(('growfs', devpth),
+                         _resize_ufs(mount_point, devpth))
+
+    @mock.patch('cloudinit.util.get_mount_info')
+    @mock.patch('cloudinit.util.get_device_info_from_zpool')
+    @mock.patch('cloudinit.util.parse_mount')
+    def test_handle_zfs_root(self, mount_info, zpool_info, parse_mount):
+        devpth = 'vmzroot/ROOT/freebsd'
+        disk = 'gpt/system'
+        fs_type = 'zfs'
+        mount_point = '/'
+
+        mount_info.return_value = (devpth, fs_type, mount_point)
+        zpool_info.return_value = disk
+        parse_mount.return_value = (devpth, fs_type, mount_point)
+
+        cfg = {'resize_rootfs': True}
+
+        with mock.patch('cloudinit.config.cc_resizefs.do_resize') as dresize:
+            handle('cc_resizefs', cfg, _cloud=None, log=LOG, args=[])
+            ret = dresize.call_args[0][0]
+
+        self.assertEqual(('zpool', 'online', '-e', 'vmzroot', disk), ret)
+
 
 class TestRootDevFromCmdline(CiTestCase):
 
@@ -305,5 +354,12 @@ class TestMaybeGetDevicePathAsWritableBlock(CiTestCase):
             ('btrfs', 'filesystem', 'resize', 'max', '/'),
             _resize_btrfs("/", "/dev/sda1"))
 
+    @mock.patch('cloudinit.util.is_FreeBSD')
+    def test_maybe_get_writable_device_path_zfs_freebsd(self, freebsd):
+        freebsd.return_value = True
+        info = 'dev=gpt/system mnt_point=/ path=/'
+        devpth = maybe_get_writable_device_path('gpt/system', info, LOG)
+        self.assertEqual('gpt/system', devpth)
+
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 67d9607..8685b8e 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -366,6 +366,56 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase):
         expected = ('none', 'tmpfs', '/run/lock')
         self.assertEqual(expected, util.parse_mount_info('/run/lock', lines))
 
+    @mock.patch('cloudinit.util.subp')
+    def test_get_device_info_from_zpool(self, zpool_output):
+        # mock subp command from util.get_mount_info_fs_on_zpool
+        zpool_output.return_value = (
+            self.readResource('zpool_status_simple.txt'), ''
+        )
+        # save function return values and do asserts
+        ret = util.get_device_info_from_zpool('vmzroot')
+        self.assertEqual('gpt/system', ret)
+        self.assertIsNotNone(ret)
+
+    @mock.patch('cloudinit.util.subp')
+    def test_get_device_info_from_zpool_on_error(self, zpool_output):
+        # mock subp command from util.get_mount_info_fs_on_zpool
+        zpool_output.return_value = (
+            self.readResource('zpool_status_simple.txt'), 'error'
+        )
+        # save function return values and do asserts
+        ret = util.get_device_info_from_zpool('vmzroot')
+        self.assertIsNone(ret)
+
+    @mock.patch('cloudinit.util.subp')
+    def test_parse_mount_with_ext(self, mount_out):
+        mount_out.return_value = (self.readResource('mount_parse_ext.txt'), '')
+        # this one is valid and exists in mount_parse_ext.txt
+        ret = util.parse_mount('/var')
+        self.assertEqual(('/dev/mapper/vg00-lv_var', 'ext4', '/var'), ret)
+        # another one that is valid and exists
+        ret = util.parse_mount('/')
+        self.assertEqual(('/dev/mapper/vg00-lv_root', 'ext4', '/'), ret)
+        # this one exists in mount_parse_ext.txt
+        ret = util.parse_mount('/sys/kernel/debug')
+        self.assertIsNone(ret)
+        # this one does not even exist in mount_parse_ext.txt
+        ret = util.parse_mount('/not/existing/mount')
+        self.assertIsNone(ret)
+
+    @mock.patch('cloudinit.util.subp')
+    def test_parse_mount_with_zfs(self, mount_out):
+        mount_out.return_value = (self.readResource('mount_parse_zfs.txt'), '')
+        # this one is valid and exists in mount_parse_zfs.txt
+        ret = util.parse_mount('/var')
+        self.assertEqual(('vmzroot/ROOT/freebsd/var', 'zfs', '/var'), ret)
+        # this one is the root, valid and also exists in mount_parse_zfs.txt
+        ret = util.parse_mount('/')
+        self.assertEqual(('vmzroot/ROOT/freebsd', 'zfs', '/'), ret)
+        # this one does not even exist in mount_parse_ext.txt
+        ret = util.parse_mount('/not/existing/mount')
+        self.assertIsNone(ret)
+
 
 class TestReadDMIData(helpers.FilesystemMockingTestCase):