cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/cloud-init:ds-ovf-use-util-find-devs-with into cloud-init:master.

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1718287 in cloud-init: "systemd mount targets fail due to device busy or already mounted"
  https://bugs.launchpad.net/cloud-init/+bug/1718287

For more details, see:
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/330995

DataSourceOVF: use util.find_devs_with(TYPE=iso9660)

DataSourceOVF attempts to find iso files via walking os.listdir('/dev/')
which is far too wide.  This approach is too invasive and can sometimes
race with systemd attempting to fsck and mount devices.

Instead, utilize cloudinit.util.find_devs_with to filter devices by
criteria (which uses blkid under the covers).  This should result in fewer
attempts to mount block devices which are not actual iso filesystems.

-- 
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:ds-ovf-use-util-find-devs-with into cloud-init:master.

diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py
index 24b45d5..4b02cbb 100644
--- a/cloudinit/sources/DataSourceOVF.py
+++ b/cloudinit/sources/DataSourceOVF.py
@@ -406,29 +406,19 @@ def transport_iso9660(require_iso=True):
     else:
         mtype = None
 
-    devs = os.listdir("/dev/")
-    devs.sort()
+    # generate a list of devices with mtype filesystem, filter by regex
+    devs = [dev for dev in
+            util.find_devs_with("TYPE=%s" % mtype if mtype else None)
+            if cdmatch.match(dev[5:])]
     for dev in devs:
-        fullp = os.path.join("/dev/", dev)
-
-        if (fullp in mounts or
-                not cdmatch.match(dev) or os.path.isdir(fullp)):
-            continue
-
         try:
-            # See if we can read anything at all...??
-            util.peek_file(fullp, 512)
-        except IOError:
-            continue
-
-        try:
-            (fname, contents) = util.mount_cb(fullp, get_ovf_env, mtype=mtype)
+            (fname, contents) = util.mount_cb(dev, get_ovf_env, mtype=mtype)
         except util.MountFailedError:
-            LOG.debug("%s not mountable as iso9660", fullp)
+            LOG.debug("%s not mountable as iso9660", dev)
             continue
 
         if contents is not False:
-            return (contents, fullp, fname)
+            return (contents, dev, fname)
 
     return (False, None, None)
 
diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py
index 28e2662..6f88a5b 100644
--- a/cloudinit/tests/helpers.py
+++ b/cloudinit/tests/helpers.py
@@ -104,6 +104,16 @@ class TestCase(unittest2.TestCase):
         super(TestCase, self).setUp()
         self.reset_global_state()
 
+    def add_patch(self, target, attr, **kwargs):
+        """Patches specified target object and sets it as attr on test
+        instance also schedules cleanup"""
+        if 'autospec' not in kwargs:
+            kwargs['autospec'] = True
+        m = mock.patch(target, **kwargs)
+        p = m.start()
+        self.addCleanup(m.stop)
+        setattr(self, attr, p)
+
 
 class CiTestCase(TestCase):
     """This is the preferred test case base class unless user
diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py
index 9dbf4dd..7c416a5 100644
--- a/tests/unittests/test_datasource/test_ovf.py
+++ b/tests/unittests/test_datasource/test_ovf.py
@@ -5,6 +5,7 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import base64
+from collections import OrderedDict
 
 from cloudinit.tests import helpers as test_helpers
 
@@ -70,4 +71,112 @@ class TestReadOvfEnv(test_helpers.TestCase):
         self.assertEqual({'password': "passw0rd"}, cfg)
         self.assertIsNone(ud)
 
+
+class TestTransportIso9660(test_helpers.CiTestCase):
+
+    def setUp(self):
+        super(TestTransportIso9660, self).setUp()
+        self.add_patch('cloudinit.util.find_devs_with',
+                       'm_find_devs_with')
+        self.add_patch('cloudinit.util.mounts', 'm_mounts')
+        self.add_patch('cloudinit.util.mount_cb', 'm_mount_cb')
+        self.add_patch('cloudinit.sources.DataSourceOVF.get_ovf_env',
+                       'm_get_ovf_env')
+        self.m_get_ovf_env.return_value = ('myfile', 'mycontent')
+
+    def test_find_already_mounted(self):
+        """Check we call get_ovf_env from on matching mounted devices"""
+        mounts = {
+            '/dev/sr9': {
+                'fstype': 'iso9660',
+                'mountpoint': 'wark/media/sr9',
+                'opts': 'ro',
+            }
+        }
+        self.m_mounts.return_value = mounts
+
+        (contents, fullp, fname) = dsovf.transport_iso9660()
+        self.assertEqual("mycontent", contents)
+        self.assertEqual("/dev/sr9", fullp)
+        self.assertEqual("myfile", fname)
+
+    def test_find_already_mounted_skips_non_iso9660(self):
+        """Check we call get_ovf_env ignoring non iso9660"""
+        mounts = {
+            '/dev/xvdb': {
+                'fstype': 'vfat',
+                'mountpoint': 'wark/foobar',
+                'opts': 'defaults,noatime',
+            },
+            '/dev/xvdc': {
+                'fstype': 'iso9660',
+                'mountpoint': 'wark/media/sr9',
+                'opts': 'ro',
+            }
+        }
+        # ensure we check xvdb before xvdc
+        self.m_mounts.return_value = (
+            OrderedDict(sorted(mounts.items(), key=lambda t: t[0])))
+
+        (contents, fullp, fname) = dsovf.transport_iso9660()
+        self.assertEqual("mycontent", contents)
+        self.assertEqual("/dev/xvdc", fullp)
+        self.assertEqual("myfile", fname)
+
+    def test_mount_cb_called_on_blkdevs_with_iso9660(self):
+        """Check we call mount_cb on blockdevs with iso9660 only"""
+        self.m_mounts.return_value = {}
+        self.m_find_devs_with.return_value = ['/dev/sr0']
+        self.m_mount_cb.return_value = ("myfile", "mycontent")
+
+        (contents, fullp, fname) = dsovf.transport_iso9660()
+
+        self.m_mount_cb.assert_called_with(
+            "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
+        self.assertEqual("mycontent", contents)
+        self.assertEqual("/dev/sr0", fullp)
+        self.assertEqual("myfile", fname)
+
+    def test_mount_cb_called_on_blkdevs_with_iso9660_check_regex(self):
+        """Check we call mount_cb on blockdevs with iso9660 and match regex"""
+        self.m_mounts.return_value = {}
+        self.m_find_devs_with.return_value = [
+            '/dev/abc', '/dev/my-cdrom', '/dev/sr0']
+        self.m_mount_cb.return_value = ("myfile", "mycontent")
+
+        (contents, fullp, fname) = dsovf.transport_iso9660()
+
+        self.m_mount_cb.assert_called_with(
+            "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
+        self.assertEqual("mycontent", contents)
+        self.assertEqual("/dev/sr0", fullp)
+        self.assertEqual("myfile", fname)
+
+    def test_mount_cb_not_called_no_matches(self):
+        """Check we don't call mount_cb if nothing matches"""
+        self.m_mounts.return_value = {}
+        self.m_find_devs_with.return_value = ['/dev/vg/myovf']
+
+        (contents, fullp, fname) = dsovf.transport_iso9660()
+
+        self.assertEqual(0, self.m_mount_cb.call_count)
+        self.assertEqual(False, contents)
+        self.assertIsNone(fullp)
+        self.assertIsNone(fname)
+
+    def test_mount_cb_called_require_iso_false(self):
+        """Check we call mount_cb on blockdevs with require_iso=False"""
+        self.m_mounts.return_value = {}
+        self.m_find_devs_with.return_value = ['/dev/xvdz']
+        self.m_mount_cb.return_value = ("myfile", "mycontent")
+
+        (contents, fullp, fname) = dsovf.transport_iso9660(require_iso=False)
+
+        self.m_mount_cb.assert_called_with(
+            "/dev/xvdz", dsovf.get_ovf_env, mtype=None)
+        self.assertEqual("mycontent", contents)
+        self.assertEqual("/dev/xvdz", fullp)
+        self.assertEqual("myfile", fname)
+
+
 # vi: ts=4 expandtab