From 7f2e99f5345c227d07849da68acdf8562b44c3e1 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 25 May 2016 17:05:09 -0400 Subject: commit to push for fear of loss. == background == DataSource Mode (dsmode) is present in many datasources in cloud-init. dsmode was originally added to cloud-init to specify when this datasource should be 'realized'. cloud-init has 4 stages of boot. a.) cloud-init --local . network is guaranteed not present. b.) cloud-init (--network). network is guaranteed present. c.) cloud-config d.) cloud-init final 'init_modules' [1] are run "as early as possible". And as such, are executed in either 'a' or 'b' based on the datasource. However, executing them means that user-data has been fully consumed. User-data and vendor-data may have '#include http://...' which then rely on the network being present. boothooks are an example of the things run in init_modules. The 'dsmode' was a way for a user to indicate that init_modules should run at 'a' (dsmode=local) or 'b' (dsmode=net) directly. Things were further confused when a datasource could provide networking configuration. Then, we needed to apply the networking config at 'a' but if the user had provided boothooks that expected networking, then the init_modules would need to be executed at 'b'. The config drive datasource hacked its way through this and applies networking if *it* detects it is a new instance. == Suggested Change == The plan is to 1. incorporate 'dsmode' into DataSource superclass 2. make all existing datasources default to network 3. apply any networking configuration from a datasource on first boot only apply_networking will always rename network devices when it runs. for bug 1579130. 4. run init_modules at cloud-init (network) time frame unless datasource is 'local'. 5. Datasources can provide a 'first_boot' method that will be called when a new instance_id is found. This will allow the config drive's write_files to be applied once. Over all, this will very much simplify things. We'll no longer have 2 sources like DataSourceNoCloud and DataSourceNoCloudNet, but would just have one source with a dsmode. == Concerns == Some things have odd reliance on dsmode. For example, OpenNebula's get_hostname uses it to determine if it should do a lookup of an ip address. == Bugs to fix here == http://pad.lv/1577982 ConfigDrive: cloud-init fails to configure network from network_data.json http://pad.lv/1579130 need to support systemd.link renaming of devices in container http://pad.lv/1577844 Drop unnecessary blocking of all net udev rules --- cloudinit/sources/DataSourceConfigDrive.py | 90 ++++++++---------------------- 1 file changed, 23 insertions(+), 67 deletions(-) (limited to 'cloudinit/sources/DataSourceConfigDrive.py') diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py index 52a9f543..20df5fcd 100644 --- a/cloudinit/sources/DataSourceConfigDrive.py +++ b/cloudinit/sources/DataSourceConfigDrive.py @@ -22,6 +22,7 @@ import copy import os from cloudinit import log as logging +from cloudinit import net from cloudinit import sources from cloudinit import util @@ -35,7 +36,6 @@ DEFAULT_MODE = 'pass' DEFAULT_METADATA = { "instance-id": DEFAULT_IID, } -VALID_DSMODES = ("local", "net", "pass", "disabled") FS_TYPES = ('vfat', 'iso9660') LABEL_TYPES = ('config-2',) POSSIBLE_MOUNTS = ('sr', 'cd') @@ -47,12 +47,12 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource): def __init__(self, sys_cfg, distro, paths): super(DataSourceConfigDrive, self).__init__(sys_cfg, distro, paths) self.source = None - self.dsmode = 'local' self.seed_dir = os.path.join(paths.seed_dir, 'config_drive') self.version = None self.ec2_metadata = None self._network_config = None self.network_json = None + self.network_eni = None self.files = {} def __str__(self): @@ -98,38 +98,22 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource): md = results.get('metadata', {}) md = util.mergemanydict([md, DEFAULT_METADATA]) - user_dsmode = results.get('dsmode', None) - if user_dsmode not in VALID_DSMODES + (None,): - LOG.warn("User specified invalid mode: %s", user_dsmode) - user_dsmode = None - dsmode = get_ds_mode(cfgdrv_ver=results['version'], - ds_cfg=self.ds_cfg.get('dsmode'), - user=user_dsmode) + self.dsmode = self._determine_dsmode( + [results.get('dsmode'), self.ds_cfg.get('dsmode'), + sources.DSMODE_PASS if results['version'] == 1 else None]) - if dsmode == "disabled": - # most likely user specified + if self.dsmode == sources.DSMODE_DISABLED: return False - # TODO(smoser): fix this, its dirty. - # we want to do some things (writing files and network config) - # only on first boot, and even then, we want to do so in the - # local datasource (so they happen earlier) even if the configured - # dsmode is 'net' or 'pass'. To do this, we check the previous - # instance-id + # This is legacy and sneaky. If dsmode is 'pass' then write + # 'injected files' and apply legacy ENI network format. prev_iid = get_previous_iid(self.paths) cur_iid = md['instance-id'] - if prev_iid != cur_iid and self.dsmode == "local": + if prev_iid != cur_iid and self.dsmode == sources.DSMODE_PASS: on_first_boot(results, distro=self.distro) - - # dsmode != self.dsmode here if: - # * dsmode = "pass", pass means it should only copy files and then - # pass to another datasource - # * dsmode = "net" and self.dsmode = "local" - # so that user boothooks would be applied with network, the - # local datasource just gets out of the way, and lets the net claim - if dsmode != self.dsmode: - LOG.debug("%s: not claiming datasource, dsmode=%s", self, dsmode) + LOG.debug("%s: not claiming datasource, dsmode=%s", self, + self.dsmode) return False self.source = found @@ -147,12 +131,11 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource): LOG.warn("Invalid content in vendor-data: %s", e) self.vendordata_raw = None - try: - self.network_json = results.get('networkdata') - except ValueError as e: - LOG.warn("Invalid content in network-data: %s", e) - self.network_json = None - + # network_config is an /etc/network/interfaces formated file and is + # obsolete compared to networkdata (from network_data.json) but both + # might be present. + self.network_eni = results.get("network_config") + self.network_json = results.get('networkdata') return True def check_instance_id(self, sys_cfg): @@ -164,40 +147,11 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource): if self._network_config is None: if self.network_json is not None: self._network_config = convert_network_data(self.network_json) + elif self.network_eni is not None: + self._network_config = net.convert_eni_data(self.network_eni) return self._network_config -class DataSourceConfigDriveNet(DataSourceConfigDrive): - def __init__(self, sys_cfg, distro, paths): - DataSourceConfigDrive.__init__(self, sys_cfg, distro, paths) - self.dsmode = 'net' - - -def get_ds_mode(cfgdrv_ver, ds_cfg=None, user=None): - """Determine what mode should be used. - valid values are 'pass', 'disabled', 'local', 'net' - """ - # user passed data trumps everything - if user is not None: - return user - - if ds_cfg is not None: - return ds_cfg - - # at config-drive version 1, the default behavior was pass. That - # meant to not use use it as primary data source, but expect a ec2 metadata - # source. for version 2, we default to 'net', which means - # the DataSourceConfigDriveNet, would be used. - # - # this could change in the future. If there was definitive metadata - # that indicated presense of an openstack metadata service, then - # we could change to 'pass' by default also. The motivation for that - # would be 'cloud-init query' as the web service could be more dynamic - if cfgdrv_ver == 1: - return "pass" - return "net" - - def read_config_drive(source_dir): reader = openstack.ConfigDriveReader(source_dir) finders = [ @@ -231,9 +185,12 @@ def on_first_boot(data, distro=None): % (type(data))) net_conf = data.get("network_config", '') if net_conf and distro: - LOG.debug("Updating network interfaces from config drive") + LOG.warn("Updating network interfaces from config drive") distro.apply_network(net_conf) - files = data.get('files', {}) + write_injected_files(data.get('files')) + + +def write_injected_files(files): if files: LOG.debug("Writing %s injected files", len(files)) for (filename, content) in files.items(): @@ -296,7 +253,6 @@ def find_candidate_devs(probe_optical=True): # Used to match classes to dependencies datasources = [ (DataSourceConfigDrive, (sources.DEP_FILESYSTEM, )), - (DataSourceConfigDriveNet, (sources.DEP_FILESYSTEM, sources.DEP_NETWORK)), ] -- cgit v1.2.3 From 399eb29662ee67f7744d3a482f63e8af377e75db Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 26 May 2016 11:22:39 -0400 Subject: config drive: log where network config came from --- cloudinit/sources/DataSourceConfigDrive.py | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'cloudinit/sources/DataSourceConfigDrive.py') diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py index 20df5fcd..2d13a32f 100644 --- a/cloudinit/sources/DataSourceConfigDrive.py +++ b/cloudinit/sources/DataSourceConfigDrive.py @@ -146,9 +146,13 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource): def network_config(self): if self._network_config is None: if self.network_json is not None: + LOG.debug("network config provided via network_json") self._network_config = convert_network_data(self.network_json) elif self.network_eni is not None: self._network_config = net.convert_eni_data(self.network_eni) + LOG.debug("network config provided via converted eni data") + else: + LOG.debug("no network configuration available") return self._network_config -- cgit v1.2.3 From d709524941ba2b4e06940a9eb0861f0819d5560f Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 2 Jun 2016 15:18:27 -0400 Subject: re-add the 'Net' classes for datasources When the .pkl file is loaded, the module that it is loaded from must have the same symbol. Ie, if booted once and got DataSourceConfigDriveNet then upgraded and rebooted, then next boot would show Can't get attribute 'DataSourceConfigDriveNet' --- cloudinit/sources/DataSourceCloudSigma.py | 3 +++ cloudinit/sources/DataSourceConfigDrive.py | 25 ++++++++++++++----------- cloudinit/sources/DataSourceOpenNebula.py | 3 +++ 3 files changed, 20 insertions(+), 11 deletions(-) (limited to 'cloudinit/sources/DataSourceConfigDrive.py') diff --git a/cloudinit/sources/DataSourceCloudSigma.py b/cloudinit/sources/DataSourceCloudSigma.py index 07e8ae11..d1f806d6 100644 --- a/cloudinit/sources/DataSourceCloudSigma.py +++ b/cloudinit/sources/DataSourceCloudSigma.py @@ -115,6 +115,9 @@ class DataSourceCloudSigma(sources.DataSource): return self.metadata['uuid'] +# Legacy: Must be present in case we load an old pkl object +DataSourceCloudSigmaNet = DataSourceCloudSigma + # Used to match classes to dependencies. Since this datasource uses the serial # port network is not really required, so it's okay to load without it, too. datasources = [ diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py index 2d13a32f..61d967d9 100644 --- a/cloudinit/sources/DataSourceConfigDrive.py +++ b/cloudinit/sources/DataSourceConfigDrive.py @@ -254,17 +254,6 @@ def find_candidate_devs(probe_optical=True): return devices -# Used to match classes to dependencies -datasources = [ - (DataSourceConfigDrive, (sources.DEP_FILESYSTEM, )), -] - - -# Return a list of data sources that match this set of dependencies -def get_datasource_list(depends): - return sources.list_from_depends(depends, datasources) - - # Convert OpenStack ConfigDrive NetworkData json to network_config yaml def convert_network_data(network_json=None): """Return a dictionary of network_config by parsing provided @@ -382,3 +371,17 @@ def convert_network_data(network_json=None): config.append(cfg) return {'version': 1, 'config': config} + + +# Legacy: Must be present in case we load an old pkl object +DataSourceConfigDriveNet = DataSourceConfigDrive + +# Used to match classes to dependencies +datasources = [ + (DataSourceConfigDrive, (sources.DEP_FILESYSTEM, )), +] + + +# Return a list of data sources that match this set of dependencies +def get_datasource_list(depends): + return sources.list_from_depends(depends, datasources) diff --git a/cloudinit/sources/DataSourceOpenNebula.py b/cloudinit/sources/DataSourceOpenNebula.py index 15819a4f..8f85b115 100644 --- a/cloudinit/sources/DataSourceOpenNebula.py +++ b/cloudinit/sources/DataSourceOpenNebula.py @@ -415,6 +415,9 @@ def read_context_disk_dir(source_dir, asuser=None): return results +# Legacy: Must be present in case we load an old pkl object +DataSourceOpenNebulaNet = DataSourceOpenNebula + # Used to match classes to dependencies datasources = [ (DataSourceOpenNebula, (sources.DEP_FILESYSTEM, )), -- cgit v1.2.3 From 6bd7fbc35ac8726a8a0f422cd802d290c236bf3b Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 2 Jun 2016 23:03:38 -0400 Subject: ConfigDrive: do not use 'id' on a link for the device name 'id' on a link in the openstack spec should be "Generic, generated ID". current implementation was to use the host's name for the host side nic. Which provided names like 'tap-adfasdffd'. We do not want to name devices like that as its quite unexpected and non user friendly. So here we use the system name for any nic that is present, but then require that the nics found also be present at the time of rendering. The end result is that if the system boots with net.ifnames=0 then it will get 'eth0' like names. and if it boots without net.ifnames then it will get enp0s1 like names. --- cloudinit/net/__init__.py | 15 +++++--- cloudinit/sources/DataSourceConfigDrive.py | 29 ++++++++++++--- .../unittests/test_datasource/test_configdrive.py | 41 ++++++++++++++++++++-- 3 files changed, 74 insertions(+), 11 deletions(-) (limited to 'cloudinit/sources/DataSourceConfigDrive.py') diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index 05152ead..f47053b2 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -970,9 +970,16 @@ def get_interface_mac(ifname): return read_sys_net(ifname, "address", enoent=False) -def get_ifname_mac_pairs(): - """Build a list of tuples (ifname, mac)""" - return [(ifname, get_interface_mac(ifname)) for ifname in get_devicelist()] - +def get_interfaces_by_mac(devs=None): + """Build a dictionary of tuples {mac: name}""" + if devs is None: + devs = get_devicelist() + ret = {} + for name in devs: + mac = get_interface_mac(name) + # some devices may not have a mac (tun0) + if mac: + ret[mac] = name + return ret # vi: ts=4 expandtab syntax=python diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py index 61d967d9..3cc9155d 100644 --- a/cloudinit/sources/DataSourceConfigDrive.py +++ b/cloudinit/sources/DataSourceConfigDrive.py @@ -255,7 +255,7 @@ def find_candidate_devs(probe_optical=True): # Convert OpenStack ConfigDrive NetworkData json to network_config yaml -def convert_network_data(network_json=None): +def convert_network_data(network_json=None, known_macs=None): """Return a dictionary of network_config by parsing provided OpenStack ConfigDrive NetworkData json format @@ -319,9 +319,15 @@ def convert_network_data(network_json=None): subnets = [] cfg = {k: v for k, v in link.items() if k in valid_keys['physical']} - cfg.update({'name': link['id']}) - for network in [net for net in networks - if net['link'] == link['id']]: + # 'name' is not in openstack spec yet, but we will support it if it is + # present. The 'id' in the spec is currently implemented as the host + # nic's name, meaning something like 'tap-adfasdffd'. We do not want + # to name guest devices with such ugly names. + if 'name' in link: + cfg['name'] = link['name'] + + for network in [n for n in networks + if n['link'] == link['id']]: subnet = {k: v for k, v in network.items() if k in valid_keys['subnet']} if 'dhcp' in network['type']: @@ -365,6 +371,21 @@ def convert_network_data(network_json=None): config.append(cfg) + need_names = [d for d in config + if d.get('type') == 'physical' and 'name' not in d] + + if need_names: + if known_macs is None: + known_macs = net.get_interfaces_by_mac() + + for d in need_names: + mac = d.get('mac_address') + if not mac: + raise ValueError("No mac_address or name entry for %s" % d) + if mac not in known_macs: + raise ValueError("Unable to find a system nic for %s" % d) + d['name'] = known_macs[mac] + for service in services: cfg = service cfg.update({'type': 'nameserver'}) diff --git a/tests/unittests/test_datasource/test_configdrive.py b/tests/unittests/test_datasource/test_configdrive.py index 195b8207..1364b39d 100644 --- a/tests/unittests/test_datasource/test_configdrive.py +++ b/tests/unittests/test_datasource/test_configdrive.py @@ -73,7 +73,7 @@ NETWORK_DATA = { 'type': 'ovs', 'mtu': None, 'id': 'tap2f88d109-5b'}, {'vif_id': '1a5382f8-04c5-4d75-ab98-d666c1ef52cc', 'ethernet_mac_address': 'fa:16:3e:05:30:fe', - 'type': 'ovs', 'mtu': None, 'id': 'tap1a5382f8-04'} + 'type': 'ovs', 'mtu': None, 'id': 'tap1a5382f8-04', 'name': 'nic0'} ], 'networks': [ {'link': 'tap2ecc7709-b3', 'type': 'ipv4_dhcp', @@ -88,6 +88,10 @@ NETWORK_DATA = { ] } +KNOWN_MACS = { + 'fa:16:3e:69:b0:58': 'enp0s1', + 'fa:16:3e:d4:57:ad': 'enp0s2'} + CFG_DRIVE_FILES_V2 = { 'ec2/2009-04-04/meta-data.json': json.dumps(EC2_META), 'ec2/2009-04-04/user-data': USER_DATA, @@ -365,10 +369,40 @@ class TestConfigDriveDataSource(TestCase): """Verify that network_data is converted and present on ds object.""" populate_dir(self.tmp, CFG_DRIVE_FILES_V2) myds = cfg_ds_from_dir(self.tmp) - network_config = ds.convert_network_data(NETWORK_DATA) + network_config = ds.convert_network_data(NETWORK_DATA, + known_macs=KNOWN_MACS) self.assertEqual(myds.network_config, network_config) +class TestConvertNetworkData(TestCase): + def _getnames_in_config(self, ncfg): + return set([n['name'] for n in ncfg['config'] + if n['type'] == 'physical']) + + def test_conversion_fills_names(self): + ncfg = ds.convert_network_data(NETWORK_DATA, known_macs=KNOWN_MACS) + expected = set(['nic0', 'enp0s1', 'enp0s2']) + found = self._getnames_in_config(ncfg) + self.assertEqual(found, expected) + + @mock.patch('cloudinit.net.get_interfaces_by_mac') + def test_convert_reads_system_prefers_name(self, get_interfaces_by_mac): + macs = KNOWN_MACS.copy() + macs.update({'fa:16:3e:05:30:fe': 'foonic1', + 'fa:16:3e:69:b0:58': 'ens1'}) + get_interfaces_by_mac.return_value = macs + + ncfg = ds.convert_network_data(NETWORK_DATA) + expected = set(['nic0', 'ens1', 'enp0s2']) + found = self._getnames_in_config(ncfg) + self.assertEqual(found, expected) + + def test_convert_raises_value_error_on_missing_name(self): + macs = {'aa:aa:aa:aa:aa:00': 'ens1'} + self.assertRaises(ValueError, ds.convert_network_data, + NETWORK_DATA, known_macs=macs) + + def cfg_ds_from_dir(seed_d): found = ds.read_config_drive(seed_d) cfg_ds = ds.DataSourceConfigDrive(settings.CFG_BUILTIN, None, @@ -387,7 +421,8 @@ def populate_ds_from_read_config(cfg_ds, source, results): cfg_ds.userdata_raw = results.get('userdata') cfg_ds.version = results.get('version') cfg_ds.network_json = results.get('networkdata') - cfg_ds._network_config = ds.convert_network_data(cfg_ds.network_json) + cfg_ds._network_config = ds.convert_network_data( + cfg_ds.network_json, known_macs=KNOWN_MACS) def populate_dir(seed_dir, files): -- cgit v1.2.3 From 42a7d2b6d44be5fd6e41734902e08897b709015d Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 3 Jun 2016 15:06:55 -0400 Subject: config drive conversion: recognize 'bridge' as a physical type, fix mtu the network json in openstack provides a type of 'bridge' when the underlying (host) type is a bridge. Silly, but we need to consider that a physical device as it will be for us. also, the 'mtu' will appear on the link, not on the route --- cloudinit/sources/DataSourceConfigDrive.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'cloudinit/sources/DataSourceConfigDrive.py') diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py index 3cc9155d..c87f57fd 100644 --- a/cloudinit/sources/DataSourceConfigDrive.py +++ b/cloudinit/sources/DataSourceConfigDrive.py @@ -293,6 +293,7 @@ def convert_network_data(network_json=None, known_macs=None): 'mac_address', 'subnets', 'params', + 'mtu', ], 'subnet': [ 'type', @@ -302,7 +303,6 @@ def convert_network_data(network_json=None, known_macs=None): 'metric', 'gateway', 'pointopoint', - 'mtu', 'scope', 'dns_nameservers', 'dns_search', @@ -342,7 +342,7 @@ def convert_network_data(network_json=None, known_macs=None): }) subnets.append(subnet) cfg.update({'subnets': subnets}) - if link['type'] in ['ethernet', 'vif', 'ovs', 'phy']: + if link['type'] in ['ethernet', 'vif', 'ovs', 'phy', 'bridge']: cfg.update({ 'type': 'physical', 'mac_address': link['ethernet_mac_address']}) -- cgit v1.2.3