Nuage Canonical Charm: vrsg

[Narinder Gupta]

This charm will provide the gateway functionality in case of Nuage SDN charms. Please review

[Charles Butler (lazypower)]

Greetings Narinder,

Thanks for the submission of the VSRG charm! I've taken some time to review the submission and I have the following notes:

The charm appears to fail automated testing during the install hook due to an apt repository issue. Relevant lines included below:

unit-nuage-vrsg-0[1935]: 2015-11-13 16:38:58 INFO unit.nuage-vrsg/0.install logger.go:40 W: Failed to fetch file:/var/lib/local-archive/VRS/dists/precise/main/binary-amd64/Packages File not found
unit-nuage-vrsg-0[1935]: 2015-11-13 16:38:58 INFO unit.nuage-vrsg/0.install logger.go:40 subprocess.CalledProcessError: Command '['apt-get', 'update']' returned non-zero exit status 100

Looking over the test, it seems very basic, I would like to see slightly more flexing of the installation - such as verifying open ports and other self-validation steps. Additional testing can be deferred to the final bundle such as verfying data sent over the wire to related services and the like.

Looking at the hook files I saw something interesting I haven't seen before, where there are 2 hooks decorated with @hooks.hook()

Without an implicit context defined, it seems that these methods would be executed on *every* hook invocation. Is this desired behavior? I dont think its inherently wrong, but it strikes me as odd that during a relationship exchange we want to inspect and possibly update apt repository settings.

Additionally there is an actions.py file in hooks/ that only performs a hookenv.log statement, this could be moved into hooks.py under the appropriate section thats calling it.

This is a good first round submission, I see a lot of good beginnings in here. With some additional work it will be ready for a charm store submission in no time.

Thanks again for the submission. I'm going to change status of this bug to "in progress" and when you're ready switch status to 'Fix Committed' and someone will be along shortly to review your work.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1442/

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1428/

[Subbarayudu Mukkamala (smukkamala)]

The fixes addresses all the review comments except:

- The automated testing of vrs-g with all values hard-coded is not possible at this point.

[David Ames (thedac)]

Subbu,

As discussed on our call I will put the requested changes as bullet points:

Rather than hooks/seutp.py please use charm helpers sync as you have in other charms

In hooks/services.py and hooks/actions.py there are the beginnings of services framework code. As the rest of the charm does not use services framework please remove these. Actions in particular is a loaded term which has other significance in juju [1]

Consider using jinja2 or cheetah rather than string replacement. At the very least for the files your charm controls.

Consider moving shared muilti-charm code to shared library
 update_config_file
 vrs_full_restart
 config_value_changed

These settings are required but state they are optional in config.yaml and have no defaults. Please document them as required. Please also use status_set and a status of blocked if any required options are not set.
 vrs-repository-url
 access-ports
 vcsd-api-version

Consider replacing config_value_changed(option) with charmhelpers.core.hookenv.config(option).changed

The following code seems very brittle. Can it be addressed in the source for bambou? Or in your PPA?
    if (config_value_changed('vsdk-packages') and (vsdk_pkgs is not None)):
        source = config('vsdk-repository-url')
        if source is None:
            pip_install(vsdk_pkgs)
            log("Changing the file")
            file_path = "/usr/local/lib/python2.7/dist-packages/" \
                        "bambou/nurest_connection.py"
            old_string = "from requests.packages.urllib3.poolmanager" \
                         " import PoolManager"
            new_string = "from urllib3.poolmanager import PoolManager"
            change_in_file(file_path, old_string, new_string)
            log("vspk packages installed via pip")

There is commented code not in use making "if config_value_changed('access-ports')" unneeded.
Line 197 - 205

Please make it clear in documentation that the config value vsc-controller-active will be overridden by the relation to vsc.

Please set status active only after completing the task rather than before. Move to after the restart.
 Line 175
 status_set('active', 'vsc_active_controller: {}'.format(vsc_active_controller))

Please fix access_port vs access_ports. Are you expecting a single port or a list?
 access_port = config('access-ports')

config_changed is redundant in upgrade_charm please remove.

Please remove the debug print statements in hooks/vsd_utils.py
    i.e. print "\n\nHello\n"

Please add to the README exactly how the vsrg charm is deployed and related and which config options are required.

[1] https://jujucharms.com/docs/1.25/actions

Thank you.

[David Ames (thedac)]

Sunny,

Point of clarification, I am looking at the /next branch for this latest review. Please correct me if I should be looking at /trunk. Eventually, /trunk will need to be updated.

> These settings are required but state they are optional in config.yaml and have no defaults. Please document them as required. Please also use status_set and a status of blocked if any required options are not set.
> access-ports
> vcs-api-version

access-ports and vsc-api-version still have no indication they will error out if not set. Please flesh out the documentation on these entries in config.yaml and the README.

> The following code seems very brittle. Can it be addressed in the source for bambou? Or in your PPA?
> if (config_value_changed('vsdk-packages') and (vsdk_pkgs is not None)):
> source = config('vsdk-repository-url')
> if source is None:
> pip_install(vsdk_pkgs)
> log("Changing the file")
> file_path = "/usr/local/lib/python2.7/dist-packages/" \
> "bambou/nurest_connection.py"
> old_string = "from requests.packages.urllib3.poolmanager" \
> " import PoolManager"
> new_string = "from urllib3.poolmanager import PoolManager"
> change_in_file(file_path, old_string, new_string)
> log("vspk packages installed via pip")

This is going to be a blocker for inclusion into the charm store. This may need to be fixed at the package level.

Related to this. Please add documentation for vsdk-repository-url and make it clear in the README the options for how VSDK gets installed.

> Please make it clear in documentation that the config value vsc-controller-active will be overridden by the relation to vsc.

In general the README is currently just restating the config.yaml entries. I would still like to see more information in config.yaml in the description for several config options *and* a more descriptive explanation in the README rather than a simple restatement.

> Please remove the debug print statements in hooks/vsd_utils.py
> i.e. print "\n\nHello\n"

Still several print statements in this file.

> Please add to the README exactly how the vsrg charm is deployed and related and which config options are required.

Again, it is not clear enough which config parameters are required (and will lead to Exceptions being thrown if not set) for a functional deployment. This needs to be spelled out clearly.

Unit tests are failing
make lint
make test

[Sunny Verma (sunny-verma)]

Hi David,

I have merged this branch with next.
Please review.

[David Ames (thedac)]

The Nuage VRSG charm has a +1 from the OpenStack Charmers and should be included in the charm store.