curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks for the refactor.  I think we can simplify a few things.  Comments inline.

Diff comments:

> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..8e5596b 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -82,22 +82,23 @@ def apply_net(target, network_state=None, network_config=None):
>      elif network_config:
>          netcfg = config.load_config(network_config)
>  
> -        # curtin will pass-through the netconfig into the target
> -        # for rendering at runtime unless the target OS does not
> -        # support NETWORK_CONFIG_V2 feature.
> -        LOG.info('Checking cloud-init in target [%s] for network '
> -                 'configuration passthrough support.', target)
> -        try:
> -            passthrough = net.netconfig_passthrough_available(target)
> -        except util.ProcessExecutionError:
> -            LOG.warning('Failed to determine if passthrough is available')
> -
> -        if passthrough:
> -            LOG.info('Passing network configuration through to target: %s',
> -                     target)
> -            net.render_netconfig_passthrough(target, netconfig=netcfg)
> -        else:
> -            ns = net.parse_net_config_data(netcfg.get('network', {}))
> +        if netcfg:

Why do we need this?  If we set net-meta to mode=disabled, then there won't be any network_config?  The call path is:

curthoooks.py:
  apply_networking:
     netconf = state.get('network_config')

This file is only written to in net_meta.py, and for 'disabled' we return without writing the file.

> +            # curtin will pass-through the netconfig into the target
> +            # for rendering at runtime unless the target OS does not
> +            # support NETWORK_CONFIG_V2 feature.
> +            LOG.info('Checking cloud-init in target [%s] for network '
> +                     'configuration passthrough support.', target)
> +            try:
> +                passthrough = net.netconfig_passthrough_available(target)
> +            except util.ProcessExecutionError:
> +                LOG.warning('Failed to determine if passthrough is available')
> +
> +            if passthrough:
> +                LOG.info('Passing network configuration through to target: %s',
> +                         target)
> +                net.render_netconfig_passthrough(target, netconfig=netcfg)
> +            else:
> +                ns = net.parse_net_config_data(netcfg.get('network', {}))
>  
>      if not passthrough:
>          LOG.info('Rendering network configuration in target')
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..e0498f6 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1155,7 +1155,7 @@ def detect_required_packages(cfg, osfamily=DISTROS.debian):
>          if not isinstance(cfg.get(cfg_type), dict):
>              continue
>  
> -        cfg_version = cfg[cfg_type].get('version')
> +        cfg_version = cfg[cfg_type].get('version', 1)

Why do we change this?

>          if not isinstance(cfg_version, int) or cfg_version not in cfg_map:
>              msg = ('Supplied configuration version "%s", for config type'
>                     '"%s" is not present in the known mapping.' % (cfg_version,
> diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
> index fdb909e..43caadf 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -80,7 +80,7 @@ def net_meta(args):
>  
>      state = util.load_command_environment()
>      cfg = config.load_command_config(args, state)
> -    if cfg.get("network") is not None:
> +    if cfg.get("network") is not None and args.mode != 'disabled':

If you handle args.mode == 'disabled' first then none of the rest of this file needs to change

>          args.mode = "custom"
>  
>      eni = "etc/network/interfaces"
> diff --git a/curtin/net/__init__.py b/curtin/net/__init__.py
> index ef2ba26..0fdd2f2 100644
> --- a/curtin/net/__init__.py
> +++ b/curtin/net/__init__.py
> @@ -251,6 +251,7 @@ def parse_net_config_data(net_config):
>      :param net_config: curtin network config dict
>      """
>      state = None
> +    net_config.setdefault('version', 1)

Why do you set the default version?  Won't we now accept files which are not actually valid network-config (ie, missing version: 1) ?

>      if 'version' in net_config and 'config' in net_config:
>          ns = network_state.NetworkState(version=net_config.get('version'),
>                                          config=net_config.get('config'))
> diff --git a/curtin/net/deps.py b/curtin/net/deps.py
> index fd9e3c0..96d3cb2 100644
> --- a/curtin/net/deps.py
> +++ b/curtin/net/deps.py
> @@ -21,8 +21,10 @@ def network_config_required_packages(network_config, mapping=None):
>      if 'network' in network_config:
>          network_config = network_config.get('network')
>  
> +    if network_config.get('config', '') == 'disabled':

network_config.get('config') is sufficient, the default return value for missing key is None.

> +        dev_configs = set()
>      # v1 has 'config' key and uses type: devtype elements
> -    if 'config' in network_config:
> +    elif 'config' in network_config:

I see.  We can do something like:

if 'config' in network_config:
   dev_configs = (set() if network_config['config'] == 'disabled' else
                  set(device['type']) for device in network_config['config'])
else
   # v2 here

>          dev_configs = set(device['type']
>                            for device in network_config['config'])
>      else:
> diff --git a/curtin/net/network_state.py b/curtin/net/network_state.py
> index ab0f277..f9290f0 100644
> --- a/curtin/net/network_state.py
> +++ b/curtin/net/network_state.py
> @@ -73,8 +73,10 @@ class NetworkState:
>      def parse_config(self):
>          # rebuild network state
>          for command in self.config:
> -            handler = self.command_handlers.get(command['type'])
> -            handler(command)
> +            # If network is disabled, there is no command to be executed
> +            if type(command) is dict:
> +                handler = self.command_handlers.get(command['type'])
> +                handler(command)

I would suggest handling this in the __init__ method,  if config is None (or 'disabled') the self.config = []

>  
>      def valid_command(self, command, required_keys):
>          if not required_keys:
> @@ -379,7 +381,7 @@ if __name__ == '__main__':
>      from curtin import net
>  
>      def load_config(nc):
> -        version = nc.get('version')
> +        version = nc.get('version', 1)

Let's not default to 1

>          config = nc.get('config')
>          return (version, config)
>  
> diff --git a/tests/unittests/test_commands_net_meta.py b/tests/unittests/test_commands_net_meta.py
> new file mode 100644
> index 0000000..987bf12
> --- /dev/null
> +++ b/tests/unittests/test_commands_net_meta.py
> @@ -0,0 +1,126 @@
> +# This file is part of curtin. See LICENSE file for copyright and license info.
> +
> +import os
> +
> +from mock import MagicMock, patch, call
> +
> +from .helpers import CiTestCase
> +
> +from curtin.commands.net_meta import net_meta
> +
> +from sys import version_info
> +if version_info.major == 2:
> +    import __builtin__ as builtins
> +else:
> +    import builtins
> +
> +
> +class NetMetaTarget:
> +    def __init__(self, target, mode=None, devices=None):
> +        self.target = target
> +        self.mode = mode
> +        self.devices = devices
> +
> +
> +class TestNetMeta(CiTestCase):
> +
> +    def setUp(self):
> +        super(TestNetMeta, self).setUp()
> +
> +        patches = [
> +            ('curtin.util.run_hook_if_exists', 'm_run_hook'),
> +            ('curtin.util.load_command_environment', 'm_command_env'),
> +            ('curtin.config.load_command_config', 'm_command_config'),
> +            ('curtin.config.dump_config', 'm_dump_config'),
> +            ('os.environ', 'm_os_environ')
> +        ]
> +
> +        for (tgt, attr) in patches:
> +            self.add_patch(tgt, attr)
> +
> +        self.args = NetMetaTarget(
> +            target='net-meta-target'
> +        )
> +
> +        self.base_network_config = {
> +            'network': {
> +                'version': 1,
> +                'config': {
> +                    'type': 'physical',
> +                    'name': 'interface0',
> +                    'mac_address': '52:54:00:12:34:00',
> +                    'subnets': {
> +                        'type': 'dhcp4'
> +                    }
> +                }
> +            }
> +        }
> +
> +        self.disabled_network_config = {
> +            'network': {
> +                'version': 1,
> +                'config': 'disabled'
> +            }
> +        }
> +
> +    @patch.object(builtins, "open")

you want to use tests/unittests/helpers.py:simple_mocked_open()

> +    def test_net_meta_with_disabled_network(self, m_open):
> +        output_network_path = '/tmp/my-network-config'

Use: self.tmp_path('my-network-config')

> +        expected_exit_code = 0
> +        self.m_run_hook.return_value = False
> +        self.m_command_env.return_value = {}
> +        self.m_command_config.return_value = self.base_network_config
> +        self.m_os_environ.get.return_value = output_network_path
> +        self.args.mode = 'disabled'
> +
> +        with self.assertRaises(SystemExit) as cm:
> +            net_meta(self.args)
> +
> +        self.assertEqual(expected_exit_code, cm.exception.code)
> +        print(self.m_run_hook.call_args_list)
> +        self.m_run_hook.assert_called_with(
> +            self.args.target, 'network-config')
> +        self.m_command_env.assert_called_with()
> +        self.m_command_config.assert_called_with(self.args, {})
> +
> +        self.assertEquals(self.args.mode, 'disabled')
> +        self.m_os_environ.get.assert_called_with(
> +            'OUTPUT_NETWORK_CONFIG', '')
> +        self.assertEqual(0, self.m_dump_config.call_count)
> +        self.assertFalse(os.path.isfile(output_network_path))
> +        self.assertEqual(0, m_open.call_count)
> +
> +    @patch.object(builtins, "open")
> +    def test_net_meta_with_config_network(self, m_open):
> +        output_network_path = '/tmp/my-network-config'
> +        network_config = self.base_network_config
> +        dump_content = 'yaml-format-network-config'
> +        expected_m_command_env_calls = 2
> +        expected_m_command_config_calls = 2
> +        expected_exit_code = 0
> +        m_file = MagicMock()
> +
> +        self.m_run_hook.return_value = False
> +        self.m_command_env.return_value = {}
> +        self.m_command_config.return_value = network_config
> +        self.m_os_environ.get.return_value = output_network_path
> +        self.m_dump_config.return_value = dump_content
> +        m_open.return_value = m_file
> +
> +        with self.assertRaises(SystemExit) as cm:
> +            net_meta(self.args)
> +
> +        self.assertEqual(expected_exit_code, cm.exception.code)
> +        self.m_run_hook.assert_called_with(
> +            self.args.target, 'network-config')
> +        self.assertEquals(self.args.mode, 'custom')
> +        self.assertEqual(
> +            expected_m_command_env_calls, self.m_command_env.call_count)
> +        self.assertEqual(
> +            expected_m_command_config_calls, self.m_command_env.call_count)
> +        self.m_dump_config.assert_called_with(network_config)
> +        self.assertEqual(
> +            call(output_network_path, 'w'), m_open.call_args_list[-1])
> +        self.assertEqual(
> +            call(dump_content),
> +            m_file.__enter__.return_value.write.call_args_list[-1])
> diff --git a/tests/vmtests/test_network_disabled.py b/tests/vmtests/test_network_disabled.py
> new file mode 100644
> index 0000000..5ab2b08
> --- /dev/null
> +++ b/tests/vmtests/test_network_disabled.py
> @@ -0,0 +1,38 @@
> +# This file is part of curtin. See LICENSE file for copyright and license info.
> +
> +from . import VMBaseClass
> +from .releases import base_vm_classes as relbase
> +
> +import os
> +import textwrap
> +
> +
> +class TestNetMetaDisabledModeBaseTestsAbs(VMBaseClass):

You can subclass from test_network.py:TestNetworkAbs

it also already copies /etc/cloud to ./etc_cloud

> +    """ Test disabled network config """
> +
> +    interactive = False
> +    test_type = 'network'
> +
> +    extra_disks = []
> +    extra_nics = []
> +
> +    conf_file = "examples/tests/network_disabled.yaml"
> +
> +    extra_collect_scripts = [textwrap.dedent("""
> +        cd OUTPUT_COLLECT_D
> +        cp -av /etc/cloud ./etc_cloud
> +
> +        exit 0
> +        """)]
> +
> +    def test_cloudinit_network_not_created(self):
> +        cc_passthrough = "cloud.cfg.d/50-curtin-networking.cfg"
> +
> +        pt_file = os.path.join(self.td.collect, 'etc_cloud',
> +                               cc_passthrough)
> +        self.assertFalse(os.path.exists(pt_file))
> +
> +
> +class FocalTestNetMetaDisabledMode(relbase.focal,
> +                                   TestNetMetaDisabledModeBaseTestsAbs):
> +    __test__ = True


-- 
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.