Refactored RelationContext multi-unit support

Refactored RelationContext multi-unit support

Added a little magic to the RelationContext to make it better able to support
multiple units and still DWIM in both cases.  This might be too magical,
though.

https://code.launchpad.net/~johnsca/charm-helpers/multi-unit/+merge/221801

(do not edit description out of merge proposal)

[johnsca, 2014-06-02 22:16:30]

Please take a look.

[johnsca, 2014-06-03 16:02:01]

Please take a look.

[benjamin.saller, 2014-06-04 20:57:39]

Thanks for this. I include some thoughts below, I hope they are agreeable.

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services...
charmhelpers/core/services.py:239: 
I make suggestions below on altering this process. Let me know what you think.

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services...
charmhelpers/core/services.py:298: over the units, not the items of the default
unit.
There is a rule I try to use when generating/process data these days, no data
should live in the key side of the mapping. In the context of a template for
iteration is simpler to iterate

context[interface][{_unit: 'unit/0', 'foo': 'bar'},
                   {_unit: 'unit/1', 'foo': bar}]

This rule makes additional sense when thinking that knowing the unit names to
request them would imply a broken charm and thus shouldn't be an index in this
context. I'd suggest transitioning to that. We could also make the rule (and
document here) that only elements with complete data appear in the list.

[johnsca, 2014-06-04 21:09:21]

On 2014/06/04 20:57:39, benjamin.saller wrote:
>
https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services...
> charmhelpers/core/services.py:298: over the units, not the items of the
default
> unit.
> There is a rule I try to use when generating/process data these days, no data
> should live in the key side of the mapping. In the context of a template for
> iteration is simpler to iterate
> 
> context[interface][{_unit: 'unit/0', 'foo': 'bar'},
>                    {_unit: 'unit/1', 'foo': bar}]
> 
> This rule makes additional sense when thinking that knowing the unit names to
> request them would imply a broken charm and thus shouldn't be an index in this
> context. I'd suggest transitioning to that. We could also make the rule (and
> document here) that only elements with complete data appear in the list.

Can you explain your reasoning behind making use of the key?  I'm not averse to
changing it to being a list instead of a mapping, but using a "special" field to
hold the unit name ('_unit', in your example) seems more dangerous, since it
could potentially conflict with actual fields, even if unlikely due to the
underscore prefix.

With your comment that the charm shouldn't actually know / care about the unit
name, maybe we can just leave it out altogether, especially if it only contains
"complete" data sets.

[johnsca, 2014-06-04 21:10:01]

That should be: "behind *not* making use of the key"

[benjamin.saller, 2014-06-04 21:22:03]

On 2014/06/04 21:09:21, johnsca wrote:
> On 2014/06/04 20:57:39, benjamin.saller wrote:
> >
>
https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services...
> > charmhelpers/core/services.py:298: over the units, not the items of the
> default
> > unit.
> > There is a rule I try to use when generating/process data these days, no
data
> > should live in the key side of the mapping. In the context of a template for
> > iteration is simpler to iterate
> > 
> > context[interface][{_unit: 'unit/0', 'foo': 'bar'},
> >                    {_unit: 'unit/1', 'foo': bar}]
> > 
> > This rule makes additional sense when thinking that knowing the unit names
to
> > request them would imply a broken charm and thus shouldn't be an index in
this
> > context. I'd suggest transitioning to that. We could also make the rule (and
> > document here) that only elements with complete data appear in the list.
> 
> Can you explain your reasoning behind making use of the key?  I'm not averse
to
> changing it to being a list instead of a mapping, but using a "special" field
to
> hold the unit name ('_unit', in your example) seems more dangerous, since it
> could potentially conflict with actual fields, even if unlikely due to the
> underscore prefix.
> 
> With your comment that the charm shouldn't actually know / care about the unit
> name, maybe we can just leave it out altogether, especially if it only
contains
> "complete" data sets.

I think for our use case we don't actually need the unitname at all and could
omit it, in the more general case I expect that there are cases where people
will want to delta the relation data vs previous hook calls (cached data) before
taking some actions.

The list vs dict thing is a pattern that I picked up working in the machine
learning space. It tends to be easier to consume and pivot flat tuple style
data. I was looking for a nice right up justifying this pattern, its even known
as "<someones> law" but I can't remember that name :-/ 

However if the value in the key side of the mapping can't be known/used its a
good indicator to me that we can flatten the structure. Maybe not very
persuasive.

[johnsca, 2014-06-05 17:36:34]

Please take a look.

[johnsca, 2014-06-05 17:48:44]

Please take a look.

[johnsca, 2014-06-06 16:07:44]

Please take a look.

[benjamin.saller, 2014-06-06 16:43:12]

Thanks for this, very close. Mostly this comes down to me wanting another pass
over the examples/docs around the multi-unit data access. 

Hopefully I pointed to what I am after clearly, if not please let me know.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloud...
File charmhelpers/contrib/cloudfoundry/contexts.py (right):

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloud...
charmhelpers/contrib/cloudfoundry/contexts.py:36: required_keys = ['address',
'port', 'user', 'password']
You have a follow on for the charms updating the templates as well?

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloud...
charmhelpers/contrib/cloudfoundry/contexts.py:69: required_keys = ['hostname',
'port']
I am a little cautious about this one as this isn't really a CF component, it
would be interesting if down the road we had something like 


EtcdInterface = charmhelpers.interfaces.Interface('etcd')

To produce this class from a interface registry. We'll want that sort of thing
for the interface testing plans anyway.

just rambling, nothing to do now.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services...
charmhelpers/core/services.py:210: """
This feels a bit magical. I'm fine with letting it in but the 'why' you'd use it
both ways should be explained a little more below. See my following comment
about the example being a bit unclear.

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services...
charmhelpers/core/services.py:308: from unit 'wordpress/0'.
I worry that this example isn't clear. self.interface and the 'foo' key are
assumed.

I'd like two examples here in the style of 

'if you need to iterate all the units of a relation, for example in a template,
you can ...'

'if you want the keys and values of any matching unit containing all the
expected keys of an interface, you can ...'


sorry to ask you to rephrase this, and thank you.

[johnsca, 2014-06-06 17:41:11]

On 2014/06/06 16:43:12, benjamin.saller wrote:
>
https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloud...
> charmhelpers/contrib/cloudfoundry/contexts.py:36: required_keys = ['address',
> 'port', 'user', 'password']
> You have a follow on for the charms updating the templates as well?

Yes, as part of the port conflict resolution.  Just testing and hashing out bugs
currently.

>
https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloud...
> charmhelpers/contrib/cloudfoundry/contexts.py:69: required_keys = ['hostname',
> 'port']
> I am a little cautious about this one as this isn't really a CF component, it
> would be interesting if down the road we had something like 
> 
> 
> EtcdInterface = charmhelpers.interfaces.Interface('etcd')
> 
> To produce this class from a interface registry. We'll want that sort of thing
> for the interface testing plans anyway.

I had considered having RelationContext.__init__ accept the interface name and
required keys as parameters so that you don't have to create stub subclasses,
and only need to subclass if you want to add extra behavior (e.g., the
MysqlRelation).  I think this is the right way to go, and will do so.


>
https://codereview.appspot.com/96680043/diff/80001/charmhelpers/core/services...
> charmhelpers/core/services.py:210: """
> This feels a bit magical. I'm fine with letting it in but the 'why' you'd use
it
> both ways should be explained a little more below. See my following comment
> about the example being a bit unclear.

I agree.  I am very on the fence as to whether to stick with the "magical"
implementation vs creating separate classes, or even just forcing it to always
be a list to force charm authors to always consider the possibility that there
may be more units.

The reason I didn't go with separate classes is because I didn't want to have to
duplicate all the stub RelationContext subclasses, but if we make it so the
interface and keys can be passed in, we can avoid creating 95% of the stub
classes, so that'd be ok.

But now I'm thinking that forcing it to be a list might be better, since charm
authors really should keep in mind that the admin could throw more units at the
relation.

[johnsca, 2014-06-06 18:26:43]

Please take a look.

[benjamin.saller, 2014-06-06 18:48:31]

Thanks, notes follow.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/service...
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/service...
charmhelpers/core/services.py:259: order: 'wordpress/0', 'wordpress/1',
'mediawiki/0'.
I think you do a fine job of explaining this now, but the use-case for it is
still unclear to me. 

If there is more than one relation with the same interface I don't think we can
silently combine those lists (the data is coming from different relation ids).
They represent different services and should expect different orchestration. 

I'd go so far as to say this is a whole in our model. 

In CF mysql has relations to UAA and CC but doesn't use the services framework
to manage this. If it did and iterated those units uniformly havoc could follow,
no?

The default case isn't this and we built for it, but unless my thinking here is
muddy, we'd need to do something different here. Currently the relation id
information would be lost. We'd at minimum have to put the rid in the data, or
break the units into collections by relation. 

Breaking them up by relation is to my mind similar to always processing this as
a list (rather than the default unit notion of before), its the more complex
case, but its one that some services need to be aware of. 

I apologize for not picking up on this sooner. If you'd like to talk about this
in the hangout let me know.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/service...
charmhelpers/core/services.py:268: {%- endfor %}
This is good, thanks, it might have to turn into 

interface[0][0]['key']

for interface[first relation][first unit][key]

lets talk.

[johnsca, 2014-06-06 19:44:16]

Please take a look.

[benjamin.saller, 2014-06-06 19:45:47]

+1 LTGM, thanks for talking through that issue.

https://codereview.appspot.com/96680043/diff/120001/charmhelpers/core/service...
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/120001/charmhelpers/core/service...
charmhelpers/core/services.py:272: set of data came from, you'll need to extend
this class to preserve
Great, thanks