Merge lp:~mattrae/charm-helpers/lp1635067 into lp:charm-helpers

[Matt Rae]

Adding functions to create an ovs bridge and check if an interface is a linux bridge.

This is needed in charm-helpers in order to add support for using a linuxbridge bridge in data-port config of neutron-gateway and neutron-openvswitch charms, such as 'juju set neutron-openvswitch data-port="br-ex:br-eno1"' where br-eno1 is the bridge created on eno1 by juju by default.

https://review.openstack.org/#/c/392212/

[Jorge Niedbalski (niedbalski)]

LGTM, however I am missing unit tests for the change.

review: Needs Fixing

[Matt Rae (mattrae)]

Thanks Jorge, I added a unit test for add_ovsbridge_linuxbridge. I'm not exactly sure how to add a test for is_linuxbridge_interface, but it might be trival enough that it doesn't need testing. let me know if i should test is_linuxbridge_interface as well.

[Billy Olsen (billy-olsen)]

Thanks Matt! A few comments inline to consider, nothing too major though.

On the unit test front, if you're adding a new method you should have some sort of coverage. I think if you look at my suggestion on the use of glob you'll find the unit test much easier to write. All you'll need to do for that is to verify that it returns True when the bridge test criteria are met and False when its not.

review: Needs Fixing

[lp:~mattrae/charm-helpers/lp1635067]

674. By Matt Rae on 2017-01-27

changing is_linuxbridge_interface to use os.path.exists and adding tests

675. By Matt Rae on 2017-01-28

cleanup formatting

676. By Matt Rae on 2017-01-28

updating add_ovsbridge_linuxbridge to be idempotent

677. By Matt Rae on 2017-01-30

adding levels to log statements

[James Page (james-page)]

review: Needs Fixing

[Matt Rae (mattrae)]

Thanks James, I'll work on how to best make the veth pair persistent. Potentially adding the configuration in /etc/network/interfaces.d/

[lp:~mattrae/charm-helpers/lp1635067]

678. By Matt Rae on 2017-02-05

making veth persistent by adding to interfaces.d

679. By Matt Rae on 2017-02-05

swap names of veth pair to make debugging easier

680. By Matt Rae on 2017-02-05

dropping log level to DEBUG

[Matt Rae (mattrae)]

Thanks James, I updated the code to make the veth pair persistent by adding a file to /etc/network/interfaces.d. Let me know if you have more suggestions.

[Matt Rae (mattrae)]

One thing to note, this change does not delete the veth pair from /etc/network/interfaces.d if data-port is changed. cleanup currently doesn't occur of ovs ports either.

I've started on code that does cleanup if that is necessary but it isn't a trivial change.

review: Needs Resubmitting

[Matt Rae (mattrae)]

I've tested these changes deploying openstack with MAAS using these steps

# check out neutron-openvswitch charm
git clone https://github.com/openstack/charm-neutron-openvswitch.git

# pull in changes from https://review.openstack.org/#/c/392212/
git fetch ssh://<email address hidden>:29418/openstack/charm-neutron-openvswitch refs/changes/12/392212/2 && git cherry-pick -n FETCH_HEAD

# sync charm helpers from lp:~mattrae/charm-helpers/lp1635067, edit charm-helpers-hooks.yaml 'branch: lp:~mattrae/charm-helpers/lp163506'
make sync

# Deploy openstack with the updated charm. Enable DVR to test routers created on compute where neutron-openvswitch charm is deployed. Replace <bridge> in data-port with the bridge created on the host by juju, for example 'data-port: br-ex:br-eno1'

  neutron-api:
    options:
      enable-dvr: true

  neutron-openvswitch:
    options:
      bridge-mappings: physnet1:br-ex
      data-port: br-ex:<bridge>

[Jorge Niedbalski (niedbalski)]

Needs some fixing, please review the comments inline.

review: Needs Fixing

[lp:~mattrae/charm-helpers/lp1635067]

681. By Matt Rae on 2017-03-03

documenting functions and removing unneeded close()

[Jorge Niedbalski (niedbalski)]

LGTM, tests/lint passing, functional test OK.

review: Approve

=== modified file 'charmhelpers/contrib/network/ovs/__init__.py'
--- charmhelpers/contrib/network/ovs/__init__.py	2016-07-06 14:41:05 +0000
+++ charmhelpers/contrib/network/ovs/__init__.py	2017-03-03 22:18:25 +0000
@@ -15,13 +15,26 @@
 ''' Helpers for interacting with OpenvSwitch '''
 import subprocess
 import os
+import netifaces
 from charmhelpers.core.hookenv import (
-    log, WARNING
+    log, WARNING, INFO, DEBUG
 )
 from charmhelpers.core.host import (
     service
 )
 
+BRIDGE_TEMPLATE = """\
+# This veth pair is required when neutron data-port is mapped to an existing linux bridge. lp:1635067
+
+auto {linuxbridge_port}
+iface {linuxbridge_port} inet manual
+    pre-up ip link add name {linuxbridge_port} type veth peer name {ovsbridge_port}
+    pre-up ip link set {ovsbridge_port} master {bridge}
+    pre-up ip link set {ovsbridge_port} up
+    up ip link set {linuxbridge_port} up
+    down ip link del {linuxbridge_port}
+"""
+
 
 def add_bridge(name, datapath_type=None):
     ''' Add the named bridge to openvswitch '''
@@ -60,6 +73,41 @@
     subprocess.check_call(["ip", "link", "set", port, "promisc", "off"])
 
 
+def add_ovsbridge_linuxbridge(name, bridge):
+    ''' Add linux bridge to the named openvswitch bridge
+    :param name: Name of ovs bridge to be added to Linux bridge
+    :param bridge: Name of Linux bridge to be added to ovs bridge
+    :returns: True if veth is added between ovs bridge and linux bridge, False otherwise'''
+
+    ovsbridge_port = "veth-" + name
+    linuxbridge_port = "veth-" + bridge
+    log('Adding linuxbridge {} to ovsbridge {}'.format(bridge, name), level=INFO)
+    interfaces = netifaces.interfaces()
+    for interface in interfaces:
+        if interface == ovsbridge_port or interface == linuxbridge_port:
+            log('Interface {} already exists'.format(interface), level=INFO)
+            return
+
+    with open('/etc/network/interfaces.d/{}.cfg'.format(linuxbridge_port), 'w') as config:
+        config.write(BRIDGE_TEMPLATE.format(linuxbridge_port=linuxbridge_port, ovsbridge_port=ovsbridge_port, bridge=bridge))
+
+    subprocess.check_call(["ifup", linuxbridge_port])
+    add_bridge_port(name, linuxbridge_port)
+
+
+def is_linuxbridge_interface(port):
+    ''' Check if the interface is a linuxbridge bridge
+    :param port: Name of an interface to check whether it is a Linux bridge
+    :returns: True if port is a Linux bridge'''
+
+    if os.path.exists('/sys/class/net/' + port + '/bridge'):
+        log('Interface {} is a Linux bridge'.format(port), level=DEBUG)
+        return True
+    else:
+        log('Interface {} is not a Linux bridge'.format(port), level=DEBUG)
+        return False
+
+
 def set_manager(manager):
     ''' Set the controller for the local openvswitch '''
     log('Setting manager for local ovs to {}'.format(manager))

=== modified file 'tests/contrib/network/test_ovs.py'
--- tests/contrib/network/test_ovs.py	2016-03-23 10:55:08 +0000
+++ tests/contrib/network/test_ovs.py	2017-03-03 22:18:25 +0000
@@ -172,6 +172,26 @@
 
     @patch.object(ovs, 'log')
     @patch('subprocess.check_call')
+    def test_add_ovsbridge_linuxbridge(self, check_call, log):
+        with patch_open() as (mock_open, mock_file):
+            ovs.add_ovsbridge_linuxbridge('br-ex', 'br-eno1')
+
+        self.assertTrue(log.call_count == 2)
+
+    @patch('os.path.exists')
+    def test_is_linuxbridge_interface_false(self, exists):
+        exists.return_value = False
+        result = ovs.is_linuxbridge_interface('eno1')
+        self.assertFalse(result)
+
+    @patch('os.path.exists')
+    def test_is_linuxbridge_interface_true(self, exists):
+        exists.return_value = True
+        result = ovs.is_linuxbridge_interface('eno1')
+        self.assertTrue(result)
+
+    @patch.object(ovs, 'log')
+    @patch('subprocess.check_call')
     def test_set_manager(self, check_call, log):
         ovs.set_manager('manager')
         check_call.assert_called_with(['ovs-vsctl', 'set-manager',