PLUMgrid charm - neutron-iovisor - review required

[Bilal Baqar]

Review is required for neutron-iovisor charm
https://code.launchpad.net/~plumgrid-team/charms/trusty/neutron-iovisor/trunk

[Charles Butler (lazypower)]

Greetings Bilal,

Thank you for your contribution to the Juju Charms ecosystem! It's great to see the fruit of the last few weeks of labor getting the charm prepped for submission. We appreciate the effort that's gone into this initial revision, and I've taken some time to review the charm and have the following notes:

The charm has some feedback when issuing `charm proof` - if you'd like to follow along at home, you can execute `charm proof` in the root of your charms code and receive instant feedback on common gotchyas when reviewing charms. Anything above an I: message is considered needs revision. The items I found were pretty minor and should be a quick fix:

I: missing recommended hook start
W: config.yaml: option install_keys does not have the keys: description
I: config.yaml: option install_keys has no default value
W: config.yaml: option install_sources does not have the keys: description

For reference, I'm including a link to the Charm Store Policy document that I will be citing as we move forward with the review:

https://jujucharms.com/docs/stable/authors-charm-policy

By updating the config.yaml options with a default description, this will clear up those W's.

Additionally it appears you wanted a Null terminated value for the option install_keys' - the null statement left there will render a stringified 'null' when being read by Juju. Simply omitting the value will give you a true null if this was the intention.

The README is pretty sparse with details, and I would like to see it updated with all the keys available from 'charm-tools' command 'charm add readme' - which will render a README.ex file, with all the headers and pointers to expected information.

I see a lot of symlinked hooks that are not implemented in the neutron_iovisor_hooks.py file - while this isn't necessarily a game-breaking pattern, Juju by default will exit 0 when entering a hook context that is not implemented. I feel its worth suggesting to remove the hook symlinks that are not in use so its very clear to any maintainers that follow you which hooks are implemented.

I like the separation of concerns you have made with the neutron_iovisor_utils.py, which is handling the lower level system operations, and the hooks remain high level execution statements. +1 on this pattern and workflow.

I also see the beginnings of unit tests, and functional tests being implemented in this release. This is an excellent start in giving us the insight as to the status of the charm as we move it forward towards the charm store. +1 on this effort, and thank you for the due dilligence in this effort! I know getting started testing was a bit of a pain point, and I encourage you to submit any feedback you have about the experience to the mailing list <email address hidden>

It appears that the primary directive of this charm is to install and load the iovisor kernel module. MTU data is being transmitted on the joined hook - but if the relationship is ever broken, nothing happens to signify that the plugin should reconfigure itself into an 'unconfigured' state - is this intentional behavior?

Thanks again for the submission. I'm going to change status of this bug to "In Progress...

[Cory Johns (johnsca)]

While attempting to deploy this charm on AWS to test plumgrid-director, I got the following error from the install hook: http://pastebin.ubuntu.com/11569118/ It appears the juju-br0 interface is only present when this charm is deployed to lxc. I then attempted to deploy it with --to lxc:0. I'm guessing it successfully installed and changed the MTU, but that caused me to lose connectivity with the machine 0 where it was deployed, which messed up my entire environment since it was the bootstrap node.

It appears this charm is constructed around a very specific topology and substrate. I think the charm should be refactored to allow for non-lxc deployment, and more importantly, to not assume that a MTU larger than 1500 will work. Perhaps the MTU should be a config option? Additionally, any deployment restrictions (this won't work on local provider, it must be deployed with --to lxc) should be noted in the README, as well as warning about potentially dangerous deployment configurations (--to lxc:0 with MTU > 1500 when not supported by the substrate causing lost access to the entire environment).

[James Page (james-page)]

Ignoring any detailed review of the charms for the moment, I'd like to review the high-level design for the PLUMgrid integration into OpenStack.

Specifically neutron-iovisor and neutron-plumgrid-plugin seem very confusing right now.

1) neutron-plugin-plumgrid

The neutron-api charm does not (yet) declare a container scoped relation for configuration of core plugin configuration in a neutron deployment, so the metadata.yaml for neutron-plumgrid-plugin is not valid.

The current way to integrate a plugin into neutron-api is to change that charm directly - one of my team is working on adding a feature to the neutron-api charm via a container scoped interface so plugins + associated config and relations can be pushed out into a sub-oridnate charm - so if we can park that for a few weeks, it would save a-lot of rework right now.

I’d also like to understand whether this charm is doing anything other than configuring the neutron-api service to point at plumgrid-director? Normally the neutron-api service is deployed and scaled in LXC containers - doing anything like loading kernel modules or running networking services is not going to fit well if that is required here.

2) neutron-iovisor

I *think* neutron-iovisor is trying to perform the same type of role as the neutron-openvswitch charm (configuration of L2 networking services on nova-compute nodes) - in which case it needs to be a suboridinate charm and must have the following interface to allow it to deploy and scale with the nova-compute charm:

 provides:
  neutron-plugin:
    interface: neutron-plugin
    scope: container

both of the above charms declare:

provides:
  plumgrid-plugin:
    interface: plumgrid-plugin

which does not look right to me. I think the core interface + charm design might need to look more like this:

<plumgrid-gateway>
  requires:
    plumgrid:
     interface: plumgrid

<plumgrid-edge>
  requires:
    plumgrid:
     interface: plumgrid

<plumgrid-director>
  provides:
    plumgrid:
     interface: plumgrid

you can probably drop the plumgrid-plugin interface - see below

<neutron-api>
   <neutron-plugin-plumgrid> (see note above about this not being supported yet)
 provides:
   plumgrid-plugin:
     interface: plumgrid-plugin

maybe this should be:

  requires:
    plumgrid:
     interface: plumgrid

<nova-compute>
    <neutron-iovisor> (maybe rename this neutron-plumgrid?)

The charm must have the following interface to allow deployment with nova-compute:

  provides:
    neutron-plugin:
      interface: neutron-plugin
      scope: container

maybe this should also have:

  requires:
    plumgrid:
     interface: plumgrid

I would expect the ‘plumgrid’ interface to allow the plumgrid director to pass:

  director_ip
  director_port
  director_admin_username
  director_admin_password

as this seems to be the pertinent information required to configure and access the director - this as then used by all other services to configure/connect.

[Antonio Rosales (arosales)]

Per comment 3 moving the status of this bug to "Incomplete" please move the status of the bug to "Fix Committed" once the fixes are complete and you are ready for a subsequent review.

-thanks,
Antonio

[Bilal Baqar (bbaqar)]

We are adding support for KILO openstack in the charms therefore were going to push a patch after that is done. ETA is this week.

@Charles: That patch will also address each of the points raised in your review.

@Cory: We hadn't tested the charms on an AWS environment at that time. We are doing that right now. To my knowledge juju-br0 was a constant in each environement but looks like thats not the case for AWS. Will change that in the next patch.

This charm can not be deployed inside an lxc as it installs the kernel module "iovisor-dkms" inside the container it is deployed in. I have added this to the "Known Limitations and Issues" section in README now.

As an SDN solution we require the underlying networking of the cloud to support MTU upto 1580, therefore it is a hard requirement from our side. I will add that to the README aswell.

@James on Neutron-plumgrid-plugin charm: Neutron-api charm allows a neutron plugin to declare only one template file in charmhelpers/contrib/openstack/neutron.py whereas PLUMgrid plugin needs to write two files inside the neutron container. I have created a sub-ordinate charm for this and am successfully getting relation data from the neutron-api charm through the container-relation hook. I do not require any change in the neutron-api charm for this to work either.
My aim has been to keep the changes required in the neutron-api charm to bare minimum so that it is easier to get them merged over there. (https://code.launchpad.net/~bbaqar/charms/trusty/neutron-api/next)

The charm is just installing deb packages that are required in the neutron-container to talk to pg-director and writing a file to pass on keystone auth token to get admin data. The kernel module is being installed and loaded in neutron-iovisor charm. Therefore it is totally safe for this charm to scale in lxc containers.

@James on Neutron-iovisor charm: The problem with having neutron-iovisor charm as a subordinate charm is that we need iovisor on nodes other than compute itself. Also our director, edge and gateway charms need to be deployed to the same container as the iovisor charm but once again i can't declare any of them as a subordinate charm as all three of them are deployed in an environment to provide our complete solution. I understand that this doesn't go well with the charms ideology of containerization but its a limitation for us and that is the only we can do it in my knowledge.

[James Page (james-page)]

Hi Bilal

"@James on Neutron-plumgrid-plugin charm: Neutron-api charm allows a neutron plugin to declare only one template file in charmhelpers/contrib/openstack/neutron.py whereas PLUMgrid plugin needs to write two files inside the neutron container. I have created a sub-ordinate charm for this and am successfully getting relation data from the neutron-api charm through the container-relation hook. I do not require any change in the neutron-api charm for this to work either.
My aim has been to keep the changes required in the neutron-api charm to bare minimum so that it is easier to get them merged over there. (https://code.launchpad.net/~bbaqar/charms/trusty/neutron-api/next)"

I was surprised this works - it actually feels like a bug in Juju as the definition in neutron-api is:

  provides:
    neutron-plugin-api:
      interface: neutron-plugin-api

and neutron-plugin-plumgrid declares:

  requires:
    container:
      interface: neutron-plugin-api
      scope: container

I *thought* that both ends of a relation had to have the same scope - but I may be wrong! I'll check with the Juju team.

RE https://code.launchpad.net/~bbaqar/charms/trusty/neutron-api/next - I'd still like to understand why your proposing to provide access to the plumgrid director via configuration and not a relation - as plumgrid-director is charmed, this seems like an obvious thing todo.

"The charm is just installing deb packages that are required in the neutron-container to talk to pg-director and writing a file to pass on keystone auth token to get admin data. The kernel module is being installed and loaded in neutron-iovisor charm. Therefore it is totally safe for this charm to scale in lxc containers."

Great - thanks for confirming.

"@James on Neutron-iovisor charm: The problem with having neutron-iovisor charm as a subordinate charm is that we need iovisor on nodes other than compute itself. Also our director, edge and gateway charms need to be deployed to the same container as the iovisor charm but once again i can't declare any of them as a subordinate charm as all three of them are deployed in an environment to provide our complete solution. I understand that this doesn't go well with the charms ideology of containerization but its a limitation for us and that is the only we can do it in my knowledge."

What's the scope of having this charm deployed by itself? I'd like to understand the use cases.

Do the director, edge and gateway charms get deploy on separate servers normally or do they all reside on a single server.

An understanding of both the logical and physical deployment structure would be useful.

[Bilal Baqar (bbaqar)]

@James

 https://code.launchpad.net/~bbaqar/charms/trusty/neutron-api/next -
> I'd still like to understand why your proposing to provide access to the
> plumgrid director via configuration and not a relation - as plumgrid-
> director is charmed, this seems like an obvious thing todo.

Re: That was my first approach to this but I left it out because for that
we would have to add a relation in the neutron-api charm to get the
virtual_ip from plumgrid-director charm. I doubt that the maintainers of
the charm would let me add that as other plugins aren't either

What's the scope of having this charm deployed by itself? I'd like to
> understand the use cases.

RE: in some of our use cases we require iovisor-dkms on the node only. That
is why we went with this architecture of the charms.

Do the director, edge and gateway charms get deploy on separate servers
> normally or do they all reside on a single server.

They get deployed on seperate servers but require neutron-iovisor charm to
be deployed on the server aswell.

> An understanding of both the logical and physical deployment structure
> would be useful.

 From a physical point of view for a basic openstack deployment deployment
with PLUMgrid we require 3 nodes.

1st node acts as the controller with all base openstack charms deployed
inside lxcs. (mysql, rabbit, keystone, nova-cloud-controller, glance,
openstack-dashboard, cinder, neutron-api)
our neutron-iovisor charm and plumgrid-director charms are deployed on this
node itself. (using the flag: --to "machine number)

2nd node has nova-compute deployed on it. A unit of neutron-iovisor charm
is deployed to the node, along with that plumgrid-edge is deployed on this
node aswell.

3rd node is going to act as our gateway. A unit of neutron-iovisor is added
to this node followed by our plumgrid-gateway charm.
As this charm is going to provide external connectivity, we need to specify
the interface which will provide external-connectivity.

All this information will be available in the README and description of the
charms in the next patch.

On Fri, Jun 26, 2015 at 2:47 PM, James Page <email address hidden> wrote:

> Hi Bilal
>
> "@James on Neutron-plumgrid-plugin charm: Neutron-api charm allows a
> neutron plugin to declare only one template file in
> charmhelpers/contrib/openstack/neutron.py whereas PLUMgrid plugin needs to
> write two files inside the neutron container. I have created a sub-ordinate
> charm for this and am successfully getting relation data from the
> neutron-api charm through the container-relation hook. I do not require any
> change in the neutron-api charm for this to work either.
> My aim has been to keep the changes required in the neutron-api charm to
> bare minimum so that it is easier to get them merged over there. (
> https://code.launchpad.net/~bbaqar/charms/trusty/neutron-api/next)"
>
> I was surprised this works - it actually feels like a bug in Juju as the
> definition in neutron-api is:
>
> provides:
> neutron-plugin-api:
> interface: neutron-plugin-api
>
> and neutron-plugin-plumgrid declares:
>
> requires:
> container:
> interface: neutron-plugin-api
> scope: container
>
>...

[James Page (james-page)]

Hi Bilal

"> I'd still like to understand why your proposing to provide access to the
> plumgrid director via configuration and not a relation - as plumgrid-
> director is charmed, this seems like an obvious thing todo.

Re: That was my first approach to this but I left it out because for that
we would have to add a relation in the neutron-api charm to get the
virtual_ip from plumgrid-director charm. I doubt that the maintainers of
the charm would let me add that as other plugins aren't either"

Actually that would not have been a problem (my team maintains this charm); this will be eased once we move to the subordinate approach we're about to land into neutron-api - your relations would just be found on the subordinate charm only.

">What's the scope of having this charm deployed by itself? I'd like to
> understand the use cases.

RE: in some of our use cases we require iovisor-dkms on the node only. That
is why we went with this architecture of the charms.

Do the director, edge and gateway charms get deploy on separate servers
> normally or do they all reside on a single server.

They get deployed on seperate servers but require neutron-iovisor charm to
be deployed on the server aswell."

OK - so right now you have to explicitly target neutron-iovisor to every other service unit that requires this feature; that's not great from an end user perspective as they have to know todo that.

A subordinate charm will grown alongside its principle, meaning that as soon as its related, juju just takes care of things from that point on - I still think neutron-iovisor falls into this category.

In terms of your standalone requirement, I'd suggest that you switch to a subordinate approach and just deploy neutron-iovisor with the 'ubuntu' charm as the principle charm.

[Bilal Baqar (bbaqar)]

>
> Actually that would not have been a problem (my team maintains this
> charm); this will be eased once we move to the subordinate approach
> we're about to land into neutron-api - your relations would just be
> found on the subordinate charm only.

Well to add all the functionality required to the neutron-api charm, we
will have to add a plumgrid specific template file to your
BASE_RESOURCE_MAP dictionary
inside neutron-api/hooks/neutron_api_utils.py. Along with that I ll have to
add three functions to the same file to get all PLUMgrid related
configuration pushed. In the next patch I am pushing in changes to enable
KILO support aswell, which will require even more changes. Isnt it possible
to let this version of the charm run like this and just as your team comes
up with the new way for plugins I can shift to that directly?

OK - so right now you have to explicitly target neutron-iovisor to
> every other service unit that requires this feature; that's not great
> from an end user perspective as they have to know todo that.

A subordinate charm will grown alongside its principle, meaning that as
> soon as its related, juju just takes care of things from that point on -
> I still think neutron-iovisor falls into this category.

You are definitely right about it not being user friendly. It was our main
concern when we started charming.
The only real problem we have with the subordinate charm approach is that
we *do not* require plumgrid-edge running on a compute node *ONLY*. there
are use cases where our director or gateway is also running on the compute
node, which are going to be impossible to deploy if either one of these
three charms is a subordinate charm of nova-compute.

The problem with creating neutron-iovisor as the subordinate is that I need
iovisor on other nodes aswell. Like my node which has all the base services
running inside lxcs, which i call the controller node. I usually create my
director on that node.

In terms of your standalone requirement, I'd suggest that you switch to
> a subordinate approach and just deploy neutron-iovisor with the 'ubuntu'
> charm as the principle charm.

I have been using the ubuntu charm as the principle charm but If
neutron-iovisor is a subordinate charm of nova-compute, how can I deploy it
on to the ubuntu node?

On Mon, Jun 29, 2015 at 3:59 PM, James Page <email address hidden> wrote:

> Hi Bilal
>
> "> I'd still like to understand why your proposing to provide access to the
> > plumgrid director via configuration and not a relation - as plumgrid-
> > director is charmed, this seems like an obvious thing todo.
>
> Re: That was my first approach to this but I left it out because for that
> we would have to add a relation in the neutron-api charm to get the
> virtual_ip from plumgrid-director charm. I doubt that the maintainers of
> the charm would let me add that as other plugins aren't either"
>
> Actually that would not have been a problem (my team maintains this
> charm); this will be eased once we move to the subordinate approach
> we're about to land into neutron-api - your relations would just be
> found on the subordinate charm only.
>
> ">What's the scope of having this charm de...

[James Page (james-page)]

Bilal

I think you're missing a fundamental feature of subordinate charms - they are not deployed explicitly - for example:

   juju deploy neutron-iovisor

would just deploy the subordinate - no code has been actually run yet, we then do:

   juju deploy nova-compute
   juju deploy plumgrid-director
   juju deploy plumgrid-gateway
   juju deploy plumgrid-edge
   juju add-relation neutron-iovisor nova-compute
   juju add-relation neutron-iovisor plumgrid-director
   juju add-relation neutron-iovisor plumgrid-dateway
   juju add-relation neutron-iovisor plumgrid-edge

which deploys the principle services, and then 'relates' them to the neutron-iovisor subordinate - this causes juju to run a unit of neutron-iovisor on every principle charm unit automatically.

[Bilal Baqar (bbaqar)]

I get what you mean. I am trying to think of a better way. Give me few
hours. Let me get back to you on this.

On Mon, Jun 29, 2015 at 6:00 PM, James Page <email address hidden> wrote:

> Bilal
>
> I think you're missing a fundamental feature of subordinate charms -
> they are not deployed explicitly - for example:
>
> juju deploy neutron-iovisor
>
> would just deploy the subordinate - no code has been actually run yet,
> we then do:
>
> juju deploy nova-compute
> juju deploy plumgrid-director
> juju deploy plumgrid-gateway
> juju deploy plumgrid-edge
> juju add-relation neutron-iovisor nova-compute
> juju add-relation neutron-iovisor plumgrid-director
> juju add-relation neutron-iovisor plumgrid-dateway
> juju add-relation neutron-iovisor plumgrid-edge
>
> which deploys the principle services, and then 'relates' them to the
> neutron-iovisor subordinate - this causes juju to run a unit of neutron-
> iovisor on every principle charm unit automatically.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1459567
>
> Title:
> PLUMgrid charm - neutron-iovisor - review required
>
> Status in Juju Charms:
> Incomplete
>
> Bug description:
> Review is required for neutron-iovisor charm
>
> https://code.launchpad.net/~plumgrid-team/charms/trusty/neutron-iovisor/trunk
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1459567/+subscriptions
>

--
Bilal Baqar
MTS - PLUMgrid Inc.
<http://bit.ly/1I0EhyF>

[Bilal Baqar (bbaqar)]

Hey guys just to get you on the same page as us, let me share our progress.
The reviews pointed out the following problems

1. Charles pointed out that our charms were not in accordance with the juju charms policy. They are basic things like passing charms through charm proof. I have already made those changes and will be pushing them in the next patch.

2. Cory pointed out that our charms were failing on AWS. We had not been able to test our charms on AWS at the time therefore they failing on AWS. We have made the changes required and will the add them in the next patch.

3. James wants us to change the architecture of the charms. Currently our charms are dependent on each other but aren't subordinates therefore we have to ensure that they get deployed on the same node. We have finalized the architecture after discussions with James. Our discussion can be followed above and also on these drawings (https://docs.google.com/drawings/d/1_QVGe-uG-iioqeAtaCfQ6H4E5DAVQYAgD7rLAEkUTnU , https://docs.google.com/drawings/d/1rnsMMt4BaiL1qB2nZHKKYuzrhlhmBw6Iba5CH4OYn5s ). We are currently implementing the proposed changes which will take some time as the changes are significant.

Hopefully we will be able to update the charms in the next week.

[Launchpad Janitor (janitor)]

[Expired for Juju Charms Collection because there has been no activity for 60 days.]