Merge lp:~raharper/curtin/trunk.net-manual into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

net: clean up e/n/i and udev rules for manually configured interfaces

Don't emit auto $iface for manually configured interfaces, except when
enslaved to bridges or bonds.
Don't emit a udev rules for nics without a mac_address
Don't write a udev rules file unless we have at least one interface to map
Add unittest to validate bonding network config which uses iface manual but in the service of a bond

[Scott Moser (smoser)]

It seems maybe arguable that you'd *want* to blank the udev rules if there were no interfaces to map.
ie, if you're fully declaritive we own that file.

thoughts?

[Ryan Harper (raharper)]

I'm not sure. My initial concern was if the file exists but was empty,
that systemd would not attempt to do persistent naming
as it expects that udev rules will handle it; then we don't do it, and the
system now has *no* persistent naming.

One could argue that's expected since user did not provide any matching
rules in the network.yaml.

If systemd applies persistent naming despite the presence of an empty udev
rules file, then that might make
the eni useless (iface eth0 does not exist).

However, if systemd doesn't do any naming because the file exists, then we
at least have a valid eni file despite
it not being persistent w.r.t which device shows up under the kname eth0.

I think I've convinced myself that we should at least ensure that systemd
doesn't do persistent naming. We could
emit a header in the file in case systemd checks for non-empty rules.

Shall I push an update or will you take in merge?

On Fri, Mar 11, 2016 at 1:17 PM, Scott Moser <email address hidden> wrote:

> It seems maybe arguable that you'd *want* to blank the udev rules if there
> were no interfaces to map.
> ie, if you're fully declaritive we own that file.
>
> thoughts?
> --
> https://code.launchpad.net/~raharper/curtin/trunk.net-manual/+merge/288799
> You are the owner of lp:~raharper/curtin/trunk.net-manual.
>

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:363
https://server-team-jenkins.canonical.com/job/curtin-ci/186/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/183/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/186/rebuild

review: Needs Fixing (continuous-integration)

=== modified file 'curtin/commands/apply_net.py'
--- curtin/commands/apply_net.py	2015-10-02 18:30:42 +0000
+++ curtin/commands/apply_net.py	2016-03-11 16:05:00 +0000
@@ -18,11 +18,15 @@
 import os
 import sys
 
+from .. import log
 import curtin.net as net
 import curtin.util as util
 from . import populate_one_subcmd
 
 
+LOG = log.LOG
+
+
 def apply_net(target, network_state=None, network_config=None):
     if network_state is None and network_config is None:
         msg = "Must provide at least config or state"
@@ -47,6 +51,8 @@
     #                   [--net-config=/config/maas_net.yml]
     state = util.load_command_environment()
 
+    log.basicConfig(stream=args.log_file, verbosity=1)
+
     if args.target is not None:
         state['target'] = args.target
 
@@ -65,10 +71,15 @@
         sys.stderr.write("Must provide at least config or state\n")
         sys.exit(2)
 
-    apply_net(target=state['target'],
-              network_state=state['network_state'],
-              network_config=state['network_config'])
+    LOG.info('Applying network configuration')
+    try:
+        apply_net(target=state['target'],
+                  network_state=state['network_state'],
+                  network_config=state['network_config'])
+    except Exception:
+        LOG.exception('failed to apply network config')
 
+    LOG.info('Applied network configuration successfully')
     sys.exit(0)
 
 
 
=== modified file 'curtin/net/__init__.py'
--- curtin/net/__init__.py	2016-02-24 17:49:17 +0000
+++ curtin/net/__init__.py	2016-03-11 16:05:00 +0000
@@ -286,11 +286,12 @@
     content = ""
     interfaces = network_state.get('interfaces')
     for iface in interfaces.values():
-        # for physical interfaces write out a persist net udev rule
-        if iface['type'] == 'physical' and \
-           'name' in iface and 'mac_address' in iface:
-            content += generate_udev_rule(iface['name'],
-                                          iface['mac_address'])
+        if iface['type'] == 'physical':
+            ifname = iface.get('name', None)
+            mac = iface.get('mac_address', '')
+            # len(macaddr) == 2 * 6 + 5 == 17
+            if ifname and mac and len(mac) == 17:
+                content += generate_udev_rule(ifname, mac)
 
     return content
 
@@ -382,11 +383,14 @@
 
     for iface in sorted(interfaces.values(),
                         key=lambda k: (order[k['type']], k['name'])):
-        content += "auto {name}\n".format(**iface)
 
+        if content[-2:] != "\n\n":
+            content += "\n"
         subnets = iface.get('subnets', {})
         if subnets:
             for index, subnet in zip(range(0, len(subnets)), subnets):
+                if content[-2:] != "\n\n":
+                    content += "\n"
                 iface['index'] = index
                 iface['mode'] = subnet['type']
                 if iface['mode'].endswith('6'):
@@ -397,19 +401,22 @@
                     iface['mode'] = 'dhcp'
 
                 if index == 0:
+                    if subnet['type'] != 'manual':
+                        content += "auto {name}\n".format(**iface)
                     content += "iface {name} {inet} {mode}\n".format(**iface)
                 else:
-                    content += "auto {name}:{index}\n".format(**iface)
+                    if subnet['type'] != 'manual':
+                        content += "auto {name}:{index}\n".format(**iface)
                     content += \
                         "iface {name}:{index} {inet} {mode}\n".format(**iface)
 
                 content += iface_add_subnet(iface, subnet)
                 content += iface_add_attrs(iface)
-                content += "\n"
         else:
+            if 'bond-master' in iface:
+                content += "auto {name}\n".format(**iface)
             content += "iface {name} {inet} {mode}\n".format(**iface)
             content += iface_add_attrs(iface)
-            content += "\n"
 
     for route in network_state.get('routes'):
         content += render_route(route)
@@ -426,11 +433,15 @@
     eni = os.path.sep.join((target, eni,))
     util.ensure_dir(os.path.dirname(eni))
     with open(eni, 'w+') as f:
+        LOG.info('Writing ' + eni)
         f.write(render_interfaces(network_state))
 
     netrules = os.path.sep.join((target, netrules,))
     util.ensure_dir(os.path.dirname(netrules))
-    with open(netrules, 'w+') as f:
-        f.write(render_persistent_net(network_state))
+    persistent_net_rules = render_persistent_net(network_state)
+    if len(persistent_net_rules) > 0:
+        with open(netrules, 'w+') as f:
+            LOG.info('Writing ' + netrules)
+            f.write(persistent_net_rules)
 
 # vi: ts=4 expandtab syntax=python
 
=== modified file 'tests/unittests/test_net.py'
--- tests/unittests/test_net.py	2016-02-24 17:49:17 +0000
+++ tests/unittests/test_net.py	2016-03-11 16:05:00 +0000
@@ -432,12 +432,14 @@
         with open(self.config_f, 'w') as fp:
             fp.write(self.config)
 
-    def get_net_config(self):
-        cfg = yaml.safe_load(self.config)
+    def get_net_config(self, config=None):
+        if config is None:
+            config = self.config
+        cfg = yaml.safe_load(config)
         return cfg.get('network')
 
-    def get_net_state(self):
-        net_cfg = self.get_net_config()
+    def get_net_state(self, config=None):
+        net_cfg = self.get_net_config(config)
         version = net_cfg.get('version')
         config = net_cfg.get('config')
         ns = network_state.NetworkState(version=version, config=config)
@@ -482,10 +484,41 @@
                   '    address 192.168.21.3/24\n' +
                   '    dns-nameservers 8.8.8.8 8.8.4.4\n' +
                   '    dns-search barley.maas sach.maas\n\n' +
-                  'auto eth1\n' + 'iface eth1 inet manual\n\n')
-        net_ifaces = net.render_interfaces(ns.network_state)
-        print(ns.network_state.get('interfaces'))
-        self.assertEqual(sorted(ifaces.split('\n')),
-                         sorted(net_ifaces.split('\n')))
+                  'iface eth1 inet manual\n\n')
+        net_ifaces = net.render_interfaces(ns.network_state)
+        print(ns.network_state.get('interfaces'))
+        self.assertEqual(sorted(ifaces.split('\n')),
+                         sorted(net_ifaces.split('\n')))
+
+    def test_render_interfaces_bonds(self):
+        bond_config = open('examples/tests/bonding_network.yaml', 'r').read()
+
+        ns = self.get_net_state(bond_config)
+        ifaces = ('auto lo\n' +
+                  'iface lo inet loopback\n\n' +
+                  'auto eth0\n' +
+                  'iface eth0 inet dhcp\n\n' +
+                  'auto eth1\n' +
+                  'iface eth1 inet manual\n' +
+                  '    bond-master bond0\n' +
+                  '    bond-mode active-backup\n\n' +
+                  'auto eth2\n' +
+                  'iface eth2 inet manual\n' +
+                  '    bond-master bond0\n' +
+                  '    bond-mode active-backup\n\n' +
+                  'auto bond0\n' +
+                  'iface bond0 inet static\n' +
+                  '    address 10.23.23.2/24\n' +
+                  '    bond-mode active-backup\n' +
+                  '    hwaddress 52:54:00:12:34:06\n' +
+                  '    bond-slaves none\n')
+        net_ifaces = net.render_interfaces(ns.network_state)
+        print("\n".join(sorted(ifaces.split('\n'))))
+        print("\n^^ LOCAL -- RENDER vv")
+        print("\n".join(sorted(net_ifaces.split('\n'))))
+        print(ns.network_state.get('interfaces'))
+        self.assertEqual(sorted(ifaces.split('\n')),
+                         sorted(net_ifaces.split('\n')))
+
 
 # vi: ts=4 expandtab syntax=python