Merge ~smoser/cloud-init:bug/lp-1731868-unset-var-in-ds-identify into cloud-init:master

[Scott Moser]

ds-identify: failure in NoCloud due to unset variable usage.

The previous OVF datasource change added a debug message that
referenced an un-used variable. The failure path would be triggered
if an image was booted with a iso9660 filesystem attached to a device
that was not a cdrom.

A unit test is added for the specific failure found.

Additional safety to avoid 'cidata' labels is also added to the OVF
checker.

LP: #1737704

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

FAILED: Continuous integration, rev:75eaa4d20584eb52969acc1b011816d7d7d099c4
https://jenkins.ubuntu.com/server/job/cloud-init-ci/619/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    FAILED: MAAS Compatability Testing

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

review: Needs Fixing (continuous-integration)

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

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

+1 on this changeset. Was able to see the unit tests reproducing the failure path there. Yeah I guess we should have exercised this path in the first place in our unittests. Thanks for the quick fix.

review: Approve

[Joshua Powers (powersj)]

+1 fixes nocloud-kvm tests

review: Approve

[Ryan Harper (raharper)]

We could look at using shellcheck; it's not perfect, sort of like pylint, but it does catch things like this; maybe just a manual run of shellcheck on ds-identify before commits?

Change looks good.

review: Approve

[Scott Moser (smoser)]

wrt shellcheck, it did not find this error, and generally complains about too many things.
I've seen it and it *can* be useful, but not immediately.

It lists 113 things, 2 of which were valid (unused variables).

The '--exclude' that you'd have to pass are coarsely grained, and I'm not interested in litering code with '#make-shellcheck-happy'

http://paste.ubuntu.com/26171686/

[Ryan Harper (raharper)]

On Tue, Dec 12, 2017 at 12:12 PM, Scott Moser <email address hidden>
wrote:

> wrt shellcheck, it did not find this error, and generally complains about
> too many things.
> I've seen it and it *can* be useful, but not immediately.
>
> It lists 113 things, 2 of which were valid (unused variables).
>

Besides the local, the rest seemed pretty valid.

>
> The '--exclude' that you'd have to pass are coarsely grained, and I'm not
> interested in litering code with '#make-shellcheck-happy'
>

I don't want to do that either, but it suggests reasonable fixes IMO.

>
> http://paste.ubuntu.com/26171686/
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/335086
> You are reviewing the proposed merge of ~smoser/cloud-init:bug/lp-
> 1731868-unset-var-in-ds-identify into cloud-init:master.
>

diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
index 3f1a671..c9234ed 100644
--- a/tests/unittests/test_ds_identify.py
+++ b/tests/unittests/test_ds_identify.py
@@ -27,6 +27,14 @@ TYPE=ext4
 PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
 """
 
+# this is a Ubuntu 18.04 disk.img output (dual uefi and bios bootable)
+BLKID_UEFI_UBUNTU = [
+    {'DEVNAME': 'vda1', 'TYPE': 'ext4', 'PARTUUID': uuid4(), 'UUID': uuid4()},
+    {'DEVNAME': 'vda14', 'PARTUUID': uuid4()},
+    {'DEVNAME': 'vda15', 'TYPE': 'vfat', 'LABEL': 'UEFI', 'PARTUUID': uuid4(),
+     'UUID': '5F55-129B'}]
+
+
 POLICY_FOUND_ONLY = "search,found=all,maybe=none,notfound=disabled"
 POLICY_FOUND_OR_MAYBE = "search,found=all,maybe=all,notfound=disabled"
 DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
@@ -340,6 +348,10 @@ class TestDsIdentify(CiTestCase):
             self._check_via_dict(
                 ovf_cdrom_by_label, rc=RC_FOUND, dslist=['OVF', DS_NONE])
 
+    def test_default_nocloud_as_vdb_iso9660(self):
+        """NoCloud is found with iso9660 filesystem on non-cdrom disk."""
+        self._test_ds_found('NoCloud')
+
 
 def blkid_out(disks=None):
     """Convert a list of disk dictionaries into blkid content."""
@@ -422,6 +434,19 @@ VALID_CFG = {
         'files': {P_PRODUCT_SERIAL: 'GoogleCloud-8f2e88f\n'},
         'mocks': [MOCK_VIRT_IS_KVM],
     },
+    'NoCloud': {
+        'ds': 'NoCloud',
+        'mocks': [
+            MOCK_VIRT_IS_KVM,
+            {'name': 'blkid', 'ret': 0,
+             'out': blkid_out(
+                 BLKID_UEFI_UBUNTU +
+                 [{'DEVNAME': 'vdb', 'TYPE': 'iso9660', 'LABEL': 'cidata'}])},
+        ],
+        'files': {
+            'dev/vdb': 'pretend iso content for cidata\n',
+        }
+    },
     'OpenStack': {
         'ds': 'OpenStack',
         'files': {P_PRODUCT_NAME: 'OpenStack Nova\n'},
diff --git a/tools/ds-identify b/tools/ds-identify
index 4c59d7b..5893a76 100755
--- a/tools/ds-identify
+++ b/tools/ds-identify
@@ -657,7 +657,7 @@ is_cdrom_ovf() {
     # skip devices that don't look like cdrom paths.
     case "$dev" in
         /dev/sr[0-9]|/dev/hd[a-z]) :;;
-        *) debug 1 "skipping iso dev $d"
+        *) debug 1 "skipping iso dev $dev"
            return 1;;
     esac
 
@@ -666,7 +666,7 @@ is_cdrom_ovf() {
 
     # explicitly skip known labels of other types. rd_rdfe is azure.
     case "$label" in
-        config-2|rd_rdfe_stable*) return 1;;
+        config-2|rd_rdfe_stable*|cidata) return 1;;
     esac
 
     local idstr="http://schemas.dmtf.org/ovf/environment/1"