Charm needed: Liferay

Reported by Jorge O. Castro on 2012-05-29
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Undecided
James Falkner

Bug Description

James Falkner (james-falkner) wrote :

Created an initial Liferay charm here:

http://bazaar.launchpad.net/~james-falkner/charms/precise/liferay/trunk/files

It uses 'wget' to fetch the latest Liferay and deploy it, and hooks it up to mysql via a relation. This charm also requires unzip and openjdk-7-jdk.

Mark Mims (mark-mims) wrote :

# review... static pass:

---

## things to fix:

- we need to cryptographically verify the download... this can be straightforward or ugly depending on the upstream hosting. There are some charm helper library bits you can install via `apt-get install charm-tools` and're located here 'http://bazaar.launchpad.net/~charmers/charm-tools/trunk/view/head:/helpers/sh/net.sh' You can see an example usage here: 'http://bazaar.launchpad.net/~charmers/charms/precise/phpmyadmin/trunk/view/head:/lib/common.sh' but again exactly what you do to verify depends on upstream

- 'website-relation-joined' should use `unit-get private-address` instead of the public one. The 'website' relation is not used for external access to the service... it's used for relating the liferay service to other services (like reverse-proxies or load-balancers), where they'll be using only _internal_ addresses to communicate

- please create an 'upgrade-charm' hook... sometimes it's enough to symlink it to the 'install' hook, but please think through the process of what an 'upgrade' means to the user of this service... and maybe add notes for it to the README

---

## discussion, not necessarily to fix:

- be nice for the README to have an explicit example usage of the charm with bootstrap, deploy, adding relations, exposing ports, etc

- installs to '/var/liferay'... this should be somewhere like '/opt/liferay' according to filesystem stds iirc... not sure. We should use the standard unless it's really common practice for the liferay community to install straight to '/var'

- there's no need to 'close-port' from within the stop hook as that's pretty much controlled externally via `juju expose <service>` and `juju unexpose <service>`. open and close ports are really just the charm telling juju which ports to open/close. Close is mostly there for when the port somehow changes... like from config changes (see next item)

- you might expose things like the port as config... i.e., add '$CHARM_DIR/config.yaml' and either set it in install via `config-get port` or allow the user to change it dynamically by adding a 'hooks/config-changed'

---

This looks great! Couple of relatively small things to fix, so I'll mark the bug as needs fixing... mark it as fix committed when you're ready for another pass.

Thanks!
Mark

Changed in charms:
status: New → Incomplete
Changed in charms:
assignee: nobody → James Falkner (james-falkner)
status: Incomplete → Fix Committed
James Falkner (james-falkner) wrote :

Hey Mark, I took another pass and made the following changes:

- Use of ch_get_file to verify the SHA-1 hash (which is now baked into the inc/latest-release file)
- Changed website-relation-joined to use private-address
- Added upgrade support. In order to support this, I am now saving the "current configuration" into $CHARM_DIR so that I can reference it during upgrade (the filesystem layout of the Liferay+Tomcat bundle changes depending on release). Also added version comparison so it avoids downgrading or upgrading to the same release. Please look at the upgrade-charm hook, I am assuming that in some cases, the operator just wants to upgrade the charm itself, not the associated Liferay, so it 'exit 0''s in a few places.
- Also, Liferay upgrades are pretty heavyweight, so my upgrade script downloads the new release, migrates config, stops the old server, deletes its bits, moves the new version in place, and restarts.
- Put the URLs/hashes of Liferay into the inc/latest-release file
- A few helper utilities in inc/common
- More content in README.md

Please review at your convenience. Thanks!

Robert Ayres (robert-ayres) wrote :

Hi James,

I've performed a second review of your charm, works well under EC2, thanks for submitting!

I have a few bugs and comments below:

Bugs
*A few directories/files under /opt/liferay/liferay-portal-.../ have world read permissions but look like they contain sensitive data:
  *data - seems to contain application data
  *portal-ext.properties - contains DB username/password

Comments
*hooks/db-relation-changed contains initialisation routines for starting the server which look like they really belong in hooks/start. I think the better behaviour for the charm is hooks/start to always start the server. Adding a MySQL DB merely changes the server's config or resets the configuration wizard for the user to reconfigure with it.

*If you can, I'd use openjdk-7-jre-headless package over the standard openjdk-7-jdk one. The standard one pulls in all the X11 libraries etc. which aren't required on a server.

*As per Mark's comment, be quite nice if you could specify the port the charm ran on via a 'config.yaml' option. Alongside this, be great if you could specify JVM parameters (e.g. heap size) for tuning the deployment. If someone deploys Liferay to a larger machine, you want to take full advantage of this :)

Please fix the bugs and consider the comments, then reopen for another review.

Changed in charms:
status: Fix Committed → Incomplete
Changed in charms:
status: Incomplete → Fix Committed
James Falkner (james-falkner) wrote :

Hey Robert, thanks for the suggestions - I have incorporated all of them:

- The 'data' directory is indeed application data and potentially sensitive, so I've chmod'd it. Even when Liferay is configured to use an external db, some stuff is stored under 'data' so it's good to protect it. Same with the portal-ext.properties, the password currently must be in cleartext in this file (ugh).

- Properly moved start logic into the start hook, and changed other hooks to take into account. Much better!

- Tested with openjdk-7-jre-headless, seems to work well. In Java 6 we required a full-on JDK, but guess not with Java 7!

- Added support for 2 config options: http-port and jvm-opts (see README.md for details)

Marco Ceppi (marcoceppi) wrote :

Reviewing now!

Marco Ceppi (marcoceppi) wrote :

Hi James,

I don't see anything blocking this charm from making it in to the store. There are more minor things but I'll open those as bugs against the charm once it's promulgated.

Thanks for your work on this so far! Should be in the charm store shortly

Marco Ceppi (marcoceppi) on 2013-07-18
Changed in charms:
status: Fix Committed → Fix Released
James Falkner (james-falkner) wrote :

Awesome, thanks Marco - I'm gonna blog it up this week! I'll take care of the incoming bugs, and see if our community can contribute with some enhancements.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers