Merge ~raharper/cloud-init:add-netplan-bridge-stp into cloud-init:master

Proposed by Ryan Harper on 2017-10-03
Status: Merged
Approved by: Scott Moser on 2017-10-05
Approved revision: 1f080197c136db43edeb6923cb4c1656123aa31e
Merged at revision: 1f080197c136db43edeb6923cb4c1656123aa31e
Proposed branch: ~raharper/cloud-init:add-netplan-bridge-stp
Merge into: cloud-init:master
Diff against target: 110 lines (+24/-6)
4 files modified
cloudinit/net/eni.py (+3/-0)
cloudinit/net/netplan.py (+3/-2)
cloudinit/net/network_state.py (+15/-2)
tests/unittests/test_net.py (+3/-2)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2017-10-05
Scott Moser 2017-10-03 Approve on 2017-10-05
Review via email: mp+331755@code.launchpad.net

Description of the change

network: bridge_stp value not always correct

Update network_state to store the bridge_stp value as a boolean.
The various renderers then can map the boolean value to the correct
output as needed; eni uses 'on/off', sysconfig uses 'yes/no' and
netplan will use the boolean directly.

Update unittest values for sysconfig and netplan. Both contained
the network_state string value which resulted in not correctly enable/disable
STP in the target system.

Update network_state comment (fd -> forward-delay, add stp as boolean) on
bridge commands to match the expected format of a netplan bridge command.

LP: #1721157

To post a comment you must log in.

PASSED: Continuous integration, rev:b7a9a86e1bd37102388298ee567391c21e42876a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/377/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/377/rebuild

review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

My only comment is whether we have to support 'off' and 'on' rather than just true Boolean values. Wouldn't it be better to centralize that fix/conversion in network State?

Give an answer. Think about it. I am fine to defer to you and/or Chad judgement.

Thanks

review: Approve
Ryan Harper (raharper) wrote :

The right thing to do is establish a boolean in state, and update
netplan/eni/sysconfig to translate the boolean to what's required in the
renderer.

Eni, for example wants 'on' or 'off'
Netplan wants the boolean
AFAICT sysconfig likes the 'on' or 'off' value currently present.

On Tue, Oct 3, 2017 at 8:59 PM, Scott Moser <email address hidden> wrote:

> Review: Approve
>
> My only comment is whether we have to support 'off' and 'on' rather than
> just true Boolean values. Wouldn't it be better to centralize that
> fix/conversion in network State?
>
> Give an answer. Think about it. I am fine to defer to you and/or Chad
> judgement.
>
> Thanks
> --
> https://code.launchpad.net/~raharper/cloud-init/+git/
> cloud-init/+merge/331755
> You are the owner of ~raharper/cloud-init:add-netplan-bridge-stp.
>

PASSED: Continuous integration, rev:1b60f4db30a27502bb8ba6273903346f42194aa5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/383/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/383/rebuild

review: Approve (continuous-integration)
Scott Moser (smoser) :
review: Needs Information
Ryan Harper (raharper) wrote :

Will fix up the one and update.

PASSED: Continuous integration, rev:04216a888b924f8c3edb7aac41d81e5f11fc2b1a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/384/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/384/rebuild

review: Approve (continuous-integration)

PASSED: Continuous integration, rev:e206522d34e3406ef3d738fb2d8f00657aa33d3c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/385/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/385/rebuild

review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

please mention
in commit message 'forward-delay'

Scott Moser (smoser) wrote :

other than that commit message request, looks good.

review: Approve

PASSED: Continuous integration, rev:1f080197c136db43edeb6923cb4c1656123aa31e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/386/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/386/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
2index bb80ec0..c6a71d1 100644
3--- a/cloudinit/net/eni.py
4+++ b/cloudinit/net/eni.py
5@@ -95,6 +95,9 @@ def _iface_add_attrs(iface, index):
6 ignore_map.append('mac_address')
7
8 for key, value in iface.items():
9+ # convert bool to string for eni
10+ if type(value) == bool:
11+ value = 'on' if iface[key] else 'off'
12 if not value or key in ignore_map:
13 continue
14 if key in multiline_keys:
15diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
16index 3b06fbf..d3788af 100644
17--- a/cloudinit/net/netplan.py
18+++ b/cloudinit/net/netplan.py
19@@ -244,9 +244,9 @@ class Renderer(renderer.Renderer):
20
21 for config in network_state.iter_interfaces():
22 ifname = config.get('name')
23- # filter None entries up front so we can do simple if key in dict
24+ # filter None (but not False) entries up front
25 ifcfg = dict((key, value) for (key, value) in config.items()
26- if value)
27+ if value is not None)
28
29 if_type = ifcfg.get('type')
30 if if_type == 'physical':
31@@ -318,6 +318,7 @@ class Renderer(renderer.Renderer):
32 (port, cost) = costval.split()
33 newvalue[port] = int(cost)
34 br_config.update({newname: newvalue})
35+
36 if len(br_config) > 0:
37 bridge.update({'parameters': br_config})
38 _extract_addresses(ifcfg, bridge)
39diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
40index 6faf01b..890dbf8 100644
41--- a/cloudinit/net/network_state.py
42+++ b/cloudinit/net/network_state.py
43@@ -48,6 +48,7 @@ NET_CONFIG_TO_V2 = {
44 'bridge_maxwait': None,
45 'bridge_pathcost': 'path-cost',
46 'bridge_portprio': None,
47+ 'bridge_stp': 'stp',
48 'bridge_waitport': None}}
49
50
51@@ -465,6 +466,18 @@ class NetworkStateInterpreter(object):
52 for param, val in command.get('params', {}).items():
53 iface.update({param: val})
54
55+ # convert value to boolean
56+ bridge_stp = iface.get('bridge_stp')
57+ if bridge_stp and type(bridge_stp) != bool:
58+ if bridge_stp in ['on', '1', 1]:
59+ bridge_stp = True
60+ elif bridge_stp in ['off', '0', 0]:
61+ bridge_stp = False
62+ else:
63+ raise ValueError("Cannot convert bridge_stp value"
64+ "(%s) to boolean", bridge_stp)
65+ iface.update({'bridge_stp': bridge_stp})
66+
67 interfaces.update({iface['name']: iface})
68
69 @ensure_command_keys(['address'])
70@@ -525,8 +538,8 @@ class NetworkStateInterpreter(object):
71 v2_command = {
72 br0: {
73 'interfaces': ['interface0', 'interface1'],
74- 'fd': 0,
75- 'stp': 'off',
76+ 'forward-delay': 0,
77+ 'stp': False,
78 'maxwait': 0,
79 }
80 }
81diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
82index f249615..17c9342 100644
83--- a/tests/unittests/test_net.py
84+++ b/tests/unittests/test_net.py
85@@ -756,6 +756,7 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
86 eth3: 50
87 eth4: 75
88 priority: 22
89+ stp: false
90 routes:
91 - to: ::/0
92 via: 2001:4800:78ff:1b::1
93@@ -820,7 +821,7 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
94 NM_CONTROLLED=no
95 ONBOOT=yes
96 PRIO=22
97- STP=off
98+ STP=no
99 TYPE=Bridge
100 USERCTL=no"""),
101 'ifcfg-eth0': textwrap.dedent("""\
102@@ -1296,7 +1297,7 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
103 NM_CONTROLLED=no
104 ONBOOT=yes
105 PRIO=22
106- STP=off
107+ STP=no
108 TYPE=Bridge
109 USERCTL=no
110 """),

Subscribers

People subscribed via source and target branches