curtin-dev team mailing list archive

[Lucas Albuquerque Medeiros de Moura <lucas.moura@xxxxxxxxxxxxx>]

raharper, I have update the PR with the proposed changes and addressed some concerns related to the version key on the network config

Diff comments:

> 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)

If cfg_version is None here, the following checks will fail and we will raise the following exception:

Supplied configuration version "None", for config type"network" is not present in the known mapping

And the curthooks commands fail and we cannot complete the install.

But maybe there is a better way to change that, we could add to the checker if cfg_version is None.

>          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':

Yes, you are totally right. I missed this because I was working on top of my past solution, but your solution is way better

>          args.mode = "custom"
>  
>      eni = "etc/network/interfaces"
> 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':

Fixed

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

I think this way we actually create a set of set, which raises the exception that a set is unhashable.

Maybe we could do:

dev_configs = set() if network_config['config'] == 'disable' else set(
    device['type']) for device in network_config['config'])

But I think the if/elif solution is clearer

>          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)

Done

>  
>      def valid_command(self, command, required_keys):
>          if not required_keys:
> 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")

Done

> +    def test_net_meta_with_disabled_network(self, m_open):
> +        output_network_path = '/tmp/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):

Done, but I had to skip tests that required the use of any key from network_state

> +    """ 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.