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

Proposed by Matt Rae
Status: Merged
Merged at revision: 706
Proposed branch: lp:~mattrae/charm-helpers/lp1635067
Merge into: lp:charm-helpers
Diff against target: 104 lines (+69/-1)
2 files modified
charmhelpers/contrib/network/ovs/__init__.py (+49/-1)
tests/contrib/network/test_ovs.py (+20/-0)
To merge this branch: bzr merge lp:~mattrae/charm-helpers/lp1635067
Reviewer Review Type Date Requested Status
Jorge Niedbalski (community) Approve
Matt Rae (community) Needs Resubmitting
James Page Needs Fixing
Billy Olsen Needs Fixing
Edward Hope-Morley Pending
Review via email: mp+314759@code.launchpad.net

This proposal supersedes a proposal from 2017-01-09.

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Jorge Niedbalski (niedbalski) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
Revision history for this message
Matt Rae (mattrae) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Billy Olsen (billy-olsen) wrote :

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 updated
674. By Matt Rae

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

675. By Matt Rae

cleanup formatting

676. By Matt Rae

updating add_ovsbridge_linuxbridge to be idempotent

677. By Matt Rae

adding levels to log statements

Revision history for this message
James Page (james-page) :
review: Needs Fixing
Revision history for this message
Matt Rae (mattrae) wrote :

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 updated
678. By Matt Rae

making veth persistent by adding to interfaces.d

679. By Matt Rae

swap names of veth pair to make debugging easier

680. By Matt Rae

dropping log level to DEBUG

Revision history for this message
Matt Rae (mattrae) wrote :

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.

Revision history for this message
Matt Rae (mattrae) wrote :

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
Revision history for this message
Matt Rae (mattrae) wrote :

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>

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

Needs some fixing, please review the comments inline.

review: Needs Fixing
lp:~mattrae/charm-helpers/lp1635067 updated
681. By Matt Rae

documenting functions and removing unneeded close()

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

LGTM, tests/lint passing, functional test OK.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/network/ovs/__init__.py'
2--- charmhelpers/contrib/network/ovs/__init__.py 2016-07-06 14:41:05 +0000
3+++ charmhelpers/contrib/network/ovs/__init__.py 2017-03-03 22:18:25 +0000
4@@ -15,13 +15,26 @@
5 ''' Helpers for interacting with OpenvSwitch '''
6 import subprocess
7 import os
8+import netifaces
9 from charmhelpers.core.hookenv import (
10- log, WARNING
11+ log, WARNING, INFO, DEBUG
12 )
13 from charmhelpers.core.host import (
14 service
15 )
16
17+BRIDGE_TEMPLATE = """\
18+# This veth pair is required when neutron data-port is mapped to an existing linux bridge. lp:1635067
19+
20+auto {linuxbridge_port}
21+iface {linuxbridge_port} inet manual
22+ pre-up ip link add name {linuxbridge_port} type veth peer name {ovsbridge_port}
23+ pre-up ip link set {ovsbridge_port} master {bridge}
24+ pre-up ip link set {ovsbridge_port} up
25+ up ip link set {linuxbridge_port} up
26+ down ip link del {linuxbridge_port}
27+"""
28+
29
30 def add_bridge(name, datapath_type=None):
31 ''' Add the named bridge to openvswitch '''
32@@ -60,6 +73,41 @@
33 subprocess.check_call(["ip", "link", "set", port, "promisc", "off"])
34
35
36+def add_ovsbridge_linuxbridge(name, bridge):
37+ ''' Add linux bridge to the named openvswitch bridge
38+ :param name: Name of ovs bridge to be added to Linux bridge
39+ :param bridge: Name of Linux bridge to be added to ovs bridge
40+ :returns: True if veth is added between ovs bridge and linux bridge, False otherwise'''
41+
42+ ovsbridge_port = "veth-" + name
43+ linuxbridge_port = "veth-" + bridge
44+ log('Adding linuxbridge {} to ovsbridge {}'.format(bridge, name), level=INFO)
45+ interfaces = netifaces.interfaces()
46+ for interface in interfaces:
47+ if interface == ovsbridge_port or interface == linuxbridge_port:
48+ log('Interface {} already exists'.format(interface), level=INFO)
49+ return
50+
51+ with open('/etc/network/interfaces.d/{}.cfg'.format(linuxbridge_port), 'w') as config:
52+ config.write(BRIDGE_TEMPLATE.format(linuxbridge_port=linuxbridge_port, ovsbridge_port=ovsbridge_port, bridge=bridge))
53+
54+ subprocess.check_call(["ifup", linuxbridge_port])
55+ add_bridge_port(name, linuxbridge_port)
56+
57+
58+def is_linuxbridge_interface(port):
59+ ''' Check if the interface is a linuxbridge bridge
60+ :param port: Name of an interface to check whether it is a Linux bridge
61+ :returns: True if port is a Linux bridge'''
62+
63+ if os.path.exists('/sys/class/net/' + port + '/bridge'):
64+ log('Interface {} is a Linux bridge'.format(port), level=DEBUG)
65+ return True
66+ else:
67+ log('Interface {} is not a Linux bridge'.format(port), level=DEBUG)
68+ return False
69+
70+
71 def set_manager(manager):
72 ''' Set the controller for the local openvswitch '''
73 log('Setting manager for local ovs to {}'.format(manager))
74
75=== modified file 'tests/contrib/network/test_ovs.py'
76--- tests/contrib/network/test_ovs.py 2016-03-23 10:55:08 +0000
77+++ tests/contrib/network/test_ovs.py 2017-03-03 22:18:25 +0000
78@@ -172,6 +172,26 @@
79
80 @patch.object(ovs, 'log')
81 @patch('subprocess.check_call')
82+ def test_add_ovsbridge_linuxbridge(self, check_call, log):
83+ with patch_open() as (mock_open, mock_file):
84+ ovs.add_ovsbridge_linuxbridge('br-ex', 'br-eno1')
85+
86+ self.assertTrue(log.call_count == 2)
87+
88+ @patch('os.path.exists')
89+ def test_is_linuxbridge_interface_false(self, exists):
90+ exists.return_value = False
91+ result = ovs.is_linuxbridge_interface('eno1')
92+ self.assertFalse(result)
93+
94+ @patch('os.path.exists')
95+ def test_is_linuxbridge_interface_true(self, exists):
96+ exists.return_value = True
97+ result = ovs.is_linuxbridge_interface('eno1')
98+ self.assertTrue(result)
99+
100+ @patch.object(ovs, 'log')
101+ @patch('subprocess.check_call')
102 def test_set_manager(self, check_call, log):
103 ovs.set_manager('manager')
104 check_call.assert_called_with(['ovs-vsctl', 'set-manager',

Subscribers

People subscribed via source and target branches