Merge ~rjschwei/cloud-init:btrfsResize into cloud-init:master

[Robert Schweikert]

btrfs: support resizing if root is mounted ro.

Resize of btrfs fails if the mount point for the file system we are trying
to resize, i.e. the root of the filesystem is read only. With this change
we use a known (currently snapper specific) rw location to work around a
flaw that blocks resizing of the ro filesystem.

LP: #1734787

Resize of btrfs fails if "/" is mounted ro. However, it should be allowed to resize the file system. Use a subvolume that is rw as the mountpoint to trick the tools into letting the filesystem be resized as desired

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

A unit test on this would be nice.
I'm fine if you just test _resize_btrfs that calling it returns the right thing
just make a good comment on it with reference to the bug. Heres an example, placed
in cloudinit/config/tests/test_resizefs.py
  http://paste.ubuntu.com/26065846/

Also,
 a.) it seems like we should check if / is in-fact read-only before taking the alternate path.
 b.) knowledge of 'ro' might be useful to pass to *all* the handlers, as if a fs is read-only
     taking a different path seems reasonable.

Lastly,
please fix your commit message
  Subject
  <blank line>
  More info.
  <blank line>
  LP: #1734787

review: Needs Fixing

fbc04d2... by Robert Schweikert on 2017-11-29

- Refactor and implement code to detect rw status of mount
- ADd unit tests

022503d... by Robert Schweikert on 2017-11-29

- Add docstring to test to reference bug#

[Robert Schweikert (rjschwei)]

@smoser

Did some refactoring and added some tests. Hopefully this is more agreeable now.

W.r.t. the commit message, sorry I am at a loss about what you are after.

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

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

review: Approve (continuous-integration)

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

This does look good, thank you.
Fix your 'Set commit message' button above to be the right form as I said earlier.

Then, I am also curious about /.snapshots as rharper was.

Sorry for the noob questions he and I just dont' have much experience with brtfs.

review: Needs Information

[Robert Schweikert (rjschwei)]

Commit message set.

As far as .snapshot is concerned, this is "snapper" [1] related. I think until proven otherwise this will do. The other solution is to walk the subvolumes and find one that's mounted rw. For now I think the subvolume walking is overkill.

[1] http://snapper.io/

e1348e3... by Robert Schweikert on 2017-12-01

- Clarifying comment

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

please rebase and address conflicts and one bit about verticle space below.
I think i seems fine after that.

09f8aeb... by Robert Schweikert on 2017-12-08

- Rebase to resolve merge conlicts

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

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

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

review: Needs Fixing (continuous-integration)

0c12fbf... by Robert Schweikert on 2017-12-08

- code style
  + more compact notation for continuation lines

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I think I'm ok with this if you fix the little comment fixes inline.

review: Needs Fixing

cd9dab4... by Robert Schweikert on 2017-12-11

- Fix typos in comment

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

thanks Robert.

review: Approve

diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
index 0d282e6..cec22bb 100644
--- a/cloudinit/config/cc_resizefs.py
+++ b/cloudinit/config/cc_resizefs.py
@@ -59,7 +59,17 @@ __doc__ = get_schema_doc(schema)  # Supplement python help()
 
 
 def _resize_btrfs(mount_point, devpth):
-    return ('btrfs', 'filesystem', 'resize', 'max', mount_point)
+    # If "/" is ro resize will fail. However it should be allowed since resize
+    # makes everything bigger and subvolumes that are not ro will benefit.
+    # Use a subvolume that is not ro to trick the resize operation to do the
+    # "right" thing. The use of ".snapshot" is specific to "snapper" a generic
+    # solution would be walk the subvolumes and find a rw mounted subvolume.
+    if (not util.mount_is_read_write(mount_point) and
+            os.path.isdir("%s/.snapshots" % mount_point)):
+        return ('btrfs', 'filesystem', 'resize', 'max',
+                '%s/.snapshots' % mount_point)
+    else:
+        return ('btrfs', 'filesystem', 'resize', 'max', mount_point)
 
 
 def _resize_ext(mount_point, devpth):
diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
new file mode 100644
index 0000000..ba6bf69
--- /dev/null
+++ b/cloudinit/tests/test_util.py
@@ -0,0 +1,46 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+"""Tests for cloudinit.util"""
+
+import logging
+
+import cloudinit.util as util
+
+from cloudinit.tests.helpers import CiTestCase, mock
+
+LOG = logging.getLogger(__name__)
+
+MOUNT_INFO = [
+    '68 0 8:3 / / ro,relatime shared:1 - btrfs /dev/sda1 ro,attr2,inode64',
+    '153 68 254:0 / /home rw,relatime shared:101 - xfs /dev/sda2 rw,attr2'
+]
+
+
+class TestUtil(CiTestCase):
+
+    def test_parse_mount_info_no_opts_no_arg(self):
+        result = util.parse_mount_info('/home', MOUNT_INFO, LOG)
+        self.assertEqual(('/dev/sda2', 'xfs', '/home'), result)
+
+    def test_parse_mount_info_no_opts_arg(self):
+        result = util.parse_mount_info('/home', MOUNT_INFO, LOG, False)
+        self.assertEqual(('/dev/sda2', 'xfs', '/home'), result)
+
+    def test_parse_mount_info_with_opts(self):
+        result = util.parse_mount_info('/', MOUNT_INFO, LOG, True)
+        self.assertEqual(
+            ('/dev/sda1', 'btrfs', '/', 'ro,relatime'),
+            result
+        )
+
+    @mock.patch('cloudinit.util.get_mount_info')
+    def test_mount_is_rw(self, m_mount_info):
+        m_mount_info.return_value = ('/dev/sda1', 'btrfs', '/', 'rw,relatime')
+        is_rw = util.mount_is_read_write('/')
+        self.assertEqual(is_rw, True)
+
+    @mock.patch('cloudinit.util.get_mount_info')
+    def test_mount_is_ro(self, m_mount_info):
+        m_mount_info.return_value = ('/dev/sda1', 'btrfs', '/', 'ro,relatime')
+        is_rw = util.mount_is_read_write('/')
+        self.assertEqual(is_rw, False)
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 11e96a7..f3eff13 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2051,7 +2051,7 @@ def expand_package_list(version_fmt, pkgs):
     return pkglist
 
 
-def parse_mount_info(path, mountinfo_lines, log=LOG):
+def parse_mount_info(path, mountinfo_lines, log=LOG, get_mnt_opts=False):
     """Return the mount information for PATH given the lines from
     /proc/$$/mountinfo."""
 
@@ -2113,11 +2113,16 @@ def parse_mount_info(path, mountinfo_lines, log=LOG):
 
         match_mount_point = mount_point
         match_mount_point_elements = mount_point_elements
+        mount_options = parts[5]
 
-    if devpth and fs_type and match_mount_point:
-        return (devpth, fs_type, match_mount_point)
+    if get_mnt_opts:
+        if devpth and fs_type and match_mount_point and mount_options:
+            return (devpth, fs_type, match_mount_point, mount_options)
     else:
-        return None
+        if devpth and fs_type and match_mount_point:
+            return (devpth, fs_type, match_mount_point)
+
+    return None
 
 
 def parse_mtab(path):
@@ -2187,7 +2192,7 @@ def parse_mount(path):
     return None
 
 
-def get_mount_info(path, log=LOG):
+def get_mount_info(path, log=LOG, get_mnt_opts=False):
     # Use /proc/$$/mountinfo to find the device where path is mounted.
     # This is done because with a btrfs filesystem using os.stat(path)
     # does not return the ID of the device.
@@ -2219,7 +2224,7 @@ def get_mount_info(path, log=LOG):
     mountinfo_path = '/proc/%s/mountinfo' % os.getpid()
     if os.path.exists(mountinfo_path):
         lines = load_file(mountinfo_path).splitlines()
-        return parse_mount_info(path, lines, log)
+        return parse_mount_info(path, lines, log, get_mnt_opts)
     elif os.path.exists("/etc/mtab"):
         return parse_mtab(path)
     else:
@@ -2604,4 +2609,10 @@ def wait_for_files(flist, maxwait, naplen=.5, log_pre=""):
     return need
 
 
+def mount_is_read_write(mount_point):
+    """Check whether the given mount point is mounted rw"""
+    result = get_mount_info(mount_point, get_mnt_opts=True)
+    mount_opts = result[-1].split(',')
+    return mount_opts[0] == 'rw'
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
index 29d5574..5aa3c49 100644
--- a/tests/unittests/test_handler/test_handler_resizefs.py
+++ b/tests/unittests/test_handler/test_handler_resizefs.py
@@ -1,7 +1,7 @@
 # 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)
+    can_skip_resize, handle, maybe_get_writable_device_path, _resize_btrfs)
 
 from collections import namedtuple
 import logging
@@ -293,5 +293,25 @@ class TestMaybeGetDevicePathAsWritableBlock(CiTestCase):
             " per kernel cmdline",
             self.logs.getvalue())
 
+    @mock.patch('cloudinit.util.mount_is_read_write')
+    @mock.patch('cloudinit.config.cc_resizefs.os.path.isdir')
+    def test_resize_btrfs_mount_is_ro(self, m_is_dir, m_is_rw):
+        """Do not resize / directly if it is read-only. (LP: #1734787)."""
+        m_is_rw.return_value = False
+        m_is_dir.return_value = True
+        self.assertEqual(
+            ('btrfs', 'filesystem', 'resize', 'max', '//.snapshots'),
+            _resize_btrfs("/", "/dev/sda1"))
+
+    @mock.patch('cloudinit.util.mount_is_read_write')
+    @mock.patch('cloudinit.config.cc_resizefs.os.path.isdir')
+    def test_resize_btrfs_mount_is_rw(self, m_is_dir, m_is_rw):
+        """Do not resize / directly if it is read-only. (LP: #1734787)."""
+        m_is_rw.return_value = True
+        m_is_dir.return_value = True
+        self.assertEqual(
+            ('btrfs', 'filesystem', 'resize', 'max', '/'),
+            _resize_btrfs("/", "/dev/sda1"))
+
 
 # vi: ts=4 expandtab