Nuage-Canonical charm: nuage-vrs

[Marco Ceppi]

Please review

[Review Queue (review-queue)]

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

[Review Queue (review-queue)]

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

[Adam Israel (aisrael)]

Hi Subbarayudu,

Thanks for all of your work putting this charm together for review. I had the opportunity to review it today and wanted to give you some (non-OpenStack-specific) feedback.

There a few common things that new charm authors (myself included) make, that will cause our automated testing to fail. The good news is that these are relatively easy to fix.

[1] `charm proof` - This tool runs a sort of lint against the charm's structure, including validation of `metadata.yaml` and `config.yaml`.

    W: Maintainer format should be "Name <Email>", not "Subbarayudu"
    I: Categories are being deprecated in favor of tags. Please rename the "categories" field to "tags".
    W: README.md includes line 6 of boilerplate README.ex
    W: config.yaml: option vsc-controller-active does not have the keys: default
    W: config.yaml: option vrs-repository-url does not have the keys: default
    W: config.yaml: option vsc-controller-standby does not have the keys: default
    W: config.yaml: option vrs-config-file does not have the keys: default

I's are informational; they're meant to inform you that something should be changed, but isn't against the current charm store policy, nor is it expected to break the charm.

W's mean that a [2] charm store policy is in violation. These are generally easy to fix.

[1] https://juju.ubuntu.com/docs/tools-charm-tools.html

[2] https://juju.ubuntu.com/docs/authors-charm-policy.html

Secondly, `make lint` runs flake8 against the python hooks. This helps to enforce best practices.

    hooks/hooks.py:5:1: F401 'local_unit' imported but unused
    hooks/hooks.py:22:12: E401 multiple imports on one line
    hooks/hooks.py:33:1: E302 expected 2 blank lines, found 1
    hooks/hooks.py:64:1: E302 expected 2 blank lines, found 1
    hooks/hooks.py:97:5: E265 block comment should start with '# '
    hooks/hooks.py:100:42: E225 missing whitespace around operator
    hooks/hooks.py:101:39: E231 missing whitespace after ','
    hooks/hooks.py:101:59: E231 missing whitespace after ','
    hooks/hooks.py:101:80: E501 line too long (86 > 79 characters)
    hooks/hooks.py:104:43: E225 missing whitespace around operator
    hooks/hooks.py:105:39: E231 missing whitespace after ','
    hooks/hooks.py:105:60: E231 missing whitespace after ','
    hooks/hooks.py:105:80: E501 line too long (88 > 79 characters)
    hooks/hooks.py:106:1: W293 blank line contains whitespace
    hooks/hooks.py:109:1: E302 expected 2 blank lines, found 1
    hooks/hooks.py:115:42: E225 missing whitespace around operator
    hooks/hooks.py:117:5: E265 block comment should start with '# '
    hooks/hooks.py:118:39: E231 missing whitespace after ','
    hooks/hooks.py:118:59: E231 missing whitespace after ','
    hooks/hooks.py:121:1: E302 expected 2 blank lines, found 1
    make: *** [lint] Error 1

Last, I'd love to see some [3] tests that our test environment could run, to stand up and configure the environment and related services, so functional tests could be run against the charm, though I understand their may be challenges with that due to the binary payload.

Thanks again for all of your work so far. If you have any questions/comments/concerns ab...

[Marco Ceppi (marcoceppi)]

Thanks for the work so far, I'm moving this to incomplete, when you're ready for another review please move this bug status to "Fix Committed" to have it appear in the queue again.

[David Ames (thedac)]

Hello, Subbu,

Thank you again for this submission. Having the Nuage services in the charmstore will be great.

For the VRS charm the big issue I see is that the VRS service is hard stopped and restarted on each config-changed hook run. Config-changed hooks get run often, sometimes at times you will not expect. For a low level network service like VRS we don't want to see it stopped unless it is absolutely necessary as it will presumably cause loss of connectivity.

So we will want to gate the restart based on configuration changes. Only restart if vrs_config_file changes or some other event that requires a service restart. You can check the md5 sum of the configuration file(s) at the beginning of the config-changed hook and compare at the end to decide if a restart is required.

Gate the following:
service_stop('nuage-openvswitch-switch')
vrs_full_restart()
Can be based on changes to vrs_config_file.

The charm has settings for vsc-controller-active and vsc-controller-standby as config options. Is there are reason VRS can't acquire vsc-controller-active and vsc-controller-standby from the relationship with nuage-vsc with the vrs-controller-service relation?
If there is a reason, please document the requirement that these config options be set in the README and in the description field of the config.yaml file.

As Adam mentioned we need to see some kind of tests. [1] Preferably these are functional tests that we can CI validate. Unit tests are required as a minimum.

[1] https://juju.ubuntu.com/docs/authors-testing.html

[Subbarayudu Mukkamala (smukkamala)]

Hi david

Thank you for the review.

The VRS service must be stopped for any of the following configuration
changes and not just changes to vrs_config_file. If you say that
Config-changed would called for other reasons as well the can you
suggest what can be done (Ideally detect stop the service of any of
the parameters in config.yaml had changed, but how to do it?).

And vsc-controller-active,vsc-controller-standby can be either
configured by the user (which vsc is not deployed using charms) or
could be learnt through relation hooks.

Thanks

Subbu

   vsc-controller-active:

    type: string
    description: Active VRS controller to use.
    default:
  vsc-controller-standby:
    type: string
    description: Standby VRS controller to use.
    default:
  vrs-packages:
    type: string
    default: "nuage-metadata-agent nuage-openvswitch-common
nuage-openvswitch-datapath-dkms nuage-python-openvswitch
nuage-openvswitch-switch"
    description: List of packages to install for VRS.
  vrs-repository-url:
    type: string
    description: Optional URL to Nuage VRS repository containing
Debian packages.
    default:
  vrs-ppa-key:
    type: string
    description: Optional KEY to Nuage VRS PPA containing Debian packages.
    default:

Regards
Subbu

On Fri, Oct 30, 2015 at 2:26 PM, David Ames <email address hidden>
wrote:

> Hello, Subbu,
>
> Thank you again for this submission. Having the Nuage services in the
> charmstore will be great.
>
> For the VRS charm the big issue I see is that the VRS service is hard
> stopped and restarted on each config-changed hook run. Config-changed
> hooks get run often, sometimes at times you will not expect. For a low
> level network service like VRS we don't want to see it stopped unless it
> is absolutely necessary as it will presumably cause loss of
> connectivity.
>
> So we will want to gate the restart based on configuration changes. Only
> restart if vrs_config_file changes or some other event that requires a
> service restart. You can check the md5 sum of the configuration file(s)
> at the beginning of the config-changed hook and compare at the end to
> decide if a restart is required.
>
> Gate the following:
> service_stop('nuage-openvswitch-switch')
> vrs_full_restart()
> Can be based on changes to vrs_config_file.
>
>
> The charm has settings for vsc-controller-active and
> vsc-controller-standby as config options. Is there are reason VRS can't
> acquire vsc-controller-active and vsc-controller-standby from the
> relationship with nuage-vsc with the vrs-controller-service relation?
> If there is a reason, please document the requirement that these config
> options be set in the README and in the description field of the
> config.yaml file.
>
>
> As Adam mentioned we need to see some kind of tests. [1] Preferably these
> are functional tests that we can CI validate. Unit tests are required as a
> minimum.
>
> [1] https://juju.ubuntu.com/docs/authors-testing.html
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/1420995
>
> Title:
> Nuage-Canonical charm: nuage-vrs
>
> Status in Juju Charms Collection:
> Fix Committed
>
> Bug descripti...

[James Page (james-page)]

Hey Subbu

As David points out - config-changed runs in all sorts of instances such as restart of juju daemons, when network configuration changes and when an end user changes config options.

The unitdata charm-helper can be used to determine whether a configuration option has been changed:

  http://bazaar.launchpad.net/~charm-helpers/charm-helpers/devel/view/head:/charmhelpers/core/unitdata.py

you could use that to detect changes that require a restart.

[Tim Van Steenburgh (tvansteenburgh)]

Hi, thanks for your work on this! I'm moving it back to In Progress - when you're ready for another review please change the status to "Fix Committed" to have it appear in the review queue again.

[Subbarayudu Mukkamala (smukkamala)]

The fixes addresses all the review comments except:

- The automated testing of vrs with all values hard-coded is not possible at this point. The nuage-vsd amulte test is being enhaced to read configuration from a yaml file.

nuage-vsd amulte test would test the following charms:

nuage-vsc
nuage-vsd
nuage-vrs
neutron-api

[Cory Johns (johnsca)]

Subbarayudu,

Thank you for addressing the previous review comments, including resolving the charm proof warnings, adding conditional restart support, and adding tests. I like that you were able to create unit tests for your hook code, and I would like to see more of that type of test in charms when it makes sense. As you noted, creating an Amulet test for a charm that depends on several other charms can be difficult and counter-productive.

When running your tests, the hard-coded path to "nosetests" caused an issue for me, but I was able to work around it by changing it to use "NOSE := /usr/bin/env nosetests" instead of "PYTHON := /usr/bin/env python". Perhaps you could switch to use that instead?

Also, I noticed that if the install hook is triggered and the vrs-repository-url option is not already set, the charm sets the status to "blocked", but then the handler for ConfigurationError calls sys.exit(1), which will put the service into an error state. In addition to masking your "blocked" status, it will make it so that the admin can't use "juju set" to resolve the issue. Furthermore, by only handling that option in the install hook, you make it so that the option can never be changed (we call this "immutable config", and it is very much frowned upon).

I would recommend moving the handling of that option to the config_changed method (note that config-changed is *always* called after install, at least once) and use your config_value_changed helper to detect changes and handle them appropriately.

One unfortunate downside of unit tests is that you have to be careful to test interactions between the hook handlers themselves. As you mention, your ACTIVE_CONTROLLER roll can be controlled by either the config option or the relation data, but there is some ambiguity as to what happens when there is a conflict between them. In particular, it is possible to have both the vsc-controller-standby config option set and the relation provide a vsc-ip-address value at the same time, in which case the config-changed and relation-changed hooks would fight over the state. This could use some test coverage and explicit resolution to prefer one over the other.

Finally, I'd just like to note that, without access to the repository, I can't fully test this charm to offer my +1. Hopefully, my code review comments are useful, but you'll need someone who either has access to the repository or more knowledge of this domain to promulgate. Keep in mind, however, that charms in user namespaces are perfectly usable without promulgation, so don't consider this a blocker to use.

Thanks again for the contribution!

[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.

> If there is a reason, please document the requirement that these config options be set in the README and in the description field of the config.yaml file.

The README is still not clear enough that the vsc controller will be overridden by the relation. It needs to spelled out when vsc-controller-active is required to be set and when it get overridden by a relation.

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.

It needs to be clear that vrs-repository-url is required in both config.yaml and the README. See also Corry John's point bellow.

Corry John's requests that still need addressing:
> When running your tests, the hard-coded path to "nosetests" caused an issue for me, but I was able to work around it by changing it to use "NOSE := /usr/bin/env nosetests" instead of "PYTHON := /usr/bin/env python". Perhaps you could switch to use that instead?
>
> Also, I noticed that if the install hook is triggered and the vrs-repository-url option is not already set, the charm sets the status to "blocked", but then the handler for ConfigurationError calls sys.exit(1), which will put the service into an error state. In addition to masking your "blocked" status, it will make it so that the admin can't use "juju set" to resolve the issue. Furthermore, by only handling that option in the install hook, you make it so that the option can never be changed (we call this "immutable config", and it is very much frowned upon).
>
> I would recommend moving the handling of that option to the config_changed method (note that config-changed is *always* called after install, at least once) and use your config_value_changed helper to detect changes and handle them appropriately.
>
> One unfortunate downside of unit tests is that you have to be careful to test interactions between the hook handlers themselves. As you mention, your ACTIVE_CONTROLLER roll can be controlled by either the config option or the relation data, but there is some ambiguity as to what happens when there is a conflict between them. In particular, it is possible to have both the vsc-controller-standby config option set and the relation provide a vsc-ip-address value at the same time, in which case the config-changed and relation-changed hooks would fight over the state. This could use some test coverage and explicit resolution to prefer one over the other.

[Sunny Verma (sunny-verma)]

Hi David,

I have merged this branch with next.
Please review.

[David Ames (thedac)]

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