Fixed relation ordering issue

Fixed relation ordering issue

Fixed an relation hook ordering dependency by refactoring to use
callbacks in charmhelpers.core.services

https://code.launchpad.net/~johnsca/charms/trusty/cf-cloud-controller/services-callback-fu/+merge/220741

(do not edit description out of merge proposal)

[johnsca, 2014-05-23 01:35:00]

Please take a look.

[benjamin.saller, 2014-05-23 15:50:09]

Thanks I like this, I'll approve it pending the answer to the one question

https://codereview.appspot.com/100680044/diff/1/hooks/hooks.py
File hooks/hooks.py (right):

https://codereview.appspot.com/100680044/diff/1/hooks/hooks.py#newcode18
hooks/hooks.py:18: def register():
why the containing method? it seems fine to register with hooks on import

https://codereview.appspot.com/100680044/diff/1/hooks/hooks.py#newcode41
hooks/hooks.py:41: 'router-relation-changed')(services.reconfigure_services)
The other pattern is more familiar but I think we could get people used to this
and it has the advantage of being very concise. :)

[johnsca, 2014-05-26 21:19:56]

Please take a look.

[whit.morriss, 2014-05-27 15:28:18]

LGTM. nice and concise.

https://codereview.appspot.com/100680044/diff/20001/hooks/charmhelpers/core/s...
File hooks/charmhelpers/core/services.py (left):

https://codereview.appspot.com/100680044/diff/20001/hooks/charmhelpers/core/s...
hooks/charmhelpers/core/services.py:5: SERVICES = {}
As discussed with Cory: we are abusing module scope and globals to emulate
object behavior.  I feel the code will age and extend better if we sacrificed a
tiny bit of api convenience and used idiomatic python classes here.

[johnsca, 2014-05-27 17:37:56]

Please take a look.

[benjamin.saller, 2014-05-27 18:06:56]

So this is reviewed in the context of a charm, but its nice to see the top level
singleton thing play out. 

I have some more bikeshedding suggestions, but they are only that. Feel free to
land this and the helpers changes. They 

LGTM +1

Thanks

https://codereview.appspot.com/100680044/diff/40001/hooks/charmhelpers/core/s...
File hooks/charmhelpers/core/services.py (right):

https://codereview.appspot.com/100680044/diff/40001/hooks/charmhelpers/core/s...
hooks/charmhelpers/core/services.py:9: class ServiceManager(object):
Even though we only use this as a Singleton this is almost certainly the proper
way to evolve the code. Better to do this now (because its a minor api change)
than later, thanks.

https://codereview.appspot.com/100680044/diff/40001/hooks/charmhelpers/core/s...
hooks/charmhelpers/core/services.py:21: "data_lost": <one or more callbacks>,
Not sure I love data_ready/data_lost. Still I feel like I'm bike shedding and
whit already reviewed this and was ok with the naming so I'll pass if you don't
like these better 

'requires': [],
'complete': []
'incomplete': []

another option is to keep with the naming we use in relations though I'll admit
'joined' is a little hard to understand in this context, so maybe

'requires', 'changed', 'broken'

I do think 'broken' is a good name, but it needs something that fits
conceptually with it. At this point though I do think what you have is
understandable and nicely documented.

https://codereview.appspot.com/100680044/diff/40001/hooks/config.py
File hooks/config.py (right):

https://codereview.appspot.com/100680044/diff/40001/hooks/config.py#newcode8
hooks/config.py:8: 
Yeah, not bad.

[johnsca, 2014-05-27 18:41:34]

On 2014/05/27 18:06:56, benjamin.saller wrote:
> Not sure I love data_ready/data_lost. Still I feel like I'm bike shedding and
> whit already reviewed this and was ok with the naming so I'll pass if you
don't
> like these better 
> 
> 'requires': [],
> 'complete': []
> 'incomplete': []
> 
> another option is to keep with the naming we use in relations though I'll
admit
> 'joined' is a little hard to understand in this context, so maybe
> 
> 'requires', 'changed', 'broken'
> 
> I do think 'broken' is a good name, but it needs something that fits
> conceptually with it. At this point though I do think what you have is
> understandable and nicely documented.

Yeah, we talked about nomenclature quite a bit on Friday.  Whit's point was that
our "dependencies" list was overloaded and actually serves two purposes:

  1) Define what the service needs to operate that it can't get for itself
(relations, config data, etc)
  2) Collect all of that information into a unified view

Naming it "dependencies" or "requirements" makes that second aspect non-obvious,
and it would be best for the API to be, as much as possible, self-documenting. 
Given that, "required_data" was the best we could come up with, as it conveys
both that this is a list of required things, and that those things provide data
somehow.  This also plays into the ability to, instead of having a semi-opaque
StaticContext class, you can now just drop a plain dict (or the results of
config()) in there directly and it will do what you expect.

Then, I also realized that I wanted to separate the "ready" (or "complete," or
whatever) event from the "start" event, so that the charms can just worry about
defining what happens between everything the service needs being available and
spinning up the service itself, without having to worry about the details of
spinning it up by manually specifying host.service_start every time, since 90%
of charms should be using Upstart or some other SysV-compatible service manager.

So, then we end up with the events of interest:

  * All of the requirements have been satisfied and the data is available to
configure and start the service
  * Something changed, and we no longer have complete information for running
the service

The names we chose, "data_ready" and "data_lost" seemed to capture those ideas
while matching up well to "required_data."  Additionally, "data_lost" specially
calls out the fact that it doesn't need to be triggered in the case that we're
still working to get set up and we've never had the service set up, so nothing
needs to be done to tear it down.

I'm not opposed to "complete" and "incomplete," or "complete" and "broken."  But
I also think that it will not be difficult to change the keys later in a
backwards-compatible way (since we just look for one of a set of keys, and it
would be all encapsulated in fire_event).

So, my inclination is to leave it as-is until / if we get more feedback that
"required_data," "data_ready," and "data_lost" are confusing.

[johnsca, 2014-05-29 18:25:40]

Please take a look.

[johnsca, 2014-05-29 18:27:34]

Please take a look.

[johnsca, 2014-05-29 20:20:45]

*** Submitted:

Fixed relation ordering issue

Fixed an relation hook ordering dependency by refactoring to use
callbacks in charmhelpers.core.services

R=benjamin.saller, whit.morriss
CC=
https://codereview.appspot.com/100680044