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

Proposed by Dimitri John Ledkov
Status: Merged
Approved by: Scott Moser
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
Ryan Harper Needs Fixing
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.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
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
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

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

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Did a force push, please re-review.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
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
47a3f8c... by Dimitri John Ledkov

Add tests as per rharper suggestion.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
index 9b71de9..d7ddf0c 100644
--- a/cloudinit/net/netplan.py
+++ b/cloudinit/net/netplan.py
@@ -41,7 +41,7 @@ NET_CONFIG_TO_V2 = {
41 'bond-num-grat-arp': 'gratuitious-arp',41 'bond-num-grat-arp': 'gratuitious-arp',
42 'bond-primary-reselect': 'primary-reselect-policy',42 'bond-primary-reselect': 'primary-reselect-policy',
43 'bond-updelay': 'up-delay',43 'bond-updelay': 'up-delay',
44 'bond-xmit_hash_policy': 'transmit_hash_policy'},44 'bond-xmit-hash-policy': 'transmit-hash-policy'},
45 'bridge': {'bridge_ageing': 'ageing-time',45 'bridge': {'bridge_ageing': 'ageing-time',
46 'bridge_bridgeprio': 'priority',46 'bridge_bridgeprio': 'priority',
47 'bridge_fd': 'forward-delay',47 'bridge_fd': 'forward-delay',
@@ -294,7 +294,7 @@ class Renderer(renderer.Renderer):
294 for match in ['bond_', 'bond-']:294 for match in ['bond_', 'bond-']:
295 bond_params = _get_params_dict_by_match(ifcfg, match)295 bond_params = _get_params_dict_by_match(ifcfg, match)
296 for (param, value) in bond_params.items():296 for (param, value) in bond_params.items():
297 newname = v2_bond_map.get(param)297 newname = v2_bond_map.get(param.replace('_', '-'))
298 if newname is None:298 if newname is None:
299 continue299 continue
300 bond_config.update({newname: value})300 bond_config.update({newname: value})
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 68a0157..ea4c789 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -518,11 +518,15 @@ auto eth1
518iface eth1 inet manual518iface eth1 inet manual
519 bond-master bond0519 bond-master bond0
520 bond-mode active-backup520 bond-mode active-backup
521 bond-xmit-hash-policy layer3+4
522 bond_miimon 100
521523
522auto eth2524auto eth2
523iface eth2 inet manual525iface eth2 inet manual
524 bond-master bond0526 bond-master bond0
525 bond-mode active-backup527 bond-mode active-backup
528 bond-xmit-hash-policy layer3+4
529 bond_miimon 100
526530
527iface eth3 inet manual531iface eth3 inet manual
528532
@@ -535,6 +539,8 @@ auto bond0
535iface bond0 inet6 dhcp539iface bond0 inet6 dhcp
536 bond-mode active-backup540 bond-mode active-backup
537 bond-slaves none541 bond-slaves none
542 bond-xmit-hash-policy layer3+4
543 bond_miimon 100
538 hwaddress aa:bb:cc:dd:ee:ff544 hwaddress aa:bb:cc:dd:ee:ff
539545
540auto br0546auto br0
@@ -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
660 - eth1666 - eth1
661 - eth2667 - eth2
662 parameters:668 parameters:
669 mii-monitor-interval: 100
663 mode: active-backup670 mode: active-backup
671 transmit-hash-policy: layer3+4
664 bridges:672 bridges:
665 br0:673 br0:
666 addresses:674 addresses:
@@ -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
751 - eth2759 - eth2
752 params:760 params:
753 bond-mode: active-backup761 bond-mode: active-backup
762 bond_miimon: 100
763 bond-xmit-hash-policy: "layer3+4"
754 subnets:764 subnets:
755 - type: dhcp6765 - type: dhcp6
756 # A Bond VLAN.766 # A Bond VLAN.

Subscribers

People subscribed via source and target branches