Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve Thanks! -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:45e02af4f40d8694897ac4a27237fc66a92d9874 https://jenkins.ubuntu.com/server/job/curtin-ci/122/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/122/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/122/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/122/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/122/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/122//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
raharper, I have updated the docstrings
Diff comments:
> diff --git a/tests/vmtests/test_network_disabled.py
> b/tests/vmtests/test_network_disabled.py
> new file mode 100644
> index 000..cf55030
> --- /dev/null
> +++ b/tests/vmtests/test_network_disabled.py
> @@ -0,0 +1,73 @@
> +# This file is part of curtin. See LICENSE file for copyright and license
> info.
> +
> +from .releases import base_vm_classes as relbase
> +from .test_network import TestNetworkBaseTestsAbs
> +
> +from unittest import SkipTest
> +
> +import os
> +
> +
> +class CurtinDisableNetworkRendering(TestNetworkBaseTestsAbs):
> +""" Test that curtin does not passthrough network config when
> +networking is disabled."""
> +conf_file = "examples/tests/network_disabled.yaml"
> +
> +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))
> +
> +def test_cloudinit_network_passthrough(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_static_routes(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_ip_output(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_etc_resolvconf(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +
> +class CurtinDisableCloudInitNetworking(TestNetworkBaseTestsAbs):
> +""" Test disabling networking with network-config
Fixed
> +with no version key."""
> +conf_file = "examples/tests/network_config_disabled.yaml"
> +
> +def test_etc_resolvconf(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_ip_output(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +
> +class CurtinDisableCloudInitNetworkingVersion1(
> +CurtinDisableCloudInitNetworking
> +):
> +""" Test disabling networking with network-config
> +with version key."""
Fixed
> +conf_file = "examples/tests/network_config_disabled_with_version.yaml"
> +
> +
> +class FocalCurtinDisableNetworkRendering(relbase.focal,
> + CurtinDisableNetworkRendering):
> +__test__ = True
> +
> +
> +class FocalCurtinDisableCloudInitNetworkingVersion1(
> +relbase.focal,
> +CurtinDisableCloudInitNetworkingVersion1
> +):
> +__test__ = True
> +
> +
> +class FocalCurtinDisableCloudInitNetworking(relbase.focal,
> +
> CurtinDisableCloudInitNetworking):
> +__test__ = True
> +
> +
> +# vi: ts=4 expandtab syntax=python
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:acc75e2e528df66ea4ae8d9134a7adb6d14b2d63 https://jenkins.ubuntu.com/server/job/curtin-ci/120/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/120/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/120/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/120/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/120/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/120//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
raharper, I have updated the code with the test refactoring. All network disable tests are now in the same file. -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
On Wed, May 20, 2020 at 1:43 PM Lucas Albuquerque Medeiros de Moura <
lucas.mo...@canonical.com> wrote:
> Wait, I think I missed something for these scenarios:
>
> network: {config: disabled}
> network: {config: disabled, version=1}
>
> We should not create the /etc/cloud/cloud.cfg.d/50-curtin-networking.cfg
> file for them ?
>
> I thought that they were valid cloudinit configs and we should propagate
> them. Right now, we are creating those files for these scenarios.
>
Yes, you're right; Looking back at my first comment we definitely want to
allow someone to write a config to disable cloud-init's networking.
Revising, I'd still like to keep all of the disabled scenarios in a single
test class file, and to help me separate them:
CurtinDisableNetworkRendering (net-meta --mode=disabled)
CurtinDisableCloudInitNetworking (network: {config: disabled})
CurtinDisableCloudInitNetworkingVersion1 (network: {config: disabled,
version=1}
--
> https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
> You are reviewing the proposed merge of
> ~lamoura/curtin:disable-networking-config into curtin:master.
>
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Wait, I think I missed something for these scenarios:
network: {config: disabled}
network: {config: disabled, version=1}
We should not create the /etc/cloud/cloud.cfg.d/50-curtin-networking.cfg file
for them ?
I thought that they were valid cloudinit configs and we should propagate them.
Right now, we are creating those files for these scenarios.
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
For the three scenarios:
net-meta mode=disabled
network: {config: disabled}
network: {config: disabled, version=1}
We need to check that in the target system, we do not find
/etc/cloud/cloud.cfg.d/50-curtin-networking.cfg
So, let's put that unittest in its own base class and then
build each scenario on top of the base class. We'll also
include all of the networking skiptests in the base class
as without networking, we must skip all of those changes.
This all can be done in just the test_network_disabled.py module.
Like so:
https://paste.ubuntu.com/p/cq366Z2BXB/
I bind the conf_file to another base class, so that we don't repeat the
conf when we add additional ubuntu classes, for example when we add
GroovyTestNetworkDisabled, we just declare an additional class specifying
the release and the scenario class we need.
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
The TestNetMetaDisabledModeBaseTestsAbs overrides some of tests that the network disable config should use. For example, this class overrides the test: test_cloudinit_network_passthrough Which is a test that should be performed for the disable cloud config tests. But I can move this override to the concrete class without an issue. But we would need to override them on each Ubuntu version that we need to test this configuration on. Also, TestNetMetaDisabledModeBaseTestsAbs, has the implementation of the test_cloudinit_network_not_created test, which checks if the cloudinit network file was not created. However, this is not the case for the network config disable tests. This means that we would need to remove this implementation from the abstract class or skip it in the network disabled config classes. Since the abstract class is specific for the net_meta disabled mode, I think it would be better to keep that test in the abstract file. With that said, the network disable config tests would need to: * skip the test_cloudinit_network_not_created test * override the conf_file Which are the two main characteristics that define the TestNetMetaDisabledModeBaseTestsAbs class. They would benefit from other network tests that are already skipped, but I don't think that this a huge benefit. But I can be missing something, so if you disagree, just let me know. -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Information
Thanks, one more question on the vmtests; it's not clear to me why we've got
network-disabled tests in test_network.py rather than both of those classes in
test_network_disabled.py and have the two different config versions subclass
from your main TestNetMetaDisabled abstract class?
Diff comments:
> diff --git a/tests/vmtests/test_network.py b/tests/vmtests/test_network.py
> index 7d2e779..c49262a 100644
> --- a/tests/vmtests/test_network.py
> +++ b/tests/vmtests/test_network.py
> @@ -491,4 +491,34 @@ class
> Centos70TestNetworkBasic(centos_relbase.centos70_xenial,
> CentosTestNetworkBasicAbs):
> __test__ = True
>
> +
> +class FocalTestNetworkDisabledConfigWithVersion(relbase.focal,
> +TestNetworkBasicAbs):
> +""" Basic network test but with network config marked as disabled
> +with a version key
> +"""
> +conf_file = "examples/tests/network_config_disabled_with_version.yaml"
> +__test__ = True
> +
> +def test_etc_resolvconf(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_ip_output(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +
Shouldn't both of these be in test_network_disabled? And derived from
TestNetMetaDisabledModeBasicTestsAbs?
> +class FocalTestNetworkDisabledConfig(relbase.focal, TestNetworkBasicAbs):
> +""" Basic network test but with network config marked as disabled
> +without a version key
> +"""
> +conf_file = "examples/tests/network_config_disabled.yaml"
> +__test__ = True
> +
> +def test_etc_resolvconf(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +def test_ip_output(self):
> +raise SkipTest('not available on %s' % self.__class__)
> +
> +
> # vi: ts=4 expandtab syntax=python
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:1bf73abf551d8bdcfa026f943f0d2403544d3af0 https://jenkins.ubuntu.com/server/job/curtin-ci/117/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/117/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/117/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/117/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/117/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/117//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
This MP should get the CI going again: https://code.launchpad.net/~legovini/curtin/+git/curtin/+merge/384245 Background info: the vmtests run on the MAAS images, which are automatically synced as part of vmtest Jenkins jobs when needed. (The logic is slightly more complex, but doesn't matter here.) What seems to be broken is the URL the vmtest-sync-images script uses to retrieve the streams (json data) listing the available images and their location. The MP above fixes the URL. -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:1bf73abf551d8bdcfa026f943f0d2403544d3af0 https://jenkins.ubuntu.com/server/job/curtin-ci/114/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/114/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/114/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/114/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/114/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/114//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
raharper, I have fixed the unittest. Also, both of these tests are currently
skipped for the disable config scenarios, for both config with and without
version key.
Diff comments:
> diff --git a/tests/unittests/test_commands_net_meta.py
> b/tests/unittests/test_commands_net_meta.py
> new file mode 100644
> index 000..2c64bb6
> --- /dev/null
> +++ b/tests/unittests/test_commands_net_meta.py
> @@ -0,0 +1,111 @@
> +# This file is part of curtin. See LICENSE file for copyright and license
> info.
> +
> +import os
> +
> +from mock import MagicMock, call
> +
> +from .helpers import CiTestCase, simple_mocked_open
> +
> +from curtin.commands.net_meta import net_meta
> +
> +
> +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()
> +
> +self.add_patch('curtin.util.run_hook_if_exists', 'm_run_hook')
> +self.add_patch('curtin.util.load_command_environment',
> 'm_command_env')
> +self.add_patch('curtin.config.load_command_config',
> 'm_command_config')
> +self.add_patch('curtin.config.dump_config', 'm_dump_config')
> +self.add_patch('os.environ', 'm_os_environ')
> +
> +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'
> +}
> +}
> +
> +self.output_network_path = self.tmp_path('my-network-config')
> +self.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 = self.output_network_path
> +
> +self.dump_content = 'yaml-format-network-config'
> +self.m_dump_config.return_value = self.dump_content
> +
> +def test_net_meta_with_disabled_network(self):
> +self.args.mode = 'disabled'
> +
> +with self.assertRaises(SystemExit) as cm:
> +with simple_mocked_open(content='') as m_open:
> +net_meta(self.args)
> +
> +self.assertEqual(self.expected_exit_code, cm.exception.code)
> +self.m_run_hook.assert_called_with(
> +self.args.target, 'network-config')
> +self.assertEqual(1, self.m_run_hook.call_count)
> +self.assertEqual(0, self.m_command_env.call_count)
> +self.assertEqual(0, self.m_command_config.call_count)
> +
> +self.assertEquals(self.args.mode, 'disabled')
> +self.assertEqual(0, self.m_os_environ.get.call_count)
> +self.assertEqual(0, self.m_dump_config.call_count)
> +self.assertFalse(os.path.isfile(self.output_network_path))
Makes sense, I have updated the code
> +self.assertEqual(0, m_open.call_count)
> +
> +def test_net_meta_with_config_network(self):
> +network_config = self.disabled_network_config
> +self.m_command_config.return_value = network_config
> +
> +expected_m_command_env_calls = 2
> +expected_m_command_config_calls = 2
> +m_file = MagicMock()
> +
> +with self.assertRaises(SystemExit) as cm:
> +with simple_mocked_open(content='') as m_open:
> +m_open.return_value = m_file
> +net_meta(self.args)
> +
> +self.assertEqual(self.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(self.output_network_path, 'w'), m_open.call_args_list[-1])
Yes, you right. Fixed
> +self.assertEqual(
> +call(self.dump_content),
> +m_file.__enter__.return_value.write.call_args_list[-1])
Fixed too
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list:
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing
I see, unittests around vmtest results. For those tests we can mark them a
skiptests; we don't expect to verify the network config of the target machine
when it's been disabled.
This looks really good. Just a couple more small points inline.
Diff comments:
> diff --git a/tests/unittests/test_commands_net_meta.py
> b/tests/unittests/test_commands_net_meta.py
> new file mode 100644
> index 000..2c64bb6
> --- /dev/null
> +++ b/tests/unittests/test_commands_net_meta.py
> @@ -0,0 +1,111 @@
> +# This file is part of curtin. See LICENSE file for copyright and license
> info.
> +
> +import os
> +
> +from mock import MagicMock, call
> +
> +from .helpers import CiTestCase, simple_mocked_open
> +
> +from curtin.commands.net_meta import net_meta
> +
> +
> +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()
> +
> +self.add_patch('curtin.util.run_hook_if_exists', 'm_run_hook')
> +self.add_patch('curtin.util.load_command_environment',
> 'm_command_env')
> +self.add_patch('curtin.config.load_command_config',
> 'm_command_config')
> +self.add_patch('curtin.config.dump_config', 'm_dump_config')
> +self.add_patch('os.environ', 'm_os_environ')
> +
> +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'
> +}
> +}
> +
> +self.output_network_path = self.tmp_path('my-network-config')
> +self.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 = self.output_network_path
> +
> +self.dump_content = 'yaml-format-network-config'
> +self.m_dump_config.return_value = self.dump_content
> +
> +def test_net_meta_with_disabled_network(self):
> +self.args.mode = 'disabled'
> +
> +with self.assertRaises(SystemExit) as cm:
> +with simple_mocked_open(content='') as m_open:
> +net_meta(self.args)
> +
> +self.assertEqual(self.expected_exit_code, cm.exception.code)
> +self.m_run_hook.assert_called_with(
> +self.args.target, 'network-config')
> +self.assertEqual(1, self.m_run_hook.call_count)
> +self.assertEqual(0, self.m_command_env.call_count)
> +self.assertEqual(0, self.m_command_config.call_count)
> +
> +self.assertEquals(self.args.mode, 'disabled')
> +self.assertEqual(0, self.m_os_environ.get.call_count)
> +self.assertEqual(0, self.m_dump_config.call_count)
> +self.assertFalse(os.path.isfile(self.output_network_path))
os.path.exists is sufficient I think; if we changed to writing content
somewhere else and symlinking it; this test would break
> +self.assertEqual(0, m_open.call_count)
> +
> +def test_net_meta_with_config_network(self):
> +network_config = self.disabled_network_config
> +self.m_command_config.return_value = network_config
> +
> +expected_m_command_env_calls = 2
> +expected_m_command_config_calls = 2
> +m_file = MagicMock()
> +
> +with self.assertRaises(SystemExit) as cm:
> +with simple_mocked_open(content='') as m_open:
> +m_open.return_value = m_file
> +net_meta(self.args)
> +
> +self.assertEqual(self.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(self.output_network_path, 'w'), m_open.call_args_list[-1])
We're only checking the last call to open, should we not assert a list of all
the calls to open?
> +self.assertEqual(
> +
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:688541726ab1137dcee06ae3999749f456ad0a68 https://jenkins.ubuntu.com/server/job/curtin-ci/113/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/113/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/113/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/113/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/113/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/113//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
rahaper, I have updated the code with your suggestions. I have also added in the comments the reason why I believe those tests need to be skipped. They are; test_etc_resolvconf test_ip_output With this solution, I have not used the property yet. But you think we can still benefit from it, just let me know and I will implement it. -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Diff comments:
> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..5f1bc23 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -99,6 +99,11 @@ def apply_net(target, network_state=None,
> network_config=None):
> else:
> ns = net.parse_net_config_data(netcfg.get('network', {}))
>
> +# We admit a None network state if we have a disabled config,
> since
> +# this config may not have a version associated with it
> +if ns is None and netcfg.get('config') == 'disabled':
> +return
I just figured out another problem related to the parse_net_config_data:
This function is being called by parse_net_config, which is called inside
vmtest/__init__.py +994. Since one of test have a version and the other not,
what happens is that the network state for one is defined as:
Network state: {'interfaces': {}, 'routes': [], 'dns': {'nameservers': [],
'search': []}}
And without the version key:
Network state: None
To solve that, i have mode the config check logic to the parse_net_config_data
function. Now both tests will behave exactly the same regarding the network
state and we also guarantee that no network_state will be generated by a
network disabled config.
> +
> if not passthrough:
> LOG.info('Rendering network configuration in target')
> net.render_network_state(target=target, network_state=ns)
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..ea835c1 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1152,7 +1152,10 @@ def detect_required_packages(cfg,
> osfamily=DISTROS.debian):
>
> # skip missing or invalid config items, configs may
> # only have network or storage, not always both
> -if not isinstance(cfg.get(cfg_type), dict):
> +# we are also skipping if the network config is marked as disabled
Done
> +cfg_type_value = cfg.get(cfg_type)
> +if (not isinstance(cfg_type_value, dict) or
> +cfg_type_value.get('config') == 'disabled'):
> continue
>
> cfg_version = cfg[cfg_type].get('version')
> diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
> index fdb909e..fe43e66 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -75,6 +75,9 @@ def net_meta(args):
>
> # if network-config hook exists in target,
> # we do not run the builtin
> +if args.mode == "disabled":
Done
> +sys.exit(0)
> +
> if util.run_hook_if_exists(args.target, 'network-config'):
> sys.exit(0)
>
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing
Thanks for updating. I've added some more inline comments.
>From your last comments:
> but I had to skip some tests that directly used the network state. However
> this is not the case for the disabled config test with a version key. Maybe
> we could always return a null network state for both cases and allow the
> tests to behave the same
Which unittests?
I wonder if instead of a None, we could add a method/property that indicates if
it's disabled; then in places using NetworkState, they could use logic like:
if NetworkState and not NetworkState.disabled:
foo
Diff comments:
> diff --git a/curtin/commands/apply_net.py b/curtin/commands/apply_net.py
> index ddc5056..5f1bc23 100644
> --- a/curtin/commands/apply_net.py
> +++ b/curtin/commands/apply_net.py
> @@ -99,6 +99,11 @@ def apply_net(target, network_state=None,
> network_config=None):
> else:
> ns = net.parse_net_config_data(netcfg.get('network', {}))
>
> +# We admit a None network state if we have a disabled config,
> since
emit
> +# this config may not have a version associated with it
> +if ns is None and netcfg.get('config') == 'disabled':
> +return
parse_net_config_data will return None for missing 'version' or missing
'config' in the config.
The primary disable config:
network: {config: disabled}
will result in a None from parse_net_config_data.
The more complete disable config, however won't (return None)
network: {config: disabled, version: 1}
Here we get a non-none NetworkState. To handle either of these
scenarios we need to use an or:
if ns is None or netconfig.get('config') == 'disabled':
return
> +
> if not passthrough:
> LOG.info('Rendering network configuration in target')
> net.render_network_state(target=target, network_state=ns)
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..ea835c1 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1152,7 +1152,10 @@ def detect_required_packages(cfg,
> osfamily=DISTROS.debian):
>
> # skip missing or invalid config items, configs may
> # only have network or storage, not always both
> -if not isinstance(cfg.get(cfg_type), dict):
> +# we are also skipping if the network config is marked as disabled
# skip missing, invalid or disabled config items. configs may
# only have network or storage, not always both
> +cfg_type_value = cfg.get(cfg_type)
> +if (not isinstance(cfg_type_value, dict) or
> +cfg_type_value.get('config') == 'disabled'):
> continue
>
> cfg_version = cfg[cfg_type].get('version')
> diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
> index fdb909e..fe43e66 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -75,6 +75,9 @@ def net_meta(args):
>
> # if network-config hook exists in target,
> # we do not run the builtin
> +if args.mode == "disabled":
This hunk needs to move down after line 44, the comment in 39/40 is for lines
44-45
We want to always attempt to run an in-target hook, if that's not present then
we can check for disabled config and exit as well.
> +sys.exit(0)
> +
> if util.run_hook_if_exists(args.target, 'network-config'):
> sys.exit(0)
>
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:072f0f151bf9b413879db512396977c00e25f15f https://jenkins.ubuntu.com/server/job/curtin-ci/108/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/108/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/108/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/108/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/108/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/108//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
raharper, I have addressed the proposed changes, Just one doubt about the
differences in testing between the disabled config with a vesion key and
without it
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)
Fair enough, but since it the dict have the following format:
'network': {
'config': 'disabled'
}
I we need to check if cfg.get(cfg_type).get('config') == disable
I will break this into variables to make the code easier to read
> 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..0454d3f 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -134,10 +137,11 @@ def net_meta(args):
>
> if not target:
> raise Exception(
> -"No target given for mode = '%s'. No where to write content:
> %s" %
> +"No target given for mode = %s. Nowhere to write content: %s" %
Fixed
> (args.mode, content))
>
> -LOG.debug("writing to file %s with network config: %s", target, content)
> +LOG.debug(
> +"writing to file %s with network config: %s", target, content)
This is a mistake. In my past solution this line had more than 79 characters,
so I made that change, but it is not necessary anymore
> if target == "-":
> sys.stdout.write(content)
> else:
> 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)
I have updated the code for this change, but I had to skip some tests that
directly used the network state. However this is not the case for the disabled
config test with a version key. Maybe we could always return a null network
state for both cases and allow the tests to behave the same
> 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..905dbfd 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':
> +dev_configs = set()
> # v1 has 'config' key and uses type: devtype elements
> -if 'config' in network_config:
> +elif 'config' in network_config:
Now I see your point. I have updated the code to use the ternary
> 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..34abd00 100644
> --- a/curtin/net/network_state.py
> +++ b/curtin/net/network_state.py
> @@ -21,7 +21,12 @@ def from_state_file(state_file):
> class NetworkState:
> def __init__(self, version=NETWORK_STATE_VERSION, config=None):
> self.version = version
> -self.config = config
> +
> +if config is None or config == 'disabled':
> +self.config = []
> +else:
> +self.config = config
> +
Fixed
> self.network_state = {
> 'interfaces': {},
> 'routes': [],
> @@ -73,6 +78,7 @@ class NetworkState:
> def parse_config(self):
> # rebuild network state
> for command in self.config:
> +# If network is disabled, there is no command to be executed
Removed
> handler = self.command_handlers.get(command['type'])
> handler(command)
>
> @@ -379,7 +385,7 @@ if __name__ == '__main__':
> from curtin import net
>
> def load_config(nc):
> -version = nc.get('version')
> +version = nc.get('version', 1)
Done
> 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 000..4f5d92b
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Thanks, a couple more comments inline.
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)
Let's just add 'config: disabled' as a skip, check before the version
if not isinstance(cfg.get(cfg_type), dict) or cfg.get(cfg_type) == 'disabled':
continue
> 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..0454d3f 100644
> --- a/curtin/commands/net_meta.py
> +++ b/curtin/commands/net_meta.py
> @@ -134,10 +137,11 @@ def net_meta(args):
>
> if not target:
> raise Exception(
> -"No target given for mode = '%s'. No where to write content:
> %s" %
> +"No target given for mode = %s. Nowhere to write content: %s" %
we can leave the ' ' around the mode string
> (args.mode, content))
>
> -LOG.debug("writing to file %s with network config: %s", target, content)
> +LOG.debug(
> +"writing to file %s with network config: %s", target, content)
Why change this?
> if target == "-":
> sys.stdout.write(content)
> else:
> 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)
Let's not set this; In net_apply.net_apply() if we call
net.parse_net_config_data() and we get a None for network_state, then we can
return. NetworkState() on a network: config: disabled should be None.
> 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..905dbfd 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':
> +dev_configs = set()
> # v1 has 'config' key and uses type: devtype elements
> -if 'config' in network_config:
> +elif 'config' in network_config:
It does seem strange to do a .get('config') and then an later check if 'config'
is in the dictionary; That's why I used the ternary. If we want to use
if/else clauses then:
if 'config' in network_config:
if network_config['config'] == 'disabled'
dev_configs = set()
else:
dev_configs = set()
else:
# handle v2 here
I think the nested if/else clause is more compact with the ternary.
> 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..34abd00 100644
> --- a/curtin/net/network_state.py
> +++ b/curtin/net/network_state.py
> @@ -21,7 +21,12 @@ def from_state_file(state_file):
> class NetworkState:
> def __init__(self, version=NETWORK_STATE_VERSION, config=None):
> self.version = version
> -self.config = config
> +
> +if config is None or config == 'disabled':
> +self.config = []
> +else:
> +self.config = config
> +
A ternary here works as well:
self.config = [] if config in [None, 'disabled'] else config
> self.network_state = {
> 'interfaces': {},
> 'routes': [],
> @@ -73,6 +78,7 @@ class NetworkState:
> def parse_config(self):
> # rebuild network state
> for command in self.config:
> +# If network is disabled, there is no command to be executed
I don't think we need this here, if anything move this up to the __init__
method where we assign self.config
> handler = self.command_handlers.get(command['type'])
> handler(command)
>
> @@ -379,7 +385,7 @@ if __name__ == '__main__':
> from curtin import net
>
> def load_config(nc):
> -version = nc.get('version')
> +version = nc.get('version', 1)
Let's not set version default to 1
>
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:b887db1007ed30af5521b20a8fbd95bb31a468c6 https://jenkins.ubuntu.com/server/job/curtin-ci/107/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/107/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/107/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/107/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/107/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/107//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
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 000..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)
> +
> +
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
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:
Yes, you are right. I missed that the env variable will be None and this part
of the code will never be called. I will undo this change
> +# 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')
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
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 =
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:1b3e7dc369889950421bde34ba519ac860aa2a86 https://jenkins.ubuntu.com/server/job/curtin-ci/103/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/103/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/103/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/103/ SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/103/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/103//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:d690b9e7263b80b63328f66ca7943b3d441407bc https://jenkins.ubuntu.com/server/job/curtin-ci/102/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/102/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/102/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/102/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/102/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/102//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
raharper I have addressed the changes you commented. I have created the
disabled option on the net_meta module and created a VMTest for this command.
I have also created two new VMTests for the {"config": "disabled"} possibility
as well. For the case of a network disabled config without a version, I had to
change some parts of the code that were getting the version key from the config
dict. But if you think I can do that differently, no problem.
Also, I have only added Focal tests, but if my solution is correct I can add
more instance types as well.
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:714e3b8a42af33a7b347c1206f04c708b5b73ee8 https://jenkins.ubuntu.com/server/job/curtin-ci/101/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/101/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/101/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/101/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/101/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/101//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
I moved the description to the commit message field, so hopefully we won't get flooded :) -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/100/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/100/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/100/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/100/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/100/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/100//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/99/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/99/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/99/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/99/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/99/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/99//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/98/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/98/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/98/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/98/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/98/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/98//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
- CI loop due to the missing commit message - Failures due to the new flake8 -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/97/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/97/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/97/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/97/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/97/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/97//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/96/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/96/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/96/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/96/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/96/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/96//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/95/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/95/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/95/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/95/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/95/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/95//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/93/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/93/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/93/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/93/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/93/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/93//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/89/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/89/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/89/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/89/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/89/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/89//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/86/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/86/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/86/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/86/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/86/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/86//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing continuous-integration No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself): https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785/+edit-commit-message https://jenkins.ubuntu.com/server/job/curtin-ci/83/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/83/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/83/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/83/ FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/83/ Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/curtin-ci/83//rebuild -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Thanks for the review Ryan :) I do recall that you commented about the net_meta options, but I misunderstood what you said for modifying the auto command to not write the network_config and not creating a new option. I will address all of the proposed changes and also thanks for the broader context you provided regarding subiquity. -- https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785 Your team curtin developers is subscribed to branch curtin:master. -- Mailing list: https://launchpad.net/~curtin-dev Post to : curtin-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~curtin-dev More help : https://help.launchpad.net/ListHelp
Re: [Curtin-dev] [Merge] ~lamoura/curtin:disable-networking-config into curtin:master
Review: Needs Fixing
Thanks for getting something started. The origin of this scenario comes from
the Subiquity installer which handles writing a network configuration into the
target outside of curtin and cloud-init. In this scenario, subiquity provides
*no* network config at all to curtin, and leaves net-meta in 'auto' mode by
default. The goal for subiquity is to have curtin not emit any config into the
target system.
For auto, the lack of network-config means curtin needs to generate a sensible
network config and it uses the environment in which it is running to make a
best-guess. We cannot use the lack of custom network config presented to
curtin as a desire to *NOT* generate a network config as this would break
existing use-cases where we do not provide network config to curtin but do
expect the target system to have networking.
I think for the subiquity scenario, we want to introduce a new mode to
net-meta, 'disabled' Which does what you describe above; namely, curtin will
not attempt to do _any_ configuration of networking in the target.
We will then request that subiquity specify a net-meta --mode=disabled in their
install command, and this still allows users which pass a "disabled" network
config to curtin to pass this through to the target. For verification of this
feature we need the following scenarios:
1) a vmtest where no 'network' config is supplied; good news is that all of the
existing storage tests do this today
2) a vmtest where we specify net-meta --mode=disabled; this is a new scenario
where we use the following config:
install:
network_commands:
disabled: ['curtin', 'net-meta', 'disabled']
3) a vmtest where we specify a network disabled config
network:
config: disabled
And possibly this variant
network;
config: disabled
version: 1
I'd like us to confirm that we can use the former config, as this is what
cloud-init accepts natively, we just need to confirm that curtin will pass
this through correctly.
Something simple like this would avoid most changes to curtin, and then
we just need unittests to verify this, and the 2 vmtest scenarios (2 and 3)
% git diff
diff --git a/curtin/commands/net_meta.py b/curtin/commands/net_meta.py
index fdb909e1..09983c0f 100644
--- a/curtin/commands/net_meta.py
+++ b/curtin/commands/net_meta.py
@@ -80,6 +80,11 @@ def net_meta(args):
state = util.load_command_environment()
cfg = config.load_command_config(args, state)
+
+if args.mode == 'disabled':
+LOG.debug("net-meta mode is '%s'. network config disabled", args.mode)
+sys.exit(0)
+
if cfg.get("network") is not None:
args.mode = "custom"
@@ -160,7 +165,7 @@ CMD_ARGUMENTS = (
'action': 'store', 'metavar': 'TARGET',
'default': os.environ.get('TARGET_MOUNT_POINT')}),
('mode', {'help': 'meta-mode to use',
- 'choices': ['dhcp', 'copy', 'auto', 'custom']})
+ 'choices': ['disabled', 'dhcp', 'copy', 'auto', 'custom']})
)
)
--
https://code.launchpad.net/~lamoura/curtin/+git/curtin/+merge/383785
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
