Please review the galera-cluster charm

[Philip Stoev]

Please review the galera-cluster charm.

[Charles Butler (lazypower)]

Greetings Philip

Thank you for the contribution to the Galera-Cluster charm! This looks like you're off to a great start. I've taken a moment to look over this submission and I've come up with the following notes:

When I ran through the documented process to deploy the galera cluster - I was left with the galera-cluster charm in an error state, with nothing present in the logs to indicate as to why it had failed. Subsequent `juju resolved -r` retry attempts did not complete successfully.

I see that there are hook unit-tests in the charm, and that's a brilliant finding! Ideally we would also like to see a deployment test written in either amulet or mojo to exercise the configuration options so we can introduce the charm into our CI infrastructure to ensure that subsequent merges do not break the charms functionality, and have a good basis of comparison when the next stable series lands so we can promote the charms automatically that have passing tests.

I like how well documented the charm is in the README. When running 'charm proof' - I received only I: messages - which are prefectly acceptable outcomes, and are to be treated as Informational, nice-to-have messages and areas of improvement. The missing icon.svg file is a good one to address before final acceptance into the charm store.

The charm does appear to have immutable configuration - which would need to be addressed before it passes the final review as well. During the code review I saw a #TODO note left at the top of the file that the 'sstuser and root' passwords need to support changes, so I feel that you have a good grasp on this being a requirement.

All in all there's only a few minor things to correct and implement before this charm is ready to hit the lime light. You're nearly there in terms of having a solid store-ready submission barring a few modifications and will have this ready in no time.

Thanks again, and all the best

Charles

[Philip Stoev (philip-stoev-f)]

Hello. Thanks for reviewing the charm.

Please note that the galera-cluster charm is a fork of the percona-cluster charm, created by Canonical, with only very minor changes, as Galera Cluster and Percona Cluster are almost identical products. The contents of the charm, the tests and the TODO comments all come from Canonical/Ubuntu, mostly created by James Page.

With respect to the failed deployment, I tried it today and it works for me. Can you help me debug the failure you are getting:
- what cloud provider are you using?
- can you provide the output from ps ax on the machine?
- can you attach /var/log/juju/* and /var/log/syslog to the bug?

Due to the size of the packages being downloaded, the charm tends to stay in a "pending" status for a few minutes .

With respect to the rest of your review comments:

* Can you point me to a charm that already contains a deployment test that would fit your requirements?

* Would it be possible for the charm to be accepted as is with respect to the immutable configuration options? For example, the "SST password" option is nothing something users would need to change dynamically, it is sort of a shared-secret that nodes use to connect to each other internally.

[Cory Johns (johnsca)]

Philip,

I was able to deploy successfully to AWS, so it's unclear what was causing the error that Charles was seeing. I am disinclined to consider it blocking, since I can't see anything in your changes from the upstream that would cause such a failure.

The immutable config is an issue, as it goes against the guidelines (https://jujucharms.com/docs/1.20/authors-charm-best-practice#juju-best-practices-and-tips-from-canonical's-infrastructure-team). However, you are correct that it is an issue with the upstream charm, so I opened a bug against percona-cluster (https://bugs.launchpad.net/charms/+source/percona-cluster/+bug/1436093) and perhaps a joint effort can be made to resolve this issue in both charms at once.

As for the functional test, we generally recommend something like an Amulet test that deploys based on the instructions from the README and performs at least some basic checks to confirm that the service is functioning as expected. An example from the apache2 charm is: http://bazaar.launchpad.net/~charmers/charms/trusty/apache2/trunk/view/head:/tests/20-mpm-test.py A functional test of this form would almost certainly apply equally well to the upstream charm, so again, perhaps it can be a joint effort.

[Matt Bruzek (mbruzek)]

Hello Philip,

Thanks for working on this charm. The previous two reviews have brought up some valid points that I feel should be addressed.

We tend to avoid immutable configuration as this breaks the user experience. The Juju user has no feedback after a configuration option is set to know if the value change was successful. Also the user has no idea which configuration options are immutable or “regular” configuration. We do however, allow immutable configuration in rare cases. I would ask for a few more things: Please confirm/research if there is any way to change the “SST password” after a deployment has started. Notice this config-changed event runs all all units of the cluster. If there is not a way to change the value after deployment. Does this even need to be an option? Do users expect to change this, or could it be hard coded?

Secondly if there is no way to make this configuration option change once deployed, then please call that out clearly in the README.md. We usually like to see a section describing all the configuration options in more detail than config.yaml. If this configuration option is not able to change after deployment, it is different than all other configuration options and should be noted in the README.

There were a few additional things with the review that I would like to point out.

# charm proof
All the messages from charm proof were “I” or informational. While technically “I” messages are OK, we would like to resolve those messages if possible.

$ charm proof
I: No icon.svg file.
I: config.yaml: option root-password has no default value
I: config.yaml: option source has no default value
I: config.yaml: option sst-password has no default value
I: config.yaml: option access-network has no default value
I: config.yaml: option vip has no default value

# icon.svg
It looks like Galara Cluster has an icon that would make this charm stand out and easily identify this charm. When I go to http://galeracluster.com I see a “G” with a circle around it as their icon. Could you try to create an icon for this charm? https://jujucharms.com/docs/1.20/authors-charm-icon

# README.md
There were a few misspelled words in the readme, repositories, minimum, Limitations and channeled. Also please consider adding a section on configuration.

 # tests
Trusty charms require tests. As was pointed out in the earlier reviews, we would like to see a `tests` directory that contains a deployment test of this charm. Tests in this format will be run in our automated testing infrastructure and help ensure the charm deploys and is of high quality.

# Conclusion

I am going to move this bug to “In Progress” while you address the comments here. When you are ready for another review please put the bug back in “New” or “Fix Committed” and someone will take a look at it shortly.

Thanks again!

[James Page (james-page)]

As this charm is based on one of my teams (percona-cluster), I'll comment on:

I: config.yaml: option root-password has no default value
I: config.yaml: option source has no default value
I: config.yaml: option sst-password has no default value
I: config.yaml: option access-network has no default value
I: config.yaml: option vip has no default value

right now, its not possible to generate passwords (root and sst) in cluster services without avoiding horrid race conditions; leadership election will help us in this respect, but for now the safest way to assure that the same password is used across all units is to provide it via config, for which we must *not* provide defaults for security reasons.

'source': optional - PPA or suchlike.

'access-network': optional network to use for access to MySQL

'vip': environment specific configuration for clustering using HA cluster - again no sane default.

These all exist in the parent charm, so I would ack them for galera-cluster as well.

[Philip Stoev (philip-stoev-f)]

Hello,

I have addressed the following issues with the charm so I would like to submit it for review once more:

* Added a test based on amulet
* Added additional documentation around the important configuration options
* fixed typos in Readme
* Added a logo

I think at this time it is safer to not allow dynamic changes to the root and SST passwords. We need to accumulate experience with Juju and the charm before we can implement such functionality.

Thank you!

[Cory Johns (johnsca)]

Phillip,

The changes look good, and the new test looks good and passes as expected. However, to work properly with the automated test runner, the 10-deploy-test.py file should be marked executable.

Regarding the passwords, I do think that we can and should avoid the immutable options in this case. I see your point that the SST password is not intended to be modified by the Juju Admin, so my recommendation there would be to generate the password inside the charm and communicate it to the other nodes via the peer relation. To get consistent agreement on the value, you will need some form of leader election, but for this simple case, a "poor man's" leader election in the form of always deferring to the value sent by a lower-numbered unit seems like it would be sufficient.

Perhaps I'm missing something, but I don't see a reason why the MySQL root password, if it is configurable at all, shouldn't be modifiable with subsequent `juju set` commands. OTOH, you could take the route of the MySQL charm (https://jujucharms.com/mysql/) and generate it in the charm as well to not have it as a config option at all.

[Philip Stoev (philip-stoev-f)]

I have made the following changes:

* the mysql root and SST passwords can both be changed at runtime
* the test script is now executable

Please consider the charm again. Thank you.

[Philip Stoev (philip-stoev-f)]

Would it be possible to review the charm once more? I would like to clean up any remaining issues as soon as possible.

Thank you.

[Charles Butler (lazypower)]

Greetings,

I see this is in the queue, and I will be reviewing shortly. Thanks for your patience!

[Tim Van Steenburgh (tvansteenburgh)]

+1 on the latest changes; tests all passing. I added a few test/lint fixups on my own branch (https://code.launchpad.net/~tvansteenburgh/charms/trusty/galera-cluster/test-fixups), then promulgated this charm to the store from there.

Congrats Philip, and thanks for the submission!