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

Proposed by Ryan Harper
Status: Approved
Approved by: Mathieu Trudel-Lapierre
Approved revision: e491050e7c965ead96efdc16642f61c0687c70a8
Proposed branch: ~raharper/netplan:fix-mtu
Merge into: ~netplan-developers/netplan/+git/netplan-lp:master
Diff against target: 274 lines (+103/-28)
6 files modified
src/netplan (+8/-0)
src/networkd.c (+5/-6)
src/parse.c (+26/-21)
src/parse.h (+3/-0)
tests/generate.py (+38/-1)
tests/integration.py (+23/-0)
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre (community) Approve
Review via email: mp+318545@code.launchpad.net

Description of the change

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)

To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

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.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

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

review: Needs Fixing
Revision history for this message
Ryan Harper (raharper) 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.

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

Revision history for this message
Ryan Harper (raharper) wrote :

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.
>

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

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
Revision history for this message
Martin Pitt (pitti) wrote :

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

Revision history for this message
Ryan Harper (raharper) wrote :

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.
>

~raharper/netplan:fix-mtu updated
86e348f... by Ryan Harper

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

5078731... by Ryan Harper

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

c8374f8... by Ryan Harper

fix typo on test-builtin command

d234129... by Ryan Harper

fix typo on test-builtin command

eb6bb11... by Ryan Harper

dont be noisy when triggering link rules

f7b6a26... by Ryan Harper

dont be noisy when triggering link rules

208f5f5... by Ryan Harper

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

4d07a39... by Ryan Harper

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

87927c0... by Ryan Harper

check_output returns string, not bytes

09cb443... by Ryan Harper

check_output returns string, not bytes

48b5427... by Ryan Harper

Add ipv6 mtu support

8f44c32... by Ryan Harper

Revert "Add ipv6 mtu support"

This reverts commit 48b542756ca555d48278ef13ebd19e338f689cc1.

226c0d6... by Ryan Harper

Sort mapping entries

e491050... by Ryan Harper

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

Revision history for this message
Ryan Harper (raharper) wrote :

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.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Looks good to me now.

review: Approve

Unmerged commits

e491050... by Ryan Harper

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

09cb443... by Ryan Harper

check_output returns string, not bytes

4d07a39... by Ryan Harper

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

f7b6a26... by Ryan Harper

dont be noisy when triggering link rules

d234129... by Ryan Harper

fix typo on test-builtin command

5078731... by Ryan Harper

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

892c160... by Ryan Harper

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

Sort mapping entries

21a9c99... by Ryan Harper

Add support for setting MTU on ethernets

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

(LP: #1668693)

8f44c32... by Ryan Harper

Revert "Add ipv6 mtu support"

This reverts commit 48b542756ca555d48278ef13ebd19e338f689cc1.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/netplan b/src/netplan
2index 900f227..7875517 100755
3--- a/src/netplan
4+++ b/src/netplan
5@@ -276,6 +276,14 @@ def command_apply(): # pragma: nocover (covered in autopkgtest)
6 continue
7 if replug(device):
8 any_replug = True
9+ else:
10+ # if the interface is up, we can still apply .link file changes
11+ logging.debug('netplan triggering .link rules for %s', device)
12+ with open(os.devnull, 'w') as fd:
13+ subprocess.check_call(['udevadm', 'test-builtin',
14+ 'net_setup_link',
15+ '/sys/class/net/' + device],
16+ stdout=fd, stderr=fd)
17 if any_replug:
18 subprocess.check_call(['udevadm', 'settle'])
19
20diff --git a/src/networkd.c b/src/networkd.c
21index 6a72370..04c7c37 100644
22--- a/src/networkd.c
23+++ b/src/networkd.c
24@@ -87,10 +87,8 @@ write_link_file(net_definition* def, const char* rootdir, const char* path)
25 {
26 GString* s = NULL;
27
28- g_assert(def->type < ND_VIRTUAL);
29-
30 /* do we need to write a .link file? */
31- if (!def->set_name && !def->wake_on_lan)
32+ if (!def->set_name && !def->wake_on_lan && !def->mtubytes)
33 return;
34
35 /* build file contents */
36@@ -102,6 +100,9 @@ write_link_file(net_definition* def, const char* rootdir, const char* path)
37 g_string_append_printf(s, "Name=%s\n", def->set_name);
38 /* FIXME: Should this be turned from bool to str and support multiple values? */
39 g_string_append_printf(s, "WakeOnLan=%s\n", def->wake_on_lan ? "magic" : "off");
40+ if (def->mtubytes)
41+ g_string_append_printf(s, "MTUBytes=%u\n", def->mtubytes);
42+
43
44 g_string_free_to_file(s, rootdir, path, ".link");
45 }
46@@ -328,9 +329,7 @@ write_networkd_conf(net_definition* def, const char* rootdir)
47
48 /* We want this for all backends when renaming, as *.link files are
49 * evaluated by udev, not networkd itself or NetworkManager. */
50- if (def->type < ND_VIRTUAL &&
51- (def->backend == BACKEND_NETWORKD || def->set_name))
52- write_link_file(def, rootdir, path_base);
53+ write_link_file(def, rootdir, path_base);
54
55 if (def->backend != BACKEND_NETWORKD) {
56 g_debug("networkd: definition %s is not for us (backend %i)", def->id, def->backend);
57diff --git a/src/parse.c b/src/parse.c
58index 2dbec4d..4312112 100644
59--- a/src/parse.c
60+++ b/src/parse.c
61@@ -898,75 +898,80 @@ const mapping_entry_handler nameservers_handlers[] = {
62 };
63
64 const mapping_entry_handler ethernet_def_handlers[] = {
65- {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
66- {"match", YAML_MAPPING_NODE, handle_match},
67- {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
68- {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
69+ {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
70 {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
71 {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
72- {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
73 {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
74 {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
75+ {"match", YAML_MAPPING_NODE, handle_match},
76+ {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
77 {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
78+ {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
79 {"routes", YAML_SEQUENCE_NODE, handle_routes},
80+ {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
81+ {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
82 {NULL}
83 };
84
85 const mapping_entry_handler wifi_def_handlers[] = {
86- {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
87- {"match", YAML_MAPPING_NODE, handle_match},
88- {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
89- {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
90+ {"access-points", YAML_MAPPING_NODE, handle_wifi_access_points},
91+ {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
92 {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
93 {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
94- {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
95 {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
96 {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
97+ {"match", YAML_MAPPING_NODE, handle_match},
98+ {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
99 {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
100- {"access-points", YAML_MAPPING_NODE, handle_wifi_access_points},
101+ {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
102 {"routes", YAML_SEQUENCE_NODE, handle_routes},
103+ {"set-name", YAML_SCALAR_NODE, handle_netdef_str, NULL, netdef_offset(set_name)},
104+ {"wakeonlan", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(wake_on_lan)},
105 {NULL}
106 };
107
108 const mapping_entry_handler bridge_def_handlers[] = {
109- {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
110+ {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
111 {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
112 {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
113- {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
114 {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
115 {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
116- {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
117 {"interfaces", YAML_SEQUENCE_NODE, handle_interfaces, NULL, netdef_offset(bridge)},
118+ {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
119+ {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
120 {"parameters", YAML_MAPPING_NODE, handle_bridge},
121+ {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
122 {"routes", YAML_SEQUENCE_NODE, handle_routes},
123 {NULL}
124 };
125
126 const mapping_entry_handler bond_def_handlers[] = {
127- {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
128+ {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
129 {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
130 {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
131- {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
132 {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
133 {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
134- {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
135 {"interfaces", YAML_SEQUENCE_NODE, handle_interfaces, NULL, netdef_offset(bond)},
136 {"macaddress", YAML_SCALAR_NODE, handle_netdef_mac, NULL, netdef_offset(set_mac)},
137- {"routes", YAML_SEQUENCE_NODE, handle_routes},
138+ {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
139+ {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
140 {"parameters", YAML_MAPPING_NODE, handle_bonding},
141+ {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
142+ {"routes", YAML_SEQUENCE_NODE, handle_routes},
143 {NULL}
144 };
145
146 const mapping_entry_handler vlan_def_handlers[] = {
147- {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
148+ {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
149 {"dhcp4", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp4)},
150 {"dhcp6", YAML_SCALAR_NODE, handle_netdef_bool, NULL, netdef_offset(dhcp6)},
151- {"addresses", YAML_SEQUENCE_NODE, handle_addresses},
152 {"gateway4", YAML_SCALAR_NODE, handle_gateway4},
153 {"gateway6", YAML_SCALAR_NODE, handle_gateway6},
154- {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
155 {"id", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(vlan_id)},
156 {"link", YAML_SCALAR_NODE, handle_netdef_id_ref, NULL, netdef_offset(vlan_link)},
157+ {"nameservers", YAML_MAPPING_NODE, NULL, nameservers_handlers},
158+ {"mtu", YAML_SCALAR_NODE, handle_netdef_guint, NULL, netdef_offset(mtubytes)},
159+ {"renderer", YAML_SCALAR_NODE, handle_netdef_renderer},
160 {"routes", YAML_SEQUENCE_NODE, handle_routes},
161 {NULL}
162 };
163diff --git a/src/parse.h b/src/parse.h
164index 23c597f..5f5d28d 100644
165--- a/src/parse.h
166+++ b/src/parse.h
167@@ -81,6 +81,9 @@ typedef struct net_definition {
168 /* Configured custom MAC address */
169 char* set_mac;
170
171+ /* interface mtu */
172+ guint mtubytes;
173+
174 /* these properties are only valid for physical interfaces (type < ND_VIRTUAL) */
175 char* set_name;
176 struct {
177diff --git a/tests/generate.py b/tests/generate.py
178index 2d69d02..542ad2f 100755
179--- a/tests/generate.py
180+++ b/tests/generate.py
181@@ -23,6 +23,7 @@ import re
182 import sys
183 import stat
184 import tempfile
185+import textwrap
186 import subprocess
187 import unittest
188
189@@ -275,6 +276,42 @@ unmanaged-devices+=interface-name:eth0,''')
190 # should not allow NM to manage everything
191 self.assertFalse(os.path.exists(self.nm_enable_all_conf))
192
193+ def test_eth_mtu(self):
194+ self.generate('''network:
195+ version: 2
196+ ethernets:
197+ eth1:
198+ mtu: 1280
199+ dhcp4: n''')
200+
201+ self.assert_networkd({'eth1.link': '[Match]\nOriginalName=eth1\n\n[Link]\nWakeOnLan=off\nMTUBytes=1280\n'})
202+
203+ def test_mtu_all(self):
204+ self.generate(textwrap.dedent("""
205+ network:
206+ version: 2
207+ ethernets:
208+ eth1:
209+ mtu: 1280
210+ dhcp4: n
211+ bonds:
212+ bond0:
213+ interfaces:
214+ - eth1
215+ mtu: 9000
216+ vlans:
217+ bond0.108:
218+ link: bond0
219+ id: 108"""))
220+ self.assert_networkd({
221+ 'bond0.108.netdev': '[NetDev]\nName=bond0.108\nKind=vlan\n\n[VLAN]\nId=108\n',
222+ 'bond0.link': '[Match]\n\n[Link]\nWakeOnLan=off\nMTUBytes=9000\n',
223+ 'bond0.netdev': '[NetDev]\nName=bond0\nKind=bond\n',
224+ 'bond0.network': '[Match]\nName=bond0\n\n[Network]\nVLAN=bond0.108\n',
225+ 'eth1.link': '[Match]\nOriginalName=eth1\n\n[Link]\nWakeOnLan=off\nMTUBytes=1280\n',
226+ 'eth1.network': '[Match]\nName=eth1\n\n[Network]\nBond=bond0\nLinkLocalAddressing=no\nIPv6AcceptRA=no\n'
227+ })
228+
229 def test_eth_match_by_driver_rename(self):
230 self.generate('''network:
231 version: 2
232@@ -1066,7 +1103,7 @@ method=link-local
233 '''})
234 # should allow NM to manage everything else
235 self.assertTrue(os.path.exists(self.nm_enable_all_conf))
236- self.assert_networkd({})
237+ self.assert_networkd({'eth0.link': '[Match]\nOriginalName=eth0\n\n[Link]\nWakeOnLan=magic\n'})
238 self.assert_udev(None)
239
240 def test_eth_match_by_driver(self):
241diff --git a/tests/integration.py b/tests/integration.py
242index 446d70b..074ba59 100755
243--- a/tests/integration.py
244+++ b/tests/integration.py
245@@ -391,6 +391,29 @@ class _CommonTests:
246 for i in [self.dev_e_client, self.dev_e2_client, 'mybr']:
247 self.assertRegex(out, '%s\s+(ethernet|bridge)\s+%s' % (i, expected_state))
248
249+ def test_eth_mtu(self):
250+ self.setup_eth(None)
251+ self.start_dnsmasq(None, self.dev_e2_ap)
252+ with open(self.config, 'w') as f:
253+ f.write('''network:
254+ renderer: %(r)s
255+ ethernets:
256+ %(ec)s:
257+ dhcp4: yes
258+ enmtus:
259+ match: {name: %(e2c)s}
260+ mtu: 1492
261+ dhcp4: yes''' % {'r': self.backend, 'ec': self.dev_e_client, 'e2c': self.dev_e2_client})
262+ self.generate_and_settle()
263+ self.assert_iface_up(self.dev_e_client,
264+ ['inet 192.168.5.[0-9]+/24'],
265+ ['master'])
266+ self.assert_iface_up(self.dev_e2_client,
267+ ['inet 192.168.6.[0-9]+/24'])
268+ out = subprocess.check_output(['ip', 'a', 'show', self.dev_e2_client],
269+ universal_newlines=True)
270+ self.assertTrue('mtu 1492' in out, "checking MTU, should be 1492")
271+
272 def test_bridge_path_cost(self):
273 self.setup_eth(None)
274 self.start_dnsmasq(None, self.dev_e2_ap)

Subscribers

People subscribed via source and target branches