Review queue: IBM xCAT

[Johnny Shieh]

Hello Team,

Here is the IBM xCAT charm for review.

Deployable ibm-xcat layered charm can be found in the below repository.

Deployable Charm Repo: https://code.launchpad.net/~ibmcharmers/charms/trusty/ibm-xcat/trunk

And, its source code can be found in the below repository.

Source Code Repo: https://code.launchpad.net/~ibmcharmers/charms/trusty/layer-ibm-xcat/trunk

Below is the charm store link and its revision

Link: https://jujucharms.com/u/ibmcharmers/ibm-xcat

Revision: cs:~ibmcharmers/trusty/ibm-xcat-3

Thanks

[Antonio Rosales (arosales)]

@Johnny,

Thanks for your contributions to the Juju Charm community. Adding xCAT as a cluster toolkit to the Charm catalog will give HPC users access to the rich Juju environment of deploy, connect, and scale.

--- General feedback ---
- no icon
- proof passes with no warning
- rename README to README.md for proper markdown parsing.

--- README ---
- Rephrase to be a bit more clear. Perhaps cut "partially."
>The partially installation steps follow the instructions given in the Sourceforget.net documentation:

Suggest singular for "charms" in this context:
>After the deployment, the user should expose the xCAT charms via:

- Add a space and indention to you override.yaml example to have it rendered as code.

- Nice touch on the giving an example override.yaml example.
- Appendix is nice with additional upstream information.
- The known limitations and issues well documented.

--- config.yaml ---
- What are the options for XCAT_REPO? Suggest to list some alternate examples in the config.yaml description as well as in the README.
- Would there be any issue of putting the default XCAT_DOMAIN to say, "test" in order to avoid the failure noted in the limitations sections? Perhaps make a default for XCAT_DOMAIN, and explicitly call out in the README that this value should be changed, but a default is in needed in order to avoid deployment issues on a stock deploy. This seemed to work in my testing, less error'ing on locating the .deb at SourceForge.

-- Deploy --

- Testing on local with a default domain value of "test" I get the following error
>unit-xcat-0: 2015-04-15 21:41:21 INFO install E: Failed to fetch http://sourceforge.net/projects/xcat/files/ubuntu/xcat-dep/pool/main/x/xnba-undi/xnba-undi_1.0.3-7_all.deb 404 Not Found

- Looking for this file at Sourceforge I indeed get a 404, as Sourgeforge returns:
>The "/ubuntu/xcat-dep/poo..201503230106_all.deb" file could not be found or is not available. Please select another file.

--- Summary ---

This is a very good first revision of the charm. As you noted in the post installations instructions I think we could also add some additional config and relations to the charm in order to make this a more robust xcat deployment. As for the current charm submitted it does need to add an icon, update the readme, details for the source config option, and resolve the 404 .deb from SourceForge.

These are relatively small fixes and less the 404 I have proposed these fixes for your review, at your convenience at:
https://code.launchpad.net/~a.rosales/charms/trusty/xcat/initial-review/+merge/256403

I will put the status of this bug in "Incomplete" while you resolve the above issues. Please update the status to "Fix Commited" when you are ready for another review.

Thanks again for your contribution and feel free to reach out to us in IRC (#juju@freenode) or on the Juju mailing list if you have any questions.

-thanks,
Antonio

[Johnny Shieh (jshieh)]

Sorry, hit submit too quickly.
Here we go again:
1. Icon - there is an icon.svg file. I downloaded icon.svg from trunk and took a screenshot. Please look at attachment called icon.png

2. renamed README to README.md

3. Reworded "partially installation" with "initial installation"

4. corrected "charms" to charm

5. added space and tabs to example yaml fragment in README.md

6. I would prefer not to list example, alternative repositories. That's up to the user, who may have a preference themselves.

7. I originally was going to put in a default domain, something like: "default_domain.com". The problem with that, is that by having a default domain, it tricks xCAT into thinking everything is fine. I'd rather have a blank domain so that the user is forced to name a domain OR that the charm fails to install off the bat since there is no defined domain. Once you install with a wrong or fake domain, it is a pain to fix it in an installed xCAT and get the value reset.

8. I clicked on the link you mentioned and it is up and running. I took a screenshot of it. This happens with sourceforge.net. One known problem is that they are the recipient of malicious traffic diversion, which results in a DOS attack. Screenshot attached as: sourceforge.png

[Johnny Shieh (jshieh)]

Adding icon.png

[amir sanjar (asanjar)]

Look at attached icon. The original icon does not render correctly .

[amir sanjar (asanjar)]

Thanks for your contributions to the Juju Charm community. Few comments:
1) Missing tests
2) Immutable config: After deploying without a valid XCAT_DOMAIN config value, changing that value doesn’t fix the domain and put the charm in a working state, because the config-changed hook is not implemented.
3) Missing implementation of http relation

[Cory Johns (johnsca)]

Follow-up to Amir's comment:

2) Note that config-changed is always fired at least once after install, so it should be fairly trivial to move the host handling entirely to config-changed

3) You can either remove the http relation from metadata, or just add a simple hook handler, such as: https://api.jujucharms.com/charmstore/v4/trusty/wordpress-2/archive/hooks/website-relation-joined

[José Antonio Rey (jose)]

Hey Johnny,

I am moving this bug to incomplete as per the last two comments. Please move the bug back to "Fix Committed" once you have solved them.

Thanks for working on the xCAT charm, I hope to see the next iteration really soon!

[Johnny Shieh (jshieh)]

In regards to the last set of review comments:

1) need a valid icon file
Apparently my editor or my editing is causing file corruption. The PTS
team was kind enough to re-do the icon file for me and I will drop the code
using this bug.

2) need a testcase
This was never asked for in the 2 previous review comments made by Antonio.
Testcases are not listed as a requirement to publication of a charm.

3) need to allow the user to change the domain name
This was never asked for in the 2 previous review comments made by Antonio.
In fact, the README states that the installation will halt and not proceed any
further unless the user specifies a domain name. Thus this request is considered
an additional function: to be able to change the domain name of a previously
installed xCAT server. A domain name change doesn't occur that often, and as
such is considered a "nice to have" feature that is not meant to be in an initial (months on end)
drop of this charm. This will not be provided in this initial charm.

[Adam Israel (aisrael)]

Hi Johnny,

I had the chance to review the past comments made on this charm and perform my own initial review, and I'd like to share that feedback with you.

One thing to note is that not all reviewers are ~charmers; some may also be members of the Juju community. While Antonio performed the initial review, that doesn't make his the authoritative review. If there are questions about the feedback you've received, though, we are always happy to clarify.

Per the Charm Store Policy (https://jujucharms.com/docs/stable/authors-charm-policy), charms:

- Must include tests for trusty series and any series afterwards. Testing is defined as unit tests, functional tests, or integration tests.

- Must have hooks that are idempotent.

Idempotent in this context means that any hook should be able to be executed multiple times and there should be no observable difference. For example, if install (or config-changed, as discussed below) were called more than once, it should not create duplicate entries in /etc/hosts. More on that here: https://jujucharms.com/docs/1.24/authors-charm-hooks

Stopping charm installation when the domain value is missing is certainly valid, but setting the value after the fact should allow installation to continue. As Cory noted above, moving that functionality to the config-changed hook will resolve that issue.

The install hook would be a good place to use status-set (https://jujucharms.com/docs/1.24/reference-status). You can use this to signal to Juju that the charm is in a blocked state. For example, the install hook can simply be a check if XCAT_DOMAIN is set. If not, call `status-set blocked "Please set the XCAT_DOMAIN."`. If it is set, call `status-set maintenance "Installing software"`. The rest of the code would be moved to config-changed. Once XCAT_DOMAIN is set, the config-changed hook will fire and continue with the installation of the xcat software.

You can also remove the following hooks:
- relation-name-relation-broken
- relation-name-relation-changed
- relation-name-relation-departed
- relation-name-relation-joined

These are part of the default charm template, and not implemented in xCAT.

In metadata.yaml, there is a small typo. The description reads "pacakges" instead of "packages". Also, as noted by Amir and Cory, a http hook is defined in metadata.yaml but is not defined. If xCAT does not use the http relation (there is no hook for one), it can be removed from the metadata.

The upgrade-charm hook does not currently work. The recommended best practice would be for it to call the install hook, followed by the config-changed hook.

Thanks for your work iterating on the feedback you've received. Once you're ready for another review, please set the status back to Fix Committed and we will take another look. Also, feel free to reach out on #juju on irc.freenode.net or the juju mailing list if you have any other questions or concerns.

[vsr (vsasi)]

Hi Adam,

  I have updated based on the review comments. Updates available at:
https://code.launchpad.net/~ibmcharmers/charms/trusty/xcat/devel

[Andrew McLeod (admcleod)]

Hi guys,

I've tried to run a test against this:

charms/trusty/xcat> bundletester -e local -l DEBUG -v

Unfortunately I get some 404s -

http://pastebin.com/vkch4uSx

And then the test times out. Could you please have a look at the locations for these packages?

Thanks

[Kevin W Monroe (kwmonroe)]

I'm moving this bug to "In Progress" awaiting resolution of the 404s mentioned in comment 12. Please switch this back to "Fix committed" when you're ready for us to take another look. Thanks!

[Prabakaran (prabacha)]

Thanks for your review on this charm !!

In config.yaml file, it is recommended to use sourceforce.net as a default value for XCAT_REPO as there is no official alternate source/repo link available for the same. Thanks!!

[Antonio Rosales (arosales)]

@prabacha / @vsr,

Thanks for your updates to the xCat, and your work to make it a solid experience for the xCat users.

I noticed there were two branches in this bug. Specifically:
lp:~ibmcharmers/charms/trusty/xcat/trunk
and
lp:~ibmcharmers/charms/trusty/xcat/devel

The changelog indicated they were both at the same latest revision (37), but to confirm I ran a diff aginst the branches and they look to be the same, thus I'll be taking a look at the trunk branch.

# Deploying the charm
- A big plus to using workload status, and being descriptive on what action I needed to take. I did a deploy on my local provider with defaults and found the following in my juju status:

[Units]
ID WORKLOAD-STATE AGENT-STATE VERSION MACHINE PORTS PUBLIC-ADDRESS MESSAGE
xcat/0 blocked idle 1.25.0.1 1 10.0.3.153 Please set the XCAT_DOMAIN name.

I consulted the readme, and did a `juju set xcat XCAT_DOMAIN="my-cluster-name"` and juju status showed me the config-changed hook was firing (as per previous comments) and the installation continued. Nice work here.

- I deployed the charm on local and amazon, after setting the xcat domain I was able to see xCat being "up" in juju status. The instructions indicated to do a juju expose but I am not sure what that accomplishes with out a web UI, and the next instructions have me ssh'ing into the unit. Suggest to add some info in the readme following the expose instructions to let the user know what exposing the service does, and what if any security implications should be considered.

- While juju status showed the service being up when I ssh'ed into the service I was not able to verify the install per the quick-start guide (http://sourceforge.net/p/xcat/wiki/Ubuntu_Quick_Start/#verify-xcat-installation). I got errors asking if the xcat daemon was no up (on both aws and local):
http://paste.ubuntu.com/13137921/

- Looking at the charm tests I see that the xCAT service is not tested as per the xCat Quick-start guide (http://sourceforge.net/p/xcat/wiki/Ubuntu_Quick_Start/#verify-xcat-installation). Suggest to add into the tests to verify xCat is indeed working. This will help give you and the user confidence the service is properly running, and configured correctly.

# Knitpics (nice to have, but won't block promotion to recommended status)

- In the readme it states:
"If XCAT_DOMAIN is not altered by the user, the charm will create a container based on the default domain name of
the system if it exists. If not, the user will have to set the XCAT_DOMAIN name. It is recommended to use default value for XCAT_REPO="sourceforge.net"."
Suggest to put a code example on how to correct this such as:
    juju set xcat XCAT_DOMAIN="my-cluster-name"

- the the readme under Known Limitations it states:
"When this charm attempts to deploy, and there is no host or domain entry configuration in /etc/hosts, then this charm will halt and log this failure in the juju debug-log. An override variable for the domain is provided in the install script."
What is the purpose of stating there is an override variable? Is the user meant to take action on it? If it is just used t...

[Kevin W Monroe (kwmonroe)]

This review has moved to the new queue:

https://review.jujucharms.com/reviews/38

The latest xcat charm has been imported and should be reviewed soon.