cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
> index 5254e18..f7cbb43 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -413,56 +412,71 @@ class DataSourceAzureNet(sources.DataSource):
>          return
>  
>  
> +def _partitions_on_device(devpath, maxnum=16):
> +    # return a list of tuples (ptnum, path) for each part on devpath
> +    for suff in ("-part", "p", ""):
> +        found = []
> +        for pnum in range(1, maxnum):
> +            ppath = devpath + suff + str(pnum)
> +            if os.path.exists(ppath):
> +                found.append((pnum, os.path.realpath(ppath)))
> +        if found:
> +            return found
> +    return []
> +
> +
> +def _has_ntfs_filesystem(devpath):
> +    ntfs_devices = util.find_devs_with("TYPE=ntfs", no_cache=True)
> +    LOG.debug('ntfs_devices found = %s', ntfs_devices)
> +    return os.path.realpath(devpath) in ntfs_devices

This may be confusing when examining the log messages since we check if realpath(device) is ntfs_devices.

Note, AFAICT, all callers of _has_ntfs_filesystem are passing in paths that have already had realpath called on them.

> +
> +
>  def can_dev_be_reformatted(devpath):
> -    # determine if the ephemeral block device path devpath
> -    # is newly formatted after a resize.
> +    """Determine if block device devpath is newly formatted ephemeral.
> +
> +    A newly formatted disk will:
> +      a.) have a partition table (dos or gpt)
> +      b.) have 1 partition that is ntfs formatted, or
> +          have 2 partitions with the second partition ntfs formatted.
> +          (larger instances with >2TB ephemeral disk have gpt, and will
> +           have a microsoft reserved partition as part 1.  LP: #1686514)
> +      c.) the ntfs partition will have no files other than possibly
> +          'dataloss_warning_readme.txt'"""
>      if not os.path.exists(devpath):
>          return False, 'device %s does not exist' % devpath
>  
> -    realpath = os.path.realpath(devpath)
> -    LOG.debug('Resolving realpath of %s -> %s', devpath, realpath)
> -
> -    # it is possible that the block device might exist, but the kernel
> -    # have not yet read the partition table and sent events.  we udevadm settle
> -    # to hope to resolve that.  Better here would probably be to test and see,
> -    # and then settle if we didn't find anything and try again.
> -    if util.which("udevadm"):
> -        util.subp(["udevadm", "settle"])

It appears that you're addressing the "Better here would probably be to test and see.."
Wouldn't udevadm settled --exist-if-exists=os.path.realpath(devpath) then?

It's also worth noting that realpath on symlinks with no targets return the symlink;
It may be worth checking that realpath actually resolved as an indicator of whether
the "real" disk is actually present.

> +    LOG.debug('Resolving realpath of %s -> %s', devpath,
> +              os.path.realpath(devpath))
>  
>      # devpath of /dev/sd[a-z] or /dev/disk/cloud/azure_resource
>      # where partitions are "<devpath>1" or "<devpath>-part1" or "<devpath>p1"
> -    part1path = None
> -    for suff in ("-part", "p", ""):
> -        cand = devpath + suff + "1"
> -        if os.path.exists(cand):
> -            if os.path.exists(devpath + suff + "2"):
> -                msg = ('device %s had more than 1 partition: %s, %s' %
> -                       devpath, cand, devpath + suff + "2")
> -                return False, msg
> -            part1path = cand
> -            break
> -
> -    if part1path is None:
> +    partitions = _partitions_on_device(devpath)
> +    if len(partitions) == 0:
>          return False, 'device %s was not partitioned' % devpath
> +    elif len(partitions) > 2:
> +        msg = ('device %s had 3 or more partitions: %s' %
> +               (devpath, ' '.join([p[1] for p in partitions])))
> +        return False, msg
> +    elif len(partitions) == 2:
> +        cand_part, cand_path = partitions[1]
> +    else:
> +        cand_part, cand_path = partitions[0]
>  
> -    real_part1path = os.path.realpath(part1path)
> -    ntfs_devices = util.find_devs_with("TYPE=ntfs", no_cache=True)
> -    LOG.debug('ntfs_devices found = %s', ntfs_devices)
> -    if real_part1path not in ntfs_devices:
> -        msg = ('partition 1 (%s -> %s) on device %s was not ntfs formatted' %
> -               (part1path, real_part1path, devpath))
> +    if not _has_ntfs_filesystem(cand_path):
> +        msg = ('partition %s (%s) on device %s was not ntfs formatted' %
> +               (cand_part, cand_path, devpath))
>          return False, msg
>  
>      def count_files(mp):
>          ignored = set(['dataloss_warning_readme.txt'])
>          return len([f for f in os.listdir(mp) if f.lower() not in ignored])
>  
> -    bmsg = ('partition 1 (%s -> %s) on device %s was ntfs formatted' %
> -            (part1path, real_part1path, devpath))
> +    bmsg = ('partition %s (%s) on device %s was ntfs formatted' %
> +            (cand_part, cand_path, devpath))
>      try:
> -        file_count = util.mount_cb(part1path, count_files)
> +        file_count = util.mount_cb(cand_path, count_files)
>      except util.MountFailedError as e:
> -        return False, bmsg + ' but mount of %s failed: %s' % (part1path, e)
> +        return False, bmsg + ' but mount of %s failed: %s' % (cand_part, e)
>  
>      if file_count != 0:
>          return False, bmsg + ' but had %d files on it.' % file_count
> diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
> index e6b0dcb..e7562bf 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -701,15 +699,197 @@ class TestAzureBounce(TestCase):
>  class TestReadAzureOvf(TestCase):
>      def test_invalid_xml_raises_non_azure_ds(self):
>          invalid_xml = "<foo>" + construct_valid_ovf_env(data={})
> -        self.assertRaises(DataSourceAzure.BrokenAzureDataSource,
> -                          DataSourceAzure.read_azure_ovf, invalid_xml)
> +        self.assertRaises(dsaz.BrokenAzureDataSource,
> +                          dsaz.read_azure_ovf, invalid_xml)
>  
>      def test_load_with_pubkeys(self):
>          mypklist = [{'fingerprint': 'fp1', 'path': 'path1', 'value': ''}]
>          pubkeys = [(x['fingerprint'], x['path'], x['value']) for x in mypklist]
>          content = construct_valid_ovf_env(pubkeys=pubkeys)
> -        (_md, _ud, cfg) = DataSourceAzure.read_azure_ovf(content)
> +        (_md, _ud, cfg) = dsaz.read_azure_ovf(content)
>          for mypk in mypklist:
>              self.assertIn(mypk, cfg['_pubkeys'])
>  
> +
> +class TestCanDevBeReformatted(CiTestCase):
> +    warning_file = 'dataloss_warning_readme.txt'
> +
> +    def _domock(self, mockpath, sattr=None):
> +        patcher = mock.patch(mockpath)
> +        setattr(self, sattr, patcher.start())
> +        self.addCleanup(patcher.stop)
> +
> +    def setUp(self):
> +        super(TestCanDevBeReformatted, self).setUp()
> +
> +    def patchup(self, devs):
> +        def realpath(d):
> +            return bypath[d].get('realpath', d)
> +
> +        def partitions_on_device(devpath):
> +            parts = bypath.get(devpath, {}).get('partitions', {})
> +            ret = []
> +            for path, data in parts.items():
> +                ret.append((data.get('num'), realpath(path)))
> +            # return sorted by partition number
> +            return sorted(ret, key=lambda d: d[0])
> +
> +        def mount_cb(device, callback):
> +            p = self.tmp_dir()
> +            for f in bypath.get(device).get('files', []):
> +                write_file(os.path.join(p, f), content=f)
> +            return callback(p)
> +
> +        def has_ntfs_fs(device):
> +            return bypath.get(device, {}).get('fs') == 'ntfs'
> +
> +        bypath = {}

I'm missing something, does this assignment make this a class attribute?  it's strange (to me) to have methods 
refer to the bypath dictionary without self;  and further to see it assigned in the self. mock objects.

> +        for path, data in devs.items():
> +            bypath[path] = data
> +            if 'realpath' in data:
> +                bypath[data['realpath']] = data
> +            for ppath, pdata in data.get('partitions', {}).items():
> +                bypath[ppath] = pdata
> +                if 'realpath' in data:
> +                    bypath[pdata['realpath']] = pdata
> +
> +        p = 'cloudinit.sources.DataSourceAzure'
> +        self._domock(p + "._partitions_on_device", 'm_partitions_on_device')
> +        self._domock(p + "._has_ntfs_filesystem", 'm_has_ntfs_filesystem')
> +        self._domock(p + ".util.mount_cb", 'm_mount_cb')
> +        self._domock(p + ".os.path.realpath", 'm_realpath')
> +        self._domock(p + ".os.path.exists", 'm_exists')
> +
> +        self.m_exists.side_effect = lambda p: p in bypath
> +        self.m_realpath.side_effect = lambda p: realpath(p)
> +        self.m_has_ntfs_filesystem.side_effect = has_ntfs_fs
> +        self.m_mount_cb.side_effect = mount_cb
> +        self.m_partitions_on_device.side_effect = partitions_on_device
> +
> +    def test_three_partitions_is_false(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1},
> +                    '/dev/sda2': {'num': 2},
> +                    '/dev/sda3': {'num': 3},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertFalse(False, value)
> +        self.assertIn("3 or more", msg.lower())
> +
> +    def test_no_partitions_is_false(self):
> +        self.patchup({'/dev/sda': {'realpath': '/dev/sda'}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(False, value)
> +        self.assertIn("not partitioned", msg.lower())
> +
> +    def test_two_partitions_not_ntfs_false(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1},
> +                    '/dev/sda2': {'num': 2, 'fs': 'ext4', 'files': []},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertFalse(False, value)
> +        self.assertIn("not ntfs", msg.lower())
> +
> +    def test_two_partitions_ntfs_populated_false(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1},
> +                    '/dev/sda2': {'num': 2, 'fs': 'ntfs',
> +                                  'files': ['secret.txt']},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertFalse(False, value)
> +        self.assertIn("files on it", msg.lower())
> +
> +    def test_two_partitions_ntfs_empty_is_true(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1},
> +                    '/dev/sda2': {'num': 2, 'fs': 'ntfs', 'files': []},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(True, value)
> +        self.assertIn("safe for", msg.lower())
> +
> +    def test_one_partition_not_ntfs_false(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1, 'fs': 'zfs'},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(False, value)
> +        self.assertIn("not ntfs", msg.lower())
> +
> +    def test_one_partition_ntfs_populated_false(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1, 'fs': 'ntfs',
> +                                  'files': ['file1.txt', 'file2.exe']},
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(False, value)
> +        self.assertIn("files on it", msg.lower())
> +
> +    def test_one_partition_ntfs_empty_is_true(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1, 'fs': 'ntfs', 'files': []}
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(True, value)
> +        self.assertIn("safe for", msg.lower())
> +
> +    def test_one_partition_ntfs_empty_with_dataloss_file_is_true(self):
> +        self.patchup({
> +            '/dev/sda': {
> +                'partitions': {
> +                    '/dev/sda1': {'num': 1, 'fs': 'ntfs',
> +                                  'files': ['dataloss_warning_readme.txt']}
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted("/dev/sda")
> +        self.assertEqual(True, value)
> +        self.assertIn("safe for", msg.lower())
> +
> +    def test_one_partition_through_realpath_is_true(self):
> +        epath = '/dev/disk/cloud/azure_resource'
> +        self.patchup({
> +            epath: {
> +                'realpath': '/dev/sdb',
> +                'partitions': {
> +                    epath + '-part1': {
> +                        'num': 1, 'fs': 'ntfs', 'files': [self.warning_file],
> +                        'realpath': '/dev/sdb1'}
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted(epath)
> +        self.assertEqual(True, value)
> +        self.assertIn("safe for", msg.lower())
> +
> +    def test_three_partition_through_realpath_is_true(self):
> +        epath = '/dev/disk/cloud/azure_resource'
> +        self.patchup({
> +            epath: {
> +                'realpath': '/dev/sdb',
> +                'partitions': {
> +                    epath + '-part1': {
> +                        'num': 1, 'fs': 'ntfs', 'files': [self.warning_file],
> +                        'realpath': '/dev/sdb1'},
> +                    epath + '-part2': {'num': 2, 'fs': 'ext3',
> +                                       'realpath': '/dev/sdb2'},
> +                    epath + '-part3': {'num': 3, 'fs': 'ext',
> +                                       'realpath': '/dev/sdb3'}
> +                }}})
> +        value, msg = dsaz.can_dev_be_reformatted(epath)
> +        self.assertEqual(False, value)
> +        self.assertIn("3 or more", msg.lower())
> +
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/323420
Your team cloud init development team is requested to review the proposed merge of ~smoser/cloud-init:bug/1686514-azure-reformat-large into cloud-init:master.