Merge lp://qastaging/~johnsca/charm-helpers/multi-unit into lp://qastaging/~cf-charmers/charm-helpers/cloud-foundry

Reviewers: mp+221801_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/96680043/

Affected files (+105, -28 lines):
   A [revision details]
   M charmhelpers/contrib/cloudfoundry/contexts.py
   M charmhelpers/core/services.py
   M tests/contrib/cloudfoundry/test_render_context.py
   M tests/core/test_services.py

Please take a look.

https://codereview.appspot.com/96680043/

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.py#newcode239
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.py#newcode298
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.

https://codereview.appspot.com/96680043/

On 2014/06/04 20:57:39, benjamin.saller wrote:

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode298
> 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.

https://codereview.appspot.com/96680043/

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

https://codereview.appspot.com/96680043/

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.py#newcode298
> > 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.

https://codereview.appspot.com/96680043/

Please take a look.

https://codereview.appspot.com/96680043/

Please take a look.

https://codereview.appspot.com/96680043/

Please take a look.

https://codereview.appspot.com/96680043/

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/cloudfoundry/contexts.py
File charmhelpers/contrib/cloudfoundry/contexts.py (right):

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode36
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/cloudfoundry/contexts.py#newcode69
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.py#newcode210
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.py#newcode308
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.

https://codereview.appspot.com/96680043/

On 2014/06/06 16:43:12, benjamin.saller wrote:

https://codereview.appspot.com/96680043/diff/80001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode36
> 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/cloudfoundry/contexts.py#newcode69
> 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.py#newcode210
> 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.

https://codereview.appspot.com/96680043/

Please take a look.

https://codereview.appspot.com/96680043/

Thanks, notes follow.

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

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py#newcode259
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/services.py#newcode268
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.

https://codereview.appspot.com/96680043/

Please take a look.

https://codereview.appspot.com/96680043/

+1 LTGM, thanks for talking through that issue.

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

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

https://codereview.appspot.com/96680043/