Charm Helpers

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

lp1635067
Merge into devel

Proposed by Matt Rae on 2017-01-14

Status:

Merged

Merged at revision:

706

Proposed branch:

lp://qastaging/~mattrae/charm-helpers/lp1635067

Merge into:

lp://qastaging/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://qastaging/~mattrae/charm-helpers/lp1635067

Medium

In Progress

Link a bug report

Reviewer	Date Requested	Status
Jorge Niedbalski (community)	2017-01-14	Approve on 2017-03-03
Matt Rae (community)		Needs Resubmitting on 2017-02-20
James Page		Needs Fixing on 2017-01-31
Billy Olsen		Needs Fixing on 2017-01-21
Edward Hope-Morley		Pending
Review via email: mp+314759@code.qastaging.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/

Revision history for this message

Jorge Niedbalski (niedbalski) wrote on 2017-01-09: 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 on 2017-01-14: 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 on 2017-01-21:

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://qastaging/~mattrae/charm-helpers/lp1635067 updated on 2017-01-30

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

Revision history for this message

James Page (james-page) on 2017-01-31:

review: Needs Fixing

Revision history for this message

Matt Rae (mattrae) wrote on 2017-02-03:

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

lp://qastaging/~mattrae/charm-helpers/lp1635067 updated on 2017-02-05

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

Revision history for this message

Matt Rae (mattrae) wrote on 2017-02-05:

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 on 2017-02-20:

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 on 2017-02-22:

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>