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

[Ryan Harper]

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

[Server Team CI bot (server-team-bot)]

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)]

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)]

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

[Server Team CI bot (server-team-bot)]

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)]

Will fix up the one and update.

[Server Team CI bot (server-team-bot)]

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)

[Server Team CI bot (server-team-bot)]

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)]

please mention
in commit message 'forward-delay'

[Scott Moser (smoser)]

other than that commit message request, looks good.

review: Approve

[Server Team CI bot (server-team-bot)]

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)

diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index bb80ec0..c6a71d1 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -95,6 +95,9 @@ def _iface_add_attrs(iface, index):
         ignore_map.append('mac_address')
 
     for key, value in iface.items():
+        # convert bool to string for eni
+        if type(value) == bool:
+            value = 'on' if iface[key] else 'off'
         if not value or key in ignore_map:
             continue
         if key in multiline_keys:
diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
index 3b06fbf..d3788af 100644
--- a/cloudinit/net/netplan.py
+++ b/cloudinit/net/netplan.py
@@ -244,9 +244,9 @@ class Renderer(renderer.Renderer):
 
         for config in network_state.iter_interfaces():
             ifname = config.get('name')
-            # filter None entries up front so we can do simple if key in dict
+            # filter None (but not False) entries up front
             ifcfg = dict((key, value) for (key, value) in config.items()
-                         if value)
+                         if value is not None)
 
             if_type = ifcfg.get('type')
             if if_type == 'physical':
@@ -318,6 +318,7 @@ class Renderer(renderer.Renderer):
                             (port, cost) = costval.split()
                             newvalue[port] = int(cost)
                         br_config.update({newname: newvalue})
+
                 if len(br_config) > 0:
                     bridge.update({'parameters': br_config})
                 _extract_addresses(ifcfg, bridge)
diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
index 6faf01b..890dbf8 100644
--- a/cloudinit/net/network_state.py
+++ b/cloudinit/net/network_state.py
@@ -48,6 +48,7 @@ NET_CONFIG_TO_V2 = {
                'bridge_maxwait': None,
                'bridge_pathcost': 'path-cost',
                'bridge_portprio': None,
+               'bridge_stp': 'stp',
                'bridge_waitport': None}}
 
 
@@ -465,6 +466,18 @@ class NetworkStateInterpreter(object):
         for param, val in command.get('params', {}).items():
             iface.update({param: val})
 
+        # convert value to boolean
+        bridge_stp = iface.get('bridge_stp')
+        if bridge_stp and type(bridge_stp) != bool:
+            if bridge_stp in ['on', '1', 1]:
+                bridge_stp = True
+            elif bridge_stp in ['off', '0', 0]:
+                bridge_stp = False
+            else:
+                raise ValueError("Cannot convert bridge_stp value"
+                                 "(%s) to boolean", bridge_stp)
+            iface.update({'bridge_stp': bridge_stp})
+
         interfaces.update({iface['name']: iface})
 
     @ensure_command_keys(['address'])
@@ -525,8 +538,8 @@ class NetworkStateInterpreter(object):
         v2_command = {
           br0: {
             'interfaces': ['interface0', 'interface1'],
-            'fd': 0,
-            'stp': 'off',
+            'forward-delay': 0,
+            'stp': False,
             'maxwait': 0,
           }
         }
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index f249615..17c9342 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -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
                                 eth3: 50
                                 eth4: 75
                             priority: 22
+                            stp: false
                         routes:
                         -   to: ::/0
                             via: 2001:4800:78ff:1b::1
@@ -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
                 NM_CONTROLLED=no
                 ONBOOT=yes
                 PRIO=22
-                STP=off
+                STP=no
                 TYPE=Bridge
                 USERCTL=no"""),
             'ifcfg-eth0': textwrap.dedent("""\
@@ -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
                 NM_CONTROLLED=no
                 ONBOOT=yes
                 PRIO=22
-                STP=off
+                STP=no
                 TYPE=Bridge
                 USERCTL=no
                 """),