This is a new charm for IBM Platform LSF for review.

[Uma]

Hello Team,

Here is a new charm for IBM Platform LSF Storage for review.

To test this charm, a license to IBM Platform LSF is required.

To test the IBM Platform LSF Storage, we need LSF Server charm and LSF Master charm as well. As per the design changes, the Platform LSF charm has been split into three charms:
IBM Platform LSF Storage
IBM Platform LSF Master
IBM Platform LSF Server.

We need all three charms to be deployed to have a functional LSF Cluster.

Repo : https://code.launchpad.net/~ibmcharmers/charms/trusty/ibm-platform-lsf-storage/trunk

And, its source code can be found in the below repository
Repo : https://code.launchpad.net/~ibmcharmers/charms/trusty/layer-ibm-platform-lsf-storage/trunk

The charm has been pushed into charm store as well
branch : cs:~ibmcharmers/trusty/ibm-platform-lsf-storage-1

Thanks

[Uma (umasv1987)]

packages are uploaded to <email address hidden>

[Charles Butler (lazypower)]

Greetings IBM charmer team,

I've taken some time to review the IBM LSF platform charm, and I'm delighted to see the overall progress here. Its a great start to providing the LSF platform to consumers of the Juju Charm ecosystem. While reviewing I compiled the following notes:

The charm passes proof, which is a brilliant way to start any review, Thank you for your attention to detail!
The icon looks of high quality, the metadata and copyright are within charm store guidelines.

There is description text however that will not render in the Juju GUI or in the charm store display. As markdown and the config.yaml are rendered to HTML, anything rendered between angle brackets: < > will be interpreted by the browser as an HTML tag, and thus not rendered. The helper text here is problematic and needs to be refactored to identify variable output, without the brackets.

There is also a concern with ease of setup for new users using the Apache server as a deliverability mechanism, but i also know aisreal has started an effort to resolve this, and I will defer to those efforts as a resolution. As this directly effects the automated CI process. However while this is still the recommended path forward, please be explicit in the instructions to setup the apache repository. It wasn't directly ovious to me to just place the binaries in /var/www/html of a fresh apache2 installation on ubuntu 14.04 and point to that units ip address to serve the files.

Looking at the test suite, 00-setup appears to be sniffing for configuration of lsf-url, and this requires manual intervention, while the 10-deploy test is looking in local.yaml for the values. I see there's some preliminary work here to do so, but it requires you to manually edit 00-setup to set this value. I'm not filled with confidence that any of this is obvious to the user, or the ci system on how to continue on this path to a resolution.

There also appears to be an error in 10-deploy test

ERROR: platform-lsf::10-deploy.py
[/tmp/bundletester-0ruIYd/platform-lsf/tests/10-deploy.py exit 1]
  File "/tmp/bundletester-0ruIYd/platform-lsf/tests/10-deploy.py", line 39
    installationpkg = config.get('platform-lsf').get('cfg_lsfinstall_pkg_name')
                                                                              ^
TabError: inconsistent use of tabs and spaces in indentation

To run these tests, i executed the following command: bundletester -F -l DEBUG -v

you can install bundletester from pypi - pip install bundlestester, as this is the tooling we use to verify all charms in CI and when running manually.

Otherwise the charm appears to be in order, all the config is mutable, there's only a few minor nitpick things to resolve, and I'm confident that once these items are resolved, the platform-lsf charm will be ready for the charm store. Thank you for your submission, dedication, and patience during the review process.

I'm going to change status of this Bug to "In Progress" and when you're ready switch the bug 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 o...

[shilpa (shilkaul)]

Hi Charles,

Thanks for reviewing my code and providing your valuable feedback.
I will be working on the review comments soon.I am also incorporating the scenario for Master node failure , where if one master goes down , another master candidate will take over and become new Master of LSF Cluster.

Thanks,
Shilpa.

[shilpa (shilkaul)]

Hi Team,

I have reworked on the LSF charm and implemented the LSF master fail-over scenario.If the LSF Master goes down , then the next LSF Master Candidate will become the new Master until the main Master comes up.

The LSF charm still requires the Apache repository to be setup.We have implemented the sftp scenario for some of the other IBM charms.I will be incorporating this approach in the next iteration for this charm.

Putting this bug in Fix Committed.

Thanks
Shilpa

[shilpa (shilkaul)]

Implemented SFTP mechanism for downloading packages. Now user does not require to configure Apache.

[shilpa (shilkaul)]

Hi Team,

Here is a new charm for IBM Platform LSF Storage for review. The previous charm IBM Platform LSF charm has been split into three as per the new design changes :
IBM Platform LSF Storage
IBM Platform LSF Master (Bug Number : #1578271)
IBM Platform LSF Server (Bug Number : #1578273)

We need all three charms to be deployed to have a functional LSF Cluster.

Deployable ibm-platform-lsf-storage layer charm can be found in the below repository.
Repo : https://code.launchpad.net/~ibmcharmers/charms/trusty/ibm-platform-lsf-storage/trunk

And, its source code can be found in the below repository
Repo : https://code.launchpad.net/~ibmcharmers/charms/trusty/layer-ibm-platform-lsf-storage/trunk

The charm has been pushed into charm store as well
branch : cs:~ibmcharmers/trusty/ibm-platform-lsf-storage-0

Thanks

[shilpa (shilkaul)]

Hi Team,

I have re-worked on the LSF Storage charm for incorporating changes for Resources and Terms.
Amulet changes are done accordingly, but getting issue while running the amulet (Issue : https://github.com/juju/amulet/issues/141)

The charm has been pushed into charm store as well
branch : cs:~ibmcharmers/trusty/ibm-platform-lsf-storage-8

Thanks,
Shilpa

[Pen Gale (pengale)]

I attempted to test, but was unable to find a download link for the "Platform" version of LSF 9.1.3 -- the product page linked in the README took me to the "Community" version of LSF instead.

What is the best way to get a copy of the tarballs mentioned in the README?

[shilpa (shilkaul)]

Hi,

The links mentioned in the README (Passport Advantage) will take you to the Licensed version of IBM Platform LSF 9.1.3. IBM and Canonical have a contract ,so we have placed all the software on a sftp site (https://canonical.brickftp.com/sessions/new). The Site has both LSF x86 and power packages present in folder /platform-lsf. You can get the files from this site. The sftp site is used for testing IBM charms by Canonical.

Thanks
Shilpa

[Pen Gale (pengale)]

Thank you for your work on this. I was able to manually deploy this, and the related charms, and follow the steps in the README to check to make sure that they deployed. charm proof passes, and this charm has an icon, maintainer, and other necessary bits.

Note that, if you include the flag "reset: false" in your tests.yaml, I'd be able to run tests against the charms that I manually deployed, and verify that tests pass.

I am +1 on this charm.

[Cory Johns (johnsca)]

I was able to deploy and verify these charms according to the README. However, I noticed a few items in the code that I think should be addressed.

First, you are using the ibm-base layer but not any of the features it provides (it seems they've been supplanted by Juju 2.0 features such as terms and resources). When the resources are not attached properly, the layers from ibm-base, apt in particular, hide the status messages that you provide to the user and make it hard to tell why the charm is not working. I recommend replacing 'layer:ibm-base' with 'layer:basic' in layer.yaml. I tested with this and it works as expected and results in a much better user experience.

I also noticed that in the storage charm, in configure_etcallowfile, you have checks against the remote service name and require that the master and server charms be deployed with exact application names (ibm-platform-lsf-master and -server). This breaks the charm if the admin deploys them as anything else (e.g., lsf-master) and is not necessary. Juju already enforces relation compatibility via interfaces, any only charms that advertise that they will work with your charm will be able to connect. In this case, that would only include the lsf-master and lsf-server charms. So, you should remove that check.

Finally, I noticed that you use "exit" in your bash reactive handlers. While this seems to work for the moment, it is not a good approach for reactive charms because it can cause handlers to be skipped in an unpredictable fashion, leading to hard to diagnose bugs. I believe your team has hit this before on other charms. From within a reactive handler (a function decorated with @when or the like), you can just replace "exit" with "return". For a function called by a reactive handler, it becomes a bit harder to surface up the fact that you wish to abort, but you can do something like:

    function foo() {
        return 1
    }

    @when 'state'
    function handler() {
        foo || return
    }

[shilpa (shilkaul)]

Hi,

Have re-worked on the charm and incorporated the suggestions given by Cory. Putting it to Fix Committed for further review.

Thanks

[Kevin W Monroe (kwmonroe)]

Thanks for the charm updates Shilpa. This review has been migrated to the new queue:

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

The latest platform-lsf-storage charm has been imported and should be reviewed shortly.