BIGTOP-2476 Add Zookeeper Charm #15

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
3 participants

petevg commented Jun 17, 2016

Based on the vanilla apache-zookeeper charm, but utlizes Bigtop to
reduce complexity in the charm itself.

@juju-solutions/bigdata: Since I'm still a newb, I'm pushing one more PR w/ the team before submitting upstream. This, I think, is the final, actually working, following all the best practices version of this charm. If you agree, please +1. If I missed something, please let me know, and I'll fix it :-)

BIGTOP-2476 Add Zookeeper Charm
Based on the vanilla apache-zookeeper charm, but utlizes Bigtop to
reduce complexity in the charm itself.
Member

kwmonroe commented Jun 21, 2016

I have a couple issues with the status reporting

  • First, please make all status_set messages lowercase. hookenv.log is fine to have proper sentences with capitalization, but we want all our status_set messages to be lower.
  • Second, I thought the old charm's reporting of suboptimal (non-quorum) status was useful. Please consider adding this functionality back. The old was of doing it was a very simple grep of zoo.cfg:

https://github.com/juju-solutions/layer-apache-zookeeper/blob/master/lib/charms/layer/zookeeper.py#L183

Followed by setting the appropriate message if we had a suboptimal quantity of peers:

https://github.com/juju-solutions/layer-apache-zookeeper/blob/master/reactive/zookeeper.py#L44

If you brought this back, you could add a reactive handler like this:

@when('zookeeper.started')
def update_status():
    num = <num_peers>
    if num is good:
        status_set('active', 'ready')
    else:
        status_set('active', 'suboptimal number of peers (num)')
status_set messages now lower case
This matches convention, per @kwmonroe

petevg commented Jun 21, 2016

Fixed the status messages.

I think that, with the messaging complexity we've introduced by not doing automatic restarts, I'd rather skip the notifications about inadequate numbers for quorum for now.

When we add the automated restarts back in, though, I like the idea of adding those messages back in as well.

Added messages notifying operator when quorum is suboptimal.
This is a port of functionality from the pre Bigtop charm.

petevg commented Jun 21, 2016

@kwmonroe I lied about the messing being too complex. Functionality from the old charm ported over. :-)

+
+def main():
+ LOG("starting restart handler main.")
+ hookenv.status_set('waiting', 'Restarting ...')
@johnsca

johnsca Jun 22, 2016

Owner

Also, it should be maintenance instead of waiting.

+ hookenv.status_set('waiting', 'Restarting ...')
+ zookeeper = Zookeeper()
+ zookeeper.install()
+ hookenv.status_set('active', 'Ready {}'.format(zookeeper.quorum_check()))
+ zookeeper.open_ports()
+ set_state('zookeeper.installed')
+ set_state('zookeeper.started')
+ hookenv.status_set('active', 'Ready {}'.format(zookeeper.quorum_check()))

petevg added some commits Jun 22, 2016

Refactored to use zkpeers.joined as our canonical state
Fixes headaches with writing files out to disk, and is more in line w/
best practices.
+ LOG("starting restart handler main.")
+ hookenv.status_set('waiting', 'Restarting ...')
+ zookeeper = Zookeeper()
+ zookeeper.install()
@johnsca

johnsca Jun 22, 2016

Owner

This should be predicated on the zookeeper.started state and should action_fail if it's not set.

petevg and others added some commits Jun 22, 2016

Fixed a couple of bugs found after more extensive testing.
We strip off our own node when running data_changed, which prevents a
false positive on first run.

We handle the case where a node is restarting when we are checking for
leadershp a bit more gracefully.
Fixed wording on message.
"even number is" -> "an even number is"
Changed "waiting" state to "maintenance"
Fits better with what the states are meant for.

Also snuck in one more uppercase -> lowercase fix.
+ Run smoke tests.
+ '''
+ for unit in self.d.sentry['zookeeper']:
+ unit.action_do("smoke-test")
@kwmonroe

kwmonroe Jun 27, 2016

Member

there is no smoke-test action in this charm.. this isn't necessarily a bad thing since it will just pick up the default smoke-test from here:

https://github.com/juju-solutions/layer-apache-bigtop-base/blob/master/actions/smoke-test

just wanted to make sure you knew this would happen, and that there was no custom smoke-test handling required for zk.

@petevg

petevg Jun 28, 2016

@kwmonroe Yep. The default smoke test action is just fine for Zookeeper -- we don't need to do any special setup.

petevg commented Jun 28, 2016

I've opened a PR upstream, so closing out this PR.

@petevg petevg closed this Jun 28, 2016

@kwmonroe kwmonroe deleted the zookeeper branch Aug 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment