Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:1c1fc3a8841a85b86b51931c9f8058832ad3b00a https://jenkins.ubuntu.com/server/job/cloud-init-ci/145/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/145/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/145/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/145/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/145/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/145/console Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/145/rebuild -- https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/320291 Your team cloud init development team is requested to review the proposed merge of ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:9bcc80e67b77dbce265de68c11f5f90916988ea0 https://jenkins.ubuntu.com/server/job/cloud-init-ci/144/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/144/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/144/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/144/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/144/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/144/console Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/144/rebuild -- https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/320291 Your team cloud init development team is requested to review the proposed merge of ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master
Diff comments:
> diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
> index 803ac74..22ae998 100755
> --- a/cloudinit/distros/__init__.py
> +++ b/cloudinit/distros/__init__.py
> @@ -73,7 +73,7 @@ class Distro(object):
>
> def _supported_write_network_config(self, network_config):
> priority = util.get_cfg_by_path(
> -self._cfg, ('network', 'renderers'), None)
> +self._cfg, ('system_info', 'network', 'renderers'), None)
I needed this change to fix up the unittests in
tests/unittests/test_distro/test_netconfig.py
I've not tested outside of unittest; If you remove that change and run
unittest on test_netconfig you can see.
It manifests itself as getting policy of None and then attempts to render eni
for everything (so netplan and sysconfig renders write out udev files etc).
>
> name, render_cls = renderers.select(priority=priority)
> LOG.debug("Selected renderer '%s' from priority list: %s",
> diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
> index 1101f02..26267c3 100644
> --- a/cloudinit/distros/debian.py
> +++ b/cloudinit/distros/debian.py
> @@ -75,7 +80,8 @@ class Distro(distros.Distro):
> self.package_command('install', pkgs=pkglist)
>
> def _write_network(self, settings):
> -util.write_file(NETWORK_CONF_FN, settings)
> +# this is always going to be legacy based
ACK
> +util.write_file(self.network_conf_fn["eni"], settings)
> return ['all']
>
> def _write_network_config(self, netconfig):
> diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
> index 90b2835..ba84059 100644
> --- a/cloudinit/net/network_state.py
> +++ b/cloudinit/net/network_state.py
> @@ -103,14 +114,21 @@ class CommandHandlerMeta(type):
>
> class NetworkState(object):
>
> -def __init__(self, network_state, version=NETWORK_STATE_VERSION):
> +def __init__(self, network_state, version=NETWORK_STATE_VERSION,
> + config=None):
Ill revisit; I'm somewhat frustrated with the NSI vs NS objects; it maybe
unneeded changes when doing the initial plumbing of getting v2 to pass through
config to the renderers which only get (NetworkState) objects.
> self._network_state = copy.deepcopy(network_state)
> self._version = version
> +self._config = copy.deepcopy(config)
> +self.use_ipv6 = network_state.get('use_ipv6', False)
>
> @property
> def version(self):
> return self._version
>
> +@property
> +def config(self):
> +return self._config
> +
> def iter_routes(self, filter_func=None):
> for route in self._network_state.get('routes', []):
> if filter_func is not None:
> @@ -407,6 +471,239 @@ class NetworkStateInterpreter(object):
> }
> routes.append(route)
>
> +# V2 handlers
ACK; it should be a simple change, we basically had 'handle_bond' (v1) and
'handle_bonds' (v2); v2 being plural; That's not easy to see.
> +def handle_bonds(self, command):
> +'''
> +v2_command = {
> + bond0: {
> +'interfaces': ['interface0', 'interface1'],
> +'miimon': 100,
> +'mode': '802.3ad',
> +'xmit_hash_policy': 'layer3+4'},
> + bond1: {
> +'bond-slaves': ['interface2', 'interface7'],
> +'mode': 1
> + }
> +}
> +
> +v1_command = {
> +'type': 'bond'
> +'name': 'bond0',
> +'bond_interfaces': [interface0, interface1],
> +'params': {
> +'bond-mode': '802.3ad',
> +'bond_miimon: 100,
> +'bond_xmit_hash_policy': 'layer3+4',
> +}
> +}
> +
> +'''
> +self._handle_bond_bridge(command, cmd_type='bond')
> +
> +def handle_bridges(self, command):
> +
> +'''
> +v2_command = {
> + br0: {
> +'interfaces': ['interface0', 'interface1'],
> +'fd': 0,
> +'stp': 'off',
> +'maxwait': 0,
> + }
> +}
> +
> +v1_command = {
> +'type': 'bridge'
> +'name': 'br0',
> +'bridge_interfaces': [interface0, interface1],
> +'params': {
> +'bridge_stp': 'off',
> +'bridge_fd: 0,
> +'bridge_maxwait': 0
> +}
> +}
> +
> +'''
> +self._handle_bond_bridge(command, cmd_type='bridge')
> +
> +def handle_ethernets(self, command):
> +'''
> +ethernets:
> + eno1:
> +match:
> + macaddress: 00:11:22:33:44:55
> +wakeonlan: true
> +dhcp4: true
> +dhcp6: false
> +addresses:
> + - 192.168.14.2/24
> + - 2001:1::1/64
> +gateway4: 192.168.14.1
> +
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master
i have some small comments.
don't worry about fixing the 'handle_' comment, but i do worry about the
confusion.
Diff comments:
> diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
> index 803ac74..22ae998 100755
> --- a/cloudinit/distros/__init__.py
> +++ b/cloudinit/distros/__init__.py
> @@ -73,7 +73,7 @@ class Distro(object):
>
> def _supported_write_network_config(self, network_config):
> priority = util.get_cfg_by_path(
> -self._cfg, ('network', 'renderers'), None)
> +self._cfg, ('system_info', 'network', 'renderers'), None)
I'm pretty sure this is wrong.
distro gets as its config the system_info. it does not get access to the full
thing. see usage of _cfg via 'get_option' and '_get_arch_package_mirror_info'
for my reasoning. I did not test though.
>
> name, render_cls = renderers.select(priority=priority)
> LOG.debug("Selected renderer '%s' from priority list: %s",
> diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
> index 1101f02..26267c3 100644
> --- a/cloudinit/distros/debian.py
> +++ b/cloudinit/distros/debian.py
> @@ -75,7 +80,8 @@ class Distro(distros.Distro):
> self.package_command('install', pkgs=pkglist)
>
> def _write_network(self, settings):
> -util.write_file(NETWORK_CONF_FN, settings)
> +# this is always going to be legacy based
# _write_network is legacy, it will always write eni
that sound reasonable?
> +util.write_file(self.network_conf_fn["eni"], settings)
> return ['all']
>
> def _write_network_config(self, netconfig):
> diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
> index 90b2835..ba84059 100644
> --- a/cloudinit/net/network_state.py
> +++ b/cloudinit/net/network_state.py
> @@ -103,14 +114,21 @@ class CommandHandlerMeta(type):
>
> class NetworkState(object):
>
> -def __init__(self, network_state, version=NETWORK_STATE_VERSION):
> +def __init__(self, network_state, version=NETWORK_STATE_VERSION,
> + config=None):
I dont understand why NetworkState is now getting a 'config', as it doesnt look
like it gets used for anything.
> self._network_state = copy.deepcopy(network_state)
> self._version = version
> +self._config = copy.deepcopy(config)
> +self.use_ipv6 = network_state.get('use_ipv6', False)
>
> @property
> def version(self):
> return self._version
>
> +@property
> +def config(self):
> +return self._config
> +
> def iter_routes(self, filter_func=None):
> for route in self._network_state.get('routes', []):
> if filter_func is not None:
> @@ -407,6 +471,239 @@ class NetworkStateInterpreter(object):
> }
> routes.append(route)
>
> +# V2 handlers
can we name these handle_v2_bonds, handle_v2_ethernets ?
just looking at this diff, I thought "Why are we adding support now for 'bonds'
and 'vlans'?" I'm sure I'll be confused by that again.
i realize that this is a bit involved due to the CommadnHandlerMeta path.. but
this is going to get confusing.
> +def handle_bonds(self, command):
> +'''
> +v2_command = {
> + bond0: {
> +'interfaces': ['interface0', 'interface1'],
> +'miimon': 100,
> +'mode': '802.3ad',
> +'xmit_hash_policy': 'layer3+4'},
> + bond1: {
> +'bond-slaves': ['interface2', 'interface7'],
> +'mode': 1
> + }
> +}
> +
> +v1_command = {
> +'type': 'bond'
> +'name': 'bond0',
> +'bond_interfaces': [interface0, interface1],
> +'params': {
> +'bond-mode': '802.3ad',
> +'bond_miimon: 100,
> +'bond_xmit_hash_policy': 'layer3+4',
> +}
> +}
> +
> +'''
> +self._handle_bond_bridge(command, cmd_type='bond')
> +
> +def handle_bridges(self, command):
> +
> +'''
> +v2_command = {
> + br0: {
> +'interfaces': ['interface0', 'interface1'],
> +'fd': 0,
> +'stp': 'off',
> +'maxwait': 0,
> + }
> +}
> +
> +v1_command = {
> +'type': 'bridge'
> +'name': 'br0',
> +'bridge_interfaces': [interface0, interface1],
> +'params': {
> +'bridge_stp': 'off',
> +'bridge_fd: 0,
> +'bridge_maxwait': 0
> +}
> +}
> +
> +'''
> +self._handle_bond_bridge(command, cmd_type='bridge')
> +
> +def handle_ethernets(self, command):
> +'''
> +ethernets:
> + eno1:
> +match:
> + macaddress: 00:11:22:33:44:55
> +wakeonlan: true
> +dhcp4: true
> +dhcp6: false
> +addresses:
>
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:c43f407e2ac41e3e474dbfd023fc6325919819a9 https://jenkins.ubuntu.com/server/job/cloud-init-ci/139/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/139/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/139/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/139/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/139/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/139/console Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/139/rebuild -- https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/320291 Your team cloud init development team is requested to review the proposed merge of ~raharper/cloud-init:rebased-netconfig-v2-passthrough into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
