Bug #1551133 “RLEC Charm Submission” : Bugs : Juju Charms Collection

Revision history for this message

Marco Ceppi (marcoceppi) wrote on 2016-03-09:

#1

Thanks for the submission, sorry for the delay we're experiencing higher than normal volume of charms in our review system. I'm taking a look at this now and will report back soon with my review!

Revision history for this message

Redis Labs (redislabs-team) wrote on 2016-03-10:

#2

Thank you Marco, please keep us updated.
Additionally, there are currently 2 'rlec' charms in the Community charms catalog- one by 's-matan' and one by 'redislabs'. It seems that it happened because I changed the account name to 'redislabs' so the charm will be presented by redislabs.

The bottom line is that the redundant rlec charm by 's-matan' should be removed- would you be able to assist with this?
Thanks!

Revision history for this message

Cory Johns (johnsca) wrote on 2016-03-10:

#3

Matan,

Thank you for your submission to the charm ecosystem! It looks like a great start, but I do have some issues that need to be addressed.

When downloading the RLEC payload from S3, you're not doing any checksum verification to ensure that the file you get is the file you expect. Because we consider this very important for security and data integrity reasons, we have some helpers available in the charmhelpers.fetch package to make this easy. You can use either the high level install_remote [1] function, or the lower-level ArchiveUrlFetchHandler.download_and_validate [2] method (I would recommend the former).

[1]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.install_remote
[2]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.archiveurl.ArchiveUrlFetchHandler.download_and_validate

You only handle the "is_leader" case in the install hook, but it is possible that the leader can change. For instance, if the original leader dies or is removed using remove-unit, a new leader will need to be elected to take its place. It would be better if you moved that bit of code from the install hook to the leader-elected hook. Note that that will only be called on the newly elected leader, and is guaranteed to be called at least once.

Your node-relation-departed hook posts to a web service to remove the departing node, but I don't see anywhere that you post to register the node. Is that something that happens implicitly? I do notice that your node-relation-changed hook has the leader put all of the peers in to the leadership settings, but then that setting is never used. I wonder if that was intended to be the registration step?

Your config-change hook seems to be left-over boilerplate from the `charm create` template. It doesn't seem to do anything to update the service configuration based on the new config values, meaning the charm will not respond to the user changing those values after deployment (something we call "immutable config"). It seems that it should update the config file and possibly restart the service. Also, because of the boilerplate, it is currently generating a "dummy" Upstart job, which is not something you want, so I'd also recommend removing the templates directory and the hooks/services.py file.

Finally, your charm doesn't have any relations to enable it to connect to other services. I would recommend that you add support to your charm for the "redis" interface so that it can provide this service to all of the other existing charms that require Redis [3]. It looks like all you would need to provide on that relation is the master_node_ip, port, and password.

[3]: https://jujucharms.com/requires/redis

Again, thank you for this contribution. I look forward to seeing your updates to address these items!

Matan,

Thank you for your submission to the charm ecosystem!  It looks like a great start, but I do have some issues that need to be addressed.

When downloading the RLEC payload from S3, you're not doing any checksum verification to ensure that the file you get is the file you expect.  Because we consider this very important for security and data integrity reasons, we have some helpers available in the charmhelpers.fetch package to make this easy.  You can use either the high level install_remote [1] function, or the lower-level ArchiveUrlFetchHandler.download_and_validate [2] method (I would recommend the former).

[1]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.install_remote
[2]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.archiveurl.ArchiveUrlFetchHandler.download_and_validate

You only handle the "is_leader" case in the install hook, but it is possible that the leader can change.  For instance, if the original leader dies or is removed using remove-unit,  a new leader will need to be elected to take its place.  It would be better if you moved that bit of code from the install hook to the leader-elected hook.  Note that that will only be called on the newly elected leader, and is guaranteed to be called at least once.

Your node-relation-departed hook posts to a web service to remove the departing node, but I don't see anywhere that you post to register the node.  Is that something that happens implicitly?  I do notice that your node-relation-changed  hook has the leader put all of the peers in to the leadership settings, but then that setting is never used.  I wonder if that was intended to be the registration step?

Your config-change hook seems to be left-over boilerplate from the `charm create` template.  It doesn't seem to do anything to update the service configuration based on the new config values, meaning the charm will not respond to the user changing those values after deployment (something we call "immutable config").  It seems that it should update the config file and possibly restart the service.  Also, because of the boilerplate, it is currently generating a "dummy" Upstart job, which is not something you want, so I'd also recommend removing the templates directory and the hooks/services.py file.

Finally, your charm doesn't have any relations to enable it to connect to other services.  I would recommend that you add support to your charm for the "redis" interface so that it can provide this  service to all of the other existing charms that require Redis [3].  It looks like all you would need to provide on that relation is the master_node_ip, port, and password.

[3]: https://jujucharms.com/requires/redis

Again, thank you for this contribution.  I look forward to seeing your updates to address these items!

Marco Ceppi (marcoceppi) on 2016-03-16

Changed in charms:
status:	New → Incomplete

Revision history for this message

Review Queue (review-queue) wrote on 2016-03-19: AWS Test Results: RLEC Charm Submission

#4

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/3211/

Revision history for this message

Redis Labs (redislabs-team) wrote on 2016-03-27:

#5

Hi Cory,

Thanks for your feedback and sorry for the delayed response, we have a lot on our plate currently.

1. I added a checksum verification to the install hook, using the ArchiveUrlFetchHandler.install method. It seems that the high level install_remote method has some issues running on trusty.

2. I added a leader-elected hook. It sets the elected leader's master_node_ip in the leader_set context, which is moved from the install hook.
As bootstrapping the node in a 'create_cluster' mode (which is done in the install hook) should be done only once on the first unit installation, i think it makes sense to do it in the install hook and not in the leader-elected hook (which will be called again whenever a new leader is elected).

3. A new node is registered to the cluster in the node-relation-joined hook- It bootstraps the new node in a 'join_cluster' node. the bootstrapping (of both 'create_cluster' and 'join_cluster' modes) is triggered by writing a specific json file to a specific directory.
The node-relation-changed hook is indeed setting the private address of each unit in the leader_set context, which is then being used in the node-relation-departed hook context:
- The leader will POST a nodes/<id>/remove request to the cluster, which should consist of the RLEC node id that should be departed.
- The departed RLEC node id is retrieved by its private address, from the leader_get context.

4. I removed the templates dir, services.py and the config-changed hook.
Currently, RLEC allows changing the password only (and not the username or the cluster's name) and it can be done via the UI only. We are planning to expose an api for that in the future, and once we do we'll be able to support it via the config-changed hook.

5. The rlec charm provides a cluster and not a redis resource endpoint, therefore it is not providing any relations.
This was indeed a point I wasn't sure about, and discussed it with the charms team. Our understanding was that the correct charm configuration is using peers. Please let me know if you have any ideas regarding.

Please let me know if you have any comments or further questions.

P.S.: please see my previous comment- there are currently 2 rlec charms exposed, and only the one by 'redislabs' should be available. Could you please look into it?

Thanks!