Cisco N1KV VSM charm review process (cwchang)

[ChingWei Chang]

new N1KV VSM charm is at lp:~cwchang/charms/precise/vsm/trunk

Please help review it, thanks !

[ChingWei Chang (cwchang)]

Thanks for approving and posting it to the charm store

I just push a new icon.svg that conforms to Charm Store standard.

[Charles Butler (lazypower)]

Greetings ChingWei,

Thank you for your contribution to the N1KV VSM charm. I've taken a bit of time to review the charm itself and what its doing, my notes are as follows:

charm-proof:
has a minor nitpick: stop hook is not executable - easy fix, chmod +x hooks/stop

Hooks:
When perusing the hooks, i noticed none of the files had a set -e flag at the top. We recommend at bare minimum that your hooks contain the set -e flag so charms don't go rogue during the installation cycle, errors are caught, and the user has a chance to intervene and correct any problems.

When attempting to deploy the following along the guide in the README/config.yaml - i was unable to successfully populate the sources.list.d file for the proposed ppa

juju set vsm n1kv-source="ppa:<my-team>/<my-ppa>, deb https://<myuser>:<mykey>@private-ppa.launchpad.net/<a-team>/<project>-5/ubuntu|<signed-repo-key-from-launchpad-listing>"

Barring feedback on how to successfully deploy the software, I am blocked on this step of the installation.

Otherwise, the source looks clean, and I can tell some work has been put into this charm. Thank you for the submission and I look forward to the next iteration of the review and seeing the n1kv charm in action.

I'm going to place the status of this bug as "incomplete" pending a follow up on the deployment instructions. Feel free to ping me on IRC, my nick is the same as my launchpad ID. When you are ready for another review, simply place the status of the bug as "new" or "fix committed" and someone will be along shortly to review your work.

Thanks again!

[ChingWei Chang (cwchang)]

Charles,
Based on the input from Chris Arges, I will corporate those comments into a merge proposal.
I'd to release commit/release this bug.

And some other engineer (millet) will take your feedback to adopt those changes.

To get access to Springfield private PPA, I think you need to be a member of it first to get those account/credential set up.
Let me work with Chris to see how to proceed with this.

There is one anomaly I need to report, I found two vsm and two vxlan-gateway charms in Charm Store.
I believe the reason is that I change the my bzr branch from ~cwchang to ~springfield-team for other engineers R/W permission.

Could you remove the one under my name ~cwchang ? So Cisco engineers can keep working on lp:~springfield-team/charms/precise/vsm/trunk for merging

Thanks !
ChingWei

[Marga Millet (millet)]

Hi Charles,

thanks a lot for looking at our charms and provide review comments.
Please check the latest code which has a much more updated version of the charm.

I think Chrish (arges) has provided you a way to get the debian package of the VSM, right?

Regarding adding "set +x" to the charm code, I think it's a good idead but we need some regression testing for this.
Right now it may catch some false errrors. I'll work on adding that.

Thanks.

[Charles Butler (lazypower)]

Marga,

I've been added to a new PPA. I've got these charms at the top of my list
for first thing tomorrow.

I'll give the deploy another go and ping arges if I run into any further
issues. Thanks for reaching out!

On Tue, Mar 25, 2014 at 5:19 PM, Marga <email address hidden> wrote:

> Hi Charles,
>
> thanks a lot for looking at our charms and provide review comments.
> Please check the latest code which has a much more updated version of the
> charm.
>
> I think Chrish (arges) has provided you a way to get the debian package
> of the VSM, right?
>
> Regarding adding "set +x" to the charm code, I think it's a good idead but
> we need some regression testing for this.
> Right now it may catch some false errrors. I'll work on adding that.
>
> Thanks.
>
> --
> You received this bug notification because you are a member of charmers,
> which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1291783
>
> Title:
> Cisco N1KV VSM charm review process (cwchang)
>
> Status in Juju Charms:
> In Progress
>
> Bug description:
> new N1KV VSM charm is at lp:~cwchang/charms/precise/vsm/trunk
>
> Please help review it, thanks !
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1291783/+subscriptions
>

[Charles Butler (lazypower)]

Marga,

Thank you for the revisions to the VSM charm. I've taken the opportunity to re-review the submission and I'm happy to report the issues I had with deploying are fully resolved. Thank you for your patience while I resolved that bit, I have a few notes about my findings...

I like how there has been shared functionality extracted into a common library. This is a practice used in many charms and adds a nice bit of abstraction to the charm.

The hooks are missing the set -e flag at the top. This is a requirement for inclusion to the charm store to prevent charms from continuing after error. I ran into a condition while deploying where I had an incorrect n1kv-source configuration option, and the unit continued configuring which took over the networking and rendered the server unable to communicate as it changed the networking configuration. With a set -e flag, it would halt upon not being able to find the n1kv-iso package.

I also noticed from the output from charm proof that the stoop hook is not executeable, but in the grand scheme that is a knitpick.

I am very pleased with this charm, the overall formatting, and layout. If you can add the set -e flag to the top of the hooks I would have no problem ack'ing this charm for promulgation.

Thanks again for the hard work and submission of the N1KV VSM charm. I'm going to move the status of the bug to "in progress" pending the requested modification. After you have added the -e flag, if you are ready for another review, simply change the status of the bug to "new" or "fix committed" and someone will be along shortly to review your work.

If you have any additional questions concerns, post back to this bug, or join us on irc.freenode.net in #juju, or email the mailing list at <email address hidden>

Thanks again, and all the best!

[Marga Millet (millet)]

Hi Charles,

thanks a lot for your review! I'll make changes and let you know for review.

Meanwhile, do you know what's the problem with our icon that it doesn't display the drawing properly?

Thanks.

[Charles Butler (lazypower)]

Marga,

Icons aren't displayed in the GUI until the charm has been promulgated. They will display the default pinwheel icon until its accepted into the charm store.

I hope this clears up any confusion, if not, feel free to reach out with more specific details.

All the best.

[David Douglas (ddouglas-austin)]

Hello Marga, have you made these changes?

[Marga Millet (millet)]

Hi Charles and David,

I've updated the VSM charm to make sure we are caching all the errors possible in it.
(I haven't used set -e all the time because I prefer to display the error explicitly but this new charm should take care of all your concerns).

Let me know if you have some other question/comment on it.

Thanks,

Marga

[ChingWei Chang (cwchang)]

Charles,
I am chiming in for Marga's comment #10. There are many places VSM script has placed in checking on $? and many system state collection ensued once $? != 0

set -e to us, albeit it's an early bail-out efficient tool, is too hasty for us.

ChingWei

[ChingWei Chang (cwchang)]

Added marga into subscription.

[Marga Millet (millet)]

Hi, thanks for approving the VSM charm and put it into the charm store.
I notice that the icon doesn't appear. How can we fix it?

Thanks.

[José Antonio Rey (jose)]

Marga,

The charm has not been approved for its inclusion in the Charm Store yet. What you are seeing is a personal branch. As Charles pointed out before, it will not display an icon until it is promulgated into the Charm Store.

Also, I see a couple things here:

 * The 'stop' hook is not executable
 * The README.md file is not in Markdown
 * The n1kv-vsm-password configuration option sets a default password, which, as far as I know, is against Charm Store policies.

A charmer should be able to review the charm soon for its inclusion on the store. Thanks again for the continued efforts on the VSM charm!

[Marga Millet (millet)]

Jose Antonio,

thanks a lot for your promptly response.
I've fixed the other issues but I ahve a question about the README.md file format.
Please, could you specify what markdown format does juju charm uses?
Is that html format?

Thanks.

[José Antonio Rey (jose)]

Marga,

Markdown is a syntax which seves to convert plain text to HTML. In a minute you will see an MP against the branch, in which I've reformatted the README.md file to Markdown.

Also, I don't see you have pushed the changes to the branch. I would recommend doing so before approving the MP.

[Marga Millet (millet)]

Gracias Jose Antonio,

thanks a lot for your changes.
However, after I've updated this bug I've found some Markdown tool online and I've pushed that format into version 28 of the branch. Are you ok, with that?

If so, then we can go with version 28. Otherwise I'll integrate your proposal into a new version and push it to the branch.

Thanks.

[Jorge Castro (jorge)]

Just as a reference the fastest way to do this would be to do `charm create readme` in the root directory of the charm, that will create a skeleton README for you in Markdown.

Here's the formatting guideline for Markdown as a reference: http://askubuntu.com/help/formatting

[José Antonio Rey (jose)]

Marga: You actually ended up converting the file to HTML. I've done an MP, you can check the diff and see what changes I did.

Jorge: The README file was good, but it contained the old template provided by the charm toold.

[Marga Millet (millet)]

Jose Antonio and Jorge.

thanks a lot for your help.
Jose Antonio, I've patched your changes and pushed to the branch (version 29).
Let me know if this time is good or if you have any more questions/suggestions.

Thanks.

[José Antonio Rey (jose)]

Marga,

Looks good to me. Now it's a matter of waiting for a charmer to review it.

Thanks again for your efforts on the charm!

[Marga Millet (millet)]

Thanks Jose Antonio!

Hi charmers,

did you got a chance to finish the review on this charm?

Thanks

[Charles Butler (lazypower)]

Greetings Team,

Thanks again for all the hard work and dedication to making this charm submission. I've taken another look though the charm and deployment and I've got the following notes:

The overall documentation for the charm in the README is still very sparse. The configuration sample on deployment is great, but it doesn't explain where the charm lives in the stack. There's a definite dependency on Openstack with the VSM Gateway that should be clearly called out. As an example

# When deploying with MAAS

The VSM charm should be deployed in the same environment as openstack, as the services relate and depend upon one another.

Another area for improvement would be the manual steps required to configure the service after deployment of the VSM charm. I've attended a meeting which illustrated the additional steps on how to utilize the service with neutron. While I dont expect reproduced documentation that lives on cisco.com - it should be clear to the user what steps are required to produce a functioning setup after deploying the charm into their openstack infrastructure.

I didn't see any inherent issues with the charm code. Its fairly clean and straight forward on what the charm itself is doing.

Barring some updates to the documentation, and a review from an openstack charmer you're very close to making it into the charm store. Thanks again for the efforts to making a high quality submission to the charm store.

Thanks again for the submission. I'm going to change status of this this bug to "incomplete" and when you're ready for another review please set the status to "fix committed" or "new" 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>

[Marga Millet (millet)]

Hi Charles,

from your comments I think you are confusing the VSM charm with the VXLAN GW charm
after attending the meeting you are mentioning. Let me clarify:
1. VSM charm uses MaaS as deployment model (and it's mentioned in the README.md file).
     VSM can be created as a VM itself outside of MaaS but then it's not using the VSM charm.
    Hence this is out of the scope of this README.md file.
2. There are NOT extra steps that user needs to make for the VSM charm, besides the ones
    already mentioned in the README (about deploying the charm itself and add relation
    with VEM). (Those extra steps are for VXGW).
3. There's NOT dependency between VSM and neutron since the VSM ip address is static.

Just to make it clearer, I've made these changes to the read.me file:

=== modified file 'README.md'
--- README.md 2014-04-15 22:11:31 +0000
+++ README.md 2014-04-18 12:18:33 +0000
@@ -16,12 +16,25 @@
 have up to 2 VSM in active/standby mode (generally in two different servers).
 When the active one fails, the standby will take over within bounded time.

+VSM charm is not directly dependent of other OpenStack nodes,
+unlike other Cisco charms like VEM or VXGW (VLAN Extended Gateway),
+which are subordinate charms.
+
+Once the VSM charm is deployed it creates a VSM Virtual Machine (VM)
+in the MaaS node. Hence a requirement for VSM to communicate with other
+Openstack/Cisco charms is to have L3 connectivity with the rest of
+the MaaS cluster nodes.
+
 # Usage

 In order to use Cisco Openstack solution we would need to install
 VSM on a cluster node as well as VEM module that goes into each
-host. In order to provide High Availability for VSM you'll need
-to deploy two VSMs (one will be the primary and another the secondary)
+host. As today the VSM charm needs to be L3 reachable to other nodes
+using the regular mgmt host interface. (This interface is specified by the
+n1kv-phy-intf-bridge configuration parameter of the charm).
+
+In order to provide High Availability for VSM you'll need to deploy
+two VSMs (one will be the primary and another the secondary)
 in two different hosts.

 In order to deploy the VSMs (both primary and secondary), you will
@@ -31,8 +44,19 @@
 on the configuration file and also you deploy as two different service
 names.

+The VSM charm will automatically create the VSM VM. However, you'll
+provide:
+ - a static IP, netmask and gateway address for network reachability
+of this VM (by specifiying the n1kv-vsm-mgmt-ip, n1kv-vsm-mgmt-netmask and
+n1kv-vsm-mgmt-gateway configuration parameters). This VM uses the
+interface specified in n1kv-phy-intf-bridge as external network
+interface.
+ - domain id that the VSM operates (by the n1kv-vsm-domain-id configuration parameter)
+ - password for the administrator to ssh to the VSM VM (n1kv-vsm-password)
+ - role which specifies if the VSM will be primary or secondary (n1kv-vsm-role)
+
 For example, you create a myconfig.yaml configuration file with the
-following info:
+following minimal mandatory information:

     vsm-primary:
         n1kv-vsm-domain-id: 101
@@ -52,6 +76,9 @@
         .....

+Check the config.yaml file for default configurat...

[Marga Millet (millet)]

Thanks Charles, I've pushed the changes into the charm (version 30)

[Charles Butler (lazypower)]

Greetings Marga,

Thanks for the clarification. I've taken another look at the charm and everything looks good to me.

I'm going to start the promulgation process and you should see the charm in the charm store within the next 30 minutes.

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>

[Charles Butler (lazypower)]

Greetings again,

The charm has been promulgated. You will find future bug reports and user feedback at https://bugs.launchpad.net/charms/+source/vsm

Thanks for your attention and revisions to the charm and seeing it through to completion!

[ChingWei Chang (cwchang)]

Charles,
Thanks for all reviewing effort and quality pointers.
ChingWei

[Marga Millet (millet)]

Hi Charles and all charmers!

thanks for approving the VSM.

The VSM charm is still not in the charm store.
What do we need to still do?

Thanks.

[José Antonio Rey (jose)]

Marga,

The charm is already in the Charm Store (https://jujucharms.com/sidebar/search/precise/vsm-1/)