Merge lp:~mpontillo/curtin/fix-improper-bond-parameters-bug-1588547 into lp:~curtin-dev/curtin/trunk

[Mike Pontillo]

Don't add additional parameters to alias interfaces.

[Mike Pontillo (mpontillo)]

Note that this change also removed the 'mtu' output from the alias interface on another test case. (I don't think it would have any effect anyway; setting the MTU on an alias just sets it on the parent.)

[Ryan Harper (raharper)]

This looks good. Comments below, mostly would like to re-use the existing bonding config as this will automatically get validated in unittest and in vmtests if you update the current bonding_network.yaml in examples/tests/ dir instead of inlining it in the unittest.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

Do the vmtests need to change as well? I only ran the unit tests when I did the fix.

[Mike Pontillo (mpontillo)]

Thanks for the review; some replies below.

[Ryan Harper (raharper)]

On Fri, Jun 3, 2016 at 2:44 PM, Mike Pontillo <email address hidden>
wrote:

> Thanks for the review; some replies below.
>
> Diff comments:
>
> > === modified file 'curtin/net/__init__.py'
> > --- curtin/net/__init__.py 2016-06-03 00:35:50 +0000
> > +++ curtin/net/__init__.py 2016-06-03 00:35:50 +0000
> > @@ -331,7 +331,12 @@
> >
> >
> > # TODO: switch to valid_map for attrs
> > -def iface_add_attrs(iface):
> > +def iface_add_attrs(iface, index):
> > + # If the index is non-zero, this is an alias interface. Alias
> interfaces
> > + # represent additional interface addresses, and cannot have
> additional
> > + # attributes. So if this is an alias, stop right here.
>
> Well, ifupdown doesn't do much in the way of model validation. ;-) An
> alias is really just a (deprecated, I think) way to add more addresses to
> an interface. (that is to say, ifupdown may well allow them, I can't think
> of any cases where that isn't arguably a bug in ifupdown.)
>
> https://bugs.launchpad.net/maas/+bug/1588547/comments/5
>
>
It's probably worth supplying what the preferred eni should look like;
I'm certainly no bonding expert but attempted to apply formatting as
suggested
by the ifenslave documentation.

Ryan

[Ryan Harper (raharper)]

Looks good. I'll handle any errors during the vmtest run.

review: Approve

=== modified file 'curtin/net/__init__.py'
--- curtin/net/__init__.py	2016-04-20 20:03:00 +0000
+++ curtin/net/__init__.py	2016-06-03 19:53:48 +0000
@@ -331,7 +331,14 @@
 
 
 # TODO: switch to valid_map for attrs
-def iface_add_attrs(iface):
+def iface_add_attrs(iface, index):
+    # If the index is non-zero, this is an alias interface. Alias interfaces
+    # represent additional interface addresses, and should not have additional
+    # attributes. (extra attributes here are almost always either incorrect,
+    # or are applied to the parent interface.) So if this is an alias, stop
+    # right here.
+    if index != 0:
+        return ""
     content = ""
     ignore_map = [
         'control',
@@ -432,13 +439,13 @@
 
                 content += iface_start_entry(iface, index)
                 content += iface_add_subnet(iface, subnet)
-                content += iface_add_attrs(iface)
+                content += iface_add_attrs(iface, index)
         else:
             # ifenslave docs say to auto the slave devices
             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 += iface_add_attrs(iface, index)
 
     for route in network_state.get('routes'):
         content += render_route(route)
 
=== modified file 'examples/tests/bonding_network.yaml'
--- examples/tests/bonding_network.yaml	2016-05-17 20:04:00 +0000
+++ examples/tests/bonding_network.yaml	2016-06-03 19:53:48 +0000
@@ -26,6 +26,8 @@
           subnets:
               - type: static
                 address: 10.23.23.2/24
+              - type: static
+                address: 10.23.24.2/24
 
 curthooks_commands:
     # use curtin to disable open-iscsi ifupdown hooks for precise; they're
 
=== modified file 'tests/unittests/test_net.py'
--- tests/unittests/test_net.py	2016-05-17 20:04:00 +0000
+++ tests/unittests/test_net.py	2016-06-03 19:53:48 +0000
@@ -517,6 +517,10 @@
                 hwaddress 52:54:00:12:34:06
                 bond-slaves none
 
+            auto bond0:1
+            iface bond0:1 inet static
+                address 10.23.24.2/24
+
             source /etc/network/interfaces.d/*.cfg
             """)
         net_ifaces = net.render_interfaces(ns.network_state)
@@ -549,7 +553,6 @@
             auto interface1:1
             iface interface1:1 inet static
                 address 192.168.14.4/24
-                mtu 1492
 
             allow-hotplug interface2
             iface interface2 inet static