Merge lp:~rvb/maas/explore-intf-bug-1375360 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Run dhclient on all the unconfigured interfaces during commissioning to create links between the NICs attached to unconfigured interfaces and the networks MAAS knows about.

I've successfully tested this on my NUCs (I used an Ethernet Dongle to get an additional NIC).

[Graham Binns (gmb)]

review: Approve

[Gavin Panella (allenap)]

Is there really no sensible scripting language available in the ephemeral environment?

[Raphaël Badin (rvb)]

> Is there really no sensible scripting language available in the ephemeral
> environment?

Well, I could have used Python but I think this script is so simple that a Shell script is all we need.

[Gavin Panella (allenap)]

> Is there really no sensible scripting language available in the
> ephemeral environment?

Python is available. See lldpd_install, lldpd_wait, and lldpd_capture. I
strongly suggest rewriting this in Python so that it can be unit-tested,
and so it doesn't completely suck :)

review: Needs Fixing

[Gavin Panella (allenap)]

That's great!

review: Approve

[Raphaël Badin (rvb)]

> That's great!

Thanks for not letting me be lazy about this ;).

[Julian Edwards (julian-edwards)]

On Monday 13 Oct 2014 12:18:47 you wrote:
> Diff comments:
> > === modified file 'src/metadataserver/models/commissioningscript.py'
> > --- src/metadataserver/models/commissioningscript.py 2014-08-13 21:49:35
> > +0000 +++ src/metadataserver/models/commissioningscript.py 2014-10-13
> > 11:44:38 +0000 @@ -214,6 +214,31 @@
> >
> > node.system_id)
> >
> > +# Run `dhclient` on all the unconfigured interfaces.
> > +# This is done to create records in the leases file for the
> > +# NICs attached to unconfigured interfaces. This way the leases
> > +# parser will be able to connect these NICs and the networks
> > +# MAAS knows about.
> > +DHCP_EXPLORE_SCRIPT = dedent("""\
> > + #!/bin/sh
> > +
> > + # Compute the list of interfaces.
> > + all_ifaces=`ifconfig -s -a | cut -d' ' -f1 | tail -n +2 | sort`
> > + echo "All interfaces:" $all_ifaces
> > + configured_ifaces=`ifconfig -s | cut -d' ' -f1 | tail -n +2 | sort`
> > + echo "Configured interfaces:" $configured_ifaces
> > +
> > + # Run dhclient on each of these interfaces.
> > + for iface in $all_ifaces; do
> > + echo $configured_ifaces | grep -q -v $iface
>
> This won't work if there are 10 or more NICs, because 'eth1' will match
> 'eth10'. There are probably other edge-cases like that.
>
> Also, use fgrep here to avoid treating $iface as a regex. That won't fix the
> edge-cases above though.

There will be some interfaces on which you don't want to run a dhclient, such
as lo. I would grep out anything starting 'lo' and 'virbr' for now.

I'm also concerned about the difference between ifconfig with the -a and
without. What is going to cause an interface to be unconfigured in the
ephemeral environment?

[Julian Edwards (julian-edwards)]

For example, I have eth0 on my laptop which appears with and without -a, but it is not "configured" as it doesn't have any IP (it's not plugged in).

review: Needs Information

[Raphaël Badin (rvb)]

[...]

Note that after the discussion with Gavin, I've rewritten the script in
Python.

> There will be some interfaces on which you don't want to run a dhclient, such
> as lo. I would grep out anything starting 'lo' and 'virbr' for now.

lo is always configured and we only run dhclient on unconfigured interfaces.

This is all happening during commissioning and thus in an environment we
control. Although a commissioning script could configure an
unconfigured virtual bridge, this is not done by default.
Additionally, running dhclient on a virbr will simply timeout.

> I'm also concerned about the difference between ifconfig with the -a and
> without. What is going to cause an interface to be unconfigured in the
> ephemeral environment?

I'm not sure how cloud-init decides which interfaces should be brought
up in the presence of multiple interfaces but this doesn't really matter
here, this code is there to "explore" the interfaces that have not been
already configured so that the leases parser will pick up these interfaces.

[Julian Edwards (julian-edwards)]

For the benefit of others reading, we had a call and discussed this. It is still possible for an interface to be unconfigured, but UP.

=== modified file 'src/metadataserver/models/commissioningscript.py'
--- src/metadataserver/models/commissioningscript.py	2014-08-13 21:49:35 +0000
+++ src/metadataserver/models/commissioningscript.py	2014-10-13 13:00:53 +0000
@@ -214,6 +214,29 @@
             node.system_id)
 
 
+# Run `dhclient` on all the unconfigured interfaces.
+# This is done to create records in the leases file for the
+# NICs attached to unconfigured interfaces.  This way the leases
+# parser will be able to connect these NICs and the networks
+# MAAS knows about.
+def dhcp_explore():
+    def get_iface_list(ifconfig_output):
+        return [
+            line.split()[0]
+            for line in ifconfig_output.splitlines()[1:]]
+
+    from subprocess import check_output, CalledProcessError, check_call
+    all_ifaces = get_iface_list(check_output(("ifconfig", "-s", "-a")))
+    configured_ifaces = get_iface_list(check_output(("ifconfig", "-s")))
+    unconfigured_ifaces = set(all_ifaces) - set(configured_ifaces)
+    for iface in sorted(unconfigured_ifaces):
+        try:
+            check_call(("sudo", "dhclient", "-w", iface))
+        except CalledProcessError:
+            # Continue trying to run dhcplient on the other interfaces.
+            pass
+
+
 # This function must be entirely self-contained. It must not use
 # variables or imports from the surrounding scope.
 def lldpd_install(config_file):
@@ -369,6 +392,10 @@
         'content': LIST_MODALIASES_SCRIPT.encode('ascii'),
         'hook': null_hook,
     },
+    '00-maas-05-dhcp-unconfigured-ifaces': {
+        'content': make_function_call_script(dhcp_explore),
+        'hook': null_hook,
+    },
     '99-maas-01-wait-for-lldpd.out': {
         'content': make_function_call_script(
             lldpd_wait, "/var/run/lldpd.socket", time_delay=60),

=== modified file 'src/metadataserver/models/tests/test_noderesults.py'
--- src/metadataserver/models/tests/test_noderesults.py	2014-09-10 16:20:31 +0000
+++ src/metadataserver/models/tests/test_noderesults.py	2014-10-13 13:00:53 +0000
@@ -42,7 +42,10 @@
     MAASServerTestCase,
     TestWithoutCrochetMixin,
     )
-from maastesting.matchers import MockCalledOnceWith
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockCallsMatch,
+    )
 from maastesting.utils import sample_binary_data
 from metadataserver.enum import RESULT_TYPE
 from metadataserver.fields import Bin
@@ -316,6 +319,60 @@
     return bytes(script)
 
 
+# The two following example outputs differ because eth2 and eth1 are not
+# configured and thus 'ifconfig -s -a' returns a list with both 'eth1'
+# and 'eth2' while 'ifconfig -s' does not contain them.
+
+# Example output of 'ifconfig -s -a':
+ifconfig_all = """
+Iface   MTU Met   RX-OK RX-ERR RX-DRP RX-OVR    TX-OK TX-ERR TX-DRP
+eth2       1500 0         0      0      0 0             0      0
+eth1       1500 0         0      0      0 0             0      0
+eth0       1500 0   1366127      0      0 0        831110      0
+lo        65536 0     38075      0      0 0         38075      0
+virbr0     1500 0         0      0      0 0             0      0
+wlan0      1500 0   2304695      0      0 0       1436049      0
+"""
+
+# Example output of 'ifconfig -s':
+ifconfig_config = """
+Iface   MTU Met   RX-OK RX-ERR RX-DRP RX-OVR    TX-OK TX-ERR TX-DRP
+eth0       1500 0   1366127      0      0 0        831110      0
+lo        65536 0     38115      0      0 0         38115      0
+virbr0     1500 0         0      0      0 0             0      0
+wlan0      1500 0   2304961      0      0 0       1436319      0
+"""
+
+
+class TestDHCPExplore(MAASServerTestCase):
+
+    def test_calls_dhclient_on_unconfigured_interfaces(self):
+        check_output = self.patch(subprocess, "check_output")
+        check_output.side_effect = [ifconfig_all, ifconfig_config]
+        check_call = self.patch(subprocess, "check_call")
+        dhcp_explore = isolate_function(cs_module.dhcp_explore)
+        dhcp_explore()
+        self.assertThat(
+            check_call,
+            MockCallsMatch(
+                call(("sudo", "dhclient", "-w", 'eth1')),
+                call(("sudo", "dhclient", "-w", 'eth2'))))
+
+    def test_swallows_errors(self):
+        check_output = self.patch(subprocess, "check_output")
+        check_output.side_effect = [ifconfig_all, ifconfig_config]
+        check_call = self.patch(subprocess, "check_call")
+        # Simulate a failure for the first call to dhclient.
+        check_call.side_effect = [CalledProcessError('boom', 2), '']
+        dhcp_explore = isolate_function(cs_module.dhcp_explore)
+        dhcp_explore()
+        self.assertThat(
+            check_call,
+            MockCallsMatch(
+                call(("sudo", "dhclient", "-w", 'eth1')),
+                call(("sudo", "dhclient", "-w", 'eth2'))))
+
+
 class TestExtractRouters(MAASServerTestCase):
 
     def test_extract_router_mac_addresses_returns_None_when_empty_input(self):