Support for Calico in OpenStack with Juju Charms

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-11038-results

Greetings, Cory. Thank you for this high-quality charm submission.

The URL for the test failure results is broken, and the correct URL is: http://reports.vapour.ws/charm-test-details/charm-bundle-test-11038-results (note that the test results is only for neutron-calico)

I notice that you have already resolved the lint failures for neutron-calico, however I'm still seeing `make test` fail due to missing dependencies. I believe the test target should also install the following required packages:

  * python-nose
  * python-mock
  * python-netaddr
  * python-netifaces

With those additions, all of the tests passed for me for this charm. The code of the charm also looks good and there are no other issues that I can see, and everything deploys fine per the instructions in the README. However, the README doesn't include any instructions on how to interact with the deployed charm to confirm that it is functioning correctly. That said, once the test target is fixed, I would give the neutron-calico portion of this review a +1.

Cory,

Thanks for the review feedback. I've just pushed r45 to neutron-calico that adds an apt-get install of the testing dependencies. This obviously places a requirement on the user running 'make unit_test' to have the ability to apt-get install packages. I don't love this (there's no reason for the unit test to require root except for that), but unless you have a different proposal it'll do for now.

Cory,

Creating a Python virtualenv is an option, but historically we've had issues getting netaddr and netifaces to install successfully in an virtualenv due to compilation issues, which may hinge on dependencies which would need to be apt-get installed anyway. However, it may be worth investigating, as I agree that requiring sudo power for the test setup is less than ideal.

One thing that I definitely would recommend, though, is splitting the `apt-get install` portion into its own make target, so that it can at least be run with sudo separately from the tests themselves.

Cory,

I forgot to mention, the testdep target (or whatever you call it) should be a dependent for the test target and should include the sudo, for the purposes of the automated test runner. The test runner does have at least sudo powers, as the `sudo apt-get install` pattern is (unfortunately) fairly common in the 00-setup case for Amulet / functional tests.

Hey Cory - I'd like to echo johnsca's sentiment and say thanks for this submission! I look forward to beefing up the Openstack offering with the charms you've created.

To the question you posed in the description, it would indeed be beneficial to submit bird and calico-acl-manager individually. The automated tester is only processing your first charm (neutron-calico).. We'd like to see bird and calico-acl-manager get picked up as well.

The same comment about neutron-calico's Makefile installing test dependencies applies to bird and calico-acl-manager as well. The good news is that once deps are installed, both bird and calico-acl-manager look good from a unit-test perspective. The bad news is that the evil linter has bitten bird:

$ make lint
hooks/setup.py:16:9: F401 'netifaces' imported but unused
hooks/setup.py:21:9: F401 'netaddr' imported but unused
make: *** [lint] Error 1

Something similar to what you did for neutron-calico's lint failures should work for bird too. Once these minor issues are handled, I'd happily +1 these charms. When ready, please do submit bird and calico-acl-manager individually. Thanks!

Cory, I've made those changes and pushed them up to neutron-calico.

Kevin, I've made the same set of changes to calico-acl-manager and bird, along with fixing up the linter problems. I then created new bugs for both bird[0] and calico-acl-manager[1].

Thank you both for your feedback! If you have more feedback, please let me know.

[0]: https://bugs.launchpad.net/charms/+bug/1431445
[1]: https://bugs.launchpad.net/charms/+bug/1431443

Hey Cory - thanks for splitting the bird/acl-manager charms into separate proposals. Deploying/relating per the README worked, and unit tests look good now that you've added a testdep target. Excellent!

That said, we have a Charm Authors best practice [0] regarding immutable configuration. For neutron-calico, this comes into play with the 'calico-origin' config option. Your description for that option is clear ("this must be set before the 'install' hook runs"), but a user might not read this (gasp!) and assume that setting a new calico-origin ppa would re-install the deployed unit. This doesn't happen currently, which might lead to confusion and a poor user experience.

While having immutable config in the charm is not a strict deal-breaker for recommended status, I would like to ask your thoughts on removing calico-origin as a config option (with the default ppa hard coded and perhaps a blurb in the README telling users how to set their own by customizing a local copy of your charm). This would align your charm with our best practices, but may not be a good alternative if you foresee users depending on this option during initial deployment. Another alternative would be to clean up and re-run install during config-changed, but that feels like a bigger can of worms than removing the option.

Let me know what you think, and again, thanks for the work here!

[0] https://jujucharms.com/docs/1.20/authors-charm-best-practice, paragraph starting with "The configuration should not be immutable."

Removing calico-origin is easy enough, though it feels a bit unfortunate. The better thing to do, as you say, would be to re-run install during config-changed. That should be do-able (there's a fixed set of packages that the calico-origin provides, so we can hardcode them and simply force a reinstall). It'll lead to a fair bit of code churn because I'lll have to re-add every file that needs updating.

The alternative would be a complex script that scans through all installed packages to see if they came from calico-origin and then reinstall all of those: that would be complete, but much more open to bugs and errors than simply hard-coding the list.

Do you have a preference, Kevin?

Per comment 9, putting this bug back in New to get this charm back into the review queue.

Charm Reviewer,
We need to resolve review points listed in comment 8 and 9.

-thanks,
Antonio

Hi Cory - huge apology here! I didn't notice your open question to me back in comment 9 :(

My preference is to axe the calico-origin for the time being, though I believe you superseded your ppa with this one, right?

https://launchpad.net/~project-calico/+archive/ubuntu/icehouse

I'd be fine with updating the default_source ppa to that and removing the config option... Mainly because I don't think changing the ppa (meaning reinstall) post deployment would be all that desirable. That's just my gut feeling though, and I'll defer to your gut if I'm downplaying the advantage of a dynamic ppa too much here.

Cory, I'm moving this bug to "In Progress" awaiting your thoughts on removing the ppa config opt. Regardless of which way you come down on that, I'm fairly sure the default_source should be set to "ppa:project-calico/icehouse" in neutron_calico_utils.py.

Please move this bug back to New or Fix Committed when you'd like a re-review (those are the only 2 bug status settings that will send this back into the review queue). Thanks again for the submission, and I apologize again for missing the open question earlier!

Hi Kevin, no worries! That's a bit my fault for not resetting the bug status, so there's plenty of blame to go around. ;)

So the key purpose of calico-origin is to ensure that if you are, say, deploying OpenStack Kilo we use the Kilo PPA (ppa:project-calico/kilo) rather than the Icehouse PPA. This unfortunate situation is because we have some patches to Nova/Neutron that we need to deploy, and for obvious reasons it's better to do that using the PPA system than to do some terrible install-time patch logic. In the *long* term this need should go away (we've landed our Nova patches into Liberty, but our Neutron patches are not yet integrated upstream), but for now we do need to know what OpenStack release we're going into.

If there's a good central location we can find that information on (e.g. over the relation with neutron-api) I'd be happy to change the charm to use that relation. Otherwise, I think the neutron-calico charm needs *some* information.

Either way, I think for the short term the answer is to, on config-changed, do an apt-add-repository and then apt-upgrade. That should ensure that we pick up any change the user performs to do *upgrade*, at the very least.

Hey Cory, I'm not versed enough in the neutron relations to know if you can programmatically determine the openstack release, so i'll leave that to an openstack charmer for comment.

The short term plan of add-repo/apt-upgrade on config-changed seems ok to me on the surface.. but consider everything I know about neutron-calico was learned from your readme ;) My only objection to having the calico-origin config option was that it didn't do anything on config-changed. As long as the service handles that option being changed post-deployment, I'm happy.

Cory/Kevin

In other OpenStack charms we've used a change in the source to detect and then trigger upgrades between openstack releases; the upgrade only gets triggered if a clear new version is detected (say icehouse -> juno), not just a switch in source for new packages (no auto-package upgrade for smaller deltas).

It would be nice if this charm did the same; but I'd be happy to see that follow as a feature post acceptance into the charm store.

ACK on that James, I think it'd be a reasonable behaviour.

Ok, this has been updated: can I get this re-reviewed?

Hey Cory, I just tried to deploy this charm, but got a fast failure upon "juju add-relation neutron-calico nova-compute". I'll attach the output shortly, but I wanted to confirm a couple things. Here are the charms revs:

cs:trusty/nova-compute-26
cs:~project-calico/trusty/neutron-calico-2

Are these the correct charms? I specifically wanted to confirm i should be deploying calico from ~project-calico and not ~cory-benfield.

The readme also specifies cs:~cory-benfield/etcd. Do you have plans to merge your etcd into the recommended version (https://jujucharms.com/etcd/)? I pulled both down, and they look significantly different. I'm curious if we need two different etcd charms, or if your calico charm will work with the promulgated version of etcd.

Thanks for the continued work on these ~project-calico charms, and especially for your patience through the review process. It's much appreciated!

Steps to reproduce:

juju deploy nova-compute
juju deploy cs:~project-calico/neutron-calico
juju add-relation neutron-calico nova-compute

Ah, yeah, looks like the most recent charmhelpers update caused us a problem. I'll see if I can fix that real quick.

Kevin,

Your juju deploy failure should be fixed now: the charm store will update shortly with the new charm release.

You should be deploying from ~project-calico, we moved the location of the source trees. The ~cory-benfield branches are no longer updated.

The readme was actually pointing to altogether the wrong charm, so I've updated that. Thanks for spotting it!

Cory

I've landed the neutron-api changes into the next branch - this will release alongside Ubuntu 15.10 in October.

nova-cc and nova-compute changes had already been landed (just to confirm)

Thanks James - I've taken over driving this from Cory.

How are we doing with the neutron-calico and bird charms? I'm not sure what the process for getting these 'community recommended' looks like - does that also have to happen alongside the 15.10 release? Is there some staging area similar to the next branches?

One other thing I was wondering about are the plans for Liberty updates to the charms. Is this going to be released alongside 15.10 as well, or will it be later?

Hi Matt

Nice to have you onboard!

All of the general changes to support Calico in the core OpenStack charms have been landed and released as of two weeks ago - that includes support for OpenStack Liberty generally, but not specifically for Calico.

Matt, the process for getting calico and bird as community recommended is
documented in

https://jujucharms.com/docs/devel/authors-charm-store#recommended-charms

Mark Wenning
Technical Partner Manager, Cloud Alliances
Canonical, Ltd
<email address hidden>
-----
http://www.brepettis.com/blog/2009/3/3/the-cult-of-done-manifesto.html

On Tue, Sep 29, 2015 at 6:17 AM, Matt Dupre <email address hidden>
wrote:

> Thanks James - I've taken over driving this from Cory.
>
> How are we doing with the neutron-calico and bird charms? I'm not sure
> what the process for getting these 'community recommended' looks like -
> does that also have to happen alongside the 15.10 release? Is there
> some staging area similar to the next branches?
>
> One other thing I was wondering about are the plans for Liberty updates
> to the charms. Is this going to be released alongside 15.10 as well,
> or will it be later?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1421230
>
> Title:
> Support for Calico in OpenStack with Juju Charms
>
> Status in Juju Charms Collection:
> New
>
> Bug description:
> This primarily adds the neutron-calico charm, which provides Juju
> charms support for Project Calico[0], an L3-only network
> virtualization tool. Patches have been submitted to neutron-api[1] and
> other existing charms to add support. Two associated charms, calico-
> acl-manager and BIRD, are being submitted for recommended status as
> well.
>
> If it would be beneficial for me to submit any of these three charms
> individually, please let me know.
>
> [0]: http://www.projectcalico.org/
> [1]:
> https://code.launchpad.net/~cory-benfield/charms/trusty/neutron-api/trunk/+merge/246993
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1421230/+subscriptions
>

Matt,

Apologies for the review delay on the bird and neutron-calico charms. I have a MP for the neutron-calico charm at https://code.launchpad.net/~johnsca/charms/trusty/neutron-calico/review/+merge/276889 to sort out some minor issues I ran into deploying it according to the README, running the tests, and running `charm proof. If you're ok with those small changes, I think this should be ready to be promoted.

Matt,

Ping on this as well. As with the bird charm, if my suggested changes look good, please merge them into this MP and I will proceed on promoting these charms.

Thanks!

Cory,

Sorry for the delay. I've merged your changes, so it would be great if you go go ahead and promote this. Please let me know if there's anything else I need to do.

Thanks,
Matt

Matt and Cory,

Thank you for your work on this charm! It has now been promulgated into the recommended namespace of the charm store, and should be available at https://jujucharms.com/neutron-calico/ within the hour, at which point it can be deployed with just:

    juju deploy neutron-calico

We, the community, very much appreciate the effort you've put in to adding these charms to the ecosystem, and look forward to your continued development and additions.