Merge ~xnox/cloud-init:dash_underscore into cloud-init:master

Proposed by Dimitri John Ledkov on 2017-05-13
Status: Merged
Approved by: Scott Moser on 2017-05-25
Approved revision: 47a3f8ceae5ac27e1497a42e5576ae7bdfa610c5
Merged at revision: 910ed46124e992eb20e49ea156b7127cd3ebbe9d
Proposed branch: ~xnox/cloud-init:dash_underscore
Merge into: cloud-init:master
Diff against target: 70 lines (+12/-2)
2 files modified
cloudinit/net/netplan.py (+2/-2)
tests/unittests/test_net.py (+10/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2017-05-22
Ryan Harper 2017-05-13 Needs Fixing on 2017-05-15
Review via email: mp+324014@code.launchpad.net

Commit message

nplan: For bonds, allow dashed or underscore names of keys.

As some of the bond paramemters are passed in as dashed, or
underscored, depending on the input source.

Also correct transmit-hash-policy netplan target key.

LP: #1690480

To post a comment you must log in.
Ryan Harper (raharper) wrote :

AFAICT, bond-* parameters are almost always directly related to the kernel sysfs parameters as defined in /sys/class/net/<bond>/bonding/* which are kernel properties of the bonding driver.

Ifupdown appears to have a habit of using bond-<sysfs-key-name>, netplan has a habit of the same, but in some cases rewords the value of the sysfs-key-name (e.g. num_grat_arp vs. gratiutous-arp).

So it's not clear that just a few adds of underbar here are going to resolve the issue.
In the renderers class, we have a currently known mapping of what's expected, but it's certainly somewhat error prone as new kernel features may surface where by both cloud-init and renderer will need updating w.r.t to validation.

It's also been expressed that netplan should not expose sysfs entries directly in the configuration but higher level names.

In any case for this; I suggest that instead of fixing a few keys, we accept either '-' or '_' in the input:

bondoption = 'bond_mode'

netplan_param = NET_CONFIG_TO_V2.get(bondoption.replace("_", "-"))

review: Needs Fixing
Dimitri John Ledkov (xnox) wrote :

Hm, i think i should start a new clean branch for these things.

Dimitri John Ledkov (xnox) wrote :

Did a force push, please re-review.

Ryan Harper (raharper) wrote :

Looks good; let's add some element to the unittest to catch and verify this change:

diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 7c5dc4e..956570d 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -517,11 +517,15 @@ auto eth1
 iface eth1 inet manual
     bond-master bond0
     bond-mode active-backup
+ bond-xmit-hash-policy layer3+4
+ bond_miimon 100

 auto eth2
 iface eth2 inet manual
     bond-master bond0
     bond-mode active-backup
+ bond-xmit-hash-policy layer3+4
+ bond_miimon 100

 iface eth3 inet manual

@@ -534,6 +538,8 @@ auto bond0
 iface bond0 inet6 dhcp
     bond-mode active-backup
     bond-slaves none
+ bond-xmit-hash-policy layer3+4
+ bond_miimon 100
     hwaddress aa:bb:cc:dd:ee:ff

 auto br0
@@ -658,7 +664,9 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
                         - eth1
                         - eth2
                         parameters:
+ mii-monitor-interval: 100
                             mode: active-backup
+ transmit-hash-policy: layer3+4
                 bridges:
                     br0:
                         addresses:
@@ -747,6 +755,8 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
                     - eth2
                   params:
                     bond-mode: active-backup
+ bond_miimon: 100
+ bond-xmit-hash-policy: "layer3+4"
                   subnets:
                     - type: dhcp6
                 # A Bond VLAN.

~xnox/cloud-init:dash_underscore updated on 2017-05-22
47a3f8c... by Dimitri John Ledkov on 2017-05-22

Add tests as per rharper suggestion.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
2index 9b71de9..d7ddf0c 100644
3--- a/cloudinit/net/netplan.py
4+++ b/cloudinit/net/netplan.py
5@@ -41,7 +41,7 @@ NET_CONFIG_TO_V2 = {
6 'bond-num-grat-arp': 'gratuitious-arp',
7 'bond-primary-reselect': 'primary-reselect-policy',
8 'bond-updelay': 'up-delay',
9- 'bond-xmit_hash_policy': 'transmit_hash_policy'},
10+ 'bond-xmit-hash-policy': 'transmit-hash-policy'},
11 'bridge': {'bridge_ageing': 'ageing-time',
12 'bridge_bridgeprio': 'priority',
13 'bridge_fd': 'forward-delay',
14@@ -294,7 +294,7 @@ class Renderer(renderer.Renderer):
15 for match in ['bond_', 'bond-']:
16 bond_params = _get_params_dict_by_match(ifcfg, match)
17 for (param, value) in bond_params.items():
18- newname = v2_bond_map.get(param)
19+ newname = v2_bond_map.get(param.replace('_', '-'))
20 if newname is None:
21 continue
22 bond_config.update({newname: value})
23diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
24index 68a0157..ea4c789 100644
25--- a/tests/unittests/test_net.py
26+++ b/tests/unittests/test_net.py
27@@ -518,11 +518,15 @@ auto eth1
28 iface eth1 inet manual
29 bond-master bond0
30 bond-mode active-backup
31+ bond-xmit-hash-policy layer3+4
32+ bond_miimon 100
33
34 auto eth2
35 iface eth2 inet manual
36 bond-master bond0
37 bond-mode active-backup
38+ bond-xmit-hash-policy layer3+4
39+ bond_miimon 100
40
41 iface eth3 inet manual
42
43@@ -535,6 +539,8 @@ auto bond0
44 iface bond0 inet6 dhcp
45 bond-mode active-backup
46 bond-slaves none
47+ bond-xmit-hash-policy layer3+4
48+ bond_miimon 100
49 hwaddress aa:bb:cc:dd:ee:ff
50
51 auto br0
52@@ -660,7 +666,9 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
53 - eth1
54 - eth2
55 parameters:
56+ mii-monitor-interval: 100
57 mode: active-backup
58+ transmit-hash-policy: layer3+4
59 bridges:
60 br0:
61 addresses:
62@@ -751,6 +759,8 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
63 - eth2
64 params:
65 bond-mode: active-backup
66+ bond_miimon: 100
67+ bond-xmit-hash-policy: "layer3+4"
68 subnets:
69 - type: dhcp6
70 # A Bond VLAN.

Subscribers

People subscribed via source and target branches