Submitting a Gluster Charm for Review

[Chris Holcombe]

This Gluster charm supports nearly everything Gluster can currently do. Every volume type is supported currently. The tiering volume type when it goes stable will also be included.

Things to note:
1. The charm can detect block devices and identify SSD vs Rotational vs Loopback devices. In future patches it will attempt to make use of this information for tuning.
2. The defaults of replica 2 and replication volume type should work for most use cases out of the box.
3. Scale testing on my bastion host succeeded at 60 instances. ( My nova limit is 60 so that's as far as I could go )
4. A mix of directories and block devices can be fed to the charm without it crashing. A mix of working and non working devices can also be given to it. Btrfs, XFS and Ext4 are supported for formatting block devices. ( Default is XFS )
5. The charm will hold off adding peers to the volume until there are enough to satisfy the replication number.
   Ex: You set replica 3 in the charm but only start 2 instances. The volume will wait for more instances to join before starting the volme
6. The default behavior is to put replicas on different hosts. This is to increase fault tolerance. I had to write a complicated function to get that to work properly. I have test data here showing that it is working:
  https://pastebin.canonical.com/134112/
7. I built 2 libraries to enable the rust community. A Gluster crate: https://crates.io/crates/gluster and a Juju crate: https://crates.io/crates/juju If others would like to build charms in rust they now can just import the crate and get rolling fast

Support for Centos/RHEL is coming in a future patch.

[Chris Holcombe (xfactor973)]

Example juju debug-log output from a local installation:
https://pastebin.canonical.com/134119/

I tried to make the log messages easy to understand. If you would like me to modify or remove any log messages feel free to let me know.

[Kevin W Monroe (kwmonroe)]

Hey Chris, thanks for the work to get a gluster charm out there for trusty! I've been watching gluster from the sidelines, but have always fallen back to boring 'ol nfs. I'm excited that this gives me a chance to kick gluster tires in juju.

I found a few issues with the current charm though. I'm not sure how many of these can be attributed to my ignorance for gluster or for rust, so please educate me if i've misinterpreted something as an issue when it's working by design. Here we go:

=====

1 - charm-proof reports the following informational items:

I: relation gluster-server has no hooks
I: relation nfs has no hooks
I: missing recommended hook config-changed

Are the gluster-server and nfs relations supposed to do something? Perhaps they are placeholders for future features, but if they're not doing anything useful, I'd recommend removing/commenting-out from metadata.yaml so people don't get confused when, for example, they relate to an nfs server and nothing happens.

More concerning is the missing config-changed hook.. I would expect changing the configurable items (like brick_paths and cluster_type) would update something in /etc/glusterfs and restart the daemon. Are these config options not meant to be changed post deployment?

Also, just a nit-pick while we're talking hooks.. I see your server-relation-joined hook sets hostname='unit-get private-address'. Why? Your main.rs already references the private addr with juju::unit_get_private_addr, so i'm curious if this hostname is really needed on the relation (and if not, perhaps the whole server-relation-joined hook can go away). Again, that's just a nit. It's not hurting anything -- i'm just not sure what it's buying you.

2 - README

The readme is cool in that it gives me an intro, config descriptions, deployment instructions, and even a section on building from source (which is awesome -- i haven't seen any other charm quite like this, and after reviewing main.rs, I think rust is really ... special).

That said, it may be good to point out that the included ./hooks/main binary is built for x86-64 and a rebuild would be required for other arches.

I would also like to see some mention of testing in the readme. After I add units to my cluster, how can I be sure it's doing the right thing? Perhaps you can come up with something like "echo hi > /mnt/brickX/foo; juju destroy-machine n; cat /mnt/brickX/foo" to verify the contents of that file survived a cluster member going away.

Lastly, I think a ## Reference section would be good.. Point people to an irc channel, mailing list, gluster.org bug tracker, etc in case they want more info from the gluster community.

3 - Tests

I've got good news and bad news for your ./tests. First, they're present! That's great news. Second, they all pass!! W00t, we're on a roll here. Third, they don't seem to do much. Boo. It looks like you're writing a ../config.json for different deployment scenarios, but then don't actually reference that config. Also, it seems these deploy a 2-unit gluster cluster but don't actually exercise that deployment. Please consider beefing up the test_x() methods to verify the deployment is acting as it sho...

[Chris Holcombe (xfactor973)]

Thanks! I'll get to work on getting these changes posted. You raised some great points that I didn't think about.

[Andrew Wilkins (axwalk)]

Chris, in case you're not aware: there is now a storage feature built into Juju (1.24+): https://jujucharms.com/docs/devel/wip-storage. It would be great if this could be used as an alternative to specifying the bricks in service config. I expect you'd need both, unless you wanted to require Juju 1.24+.

I don't know Rust, so it'd probably take me a while to figure out how to add it myself, but to add support you'd need to do the following.

Update metadata.yaml with something like:
    storage:
        brick:
            type: block
            multiple:
                range: 1+

Then you'd need to supply another hook, "brick-storage-attached", which is fired whenever a new block device is added. Use "storage-get location" to get the block device's path. The "install" hook would not be run until at least one block-device is attached.

To deploy, you'd run:
    # deploy with 5x 100G volumes, using the provider's native storage (e.g. EBS, Cinder, ...)
    juju deploy gluster --storage brick=5,100G

[Chris Holcombe (xfactor973)]

Hi Andrew,
I looked at the storage feature in juju. I remember why now I didn't go forward with it. I was a little worried about the 'WIP' status of it. I definitely want to add support for this into the charm and I think it would go far towards resolving the issue with the configuration that Kevin brought up. I'd like to ask if I could add that feature in a new bug/pull request. That will give me some time to understand how it works and make sure it fits the charms need. :)

[Chris Holcombe (xfactor973)]

Here's the public version of my pastebin. I didn't realize they were private:
Juju Output: https://gist.github.com/cholcombe973/ea99f3eebf5adef8465a

Vol info: https://gist.github.com/cholcombe973/368b1b2bea0cdbf0ce52

[Chris Holcombe (xfactor973)]

@axwalk
The charm now supports juju storage!

I updated the charm to support juju storage and also be deployable on xenial. I've tested on EC2 so mileage may vary on other cloud or metal services. I plan on adding a mojo spec soon to exercise the charm more.

[Chris Holcombe (xfactor973)]

Source code can be found here: https://github.com/cholcombe973/gluster-charm The new juju push/publish functionality allows me to get rid of the launchpad mirror I was using.

[Charles Butler (lazypower)]

@cholcombe - I've moved the status of this bug from "in progress" to "fix committed" so it will be ingested by the review queue.

Someone will be along shortly to review your work. Thanks for your patience and the contribution

[Chris MacNaughton (chris.macnaughton)]

Some bad news: This won't compile on the latest stable, so either needs to be updated, or have the readme specify which Rust versions it works with.

Once I can compile this, I can continue reviewing it :)

[Chris MacNaughton (chris.macnaughton)]

Ok, looks like it was a dependency error, should document the dependency on libudev-dev

[Chris MacNaughton (chris.macnaughton)]

Status messages show 'Installing charm software' even after the cluster is live

[Chris Holcombe (xfactor973)]

Excellent feedback! I'll work on getting the update-status message to show better details about the health of the cluster. I cleaned up the 'Installing charm software' message. I also updated the README.md to include more information about juju storage and the dependencies required to compile the charm.

[Chris MacNaughton (chris.macnaughton)]

+1 to the updated version on Github at https://github.com/cholcombe973/gluster-charm , how do we handle promulgation now that l-p ingestion is gone?

[Chris Holcombe (xfactor973)]

I pushed it using the charm push / publish tools. Good to go :)

[Chris Holcombe (xfactor973)]

Mojo spec for testing has been posted here: https://code.launchpad.net/~xfactor973/openstack-mojo-specs/gluster/+merge/298845 I had to use mojo because amulet doesn't support juju-storage.

[Cory Johns (johnsca)]

This charm has now been promulgated. It is available at https://jujucharms.com/gluster/ and can be deployed with:

  juju deploy cs:gluster

The charm is multi-series and supports both --series=trusty and --series=xenial