cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

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:
> +              - 192.168.14.2/24
> +              - 2001:1::1/64
> +            gateway4: 192.168.14.1
> +            gateway6: 2001:1::2
> +            nameservers:
> +              search: [foo.local, bar.local]
> +              addresses: [8.8.8.8, 8.8.4.4]
> +          lom:
> +            match:
> +              driver: ixgbe
> +            set-name: lom1
> +            dhcp6: true
> +          switchports:
> +            match:
> +              name: enp2*
> +            mtu: 1280
> +
> +        command = {
> +            'type': 'physical',
> +            'mac_address': 'c0:d6:9f:2c:e8:80',
> +            'name': 'eth0',
> +            'subnets': [
> +                {'type': 'dhcp4'}
> +             ]
> +        }
> +        '''
> +        for eth, cfg in command.items():
> +            phy_cmd = {
> +                'type': 'physical',
> +                'name': cfg.get('set-name', eth),
> +            }
> +            mac_address = cfg.get('match', {}).get('macaddress', None)
> +            if not mac_address:
> +                LOG.warning('NetworkState Version2: missing macaddress')
> +
> +            for key in ['mtu', 'match', 'wakeonlan']:
> +                if key in cfg:
> +                    phy_cmd.update({key: cfg.get(key)})
> +
> +            subnets = self._v2_to_v1_ipcfg(cfg)
> +            if len(subnets) > 0:
> +                phy_cmd.update({'subnets': subnets})
> +
> +            LOG.debug('v2(ethernets) -> v1(physical):\n%s', phy_cmd)
> +            self.handle_physical(phy_cmd)
> +
> +    def handle_vlans(self, command):
> +        '''
> +        v2_vlans = {
> +            'eth0.123': {
> +                'id': 123,
> +                'link': 'eth0',
> +                'dhcp4': True,
> +            }
> +        }
> +
> +        v1_command = {
> +            'type': 'vlan',
> +            'name': 'eth0.123',
> +            'vlan_link': 'eth0',
> +            'vlan_id': 123,
> +            'subnets': [{'type': 'dhcp4'}],
> +        }
> +        '''
> +        for vlan, cfg in command.items():
> +            vlan_cmd = {
> +                'type': 'vlan',
> +                'name': vlan,
> +                'vlan_id': cfg.get('id'),
> +                'vlan_link': cfg.get('link'),
> +            }
> +            subnets = self._v2_to_v1_ipcfg(cfg)
> +            if len(subnets) > 0:
> +                vlan_cmd.update({'subnets': subnets})
> +            LOG.debug('v2(vlans) -> v1(vlan):\n%s', vlan_cmd)
> +            self.handle_vlan(vlan_cmd)
> +
> +    def handle_wifis(self, command):
> +        raise NotImplemented('NetworkState V2: Skipping wifi configuration')
> +
> +    def _v2_common(self, cfg):
> +        LOG.debug('v2_common: handling config:\n%s', cfg)
> +        if 'nameservers' in cfg:
> +            search = cfg.get('nameservers').get('search', [])
> +            dns = cfg.get('nameservers').get('addresses', [])
> +            name_cmd = {'type': 'nameserver'}
> +            if len(search) > 0:
> +                name_cmd.update({'search': search})
> +            if len(dns) > 0:
> +                name_cmd.update({'addresses': dns})
> +            LOG.debug('v2(nameserver) -> v1(nameserver):\n%s', name_cmd)
> +            self.handle_nameserver(name_cmd)
> +
> +    def _handle_bond_bridge(self, command, cmd_type=None):
> +        """Common handler for bond and bridge types"""
> +        for item_name, item_cfg in command.items():
> +            item_params = dict((key, value) for (key, value) in
> +                               item_cfg.items() if key not in
> +                               NETWORK_V2_KEY_FILTER)
> +            v1_cmd = {
> +                'type': cmd_type,
> +                'name': item_name,
> +                cmd_type + '_interfaces': item_cfg.get('interfaces'),
> +                'params': item_params,
> +            }
> +            subnets = self._v2_to_v1_ipcfg(item_cfg)
> +            if len(subnets) > 0:
> +                v1_cmd.update({'subnets': subnets})
> +
> +            LOG.debug('v2(%ss) -> v1(%s):\n%s', cmd_type, cmd_type, v1_cmd)
> +            self.handle_bridge(v1_cmd)
> +
> +    def _v2_to_v1_ipcfg(self, cfg):
> +        """Common ipconfig extraction from v2 to v1 subnets array."""
> +
> +        subnets = []
> +        if 'dhcp4' in cfg:
> +            subnets.append({'type': 'dhcp4'})
> +        if 'dhcp6' in cfg:
> +            self.use_ipv6 = True
> +            subnets.append({'type': 'dhcp6'})
> +
> +        gateway4 = None
> +        gateway6 = None
> +        for address in cfg.get('addresses', []):
> +            subnet = {
> +                'type': 'static',
> +                'address': address,
> +            }
> +
> +            routes = []
> +            for route in cfg.get('routes', []):
> +                route_addr = route.get('to')
> +                if "/" in route_addr:
> +                    route_addr, route_cidr = route_addr.split("/")
> +                route_netmask = cidr2mask(route_cidr)
> +                subnet_route = {
> +                    'address': route_addr,
> +                    'netmask': route_netmask,
> +                    'gateway': route.get('via')
> +                }
> +                routes.append(subnet_route)
> +            if len(routes) > 0:
> +                subnet.update({'routes': routes})
> +
> +            if ":" in address:
> +                if 'gateway6' in cfg and gateway6 is None:
> +                    gateway6 = cfg.get('gateway6')
> +                    subnet.update({'gateway': gateway6})
> +            else:
> +                if 'gateway4' in cfg and gateway4 is None:
> +                    gateway4 = cfg.get('gateway4')
> +                    subnet.update({'gateway': gateway4})
> +
> +            subnets.append(subnet)
> +        return subnets
> +
> +
> +def subnet_is_ipv6(subnet):
> +    """ Common helper for checking network_state subnets for ipv6"""
> +    # 'static6' or 'dhcp6'
> +    if subnet['type'].endswith('6'):
> +        # This is a request for DHCPv6.
> +        return True
> +    elif subnet['type'] == 'static' and ":" in subnet['address']:
> +        return True
> +    return False
> +
>  
>  def cidr2mask(cidr):
>      mask = [0, 0, 0, 0]
> diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
> index 117b515..23ac2e3 100644
> --- a/cloudinit/net/sysconfig.py
> +++ b/cloudinit/net/sysconfig.py
> @@ -390,19 +391,32 @@ class Renderer(renderer.Renderer):
>          return contents
>  
>      def render_network_state(self, network_state, target=None):
> +        file_mode = 0o644
>          base_sysconf_dir = util.target_path(target, self.sysconf_dir)
>          for path, data in self._render_sysconfig(base_sysconf_dir,
>                                                   network_state).items():
> -            util.write_file(path, data)
> +            util.write_file(path, data, file_mode)
>          if self.dns_path:
>              dns_path = util.target_path(target, self.dns_path)
>              resolv_content = self._render_dns(network_state,
>                                                existing_dns_path=dns_path)
> -            util.write_file(dns_path, resolv_content)
> +            util.write_file(dns_path, resolv_content, file_mode)
>          if self.netrules_path:
>              netrules_content = self._render_persistent_net(network_state)
>              netrules_path = util.target_path(target, self.netrules_path)
> -            util.write_file(netrules_path, netrules_content)
> +            util.write_file(netrules_path, netrules_content, file_mode)
> +
> +        # always write /etc/sysconfig/network configuration
> +        sysconfig_path = util.target_path(target, "etc/sysconfig/network")
> +        netcfg = [
> +            ('# Created by cloud-init on instance boot automatically, '

i think this should be 
netcg = _make_header() + ["NETWORKING="yes"]

> +             'do not edit.'),
> +            'NETWORKING=yes',
> +        ]
> +        if network_state.use_ipv6:
> +            netcfg.append('NETWORKING_IPV6=yes')
> +            netcfg.append('IPV6_AUTOCONF=no')
> +        util.write_file(sysconfig_path, "\n".join(netcfg) + "\n", file_mode)
>  
>  
>  def available(target=None):


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