Merge ~chad.smith/cloud-init:bug/1776701-openstack-local-no-probe-on-ec2 into cloud-init:master

[Chad Smith]

openstack: avoid unneeded metadata probe on non-openstack platforms

OpenStack datasource is now discovered in init-local stage. In order to
probe whether OpenStack metadata is present, it performs a costly
sandboxed dhclient setup and metadata probe against http://169.254.169.254
for openstack data.

Cloud-init properly detects non-OpenStack on EC2, but it spends precious
time probing the metadata service also resulting in a confusing WARNING
log about 'metadata not present'. To avoid the wasted cycles, and
confusing warning, get_data will call a detect_openstack function to
quickly determine whether the platform looks like OpenStack before trying
to setup network to probe and crawl the metadata service.

LP: #1776701

detect_openstack function is basically logic adapted from ./tools/ds-identify for detecting OpenStack.

Jenkins failures which showcased this warning message
https://jenkins.ubuntu.com/server/view/cloud-init,%20curtin,%20streams/job/cloud-init-integration-ec2-x/23/console

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

If not already done so, we should update the OpenStack datasource docs to indicate which product strings/chassis ids are used to positively identify OpenStack datasource on VMs and containers.

[Scott Moser (smoser)]

some minor things.

c84f679... by Chad Smith on 2018-06-14

address review comments

- magic variables at the top of DataSourceOpenStack
- use util.get_proc_env instead of rolling our own
- add util.is_x86 with efficient string checks for matching
architecture use is_x86 in detect_openstack

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

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

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

review: Needs Fixing (continuous-integration)

9c9f6ab... by Chad Smith on 2018-06-14

add Discovery section to OpenStack datasource RTD

9a475d0... by Chad Smith on 2018-06-14

doc tweak

2effe5c... by Chad Smith on 2018-06-14

fix unit tests given use of get_proc_env

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

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

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

review: Needs Fixing (continuous-integration)

44caa4a... by Chad Smith on 2018-06-15

pyflakes and pycodestyle

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

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

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

review: Needs Fixing (continuous-integration)

760279f... by Chad Smith on 2018-06-15

fix unit tests to mock is_x86 instead of uname

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

trivial suggestions.

4055f7d... by Chad Smith on 2018-06-15

use is_x86 in read_dmi_data and add unit test for it

c3183e9... by Chad Smith on 2018-06-15

define top-level constants VALID_DMI_PRODUCT_NAMES and VALID_DMI_ASSET_TAGS

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

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

review: Approve (continuous-integration)

69b6200... by Chad Smith on 2018-06-15

doc update

[Chad Smith (chad.smith)]

Thank you for your merge proposal.

Your branch has been set to 'Work in progress'.
Please set the branch back to 'Needs Review' after resolving the issues below.

Thanks again,
Your friendly neighborhood cloud-init robot.

Please fix the following issues:
------------------------------
Commit message lints:
 - Line #2 has 506 too many characters. Line starts with: "OpenStack datasource"...
------------------------------

For more information, see commit message guidelines at
https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-for-each-feature-or-bug

review: Needs Fixing

[Chad Smith (chad.smith)]

Thank you for your merge proposal.

Your branch has been set to 'Work in progress'.
Please set the branch back to 'Needs Review' after resolving the issues below.

Thanks again,
Your friendly neighborhood cloud-init robot.

Please fix the following issues:
------------------------------
Commit message lints:
 - Line #2 has 3 too many characters. Line starts with: "OpenStack datasource"... - Line #3 has 4 too many characters. Line starts with: "whether OpenStack metadata"... - Line #4 has 1 too many characters. Line starts with: "setup and metadata probe"... - Line #6 has 3 too many characters. Line starts with: "Cloud-init properly detects"... - Line #7 has 2 too many characters. Line starts with: "probing the metadata"... - Line #9 has 5 too many characters. Line starts with: "get_data will call a"... - Line #10 has 5 too many characters. Line starts with: "platform looks like OpenStack"...
------------------------------

For more information, see commit message guidelines at
https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-for-each-feature-or-bug

review: Needs Fixing

[Scott Moser (smoser)]

review: Approve

8266327... by Chad Smith on 2018-06-16

proper mock of get_proc_env in unit tests

[Chad Smith (chad.smith)]

review: Abstain

[Chad Smith (chad.smith)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=1efa8a0a

diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py
index 1a12a3f..365af96 100644
--- a/cloudinit/sources/DataSourceOpenStack.py
+++ b/cloudinit/sources/DataSourceOpenStack.py
@@ -23,6 +23,13 @@ DEFAULT_METADATA = {
     "instance-id": DEFAULT_IID,
 }
 
+# OpenStack DMI constants
+DMI_PRODUCT_NOVA = 'OpenStack Nova'
+DMI_PRODUCT_COMPUTE = 'OpenStack Compute'
+VALID_DMI_PRODUCT_NAMES = [DMI_PRODUCT_NOVA, DMI_PRODUCT_COMPUTE]
+DMI_ASSET_TAG_OPENTELEKOM = 'OpenTelekomCloud'
+VALID_DMI_ASSET_TAGS = [DMI_ASSET_TAG_OPENTELEKOM]
+
 
 class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource):
 
@@ -114,6 +121,8 @@ class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource):
             False when unable to contact metadata service or when metadata
             format is invalid or disabled.
         """
+        if not detect_openstack():
+            return False
         if self.perform_dhcp_setup:  # Setup networking in init-local stage.
             try:
                 with EphemeralDHCPv4(self.fallback_interface):
@@ -205,6 +214,20 @@ def read_metadata_service(base_url, ssl_details=None,
     return reader.read_v2()
 
 
+def detect_openstack():
+    """Return True when a potential OpenStack platform is detected."""
+    if not util.is_x86():
+        return True  # Non-Intel cpus don't properly report dmi product names
+    product_name = util.read_dmi_data('system-product-name')
+    if product_name in VALID_DMI_PRODUCT_NAMES:
+        return True
+    elif util.read_dmi_data('chassis-asset-tag') in VALID_DMI_ASSET_TAGS:
+        return True
+    elif util.get_proc_env(1).get('product_name') == DMI_PRODUCT_NOVA:
+        return True
+    return False
+
+
 # Used to match classes to dependencies
 datasources = [
     (DataSourceOpenStackLocal, (sources.DEP_FILESYSTEM,)),
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 26a4112..6da9511 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2629,6 +2629,16 @@ def _call_dmidecode(key, dmidecode_path):
         return None
 
 
+def is_x86(uname_arch=None):
+    """Return True if platform is x86-based"""
+    if uname_arch is None:
+        uname_arch = os.uname()[4]
+    x86_arch_match = (
+        uname_arch == 'x86_64' or
+        (uname_arch[0] == 'i' and uname_arch[2:] == '86'))
+    return x86_arch_match
+
+
 def read_dmi_data(key):
     """
     Wrapper for reading DMI data.
@@ -2656,8 +2666,7 @@ def read_dmi_data(key):
 
     # running dmidecode can be problematic on some arches (LP: #1243287)
     uname_arch = os.uname()[4]
-    if not (uname_arch == "x86_64" or
-            (uname_arch.startswith("i") and uname_arch[2:] == "86") or
+    if not (is_x86(uname_arch) or
             uname_arch == 'aarch64' or
             uname_arch == 'amd64'):
         LOG.debug("dmidata is not supported on %s", uname_arch)
diff --git a/doc/rtd/topics/datasources/openstack.rst b/doc/rtd/topics/datasources/openstack.rst
index 0ea8994..421da08 100644
--- a/doc/rtd/topics/datasources/openstack.rst
+++ b/doc/rtd/topics/datasources/openstack.rst
@@ -7,6 +7,21 @@ This datasource supports reading data from the
 `OpenStack Metadata Service
 <https://docs.openstack.org/nova/latest/admin/networking-nova.html#metadata-service>`_.
 
+Discovery
+-------------
+To determine whether a platform looks like it may be OpenStack, cloud-init
+checks the following environment attributes as a potential OpenStack platform:
+
+ * Maybe OpenStack if
+
+   * **non-x86 cpu architecture**: because DMI data is buggy on some arches
+ * Is OpenStack **if x86 architecture and ANY** of the following
+
+   * **/proc/1/environ**: Nova-lxd contains *product_name=OpenStack Nova*
+   * **DMI product_name**: Either *Openstack Nova* or *OpenStack Compute*
+   * **DMI chassis_asset_tag** is *OpenTelekomCloud*
+
+
 Configuration
 -------------
 The following configuration can be set for the datasource in system
diff --git a/tests/unittests/test_datasource/test_openstack.py b/tests/unittests/test_datasource/test_openstack.py
index fad73b2..585acc3 100644
--- a/tests/unittests/test_datasource/test_openstack.py
+++ b/tests/unittests/test_datasource/test_openstack.py
@@ -69,6 +69,8 @@ EC2_VERSIONS = [
     'latest',
 ]
 
+MOCK_PATH = 'cloudinit.sources.DataSourceOpenStack.'
+
 
 # TODO _register_uris should leverage test_ec2.register_mock_metaserver.
 def _register_uris(version, ec2_files, ec2_meta, os_files):
@@ -231,7 +233,10 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
         ds_os = ds.DataSourceOpenStack(
             settings.CFG_BUILTIN, None, helpers.Paths({'run_dir': self.tmp}))
         self.assertIsNone(ds_os.version)
-        found = ds_os.get_data()
+        mock_path = MOCK_PATH + 'detect_openstack'
+        with test_helpers.mock.patch(mock_path) as m_detect_os:
+            m_detect_os.return_value = True
+            found = ds_os.get_data()
         self.assertTrue(found)
         self.assertEqual(2, ds_os.version)
         md = dict(ds_os.metadata)
@@ -260,7 +265,10 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
             'broadcast-address': '192.168.2.255'}]
 
         self.assertIsNone(ds_os_local.version)
-        found = ds_os_local.get_data()
+        mock_path = MOCK_PATH + 'detect_openstack'
+        with test_helpers.mock.patch(mock_path) as m_detect_os:
+            m_detect_os.return_value = True
+            found = ds_os_local.get_data()
         self.assertTrue(found)
         self.assertEqual(2, ds_os_local.version)
         md = dict(ds_os_local.metadata)
@@ -284,7 +292,10 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
                                        None,
                                        helpers.Paths({'run_dir': self.tmp}))
         self.assertIsNone(ds_os.version)
-        found = ds_os.get_data()
+        mock_path = MOCK_PATH + 'detect_openstack'
+        with test_helpers.mock.patch(mock_path) as m_detect_os:
+            m_detect_os.return_value = True
+            found = ds_os.get_data()
         self.assertFalse(found)
         self.assertIsNone(ds_os.version)
         self.assertIn(
@@ -306,15 +317,16 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
             'timeout': 0,
         }
         self.assertIsNone(ds_os.version)
-        found = ds_os.get_data()
+        mock_path = MOCK_PATH + 'detect_openstack'
+        with test_helpers.mock.patch(mock_path) as m_detect_os:
+            m_detect_os.return_value = True
+            found = ds_os.get_data()
         self.assertFalse(found)
         self.assertIsNone(ds_os.version)
 
     def test_network_config_disabled_by_datasource_config(self):
         """The network_config can be disabled from datasource config."""
-        mock_path = (
-            'cloudinit.sources.DataSourceOpenStack.openstack.'
-            'convert_net_json')
+        mock_path = MOCK_PATH + 'openstack.convert_net_json'
         ds_os = ds.DataSourceOpenStack(
             settings.CFG_BUILTIN, None, helpers.Paths({'run_dir': self.tmp}))
         ds_os.ds_cfg = {'apply_network_config': False}
@@ -327,9 +339,7 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
 
     def test_network_config_from_network_json(self):
         """The datasource gets network_config from network_data.json."""
-        mock_path = (
-            'cloudinit.sources.DataSourceOpenStack.openstack.'
-            'convert_net_json')
+        mock_path = MOCK_PATH + 'openstack.convert_net_json'
         example_cfg = {'version': 1, 'config': []}
         ds_os = ds.DataSourceOpenStack(
             settings.CFG_BUILTIN, None, helpers.Paths({'run_dir': self.tmp}))
@@ -345,9 +355,7 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
 
     def test_network_config_cached(self):
         """The datasource caches the network_config property."""
-        mock_path = (
-            'cloudinit.sources.DataSourceOpenStack.openstack.'
-            'convert_net_json')
+        mock_path = MOCK_PATH + 'openstack.convert_net_json'
         example_cfg = {'version': 1, 'config': []}
         ds_os = ds.DataSourceOpenStack(
             settings.CFG_BUILTIN, None, helpers.Paths({'run_dir': self.tmp}))
@@ -374,7 +382,10 @@ class TestOpenStackDataSource(test_helpers.HttprettyTestCase):
             'timeout': 0,
         }
         self.assertIsNone(ds_os.version)
-        found = ds_os.get_data()
+        mock_path = MOCK_PATH + 'detect_openstack'
+        with test_helpers.mock.patch(mock_path) as m_detect_os:
+            m_detect_os.return_value = True
+            found = ds_os.get_data()
         self.assertFalse(found)
         self.assertIsNone(ds_os.version)
 
@@ -438,4 +449,89 @@ class TestVendorDataLoading(test_helpers.TestCase):
         data = {'foo': 'bar', 'cloud-init': ['VD_1', 'VD_2']}
         self.assertEqual(self.cvj(data), data['cloud-init'])
 
+
+@test_helpers.mock.patch(MOCK_PATH + 'util.is_x86')
+class TestDetectOpenStack(test_helpers.CiTestCase):
+
+    def test_detect_openstack_non_intel_x86(self, m_is_x86):
+        """Return True on non-intel platforms because dmi isn't conclusive."""
+        m_is_x86.return_value = False
+        self.assertTrue(
+            ds.detect_openstack(), 'Expected detect_openstack == True')
+
+    @test_helpers.mock.patch(MOCK_PATH + 'util.get_proc_env')
+    @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data')
+    def test_not_detect_openstack_intel_x86_ec2(self, m_dmi, m_proc_env,
+                                                m_is_x86):
+        """Return False on EC2 platforms."""
+        m_is_x86.return_value = True
+        # No product_name in proc/1/environ
+        m_proc_env.return_value = {'HOME': '/'}
+
+        def fake_dmi_read(dmi_key):
+            if dmi_key == 'system-product-name':
+                return 'HVM domU'  # Nothing 'openstackish' on EC2
+            if dmi_key == 'chassis-asset-tag':
+                return ''  # Empty string on EC2
+            assert False, 'Unexpected dmi read of %s' % dmi_key
+
+        m_dmi.side_effect = fake_dmi_read
+        self.assertFalse(
+            ds.detect_openstack(), 'Expected detect_openstack == False on EC2')
+        m_proc_env.assert_called_with(1)
+
+    @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data')
+    def test_detect_openstack_intel_product_name_compute(self, m_dmi,
+                                                         m_is_x86):
+        """Return True on OpenStack compute and nova instances."""
+        m_is_x86.return_value = True
+        openstack_product_names = ['OpenStack Nova', 'OpenStack Compute']
+
+        for product_name in openstack_product_names:
+            m_dmi.return_value = product_name
+            self.assertTrue(
+                ds.detect_openstack(), 'Failed to detect_openstack')
+
+    @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data')
+    def test_detect_openstack_opentelekomcloud_chassis_asset_tag(self, m_dmi,
+                                                                 m_is_x86):
+        """Return True on OpenStack reporting OpenTelekomCloud asset-tag."""
+        m_is_x86.return_value = True
+
+        def fake_dmi_read(dmi_key):
+            if dmi_key == 'system-product-name':
+                return 'HVM domU'  # Nothing 'openstackish' on OpenTelekomCloud
+            if dmi_key == 'chassis-asset-tag':
+                return 'OpenTelekomCloud'
+            assert False, 'Unexpected dmi read of %s' % dmi_key
+
+        m_dmi.side_effect = fake_dmi_read
+        self.assertTrue(
+            ds.detect_openstack(),
+            'Expected detect_openstack == True on OpenTelekomCloud')
+
+    @test_helpers.mock.patch(MOCK_PATH + 'util.get_proc_env')
+    @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data')
+    def test_detect_openstack_by_proc_1_environ(self, m_dmi, m_proc_env,
+                                                m_is_x86):
+        """Return True when nova product_name specified in /proc/1/environ."""
+        m_is_x86.return_value = True
+        # Nova product_name in proc/1/environ
+        m_proc_env.return_value = {
+            'HOME': '/', 'product_name': 'OpenStack Nova'}
+
+        def fake_dmi_read(dmi_key):
+            if dmi_key == 'system-product-name':
+                return 'HVM domU'  # Nothing 'openstackish'
+            if dmi_key == 'chassis-asset-tag':
+                return ''  # Nothin 'openstackish'
+            assert False, 'Unexpected dmi read of %s' % dmi_key
+
+        m_dmi.side_effect = fake_dmi_read
+        self.assertTrue(
+            ds.detect_openstack(),
+            'Expected detect_openstack == True on OpenTelekomCloud')
+        m_proc_env.assert_called_with(1)
+
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 20479f6..7a203ce 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -468,6 +468,29 @@ class TestMountinfoParsing(helpers.ResourceUsingTestCase):
         self.assertIsNone(ret)
 
 
+class TestIsX86(helpers.CiTestCase):
+
+    def test_is_x86_matches_x86_types(self):
+        """is_x86 returns True if CPU architecture matches."""
+        matched_arches = ['x86_64', 'i386', 'i586', 'i686']
+        for arch in matched_arches:
+            self.assertTrue(
+                util.is_x86(arch), 'Expected is_x86 for arch "%s"' % arch)
+
+    def test_is_x86_unmatched_types(self):
+        """is_x86 returns Fale on non-intel x86 architectures."""
+        unmatched_arches = ['ia64', '9000/800', 'arm64v71']
+        for arch in unmatched_arches:
+            self.assertFalse(
+                util.is_x86(arch), 'Expected not is_x86 for arch "%s"' % arch)
+
+    @mock.patch('cloudinit.util.os.uname')
+    def test_is_x86_calls_uname_for_architecture(self, m_uname):
+        """is_x86 returns True if platform from uname matches."""
+        m_uname.return_value = [0, 1, 2, 3, 'x86_64']
+        self.assertTrue(util.is_x86())
+
+
 class TestReadDMIData(helpers.FilesystemMockingTestCase):
 
     def setUp(self):