cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

https://gist.github.com/smoser/a5435f49b61e2ae45c367bbb266bfac3

Add DI_DMI_CHASSIS_ASSET_TAG to _print_info in tools/ds-identify.



Diff comments:

> diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
> index b9458ff..9dc9157 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -319,7 +321,11 @@ class DataSourceAzureNet(sources.DataSource):
>      def get_data(self):
>          # azure removes/ejects the cdrom containing the ovf-env.xml
>          # file on reboot.  So, in order to successfully reboot we
> -        # need to look in the datadir and consider that valid
> +        # need to look in the datadir and consider that valida
> +        asset_tag = util.read_dmi_data('chassis-asset-tag')
> +        if asset_tag != AZURE_CHASSIS_ASSET_TAG:
> +            LOG.info("Non-Azure DMI asset tag '%s' discovered.", asset_tag)

This is debug.
info by default goes to stderr, which means it shows up on the serial console.
Messages saying "Nothing to see here, move along" aren't really "info".

(larger discussion, i think we need a 'trace' level that we do not have... currently so much stuff is at debug that theres just a lot of noise in the log).

> +            return False
>          ddir = self.ds_cfg['data_dir']
>  
>          candidates = [self.seed_dir]
> @@ -817,7 +823,8 @@ def load_azure_ds_dir(source_dir):
>      ovf_file = os.path.join(source_dir, "ovf-env.xml")
>  
>      if not os.path.isfile(ovf_file):
> -        raise NonAzureDataSource("No ovf-env file found")
> +        raise NonAzureDataSource(
> +            "No ovf-env.xml file found at %s" % (source_dir))

This will print a useless temporary path when called from util.mount_cb .
If you wanted to, you could make a functools.partial that called load_azure_ds_dir
that passed an optional 'device' or something. so that we could then have useful
message like:
  No ovf-env.xml file found at /dev/cdrom

Also acceptable to leave it as it is, i'm just pointing out that most of the
time the message will not be useful.

>  
>      with open(ovf_file, "rb") as fp:
>          contents = fp.read()
> diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
> index 852ec70..f42d9c2 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -241,6 +249,23 @@ fdescfs            /dev/fd          fdescfs rw              0 0
>              res = get_path_dev_freebsd('/etc', mnt_list)
>              self.assertIsNotNone(res)
>  
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
> +    def test_non_azure_dmi_chassis_asset_tag(self, m_read_dmi_data):
> +        """Report non-azure when DMI's chassis asset tag doesn't match.
> +
> +        Return False when the asset tag doesn't match Azure's static
> +        AZURE_CHASSIS_ASSET_TAG.
> +        """
> +        # Return a non-matching asset tag value
> +        nonazure_tag = dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'
> +        m_read_dmi_data.return_value = nonazure_tag
> +        dsrc = dsaz.DataSourceAzureNet(
> +            {}, distro=None, paths=self.paths)
> +        self.assertFalse(dsrc.get_data())
> +        self.assertEqual(

this should be assertIn ?  assertEqual seems awfully specific.
someone adding a debug message (or future 'trace' message) makes this test fail.

> +            "Non-Azure DMI asset tag '{0}' discovered.\n".format(nonazure_tag),
> +            self.logs.getvalue())
> +
>      def test_basic_seed_dir(self):
>          odata = {'HostName': "myhost", 'UserName': "myuser"}
>          data = {'ovfcontent': construct_valid_ovf_env(data=odata),
> @@ -696,6 +721,33 @@ class TestAzureBounce(TestCase):
>          self.assertEqual(0, self.set_hostname.call_count)
>  
>  
> +class TestLoadAzureDsDir(CiTestCase):
> +    """Tests for load_azure_ds_dir."""
> +
> +    def setUp(self):
> +        self.source_dir = self.tmp_dir()
> +        super(TestLoadAzureDsDir, self).setUp()
> +
> +    def test_missing_ovf_env_xml_raises_non_azure_datasource_error(self):
> +        """load_azure_ds_dir raises an error When ovf-env.xml doesn't exit."""
> +        with self.assertRaises(dsaz.NonAzureDataSource) as context_manager:
> +            dsaz.load_azure_ds_dir(self.source_dir)
> +        self.assertEqual(
> +            'No ovf-env.xml file found at {0}'.format(self.source_dir),
> +            str(context_manager.exception))
> +
> +    def test_wb_invalid_ovf_env_xml_calls_read_azure_ovf(self):
> +        """load_azure_ds_dir calls read_azure_ovf to parse the xml."""
> +        ovf_path = os.path.join(self.source_dir, 'ovf-env.xml')
> +        with open(ovf_path, 'wb') as stream:
> +            stream.write('invalid xml')
> +        with self.assertRaises(dsaz.BrokenAzureDataSource) as context_manager:
> +            dsaz.load_azure_ds_dir(self.source_dir)
> +        self.assertEqual(
> +            'Invalid ovf-env.xml: syntax error: line 1, column 0',

Isnt this overly specific?  we're writing a unit test that expects an explicit format of the string in an exception that we don't control?

> +            str(context_manager.exception))
> +
> +
>  class TestReadAzureOvf(TestCase):
>      def test_invalid_xml_raises_non_azure_ds(self):
>          invalid_xml = "<foo>" + construct_valid_ovf_env(data={})
> diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
> index 5c26e65..31335ac 100644
> --- a/tests/unittests/test_ds_identify.py
> +++ b/tests/unittests/test_ds_identify.py
> @@ -268,9 +280,24 @@ def _print_run_output(rc, out, err, cfg, files):
>  
>  
>  VALID_CFG = {
> +<<<<<<< tests/unittests/test_ds_identify.py
>      'AliYun': {
>          'ds': 'AliYun',
>          'files': {P_PRODUCT_NAME: 'Alibaba Cloud ECS\n'},
> +=======

merge conflicts. need rebase.

> +    'Azure-dmi-detection': {
> +        'ds': 'Azure',
> +        'files': {
> +            P_CHASSIS_ASSET_TAG: '7783-7084-3265-9085-8269-3286-77\n',
> +        }
> +    },
> +    'Azure-seed-detection': {
> +        'ds': 'Azure',
> +        'files': {
> +            P_CHASSIS_ASSET_TAG: 'No-match\n',
> +            os.path.join(P_SEED_DIR, 'azure', 'ovf-env.xml'): 'present\n',
> +        }
> +>>>>>>> tests/unittests/test_ds_identify.py
>      },
>      'Ec2-hvm': {
>          'ds': 'Ec2',
> diff --git a/tools/ds-identify b/tools/ds-identify
> index 5fc500b..12633c7 100755
> --- a/tools/ds-identify
> +++ b/tools/ds-identify
> @@ -402,11 +417,6 @@ dmi_product_serial_matches() {
>      return 1
>  }
>  

This is fine. Because I tested performance and its very close.
https://gist.github.com/smoser/a5435f49b61e2ae45c367bbb266bfac3

> -dmi_product_name_is() {
> -    is_container && return 1
> -    [ "${DI_DMI_PRODUCT_NAME}" = "$1" ]
> -}
> -
>  dmi_sys_vendor_is() {
>      is_container && return 1
>      [ "${DI_DMI_SYS_VENDOR}" = "$1" ]


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324875
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:azure-di-id-asset-tag into cloud-init:master.