Merge ~smoser/cloud-init:fix/ds-identify-smartos-is-container into cloud-init:master

[Scott Moser]

ds-identify: recognize container-other as a container, test SmartOS.

In playing with a SmartOS container I found that ds-identify did
not identify the container there as a container. Systemd-detect-virt
identifies it as 'container-other'.

Also here are tests for ds-identify for the SmartOS platform
identification, and some indentation fixes in ds-identify.

see commit message

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

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

review: Approve (continuous-integration)

[Mike Gerdts (mgerdts)]

Code seems fine, but this is the first time I've looked at this part of cloud-init. Would be best to have a second reviewer.

review: Approve

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

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

review: Needs Information

[Chad Smith (chad.smith)]

Also shouldntx`

[Scott Moser (smoser)]

thanks for the thoughtful review.

i responded inline.

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

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

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

+1 here, thanks for this change I'll trust "${PATH_ROOT}/native/.zonecontrol/metadata.sock" exists on SmartOS for our use case here. I validated that we don't accidentally detect other datasources even if SmartOS is first in datasource_list. The only question I had (as I have no SmartOS account) is whether we still detect SmartOS on the desired non-container platform and it feels a bit strange that we aren't detecting/rejecting using the same type of logic in DataSourceSmartOS that ds-identify is using, but that may be minutia that doesn't really matter (as it'd be platform sub-type detection difference which would still be using the proper base datasource).

review: Approve

[Scott Moser (smoser)]

What did you think we were missing? Anything more than just checking for
the socket file in get_smartos_environ?
http://paste.ubuntu.com/p/3z7R6J8fV5/

I think if we do that , then get_data will get out quickly . As it is
right now, in a lxc container inside the smart-os-container get_data would
take the path:

if not self.smartos_type:
            LOG.debug("Not running on smartos")

We should do that.

On Thu, May 10, 2018 at 3:19 PM, Chad Smith <email address hidden>
wrote:

> Review: Approve
>
> +1 here, thanks for this change I'll trust "${PATH_ROOT}/native/.zonecontrol/metadata.sock"
> exists on SmartOS for our use case here. I validated that we don't
> accidentally detect other datasources even if SmartOS is first in
> datasource_list. The only question I had (as I have no SmartOS account) is
> whether we still detect SmartOS on the desired non-container platform and
> it feels a bit strange that we aren't detecting/rejecting using the same
> type of logic in DataSourceSmartOS that ds-identify is using, but that may
> be minutia that doesn't really matter (as it'd be platform sub-type
> detection difference which would still be using the proper base datasource).
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/343738
> You are the owner of ~smoser/cloud-init:fix/ds-
> identify-smartos-is-container.
>

[Chad Smith (chad.smith)]

The difference between python's get_smartos_environ() and ds-identify:

1. Ordering of the checks (kernel version versus system-product-name) is reversed in python. As you mentioned, maybe it's faster in python to field the uname call than to parse and lower system-product-name sysfs.

2. Less strict matching of system-product-name in python than in ds-identify
 'smartdc' in system_type.lower() instead of system_type.startswith('SmartDC')

3. Missing the sockfile test when matching the kernel and we match either upper or lowercase 'BrandZ virtual linux' in python.

they are minor differences, but those differences could lead to mis-matches from ds-identify where python would pass. I just wanted to see those differences tidied up a bit where sensible. I'm ok if you believe that is overkill in this case.

[Scott Moser (smoser)]

1. Order doesn't really matter, the end result is the same. we can change them but I dont consider that a problem unless I'm misunderstanding.
2. shell case insenstive matching is costly, so I had just done case sensitive there and the case insensitive in python.
I've added a commit to put python in case-sensitive match.
3. I think we *are* correctly going to return None in get_data on SmartOS if there is no METADATA_SOCKFILE .

get_smartos_environ doesn't check for the file, but when the client is created, a 'exists' is immediately done, and *that* will check for the presene of that file.

[Chad Smith (chad.smith)]

+1 Scott thanks for the back and forth

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=0d7ee559

diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
index 4ea00eb..fcb46b1 100644
--- a/cloudinit/sources/DataSourceSmartOS.py
+++ b/cloudinit/sources/DataSourceSmartOS.py
@@ -745,7 +745,7 @@ def get_smartos_environ(uname_version=None, product_name=None):
     # report 'BrandZ virtual linux' as the kernel version
     if uname_version is None:
         uname_version = uname[3]
-    if uname_version.lower() == 'brandz virtual linux':
+    if uname_version == 'BrandZ virtual linux':
         return SMARTOS_ENV_LX_BRAND
 
     if product_name is None:
@@ -753,7 +753,7 @@ def get_smartos_environ(uname_version=None, product_name=None):
     else:
         system_type = product_name
 
-    if system_type and 'smartdc' in system_type.lower():
+    if system_type and system_type.startswith('SmartDC'):
         return SMARTOS_ENV_KVM
 
     return None
diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
index 4d8a436..7f12be5 100644
--- a/tests/unittests/test_ds_identify.py
+++ b/tests/unittests/test_ds_identify.py
@@ -10,7 +10,8 @@ from cloudinit import util
 from cloudinit.tests.helpers import (
     CiTestCase, dir2dict, populate_dir, populate_dir_with_ts)
 
-from cloudinit.sources import DataSourceIBMCloud as dsibm
+from cloudinit.sources import DataSourceIBMCloud as ds_ibm
+from cloudinit.sources import DataSourceSmartOS as ds_smartos
 
 UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
                "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
@@ -69,8 +70,12 @@ P_DSID_CFG = "etc/cloud/ds-identify.cfg"
 
 IBM_CONFIG_UUID = "9796-932E"
 
+MOCK_VIRT_IS_CONTAINER_OTHER = {'name': 'detect_virt',
+                                'RET': 'container-other', 'ret': 0}
 MOCK_VIRT_IS_KVM = {'name': 'detect_virt', 'RET': 'kvm', 'ret': 0}
 MOCK_VIRT_IS_VMWARE = {'name': 'detect_virt', 'RET': 'vmware', 'ret': 0}
+# currenty' SmartOS hypervisor "bhyve" is unknown by systemd-detect-virt.
+MOCK_VIRT_IS_VM_OTHER = {'name': 'detect_virt', 'RET': 'vm-other', 'ret': 0}
 MOCK_VIRT_IS_XEN = {'name': 'detect_virt', 'RET': 'xen', 'ret': 0}
 MOCK_UNAME_IS_PPC64 = {'name': 'uname', 'out': UNAME_PPC64EL, 'ret': 0}
 
@@ -330,7 +335,7 @@ class TestDsIdentify(DsIdentifyBase):
                 break
         if not offset:
             raise ValueError("Expected to find 'blkid' mock, but did not.")
-        data['mocks'][offset]['out'] = d['out'].replace(dsibm.IBM_CONFIG_UUID,
+        data['mocks'][offset]['out'] = d['out'].replace(ds_ibm.IBM_CONFIG_UUID,
                                                         "DEAD-BEEF")
         self._check_via_dict(
             data, rc=RC_FOUND, dslist=['ConfigDrive', DS_NONE])
@@ -516,6 +521,20 @@ class TestDsIdentify(DsIdentifyBase):
         """Hetzner cloud is identified in sys_vendor."""
         self._test_ds_found('Hetzner')
 
+    def test_smartos_bhyve(self):
+        """SmartOS cloud identified by SmartDC in dmi."""
+        self._test_ds_found('SmartOS-bhyve')
+
+    def test_smartos_lxbrand(self):
+        """SmartOS cloud identified on lxbrand container."""
+        self._test_ds_found('SmartOS-lxbrand')
+
+    def test_smartos_lxbrand_requires_socket(self):
+        """SmartOS cloud should not be identified if no socket file."""
+        mycfg = copy.deepcopy(VALID_CFG['SmartOS-lxbrand'])
+        del mycfg['files'][ds_smartos.METADATA_SOCKFILE]
+        self._check_via_dict(mycfg, rc=RC_NOT_FOUND, policy_dmi="disabled")
+
 
 class TestIsIBMProvisioning(DsIdentifyBase):
     """Test the is_ibm_provisioning method in ds-identify."""
@@ -777,7 +796,7 @@ VALID_CFG = {
                  [{'DEVNAME': 'xvda1', 'TYPE': 'ext3', 'PARTUUID': uuid4(),
                    'UUID': uuid4(), 'LABEL': 'cloudimg-bootfs'},
                   {'DEVNAME': 'xvdb', 'TYPE': 'vfat', 'LABEL': 'config-2',
-                   'UUID': dsibm.IBM_CONFIG_UUID},
+                   'UUID': ds_ibm.IBM_CONFIG_UUID},
                   {'DEVNAME': 'xvda2', 'TYPE': 'ext4',
                    'LABEL': 'cloudimg-rootfs', 'PARTUUID': uuid4(),
                    'UUID': uuid4()},
@@ -798,6 +817,32 @@ VALID_CFG = {
              },
         ],
     },
+    'SmartOS-bhyve': {
+        'ds': 'SmartOS',
+        'mocks': [
+            MOCK_VIRT_IS_VM_OTHER,
+            {'name': 'blkid', 'ret': 0,
+             'out': blkid_out(
+                 [{'DEVNAME': 'vda1', 'TYPE': 'ext4',
+                   'PARTUUID': '49ec635a-01'},
+                  {'DEVNAME': 'vda2', 'TYPE': 'swap',
+                   'LABEL': 'cloudimg-swap', 'PARTUUID': '49ec635a-02'}]),
+             },
+        ],
+        'files': {P_PRODUCT_NAME: 'SmartDC HVM\n'},
+    },
+    'SmartOS-lxbrand': {
+        'ds': 'SmartOS',
+        'mocks': [
+            MOCK_VIRT_IS_CONTAINER_OTHER,
+            {'name': 'uname', 'ret': 0,
+             'out': ("Linux d43da87a-daca-60e8-e6d4-d2ed372662a3 4.3.0 "
+                     "BrandZ virtual linux x86_64 GNU/Linux")},
+            {'name': 'blkid', 'ret': 2, 'out': ''},
+        ],
+        'files': {ds_smartos.METADATA_SOCKFILE: 'would be a socket\n'},
+    }
+
 }
 
 # vi: ts=4 expandtab
diff --git a/tools/ds-identify b/tools/ds-identify
index 435a5bc..dc7ac3a 100755
--- a/tools/ds-identify
+++ b/tools/ds-identify
@@ -261,7 +261,7 @@ read_virt() {
 
 is_container() {
     case "${DI_VIRT}" in
-        lxc|lxc-libvirt|systemd-nspawn|docker|rkt) return 0;;
+        container-other|lxc|lxc-libvirt|systemd-nspawn|docker|rkt) return 0;;
         *) return 1;;
     esac
 }
@@ -990,12 +990,14 @@ dscheck_SmartOS() {
     # joyent cloud has two virt types: kvm and container
     # on kvm, product name on joyent public cloud shows 'SmartDC HVM'
     # on the container platform, uname's version has: BrandZ virtual linux
+    # for container, we also verify that the socketfile exists to protect
+    # against embedded containers (lxd running on brandz)
     local smartdc_kver="BrandZ virtual linux"
+    local metadata_sockfile="${PATH_ROOT}/native/.zonecontrol/metadata.sock"
     dmi_product_name_matches "SmartDC*" && return $DS_FOUND
-    if [ "${DI_UNAME_KERNEL_VERSION}" = "${smartdc_kver}" ] &&
-       [ "${DI_VIRT}" = "container-other" ]; then
-       return ${DS_FOUND}
-    fi
+    [ "${DI_UNAME_KERNEL_VERSION}" = "${smartdc_kver}" ] &&
+        [ -e "${metadata_sockfile}" ] &&
+        return ${DS_FOUND}
     return ${DS_NOT_FOUND}
 }