[VMware] Support cloudinit raw data feature by xiaofengw1205 · Pull Request #691 · canonical/cloud-init

xiaofengw1205 · 2020-11-25T11:54:57Z

VMware product will allow user to specify their cloud-init meta data and user data to customize the VM.

summary:

This feature will modify VMware datasource to read from meta data and user data which are specified by VMware vSphere user. If meta data/user data are found in cloud-init configuration directory, datasource will parse the meta data/network and user data from the configuration file, otherwise it will continue to parse them from traditional customization configuration file as before. The supported meta data file is in json or yaml format.

Test Steps

Unit tests are added to test_ovf.py and ensure get_data() works as expected.
For the end to end test, user need to deploy latest version of VMware vSphere and customize the VM with cloud-init raw meta data and user data. It will be covered by our integration test.

Checklist:

[ x] My code follows the process laid out in the documentation
[x ] I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

xiaofengw1205 · 2020-11-25T12:08:23Z

Please help to assign a reviewer and give us the comments, thanks a lot.

xiaofengw1205 · 2020-12-04T11:59:54Z

Can anybody help to review this pull request? Thanks a lot.

smoser

I'm really happy to see this, and I hope another reviewer (@OddBloke or @blackboxsw ) have time to look at this. Having cloud-init support in Vsphere is really a huge win.

I've done a cursory look. There are some changes needed.

….com:xiaofengw-vmware/cloud-init into xiaofengw-support-cloudinit-raw-data-feature

blackboxsw

Thank you for taking this on @xiaofengw-vmware I've given your branch a review too with a couple of comments as well to try to cut down on some duplication of cloudinit.util functions if we can. Again this looks like a good approach for cloud-init users let's polish it a bit and get it into master :)

blackboxsw · 2020-12-07T23:19:23Z

+    else:
+        nicsPath = os.path.join(VMWARE_IMC_DIR, "nics.txt")
+        if not os.path.exists(nicsPath):
+            LOG.debug('nics.txt is not exist.')


Probably worth actually parameterizing these logs since your log messages redact the full path of metadata, userdat and nics.txt.

Suggested change

LOG.debug('nics.txt is not exist.')

LOG.debug('%s does not exist.', nics_path)

Fixed, PTAL.

blackboxsw · 2020-12-07T23:48:59Z

+    ud = None
+    network = None
+
+    md = load(util.load_file(metaPath))


I think we can drop both "def load and this callsite and just use util.read_conf(md_path) which will default to {} if file doesn't exist, is unparseable as YAML (emitting a more specific YAML format warnings in logs).

util.read_conf() will return {} if the meta data is not a valid YAML, it won't raise exception for this case. Here we want to report to vSphere user about the invalid YAML format and stop customization, so I need to add this new function.

blackboxsw · 2020-12-07T23:49:33Z

+    return md, ud, cfg, network
+
+
+def load(data):


might be able to drop in favor of util.read_conf which behaves with this default {} for unparseable or empty files.

util.read_conf() doesn't raise exception if it's not a valid yaml, it will log a warning trace and return {}. But here we want to catch the format exception and ask our vSphere user to modify it.

+1 let's rename this function to give a bit more clarity:

Suggested change

def load(data):

def safeload_yaml_or_dict(data):

xiaofengw1205 · 2020-12-09T03:05:32Z

Thank you for taking this on @xiaofengw-vmware I've given your branch a review too with a couple of comments as well to try to cut down on some duplication of cloudinit.util functions if we can. Again this looks like a good approach for cloud-init users let's polish it a bit and get it into master :)

Thanks Chad for your quick response, I just addressed your comments, please have another review. Thanks for your time.

xiaofengw1205 · 2020-12-14T12:15:11Z

Hi all,
Sorry to disturb you. I know it's close to the holiday, I wonder whether you could give me another review before the holiday so that I could deliver the codes within this month. Thank you all for the help.

xiaofengw1205 · 2020-12-18T02:21:22Z

Hi Scott,
Sorry I didn't click the "resolve conversation" button so you couldn't see my reply. Please help to check the reply when you have time. Thanks a lot.

OddBloke · 2021-01-04T21:14:26Z

Sounds like this is waiting for another pass from Scott and/or Chad?

xiaofengw1205 · 2021-01-05T01:11:31Z

Sounds like this is waiting for another pass from Scott and/or Chad?

Yes, I modify the codes per their comments and need their approval.

xiaofengw1205 · 2021-01-05T05:47:09Z

Hi all,
Happy new year! Please help to take another look when you are back from the holiday. Thanks a lot. :)

xiaofengw1205 · 2021-01-08T05:48:13Z

Hi all,
The changes are made per Scott's comments, PTAL.
Thanks.

xiaofengw1205 · 2021-01-11T08:49:40Z

Hi all,
Sorry to disturb you again. Could you help to review the changes? We want to deliver the codes as early as possible so that we could start the integration test. Thanks for your time. :)
@smoser @raharper @blackboxsw

blackboxsw

@xiaofengw-vmware Again thanks for your patience here. I got a bit wrapped up with holidays on this.

I've made individual comments on your PR in this last round to try to improve a bit of maintainability of the code/docs for when we look at this in the future and forget the intent of some of this logic.

Here is the consolidated diff of my comments for you to review and consider what you think is applicable.

diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py
index da8d832d..d3a50113 100644
--- a/cloudinit/sources/DataSourceOVF.py
+++ b/cloudinit/sources/DataSourceOVF.py
@@ -119,7 +119,9 @@ class DataSourceOVF(sources.DataSource):
                     # When the VM is powered on, the "VMware Tools" daemon
                     # copies the customization specification file to
                     # /var/run/vmware-imc directory. cloud-init code needs
-                    # to search for the file in that directory.
+                    # to search for the file in that directory which indicates
+                    # that required metadata and userdata files are now
+                    # present.
                     max_wait = get_max_wait_from_cfg(self.ds_cfg)
                     vmwareImcConfigFilePath = util.log_time(
                         logfunc=LOG.debug,
@@ -137,9 +139,9 @@ class DataSourceOVF(sources.DataSource):
                     LOG.debug("Found VMware Customization Config File at %s",
                               vmwareImcConfigFilePath)
                     try:
-                        (md_path, ud_path, nicspath) = collect_imc_files(
+                        (md_path, ud_path, nicspath) = collect_imc_file_paths(
                             self._vmware_cust_conf)
-                    except Exception as e:
+                    except FileNotFoundError as e:
                         _raise_error_status(
                             "File(s) missing in directory",
                             e,
@@ -149,7 +151,7 @@ class DataSourceOVF(sources.DataSource):
                 else:
                     LOG.debug("Did not find VMware Customization Config File")
 
-                # ignore disable_vmware_customization if meta data is available
+                # Honor disable_vmware_customization setting on metadata absent
                 if not md_path:
                     if util.get_cfg_option_bool(self.sys_cfg,
                                                 "disable_vmware_customization",
@@ -159,13 +161,12 @@ class DataSourceOVF(sources.DataSource):
                         # reset vmwareImcConfigFilePath to None to avoid
                         # customization for VMware platform
                         vmwareImcConfigFilePath = None
-
-        if vmwareImcConfigFilePath and md_path:
+        use_raw_userdata = bool(vmwareImcConfigFilePath and md_path)
+        if use_raw_userdata:
             set_gc_status(self._vmware_cust_conf, "Started")
             LOG.debug("Start to load cloud-init meta data and user data")
             try:
-                (md, ud, cfg, network) = load_cloudinit_data(
-                    md_path, ud_path)
+                (md, ud, cfg, network) = load_cloudinit_data(md_path, ud_path)
 
                 if network:
                     self._network_config = network
@@ -199,6 +200,7 @@ class DataSourceOVF(sources.DataSource):
             set_gc_status(self._vmware_cust_conf, "Successful")
 
         elif vmwareImcConfigFilePath:
+            # Load configuration from vmware_imc
             self._vmware_nics_to_enable = ""
             try:
                 set_gc_status(self._vmware_cust_conf, "Started")
@@ -745,26 +747,29 @@ def load_cloudinit_data(md_path, ud_path):
     """
     Load the cloud-init meta data, user data, cfg and network from the
     given files
+
+    @return: 4-tuple of configuration
+        metadata, userdata, cfg={}, network
+
+    @raises: FileNotFoundError if md_path or ud_path are absent
     """
     LOG.debug('load meta data from: %s: user data from: %s',
               md_path, ud_path)
     md = {}
-    cfg = {}
     ud = None
     network = None
 
-    md = load(util.load_file(md_path))
+    md = safeload_yaml_or_dict(util.load_file(md_path))
 
     if 'network' in md:
         network = md['network']
-        del md['network']
 
     if ud_path:
         ud = util.load_file(ud_path).replace("\r", "")
-    return md, ud, cfg, network
+    return md, ud, {}, network
 
 
-def load(data):
+def safeload_yaml_or_dict(data):
     '''
     The meta data could be JSON or YAML. Since YAML is a strict superset of
     JSON, we will unmarshal the data as YAML. If data is None then a new
@@ -775,10 +780,20 @@ def load(data):
     return safeyaml.load(data)
 
 
-def collect_imc_files(cust_conf):
+def collect_imc_file_paths(cust_conf):
     '''
-    collect all the other imc files. metadata/userdata must be present
-    if they are specified in customization configuration.
+    collect all the other imc files.
+
+    metadata is preferred to nics.txt configuration data.
+
+    If metadata file exists because it is specified in customization
+    configuration, then both metadata an userdata are required.
+
+    @return a 3-tuple containing desired configuration file paths if present
+        Expected returns:
+             1. user provided metadata (md_path, ud_path, None)
+             2. user-provided network config (None, None, nics_path)
+             3. No config found (None, None, None)
     '''
     md_path = None
     ud_path = None
@@ -799,7 +814,7 @@ def collect_imc_files(cust_conf):
     else:
         nics_path = os.path.join(VMWARE_IMC_DIR, "nics.txt")
         if not os.path.exists(nics_path):
-            LOG.debug('%s is not exist.', nics_path)
+            LOG.debug('%s does not exist.', nics_path)
             nics_path = None
 
     return md_path, ud_path, nics_path

--- a/tests/unittests/test_datasource/test_ovf.py
+++ b/tests/unittests/test_datasource/test_ovf.py
@@ -402,7 +402,7 @@ class TestDatasourceOVF(CiTestCase):
                  'util.del_dir': True,
                  'search_file': self.tdir,
                  'wait_for_imc_cfg_file': conf_file,
-                 'collect_imc_files': [self.tdir + '/test-meta', '', ''],
+                 'collect_imc_file_paths': [self.tdir + '/test-meta', '', ''],
                  'get_nics_to_enable': ''},
                 ds._get_data)
 
@@ -450,7 +450,7 @@ class TestDatasourceOVF(CiTestCase):
                  'util.del_dir': True,
                  'search_file': self.tdir,
                  'wait_for_imc_cfg_file': conf_file,
-                 'collect_imc_files': [self.tdir + '/test-meta', '', ''],
+                 'collect_imc_file_paths': [self.tdir + '/test-meta', '', ''],
                  'get_nics_to_enable': ''},
                 ds._get_data)
 
@@ -490,7 +490,9 @@ class TestDatasourceOVF(CiTestCase):
                      'util.del_dir': True,
                      'search_file': self.tdir,
                      'wait_for_imc_cfg_file': conf_file,
-                     'collect_imc_files': [self.tdir + '/test-meta', '', ''],
+                     'collect_imc_file_paths': [
+                         self.tdir + '/test-meta', '', ''
+                     ],
                      'get_nics_to_enable': ''},
                     ds.get_data)
 
@@ -572,8 +574,8 @@ class TestDatasourceOVF(CiTestCase):
                  'util.del_dir': True,
                  'search_file': self.tdir,
                  'wait_for_imc_cfg_file': conf_file,
-                 'collect_imc_files': [self.tdir + '/test-meta',
-                                       self.tdir + '/test-user', ''],
+                 'collect_imc_file_paths': [self.tdir + '/test-meta',
+                                            self.tdir + '/test-user', ''],
                  'get_nics_to_enable': ''},
                 ds._get_data)

blackboxsw · 2021-01-12T19:38:50Z

+    return md, ud, cfg, network
+
+
+def load(data):


+1 let's rename this function to give a bit more clarity:

Suggested change

def load(data):

def safeload_yaml_or_dict(data):

blackboxsw

Thank you so much for working to get this in @xiaofengw-vmware . Let's land it!

@xiaofeng

Re-reading smoser's comments I believe all were addressed in subsequent pushed from @xiaofeng

mweinelt · 2021-06-24T15:10:18Z

@xiaofengw-vmware Can you clarify what version of VSphere is required to use raw cloud-init data?

xiaofengw1205 · 2021-06-25T02:42:49Z

@xiaofengw-vmware Can you clarify what version of VSphere is required to use raw cloud-init data?

It will be supported from vSphere 7.0 update 3 which will be released soon. Thanks.

xiaofengw1205 added 4 commits November 16, 2020 02:32

VMware: support raw cloud-init data in vm customization

4725bd6

VMware: support raw cloud-init data in vm customization

1c19fef

VMware: support raw cloud-init data in vm customization

0fb205b

fix the failure test cases on travis ci

71230a7

Merge branch 'master' into xiaofengw-support-cloudinit-raw-data-feature

f19ea3d

smoser previously requested changes Dec 4, 2020

View reviewed changes

raharper reviewed Dec 4, 2020

View reviewed changes

Comment thread cloudinit/sources/DataSourceOVF.py Outdated

Comment thread cloudinit/sources/DataSourceOVF.py Outdated

blackboxsw self-assigned this Dec 4, 2020

xiaofengw1205 added 2 commits December 7, 2020 10:57

[VMware] Support cloudinit raw data feature:address comments

462612e

Merge branch 'xiaofengw-support-cloudinit-raw-data-feature' of github…

9432647

….com:xiaofengw-vmware/cloud-init into xiaofengw-support-cloudinit-raw-data-feature

blackboxsw requested changes Dec 7, 2020

View reviewed changes

[VMware] Support cloudinit raw data feature:address Chad's comments

b099009

xiaofengw1205 requested review from blackboxsw, raharper and smoser December 11, 2020 02:24

smoser reviewed Dec 14, 2020

View reviewed changes

Comment thread cloudinit/sources/DataSourceOVF.py

xiaofengw1205 requested a review from smoser December 15, 2020 04:30

smoser reviewed Jan 5, 2021

View reviewed changes

Comment thread tests/unittests/test_datasource/test_ovf.py Outdated

[VMware] Support cloudinit raw data feature: fix typo

27ec8d0

xiaofengw1205 requested a review from smoser January 6, 2021 03:46

blackboxsw requested changes Jan 12, 2021

View reviewed changes

ubuntu-server-builder reviewed Jan 12, 2021

View reviewed changes

Comment thread cloudinit/sources/DataSourceOVF.py Outdated

[VMware] Support cloudinit raw data feature: address comments

59cac2d

xiaofengw1205 requested a review from blackboxsw January 13, 2021 04:31

Merge branch 'master' into xiaofengw-support-cloudinit-raw-data-feature

bcf2361

blackboxsw approved these changes Jan 13, 2021

View reviewed changes

Merge branch 'master' into xiaofengw-support-cloudinit-raw-data-feature

e0cf589

blackboxsw merged commit 1163004 into canonical:master Jan 13, 2021

This was referenced May 12, 2023

Release 21.1 #3846

Closed

Randomly set credentials written in cleartext to world-readable file #3854

Closed

	LOG.debug('nics.txt is not exist.')
	LOG.debug('%s does not exist.', nics_path)

Conversation

xiaofengw1205 commented Nov 25, 2020

summary:

Test Steps

Checklist:

Uh oh!

xiaofengw1205 commented Nov 25, 2020

Uh oh!

xiaofengw1205 commented Dec 4, 2020

Uh oh!

smoser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blackboxsw Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

xiaofengw1205 Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blackboxsw Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

xiaofengw1205 Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackboxsw Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

xiaofengw1205 Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

blackboxsw Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaofengw1205 commented Dec 9, 2020

Uh oh!

xiaofengw1205 commented Dec 14, 2020

Uh oh!

Uh oh!

xiaofengw1205 commented Dec 18, 2020

Uh oh!

OddBloke commented Jan 4, 2021

Uh oh!

xiaofengw1205 commented Jan 5, 2021

Uh oh!

xiaofengw1205 commented Jan 5, 2021

Uh oh!

Uh oh!

xiaofengw1205 commented Jan 8, 2021

Uh oh!

xiaofengw1205 commented Jan 11, 2021

Uh oh!

blackboxsw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackboxsw Jan 12, 2021

xiaofengw1205 Dec 9, 2020 •

edited

Loading

blackboxsw left a comment •

edited

Loading