On Fri, Aug 26, 2016 at 3:59 AM, ChristianEhrhardt <
<email address hidden>> wrote:

> Hi Ryan,
> great work - a few minor comments and discussion points added inline.
>

Thanks for the review!


>
> Diff comments:
>
> > === modified file 'curtin/commands/apply_net.py'
> > --- curtin/commands/apply_net.py      2016-07-13 07:50:49 +0000
> > +++ curtin/commands/apply_net.py      2016-08-25 23:32:25 +0000
> > @@ -26,6 +26,54 @@
> >
> >  LOG = log.LOG
> >
> > +IFUPDOWN_IPV6_MTU_PRE_HOOK = """#!/bin/sh -x
>
> Maybe play it even safer with -e to better fail than echoing potentially
> weird stuff to the mtu file.
>
> Same for line 23
>

Yeah, actually should drop the -x;  I'll look at other ifupdown hooks and
see what they use.


>
> > +# injected by curtin installer
> > +
> > +[ "$IFACE" != "lo" ] || exit 0
> > +
> > +# Trigger only if MTU configured
> > +[ -n "$IF_MTU" ] || exit 0
> > +
> > +CUR_DEV_MTU=$(/bin/cat /sys/class/net/$IFACE/mtu ||:)
> > +CUR_IPV6_MTU=$(sysctl -n net.ipv6.conf.$IFACE.mtu ||:)
> > +[ -n "$CUR_DEV_MTU" ] && echo $CUR_DEV_MTU > /run/network/$IFACE_dev.mtu
> > +[ -n "$CUR_IPV6_MTU" ] && echo $CUR_IPV6_MTU >
> /run/network/$IFACE_ipv6.mtu
> > +exit 0
> > +"""
> > +
> > +IFUPDOWN_IPV6_MTU_POST_HOOK = """#!/bin/sh -x
> > +# injected by curtin installer
> > +
> > +[ "$IFACE" != "lo" ] || exit 0
> > +
> > +# Trigger only if MTU configured
> > +[ -n "$IF_MTU" ] || exit 0
> > +
> > +PRE_DEV_MTU=$(cat /run/network/$IFACE_dev.mtu)
> > +CUR_DEV_MTU=$(/bin/cat /sys/class/net/$IFACE/mtu)
> > +PRE_IPV6_MTU=$(cat /run/network/$IFACE_ipv6.mtu)
>
> I'm not sure if ${var} references are compliant, but here it would really
> enhance readability and maintainability. e.g. at ${IFACE}_ipv6 which would
> make is clearer where var and string meet.
>

It appears that ${VAR} works; most of the hooks don't use that; not
entirely sure why.  I can update.


>
> > +CUR_IPV6_MTU=$(sysctl -n net.ipv6.conf.$IFACE.mtu)
> > +
> > +if [ "$ADDRFAM" = "inet6" ]; then
> > +  # We need to check the underlying interface MTU and
> > +  # raise it if the IPV6 mtu is larger
> > +  if [ $CUR_DEV_MTU -lt $IF_MTU ]; then
> > +      /bin/ip link set $IFACE mtu $IF_MTU
> > +  fi
> > +  /sbin/sysctl -q -e -w net.ipv6.conf.$IFACE.mtu=$IF_MTU
>
> I'm not sure if it would be wiser to put all the binary paths into an
> initial INIT phase.
> Probably with a which or anything like it, just for the case that ip,
> sysctl or others ever move to a different place.
> A which would make it fully dynamic, a central definition easier to
> maintain at one place.
> Open for discussion thou since it can have drawbacks like resolving to the
> wrong one in the worst case.
>

All of the existing tooks (and ifupdown itself) call via full path.


>
> > +
> > +elif [ "$ADDRFAM" = "inet" ]; then
> > +  # handle the clobber case where inet mtu changes v6 mtu
> > +  # ifupdown will already have set dev mtu, so lower mtu
> > +  # if needed.  If v6 mtu was larger, it get's clamped down
> > +  # to the dev MTU value.
> > +  if [ $PRE_IPV6_MTU -lt $CUR_IPV6_MTU ]; then
> > +    /sbin/sysctl -q -e -w net.ipv6.conf.$IFACE.mtu=$PRE_IPV6_MTU
> > +  fi
> > +fi
> > +exit 0
> > +"""
> > +
> >
> >  def apply_net(target, network_state=None, network_config=None):
> >      if network_state is None and network_config is None:
> > @@ -45,6 +93,111 @@
> >
> >      net.render_network_state(target=target, network_state=ns)
> >
> > +    _maybe_remove_legacy_eth0(target)
> > +    LOG.info('Attempting to remove ipv6 privacy extensions')
> > +    _disable_ipv6_privacy_extensions(target)
> > +    _patch_ifupdown_ipv6_mtu_hook(target)
> > +
> > +
> > +def _patch_ifupdown_ipv6_mtu_hook(target,
> > +                                  prehookfn="etc/network/if-pre-
> up.d/mtuipv6",
> > +                                  posthookfn="etc/network/if-up.
> d/mtuipv6"):
> > +
> > +    contents = {
> > +        'prehook': IFUPDOWN_IPV6_MTU_PRE_HOOK,
> > +        'posthook': IFUPDOWN_IPV6_MTU_POST_HOOK,
> > +    }
> > +
> > +    hookfn = {
> > +        'prehook': prehookfn,
> > +        'posthook': posthookfn,
> > +    }
> > +
> > +    for hook in ['prehook', 'posthook']:
> > +        fn = hookfn[hook]
> > +        cfg = os.path.sep.join([target, fn])
> > +        LOG.info('Injecting fix for ipv6 mtu settings: %s', cfg)
> > +        util.write_file(cfg, contents[hook], mode=0o755)
> > +
> > +
> > +def _disable_ipv6_privacy_extensions(target,
> > +                                     path="etc/sysctl.d/10-ipv6-
> privacy.conf"):
> > +
> > +    """Ubuntu server image sets a preference to use IPv6 privacy
> extensions
> > +       by default; this races with the cloud-image desire to disable
> them.
> > +       Resolve this by allowing the cloud-image setting to win. """
> > +
> > +    if path.startswith("/"):
> > +        raise ValueError("path parameter must not start with '/': %s",
> path)
> > +
> > +    cfg = os.path.normpath(os.path.sep.join([target, path]))
> > +    if not os.path.exists(cfg):
> > +        LOG.warn('Failed to find legacy conf file %s', cfg)
> > +        return
> > +
> > +    bmsg = "Disabling IPv6 privacy extensions config may not apply."
> > +    try:
> > +        contents = util.load_file(cfg)
> > +        known_contents = ["net.ipv6.conf.all.use_tempaddr = 2",
> > +                          "net.ipv6.conf.default.use_tempaddr = 2"]
> > +        lines = [f.strip() for f in contents.splitlines()
> > +                 if not f.startswith("#")]
> > +        if lines == known_contents:
> > +            LOG.info('deleting file: %s', cfg)
> > +            util.del_file(cfg)
> > +            msg = "removed %s with known contents" % cfg
> > +            curtin_contents = '\n'.join(
> > +                ["# IPv6 Privacy Extensions (RFC 4941)",
> > +                 "# Disabled by curtin",
> > +                 "# net.ipv6.conf.all.use_tempaddr = 2",
> > +                 "# net.ipv6.conf.default.use_tempaddr = 2"])
> > +            util.write_file(cfg, curtin_contents)
> > +        else:
> > +            LOG.info('skipping, content didnt match')
>
> smoser will tell you that this is a lot for info level logging :-)
>

It's a one line output;  I do think it's useful but I'm happy to mark it
debug.


>
> > +            LOG.info(lines)
> > +            LOG.info(known_contents)
> > +            msg = (bmsg + " '%s' exists with user configured content."
> % cfg)
> > +    except:
> > +        msg = bmsg + " %s exists, but could not be read." % cfg
> > +        LOG.exception(msg)
> > +        return
> > +
> > +
> > +def _maybe_remove_legacy_eth0(target,
>
> I dislike "maybe" in a method if it is no using /dev/random or such a
> thing :-)
> Really couldn't we just call it "_remove"_legacy_eth0" - it is still ok to
> not fail if there never was one IMHO.
>

It's pulled from the cloud-init version of the same function.


>
> > +                              path="etc/network/interfaces.
> d/eth0.cfg"):
> > +    """Ubuntu cloud images previously included a 'eth0.cfg' that had
> > +       hard coded content.  That file would interfere with the rendered
> > +       configuration if it was present.
> > +
> > +       if the file does not exist do nothing.
> > +       If the file exists:
> > +         - with known content, remove it and warn
> > +         - with unknown content, leave it and warn
> > +    """
> > +
> > +    cfg = os.path.normpath(os.path.sep.join([target, path]))
> > +    if not os.path.exists(cfg):
> > +        LOG.warn('Failed to find legacy conf file %s', cfg)
> > +        return
> > +
> > +    bmsg = "Dynamic networking config may not apply."
> > +    try:
> > +        contents = util.load_file(cfg)
> > +        known_contents = ["auto eth0", "iface eth0 inet dhcp"]
> > +        lines = [f.strip() for f in contents.splitlines()
> > +                 if not f.startswith("#")]
> > +        if lines == known_contents:
> > +            util.del_file(cfg)
> > +            msg = "removed %s with known contents" % cfg
> > +        else:
> > +            msg = (bmsg + " '%s' exists with user configured content."
> % cfg)
> > +    except:
> > +        msg = bmsg + " %s exists, but could not be read." % cfg
> > +        LOG.exception(msg)
> > +        return
> > +
> > +    LOG.warn(msg)
>
> Even warn for the "good case" where you print "removed %s with known
> contents" % cfg" ?
> I think in that case info or even debug would do.
> While OTOH the user content and the could not be read qualify as a warn.
>

Note this is code-motion, not newly included functions.  I don't expect to
change these.


>
> > +
> >
> >  def apply_net_main(args):
> >      #  curtin apply_net [--net-state=/config/netstate.yml] [--target=/]
> >
> > === modified file 'curtin/net/__init__.py'
> > --- curtin/net/__init__.py    2016-06-16 17:43:17 +0000
> > +++ curtin/net/__init__.py    2016-08-25 23:32:25 +0000
> > @@ -397,6 +423,37 @@
> >              "iface {fullname} {inet} {mode}\n").format(**subst)
> >
> >
> > +def subnet_is_ipv6(subnet):
> > +    # '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 subnet_is_ipv4(subnet):
> > +    return not subnet_is_ipv6(subnet)
> > +
> > +
> > +def list_ipv4_subnets(subnets):
> > +    return [sn for sn in subnets if subnet_is_ipv4(sn)]
> > +
> > +
> > +def iface_add_postup(interface, alias_idx):
> > +    content = ""
> > +    tmpl = "    post-up ifup %s:%s\n"
>
> at render_route you made the indent an argument which would probably be
> good here as well.
> I'd even think of the indent not beeing "    " or "        " but if it is
> numeric add 4xnum spaces - would that be reasonable?
>

it could take indent, however;  it's an unused function now, so I'll drop
it.  We don't need postup any more. \o/


>
> > +    subnets = interface.get('subnets', [])
> > +    for idx, subnet in [(idx, sn) for (idx, sn) in enumerate(subnets)
> > +                        if idx != 0 and subnet_is_ipv4(sn)]:
> > +        if idx == alias_idx:
> > +            content += tmpl % (interface['name'], idx)
> > +            break
> > +
> > +    return content
> > +
> > +
> >  def render_interfaces(network_state):
> >      ''' Given state, emit etc/network/interfaces content '''
> >
> >
> > === added file 'tests/vmtests/test_network_bonding.py'
> > --- tests/vmtests/test_network_bonding.py     1970-01-01 00:00:00 +0000
> > +++ tests/vmtests/test_network_bonding.py     2016-08-25 23:32:25 +0000
> > @@ -0,0 +1,63 @@
> > +from . import logger
> > +from .releases import base_vm_classes as relbase
> > +from .test_network import TestNetworkBaseTestsAbs
> > +
> > +import textwrap
> > +
> > +
> > +class TestNetworkBondingAbs(TestNetworkBaseTestsAbs):
> > +    conf_file = "examples/tests/bonding_network.yaml"
> > +    collect_scripts = TestNetworkBaseTestsAbs.collect_scripts + [
> > +        textwrap.dedent("""
> > +        cd OUTPUT_COLLECT_D
> > +        dpkg-query -W -f '${Status}' ifenslave > ifenslave_installed
> > +        """)]
> > +
> > +    def test_output_files_exist_ifenslave(self):
> > +        self.output_files_exist(["ifenslave_installed"])
> > +
> > +    def test_ifenslave_installed(self):
> > +        status = self.load_collect_file("ifenslave_installed")
> > +        logger.debug('ifenslave installed: {}'.format(status))
> > +        self.assertEqual('install ok installed', status)
> > +
> > +
> > +class PreciseHWETTestBonding(relbase.precise_hwe_t,
> TestNetworkBondingAbs):
> > +    __test__ = True
> > +    # package names on precise are different, need to check on
> ifenslave-2.6
> > +    collect_scripts = TestNetworkBondingAbs.collect_scripts + [
> > +        textwrap.dedent("""
> > +        cd OUTPUT_COLLECT_D
> > +        dpkg-query -W -f '${Status}' ifenslave-2.6 > ifenslave_installed
> > +        """)]
> > +
> > +
> > +class TrustyTestBonding(relbase.trusty, TestNetworkBondingAbs):
> > +    __test__ = False
> > +
> > +
> > +class TrustyHWEUTestBonding(relbase.trusty_hwe_u, TrustyTestBonding):
> > +    __test__ = True
> > +
> > +
> > +class TrustyHWEVTestBonding(relbase.trusty_hwe_v, TrustyTestBonding):
> > +    # Working, but off by default to safe test suite runtime
> > +    # oldest/newest HWE-* covered above/below
> > +    __test__ = False
> > +
> > +
> > +class TrustyHWEWTestBonding(relbase.trusty_hwe_w, TrustyTestBonding):
> > +    __test__ = True
> > +
> > +
> > +class WilyTestBonding(relbase.wily, TestNetworkBondingAbs):
>
> Do we need to move it since it is EOL anyway - just drop ?
>

We merged another branch that leaves them (incase we need to reproduce
something)
but disables it.



>
> > +    # EOL - 2016-07-28
> > +    __test__ = False
> > +
> > +
> > +class XenialTestBonding(relbase.xenial, TestNetworkBondingAbs):
> > +    __test__ = True
> > +
> > +
> > +class YakketyTestBonding(relbase.yakkety, TestNetworkBondingAbs):
> > +    __test__ = True
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.more-ipv6/+merge/304027
> You are the owner of lp:~raharper/curtin/trunk.more-ipv6.
>