I have some nits there, but I think this looks good. Thanks.
Diff comments:
> diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
> index a1b0db1..c3a8add 100644
> --- a/cloudinit/net/__init__.py
> +++ b/cloudinit/net/__init__.py
> @@ -275,20 +275,45 @@ def apply_network_config_names(netcfg,
> strict_present=True, strict_busy=True):
> expected that the network system will create other devices with the
> correct name in place."""
> renames = []
> - for ent in netcfg.get('config', {}):
> - if ent.get('type') != 'physical':
> - continue
> - mac = ent.get('mac_address')
> - if not mac:
> - continue
> - name = ent.get('name')
> - driver = ent.get('params', {}).get('driver')
> - device_id = ent.get('params', {}).get('device_id')
> - if not driver:
> - driver = device_driver(name)
> - if not device_id:
> - device_id = device_devid(name)
> - renames.append([mac, name, driver, device_id])
> +
> + def _version_1(netcfg):
> + for ent in netcfg.get('config', {}):
> + if ent.get('type') != 'physical':
> + continue
> + mac = ent.get('mac_address')
> + if not mac:
> + continue
> + name = ent.get('name')
> + driver = ent.get('params', {}).get('driver')
> + device_id = ent.get('params', {}).get('device_id')
> + if not driver:
> + driver = device_driver(name)
> + if not device_id:
> + device_id = device_devid(name)
> + renames.append([mac, name, driver, device_id])
> +
> + def _version_2(netcfg):
> + for key, ent in netcfg.get('ethernets', {}).items():
> + # only rename if configured to do so
> + name = ent.get('set-name')
> + if not name:
> + continue
> + # cloud-init requires macaddress for renaming
> + mac = ent.get('match', {}).get('macaddress')
> + if not mac:
> + continue
> + driver = ent.get('match', {}).get('driver')
> + device_id = ent.get('match', {}).get('device_id')
> + if not driver:
> + driver = device_driver(name)
> + if not device_id:
> + device_id = device_devid(name)
> + renames.append([mac, name, driver, device_id])
> +
> + if netcfg.get('version') == 1:
can you just make _version_2 and _version_1 return renames ?
rather than modifying the locally scoped rename ?
it just is more obvious i think.
then i guess we should probably raise a RuntimeError if we get something else.
Right?
> + _version_1(netcfg)
> + elif netcfg.get('version') == 2:
> + _version_2(netcfg)
>
> return _rename_interfaces(renames)
>
> diff --git a/cloudinit/net/tests/test_init.py
> b/cloudinit/net/tests/test_init.py
> index 8cb4114..7ac9041 100644
> --- a/cloudinit/net/tests/test_init.py
> +++ b/cloudinit/net/tests/test_init.py
> @@ -520,3 +522,88 @@ class TestEphemeralIPV4Network(CiTestCase):
> with net.EphemeralIPv4Network(**params):
> self.assertEqual(expected_setup_calls, m_subp.call_args_list)
> m_subp.assert_has_calls(expected_teardown_calls)
> +
> +
> +class TestApplyNetworkCfgNames(CiTestCase):
> + V1_CONFIG = textwrap.dedent("""\
> + version: 1
> + config:
> + - type: physical
> + name: interface0
> + mac_address: "52:54:00:12:34:00"
> + subnets:
> + - type: static
> + address: 10.0.2.15
> + netmask: 255.255.255.0
> + gateway: 10.0.2.2
> + """)
> + V2_CONFIG = textwrap.dedent("""\
> + version: 2
> + ethernets:
> + interface0:
> + match:
> + macaddress: "52:54:00:12:34:00"
> + addresses:
> + - 10.0.2.15/24
> + gateway4: 10.0.2.2
> + set-name: interface0
> + """)
> +
> + V2_CONFIG_NO_SETNAME = textwrap.dedent("""\
> + version: 2
> + ethernets:
> + interface0:
> + match:
> + macaddress: "52:54:00:12:34:00"
> + addresses:
> + - 10.0.2.15/24
> + gateway4: 10.0.2.2
> + """)
> +
> + V2_CONFIG_NO_MAC = textwrap.dedent("""\
> + version: 2
> + ethernets:
> + interface0:
> + match:
> + driver: virtio-net
> + addresses:
> + - 10.0.2.15/24
> + gateway4: 10.0.2.2
> + set-name: interface0
> + """)
> +
> + @mock.patch('cloudinit.net.device_devid')
> + @mock.patch('cloudinit.net.device_driver')
> + @mock.patch('cloudinit.net._rename_interfaces')
> + def test_apply_v1_renames(self, m_rename_interfaces, m_device_driver,
> + m_device_devid):
> + m_device_driver.return_value = 'virtio_net'
> + m_device_devid.return_value = '0x15d8'
> +
> + net.apply_network_config_names(yaml.load(self.V1_CONFIG))
the yaml.load() doesnt give us much does it?
compared to:
V2_CONFIG_NO_MAC = {
'version': 2,
'ethernets': {
'interface0': {
'match': {'driver': 'virtio-net'},
'addresses': ['10.0.2.15/24'],
'gateway4': [10.0.2.2], 'set-name': 'interface0'}}}
then net.apply_network_config_names(V2_CONFIG_NO_MAC)
> +
> + call = ['52:54:00:12:34:00', 'interface0', 'virtio_net', '0x15d8']
> + m_rename_interfaces.assert_called_with([call])
> +
> + @mock.patch('cloudinit.net.device_devid')
> + @mock.patch('cloudinit.net.device_driver')
> + @mock.patch('cloudinit.net._rename_interfaces')
> + def test_apply_v2_renames(self, m_rename_interfaces, m_device_driver,
> + m_device_devid):
> + m_device_driver.return_value = 'virtio_net'
> + m_device_devid.return_value = '0x15d8'
> +
> + net.apply_network_config_names(yaml.load(self.V2_CONFIG))
> +
> + call = ['52:54:00:12:34:00', 'interface0', 'virtio_net', '0x15d8']
> + m_rename_interfaces.assert_called_with([call])
> +
> + @mock.patch('cloudinit.net._rename_interfaces')
> + def test_apply_v2_renames_skips_without_setname(self,
> m_rename_interfaces):
> + net.apply_network_config_names(yaml.load(self.V2_CONFIG_NO_SETNAME))
> + m_rename_interfaces.assert_called_with([])
> +
> + @mock.patch('cloudinit.net._rename_interfaces')
> + def test_apply_v2_renames_skips_without_mac(self, m_rename_interfaces):
> + net.apply_network_config_names(yaml.load(self.V2_CONFIG_NO_MAC))
> + m_rename_interfaces.assert_called_with([])
--
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/336464
Your team cloud-init commiters is requested to review the proposed merge of
~raharper/cloud-init:fix/cloud-init-network-rename-with-v2 into
cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
