Merge ~raharper/netplan:fix-mtu into ~netplan-developers/netplan/+git/netplan-lp:master

[Ryan Harper]

Add support for MTU setting in all network devices

Any interface, physical or netdev can have a .link file to configure the MTU
of a device (eth, bond, bridge, etc).

(LP: #1668693)

[Ryan Harper (raharper)]

There is a superfluous sorting of the mapping keys which makes the diff harder to read; however, it's easier to see if the a mapping key is implemented when scanning the code.

[Mathieu Trudel-Lapierre (cyphermox)]

I wasn't able to successfully verify that this works on both NM and networkd. Could you add integration tests too?

review: Needs Fixing

[Ryan Harper (raharper)]

On Mon, Mar 6, 2017 at 11:13 AM, Mathieu Trudel-Lapierre <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> I wasn't able to successfully verify that this works on both NM and
> networkd. Could you add integration tests too?
>

I tested networkd only; I'll look at the integration tests.

> --
> https://code.launchpad.net/~raharper/netplan/+git/netplan/+merge/318545
> You are the owner of ~raharper/netplan:fix-mtu.
>

[Ryan Harper (raharper)]

On Mon, Mar 6, 2017 at 11:18 AM, Ryan Harper <email address hidden>
wrote:

> On Mon, Mar 6, 2017 at 11:13 AM, Mathieu Trudel-Lapierre <
> <email address hidden>> wrote:
>
> > Review: Needs Fixing
> >
> > I wasn't able to successfully verify that this works on both NM and
> > networkd. Could you add integration tests too?
> >
>
> I tested networkd only; I'll look at the integration tests.
>

I'm struggling with getting the tests to work reliably; If anyone of the
tests fails,
the system ends up in a bad state (modules previously loaded fail to a load
a second time); veth devices stick around, and data is written into /run but
not cleaned up between test-case runs. The tests don't run with via nose;
but
rather calling python directly and only from within the tests directory.
Finally the test invokes 'netplan' which references whatever netplan is
installed
on the system rather than the netplan from the source tree.

I didn't verify NM side as I didn't add any changes for NM side;

>
> > --
> > https://code.launchpad.net/~raharper/netplan/+git/netplan/+merge/318545
> > You are the owner of ~raharper/netplan:fix-mtu.
> >
>
> --
> https://code.launchpad.net/~raharper/netplan/+git/netplan/+merge/318545
> You are the owner of ~raharper/netplan:fix-mtu.
>

[Mathieu Trudel-Lapierre (cyphermox)]

These tests should be run from within autopkgtest:

(from the netplan source tree root)
autopkgtest -U --apt-pocket=proposed -s --test-name integration.py . -- qemu ~/adt-zesty-amd64.img

You can build the necessary zesty qemu image for autpkgtests with:

adt-buildvm-ubuntu-cloud -r zesty -a amd64

This is important, my initial manual testing showed that neither networkd nor NM devices ended up having their MTU set; so we should still test both NM and networkd renderers to make sure that the MTU is set properly on the underlying devices when this runs, and to properly automate testing this behavior (given that it's reportedly playing with udev, so there are possibly other steps to take before the changes will really be applied).

review: Needs Fixing

[Martin Pitt (pitti)]

Wrt. autopkgtest invocation there's a slight optimization: If you don't want autopkgtest to waste time on building the binary packages first but you already built them (in schroot or whereever), you can add them to the command line. Also, I'd suggest to not test against -proposed locally -- it's not what happens in production CI either and only slows down local iterations:

$ autopkgtest -s --test-name integration.py . /tmp/build-area/*.deb -- qemu ~/adt-zesty-amd64.img

[Ryan Harper (raharper)]

On Mon, Mar 6, 2017 at 7:04 PM, Mathieu Trudel-Lapierre <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> These tests should be run from within autopkgtest:
>
> (from the netplan source tree root)
> autopkgtest -U --apt-pocket=proposed -s --test-name integration.py . --
> qemu ~/adt-zesty-amd64.img
>
> You can build the necessary zesty qemu image for autpkgtests with:
>
> adt-buildvm-ubuntu-cloud -r zesty -a amd64
>

Ah, thanks for that.

>
> This is important, my initial manual testing showed that neither networkd
> nor NM devices ended up having their MTU set; so we should still test both
> NM and networkd renderers to make sure that the MTU is set properly on the
> underlying devices when this runs, and to properly automate testing this
> behavior (given that it's reportedly playing with udev, so there are
> possibly other steps to take before the changes will really be applied).
>

.link properties are handled by udev, not networkd. I'm not sure if NM has
separate MTU handling; that may be problematic if we're delegating this to
.link files for both networkd and NM.

Currently 'netplan apply' does guarantee .link files are re-run. I've a
separate bug opened to understand why re-triggering the net subsystem
doesn't always ensure .link files are processed

https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1669564

In the mean time, we could look at calling "udevadm test-builtin
setup_net_link /sys/class/net/<iface>" will indeed run the rules including
.link files to verify MTU is applied.

> --
> https://code.launchpad.net/~raharper/netplan/+git/netplan/+merge/318545
> You are the owner of ~raharper/netplan:fix-mtu.
>

86e348f... by Ryan Harper on 2017-03-07

Add mtu integration test, have netplan apply re-run .link files

5078731... by Ryan Harper on 2017-03-07

Add mtu integration test, have netplan apply re-run .link files

c8374f8... by Ryan Harper on 2017-03-07

fix typo on test-builtin command

d234129... by Ryan Harper on 2017-03-07

fix typo on test-builtin command

eb6bb11... by Ryan Harper on 2017-03-07

dont be noisy when triggering link rules

f7b6a26... by Ryan Harper on 2017-03-07

dont be noisy when triggering link rules

208f5f5... by Ryan Harper on 2017-03-07

integration: fix second interface ip address, add message for mtu test

4d07a39... by Ryan Harper on 2017-03-07

integration: fix second interface ip address, add message for mtu test

87927c0... by Ryan Harper on 2017-03-07

check_output returns string, not bytes

09cb443... by Ryan Harper on 2017-03-07

check_output returns string, not bytes

48b5427... by Ryan Harper on 2017-03-07

Add ipv6 mtu support

8f44c32... by Ryan Harper on 2017-03-09

Revert "Add ipv6 mtu support"

This reverts commit 48b542756ca555d48278ef13ebd19e338f689cc1.

226c0d6... by Ryan Harper on 2017-03-09

Sort mapping entries

e491050... by Ryan Harper on 2017-03-09

Merge branch 'fix-mtu' of ssh://git.launchpad.net/~raharper/netplan into fix-mtu

[Ryan Harper (raharper)]

OK,

Rebased on master (0.19)

I've added an integration test as well as fix for netplan apply to ensure we re-process .link files.

I've run this branch under adt and the mtu integration tests are passing in both (a property of having udev handle setting the MTU.

[Mathieu Trudel-Lapierre (cyphermox)]

Looks good to me now.

review: Approve

Unmerged commits

e491050... by Ryan Harper on 2017-03-09

Merge branch 'fix-mtu' of ssh://git.launchpad.net/~raharper/netplan into fix-mtu

09cb443... by Ryan Harper on 2017-03-07

check_output returns string, not bytes

4d07a39... by Ryan Harper on 2017-03-07

integration: fix second interface ip address, add message for mtu test

f7b6a26... by Ryan Harper on 2017-03-07

dont be noisy when triggering link rules

d234129... by Ryan Harper on 2017-03-07

fix typo on test-builtin command

5078731... by Ryan Harper on 2017-03-07

Add mtu integration test, have netplan apply re-run .link files

892c160... by Ryan Harper on 2017-02-28

Expand MTU to all device types

- Add MTU to all device types
- Always write link files, comment indicates that udev reads .link files not
  networkd
- Update test-cases to handle newly written link files for WakeOnLan setting

(LP: #1668693)

226c0d6... by Ryan Harper on 2017-03-09

Sort mapping entries

21a9c99... by Ryan Harper on 2017-02-28

Add support for setting MTU on ethernets

Allow setting MTU on ethernets, rendering in .link files for systemd

(LP: #1668693)

8f44c32... by Ryan Harper on 2017-03-09

Revert "Add ipv6 mtu support"

This reverts commit 48b542756ca555d48278ef13ebd19e338f689cc1.

diff --git a/src/netplan b/src/netplan
index 900f227..7875517 100755
--- a/src/netplan
+++ b/src/netplan
@@ -276,6 +276,14 @@ def command_apply():  # pragma: nocover (covered in autopkgtest)
             continue
         if replug(device):
             any_replug = True
+        else:
+            # if the interface is up, we can still apply .link file changes
+            logging.debug('netplan triggering .link rules for %s', device)
+            with open(os.devnull, 'w') as fd:
+                subprocess.check_call(['udevadm', 'test-builtin',
+                                       'net_setup_link',
+                                       '/sys/class/net/' + device],
+                                      stdout=fd, stderr=fd)
     if any_replug:
         subprocess.check_call(['udevadm', 'settle'])
 
diff --git a/src/networkd.c b/src/networkd.c
index 6a72370..04c7c37 100644
--- a/src/networkd.c
+++ b/src/networkd.c
@@ -87,10 +87,8 @@ write_link_file(net_definition* def, const char* rootdir, const char* path)
 {
     GString* s = NULL;
 
-    g_assert(def->type < ND_VIRTUAL);
-
     /* do we need to write a .link file? */
-    if (!def->set_name && !def->wake_on_lan)
+    if (!def->set_name && !def->wake_on_lan && !def->mtubytes)
         return;
 
     /* build file contents */
@@ -102,6 +100,9 @@ write_link_file(net_definition* def, const char* rootdir, const char* path)
         g_string_append_printf(s, "Name=%s\n", def->set_name);
     /* FIXME: Should this be turned from bool to str and support multiple values? */
     g_string_append_printf(s, "WakeOnLan=%s\n", def->wake_on_lan ? "magic" : "off");
+    if (def->mtubytes)
+        g_string_append_printf(s, "MTUBytes=%u\n", def->mtubytes);
+
 
     g_string_free_to_file(s, rootdir, path, ".link");
 }
@@ -328,9 +329,7 @@ write_networkd_conf(net_definition* def, const char* rootdir)
 
     /* We want this for all backends when renaming, as *.link files are
      * evaluated by udev, not networkd itself or NetworkManager. */
-    if (def->type < ND_VIRTUAL &&
-            (def->backend == BACKEND_NETWORKD || def->set_name))
-        write_link_file(def, rootdir, path_base);
+    write_link_file(def, rootdir, path_base);
 
     if (def->backend != BACKEND_NETWORKD) {
         g_debug("networkd: definition %s is not for us (backend %i)", def->id, def->backend);
diff --git a/src/parse.c b/src/parse.c
index 2dbec4d..4312112 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -898,75 +898,80 @@ const mapping_entry_handler nameservers_handlers[] = {
 };
 
 const mapping_entry_handler ethernet_def_handlers[] = {
-    {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
-    {"match", YAML_MAPPING_NODE, handle_match},
-    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
-    {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
+    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
     {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
-    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
     {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
+    {"match", YAML_MAPPING_NODE, handle_match},
+    {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
     {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
+    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
     {"routes", YAML_SEQUENCE_NODE, handle_routes},
+    {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
+    {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
     {NULL}
 };
 
 const mapping_entry_handler wifi_def_handlers[] = {
-    {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
-    {"match", YAML_MAPPING_NODE, handle_match},
-    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
-    {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
+    {"access-points", YAML_MAPPING_NODE, handle_wifi_access_points},
+    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
     {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
-    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
     {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
+    {"match", YAML_MAPPING_NODE, handle_match},
+    {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
     {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
-    {"access-points", YAML_MAPPING_NODE, handle_wifi_access_points},
+    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
     {"routes", YAML_SEQUENCE_NODE, handle_routes},
+    {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
+    {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
     {NULL}
 };
 
 const mapping_entry_handler bridge_def_handlers[] = {
-    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
+    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
     {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
-    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
     {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
-    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
     {"interfaces", YAML_SEQUENCE_NODE, handle_interfaces, NULL, netdef_offset(bridge)},
+    {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
+    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
     {"parameters", YAML_MAPPING_NODE, handle_bridge},
+    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
     {"routes", YAML_SEQUENCE_NODE, handle_routes},
     {NULL}
 };
 
 const mapping_entry_handler bond_def_handlers[] = {
-    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
+    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
     {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
-    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
     {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
-    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
     {"interfaces", YAML_SEQUENCE_NODE, handle_interfaces, NULL, netdef_offset(bond)},
     {"macaddress", YAML_SCALAR_NODE, handle_netdef_mac, NULL, netdef_offset(set_mac)},
-    {"routes", YAML_SEQUENCE_NODE, handle_routes},
+    {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
+    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
     {"parameters", YAML_MAPPING_NODE, handle_bonding},
+    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
+    {"routes", YAML_SEQUENCE_NODE, handle_routes},
     {NULL}
 };
 
 const mapping_entry_handler vlan_def_handlers[] = {
-    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
+    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
     {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
-    {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
     {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
     {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
-    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
     {"id", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(vlan_id)},
     {"link", YAML_SCALAR_NODE, handle_netdef_id_ref, NULL, netdef_offset(vlan_link)},
+    {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
+    {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
+    {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
     {"routes", YAML_SEQUENCE_NODE, handle_routes},
     {NULL}
 };
diff --git a/src/parse.h b/src/parse.h
index 23c597f..5f5d28d 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -81,6 +81,9 @@ typedef struct net_definition {
     /* Configured custom MAC address */
     char* set_mac;
 
+    /* interface mtu */
+    guint mtubytes;
+
     /* these properties are only valid for physical interfaces (type < ND_VIRTUAL) */
     char* set_name;
     struct {
diff --git a/tests/generate.py b/tests/generate.py
index 2d69d02..542ad2f 100755
--- a/tests/generate.py
+++ b/tests/generate.py
@@ -23,6 +23,7 @@ import re
 import sys
 import stat
 import tempfile
+import textwrap
 import subprocess
 import unittest
 
@@ -275,6 +276,42 @@ unmanaged-devices+=interface-name:eth0,''')
         # should not allow NM to manage everything
         self.assertFalse(os.path.exists(self.nm_enable_all_conf))
 
+    def test_eth_mtu(self):
+        self.generate('''network:
+  version: 2
+  ethernets:
+    eth1:
+      mtu: 1280
+      dhcp4: n''')
+
+        self.assert_networkd({'eth1.link': '[Match]\nOriginalName=eth1\n\n[Link]\nWakeOnLan=off\nMTUBytes=1280\n'})
+
+    def test_mtu_all(self):
+        self.generate(textwrap.dedent("""
+            network:
+              version: 2
+              ethernets:
+                eth1:
+                  mtu: 1280
+                  dhcp4: n
+              bonds:
+                bond0:
+                  interfaces:
+                  - eth1
+                  mtu: 9000
+              vlans:
+                bond0.108:
+                  link: bond0
+                  id: 108"""))
+        self.assert_networkd({
+            'bond0.108.netdev': '[NetDev]\nName=bond0.108\nKind=vlan\n\n[VLAN]\nId=108\n',
+            'bond0.link': '[Match]\n\n[Link]\nWakeOnLan=off\nMTUBytes=9000\n',
+            'bond0.netdev': '[NetDev]\nName=bond0\nKind=bond\n',
+            'bond0.network': '[Match]\nName=bond0\n\n[Network]\nVLAN=bond0.108\n',
+            'eth1.link': '[Match]\nOriginalName=eth1\n\n[Link]\nWakeOnLan=off\nMTUBytes=1280\n',
+            'eth1.network': '[Match]\nName=eth1\n\n[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n'
+        })
+
     def test_eth_match_by_driver_rename(self):
         self.generate('''network:
   version: 2
@@ -1066,7 +1103,7 @@ method=link-local
 '''})
         # should allow NM to manage everything else
         self.assertTrue(os.path.exists(self.nm_enable_all_conf))
-        self.assert_networkd({})
+        self.assert_networkd({'eth0.link': '[Match]\nOriginalName=eth0\n\n[Link]\nWakeOnLan=magic\n'})
         self.assert_udev(None)
 
     def test_eth_match_by_driver(self):
diff --git a/tests/integration.py b/tests/integration.py
index 446d70b..074ba59 100755
--- a/tests/integration.py
+++ b/tests/integration.py
@@ -391,6 +391,29 @@ class _CommonTests:
         for i in [self.dev_e_client, self.dev_e2_client, 'mybr']:
             self.assertRegex(out, '%s\s+(ethernet|bridge)\s+%s' % (i, expected_state))
 
+    def test_eth_mtu(self):
+        self.setup_eth(None)
+        self.start_dnsmasq(None, self.dev_e2_ap)
+        with open(self.config, 'w') as f:
+            f.write('''network:
+  renderer: %(r)s
+  ethernets:
+    %(ec)s:
+      dhcp4: yes
+    enmtus:
+      match: {name: %(e2c)s}
+      mtu: 1492
+      dhcp4: yes''' % {'r': self.backend, 'ec': self.dev_e_client, 'e2c': self.dev_e2_client})
+        self.generate_and_settle()
+        self.assert_iface_up(self.dev_e_client,
+                             ['inet 192.168.5.[0-9]+/24'],
+                             ['master'])
+        self.assert_iface_up(self.dev_e2_client,
+                             ['inet 192.168.6.[0-9]+/24'])
+        out = subprocess.check_output(['ip', 'a', 'show', self.dev_e2_client],
+                                      universal_newlines=True)
+        self.assertTrue('mtu 1492' in out, "checking MTU, should be 1492")
+
     def test_bridge_path_cost(self):
         self.setup_eth(None)
         self.start_dnsmasq(None, self.dev_e2_ap)