cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

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.