New charm: midonet-gateway

[James Page]

TBD

[Konstantinos Tsakalozos (kos.tsakalozos)]

Hi,

We appreciate the time and effort you have put into this charm. There are a couple of point we would like to see before moving on with it.

- There are no tests included in this charm. Using amulet (https://jujucharms.com/docs/1.25/tools-amulet) you can write tests that deploy this charm (along with any other ones needed) and then test any kind of functionality you deem important. In case the charm is part of a bundle, and testing makes sense only in the context of the bundle, please submit a review request for the bundle as well. Tests are essential for Juju since they guard from future regressions and help in maintaining a good code quality.

- I tried to deploy the charm using the readme but came across two problems a) it was not clear what to put in the configuration.yaml. Would it be possible to provide an example configuration file or smart defaults for a minimum set of values, so that the charm works out of the box? b) There is no description on how to verify that this charm works. After adding the relations how can I see the effects of the charm (eg a webservice is up)?

Minor issue, README typos:
conecpt
config
discoraged
don
expalined

Thank you,
Konstantinos

[Ryo Tagami (ryo-tagami)]

Hi Konstantinos,

Thank you for reviewing the charm. I have updated my charm according to your instructions (revision 4).

> - There are no tests included in this charm. Using amulet (https://jujucharms.com/docs/1.25/tools-amulet) you can write tests that deploy this charm (along with any other ones needed) and then test any kind of functionality you deem important. In case the charm is part of a bundle, and testing makes sense only in the context of the bundle, please submit a review request for the bundle as well. Tests are essential for Juju since they guard from future regressions and help in maintaining a good code quality.

Tests are now included in the charm, under `tests/` and `/lib/amulet/`.
Also, a bundle is now included under `bundles/`. To make manual testing easier, `midonet-gateway` charm is referenced by relative file path. Once this charm is promulgated, I will send a bundle to review with `midonet-gateway` charm referenced by charm store URL.

> - I tried to deploy the charm using the readme but came across two problems a) it was not clear what to put in the configuration.yaml. Would it be possible to provide an example configuration file or smart defaults for a minimum set of values, so that the charm works out of the box? b) There is no description on how to verify that this charm works. After adding the relations how can I see the effects of the charm (eg a webservice is up)?

a) Since default configuration value will work out of the box (to some extent), I have removed the mention of `configuration.yaml`.
b) There are two ways to confirm if this charm works, white box testing approach and black box testing approach.
   To confirm if this charm works by white box tests, you can check the deployed midonet-gateway units' configuration against [https://docs.midonet.org/docs/v2015.06/en/operations-guide/content/static_setup.html] this documentation and confirm that everything mentioned in this documentation is correctly configured in the actual unit.
   To confirm if this charm works by black box tests, you can spin up a VM in the deployed OpenStack environment and test the network connectivity, for example you should be able to reach the internet by curl (provided that midonet-gateway unit has a internet reachability). One thing to bear in mind is that floating IP network reachability is only provided within the midonet-gateway unit itself, so if you want do a test that is destined to the floating IP (for example pinging to the VM's floating IP), those tests should be done from inside the midonet-gateway unit. You can change this behavior and make floating IP reachable from elsewhere, by setting the `static-outgoing-masquerade` option to `False`, however if you do so, you have to have a routing entry in the upstream router (in other words the default gateway of the midonet-gateway unit) to route the floating IP network to the midonet-gateway unit.
   Since it is quite difficult to automate black box tests, and considering the fact that neutron-gateway charm only implements white box tests, amulet tests are completely based on white box testing approach.

> Minor issue, README typos:
> conecpt
> config
> discoraged
> don
> exp...

[Ryo Tagami (ryo-tagami)]

Found out a problem specific to MaaS environment, revision is now 5.

[Cory Johns (johnsca)]

Ryo,

The changes look good, and the tests look very thorough. Also, I think the whitebox test is perfectly acceptable.

I'm not able to run the tests myself, but from review I give this my +1 and defer to James to run the tests and give final sign-off.

[James Page (james-page)]

I'll try get to this this week for testing.

[Ryo Tagami (ryo-tagami)]

Updated functionality, revision is now 6.

[Ryo Tagami (ryo-tagami)]

Fix some flakiness, revision is now 7.

[Ryo Tagami (ryo-tagami)]

Just FYI, Ashley Lai at OIL has kindly tested this charm and succeeded.
I would appreciate it if this charm could get a review.

[Adam Israel (aisrael)]

Hi Ryo,

I've taken a fresh look at the charm. I'm able to run the opensource tests successfully. I don't think the enterprise test should be a fail when the env isn't setup for it. To that end, I've opened an issue [1] against bundletester to allow for tests to be marked as SKIP when one is unable to run. This is not a blocker for this charm, though.

I'm not familiar enough with openstack to fully exercise the deployment outside of the tests, but like Cory, I'm fine with the whitebox approach and your own testing that has passed. You have my +1

In order to get this pushed to the store, we need the ~charmers group to have access to the ~midonet-charmers team. Once we've sorted that out, we'll promulgate this to the charm store.

[1] https://github.com/juju-solutions/bundletester/issues/48

[Adam Israel (aisrael)]

I'm happy to report that the midonet-gateway charm is now promulgated to the Juju charm store! Thanks for all of your time and effort bringing this charm to life.

[Adam Israel (aisrael)]

One note I forgot to mention: The icon.svg included in your charm was unable to render in any browser I tried. I opened it in inkscape with no problems, saved it, and it rendered in the browser. I replaced the icon.svg in the released charm, so you may want to update your source accordingly.