Merge lp://staging/~menno.smits/juju-core/1194481-relation_name_in_status.0 into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+219973_code.launchpad.net,

Message:
Please take a look.

Description:
state/api{,server}: StatusData through client API

Machines and units have a StatusData map which hold extra status
information that a client may use when displaying status but this
wasn't being passed through the client API.

The change is generally useful but is specifically required to support
reporting of the affected relation when a hook fails (coming soon).

The client API test scenario has been extended to include a case of a
unit with an error with status data included.

https://code.launchpad.net/~menno.smits/juju-core/1194481-relation_name_in_status.0/+merge/219973

(do not edit description out of merge proposal)

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

Affected files (+33, -12 lines):
   A [revision details]
   M state/api/client.go
   M state/apiserver/client/api_test.go
   M state/apiserver/client/perm_test.go
   M state/apiserver/client/status.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140515103108-2gr30jafikdtijak
+New revision: <email address hidden>

Index: state/api/client.go
=== modified file 'state/api/client.go'
--- state/api/client.go 2014-05-13 04:30:48 +0000
+++ state/api/client.go 2014-05-16 04:14:32 +0000
@@ -50,6 +50,7 @@
   Err error
   AgentState params.Status
   AgentStateInfo string
+ AgentStateData params.StatusData
   AgentVersion string
   DNSName string
   InstanceId instance.Id
@@ -82,6 +83,7 @@
   Err error
   AgentState params.Status
   AgentStateInfo string
+ AgentStateData params.StatusData
   AgentVersion string
   Life string
   Machine string

Index: state/apiserver/client/api_test.go
=== modified file 'state/apiserver/client/api_test.go'
--- state/apiserver/client/api_test.go 2014-05-13 04:30:48 +0000
+++ state/apiserver/client/api_test.go 2014-05-16 04:14:32 +0000
@@ -152,6 +152,7 @@
     InstanceId: instance.Id("i-machine-0"),
     AgentState: "down",
     AgentStateInfo: "(started)",
+ AgentStateData: params.StatusData{},
     Series: "quantal",
     Containers: map[string]api.MachineStatus{},
     Jobs: []params.MachineJob{params.JobManageEnviron},
@@ -163,6 +164,7 @@
     InstanceId: instance.Id("i-machine-1"),
     AgentState: "down",
     AgentStateInfo: "(started)",
+ AgentStateData: params.StatusData{},
     Series: "quantal",
     Containers: map[string]api.MachineStatus{},
     Jobs: []params.MachineJob{params.JobHostUnits},
@@ -174,6 +176,7 @@
     InstanceId: instance.Id("i-machine-2"),
     AgentState: "down",
     AgentStateInfo: "(started)",
+ AgentStateData: params.StatusData{},
     Series: "quantal",
     Containers: map[string]api.MachineStatus{},
     Jobs: []params.MachineJob{params.JobHostUnits},
@@ -203,20 +206,25 @@
     SubordinateTo: []string{},
     Units: map[string]api.UnitStatus{
      "wordpress/0": api.UnitSta...

I don't think this quite works. It's not about just dumping the internal
state in front of the user and letting them figure it out, it's about
helping them figure out what the problem is.

To be fair, I'm not sure how to show it cleanly (and machine-parseably);
I can come on tonight for a chat if you like.

https://codereview.appspot.com/93480044/diff/1/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/1/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
How does this look in practice? The repetition of the failed hook is
going to be a bit stuttery. And... where in the rest of status do we
output relation ids? I'm not sure we ever even accept them on the
command line. I think we need relation names, and not to expose
status-data at all: if its contents go straight into the user's face we
have much less freedom about how we actually use it in future.

https://codereview.appspot.com/93480044/

Please take a look.

https://codereview.appspot.com/93480044/

I know we need to keep the "(status: info)" bit for old clients, but I
thought we were going to do it properly as well? Otherwise looks good.

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations map[string]RelationStatus
so we're turning int ids into strings to satisfy json? fair enough, I
guess, but I have a bit of the same old "eww" reaction.

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
Were we going to add the components of the "(error: blam)" stuff into
StatusData (or something similar)?

The contents of StatusData are technically arbitrary, but they're
actually pretty well-defined. We could tighten them up to prevent key
collisions with whatever we pick; or we could put the arbitrary data
into its own dict?

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/status.go
File state/apiserver/client/status.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/status.go#newcode536
state/apiserver/client/status.go:536: subordSet.Add(ep.ServiceName)
I was thinking it might be simpler to keep all the relation processing
together, but then I realised that, no, we need to have all the
relations and all the services in memory together before we can do it
smartly.

https://codereview.appspot.com/93480044/

Replies inline.

Given that there is another branch in the works which builds on this
one, adding the extra items to StatusData when required and actually
using them in the client, what's still needed to get this branch ready
for landing?

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations map[string]RelationStatus
On 2014/05/26 06:45:34, fwereade wrote:
> so we're turning int ids into strings to satisfy json? fair enough, I
guess, but
> I have a bit of the same old "eww" reaction.

I discovered the hard way what happens if you try to serialise map[int]
over the API: seemingly hung tests (perhaps due to API retries?)

I didn't like converting the relation ids to strings much either. I was
considering making Relations a []RelationStatus instead and including Id
in RelationStatus. That way the client may end up generating a map
itself which is bit crappy but hey. Would that be preferable?

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
On 2014/05/26 06:45:34, fwereade wrote:
> Were we going to add the components of the "(error: blam)" stuff into
StatusData
> (or something similar)?

Yes that's coming (in a separate branch). This branch is just about
getting the supporting infrastructure in place.

The test data here isn't realistic. This test is just checking that
StatusData is making it where it should the status response.

> The contents of StatusData are technically arbitrary, but they're
actually
> pretty well-defined. We could tighten them up to prevent key
collisions with
> whatever we pick; or we could put the arbitrary data into its own
dict?

Agreed that we should be careful about what we put in StatusData. How
about namespacing the keys along the lines of "category.item" to help
avoid collisions?

https://codereview.appspot.com/93480044/

LGTM with relation ids in the array (assuming followups as discussed).

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations map[string]RelationStatus
On 2014/05/26 21:47:43, menn0 wrote:
> On 2014/05/26 06:45:34, fwereade wrote:
> > so we're turning int ids into strings to satisfy json? fair enough,
I guess,
> but
> > I have a bit of the same old "eww" reaction.

> I discovered the hard way what happens if you try to serialise
map[int] over the
> API: seemingly hung tests (perhaps due to API retries?)

> I didn't like converting the relation ids to strings much either. I
was
> considering making Relations a []RelationStatus instead and including
Id in
> RelationStatus. That way the client may end up generating a map itself
which is
> bit crappy but hey. Would that be preferable?

Hmm... yeah, I think it probably would. I also find myself wondering
whether we should actually be representing the endpoint data as well,
lest clients have to mess around parsing keys? Don't let this block you,
it's addressable in a followup, but take a quick look at the AddRelation
API: what we chose there *may* have bearing on what we should expose
here.

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
On 2014/05/26 21:47:43, menn0 wrote:
> On 2014/05/26 06:45:34, fwereade wrote:
> > Were we going to add the components of the "(error: blam)" stuff
into
> StatusData
> > (or something similar)?

> Yes that's coming (in a separate branch). This branch is just about
getting the
> supporting infrastructure in place.

Great, sorry for the confusion, I fear you will have to get used to me
asking stupid questions to which I should already know the answers :).

> The test data here isn't realistic. This test is just checking that
StatusData
> is making it where it should the status response.

> > The contents of StatusData are technically arbitrary, but they're
actually
> > pretty well-defined. We could tighten them up to prevent key
collisions with
> > whatever we pick; or we could put the arbitrary data into its own
dict?

> Agreed that we should be careful about what we put in StatusData. How
about
> namespacing the keys along the lines of "category.item" to help avoid
> collisions?

Depends what we mean by StatusData -- the stuff that gets set by the
agent, or the stuff we return over status? For the latter, I think it
may be better to separate the explicitly-set-by-agent bits from the
also-relevant-to-status bits.

https://codereview.appspot.com/93480044/

Replies to latest review comments in-line.

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations map[string]RelationStatus
> Hmm... yeah, I think it probably would. I also find myself wondering
whether we
> should actually be representing the endpoint data as well, lest
clients have to
> mess around parsing keys? Don't let this block you, it's addressable
in a
> followup, but take a quick look at the AddRelation API: what we chose
there
> *may* have bearing on what we should expose here.

Funnily enough, my initial implementation did include endpoint data in
RelationStatus but I removed it because the data was mostly repeating
what was already in the relation name. I'm happy to put this back in
though.

Looking at the AddRelation API, it returns a map[string]charm.Relation
where the key is the service name. I'm not sure that it makes sense for
RelationStatus to represent endpoints in a map - a slice probably makes
more sense.

How about each RelationStatus holds a slice of EndpointStatus structs
which have a subset of what AddRelation returns? The EndpointStatus
fields would be something like:

   ServiceName string
   Interface string
   Role charm.RelationRole (string)

How does that sound?

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
On 2014/05/28 08:52:38, fwereade wrote:
> On 2014/05/26 21:47:43, menn0 wrote:
> > On 2014/05/26 06:45:34, fwereade wrote:
> > > Were we going to add the components of the "(error: blam)" stuff
into
> > StatusData
> > > (or something similar)?
> >
> > Yes that's coming (in a separate branch). This branch is just about
getting
> the
> > supporting infrastructure in place.

> Great, sorry for the confusion, I fear you will have to get used to me
asking
> stupid questions to which I should already know the answers :).

> > The test data here isn't realistic. This test is just checking that
StatusData
> > is making it where it should the status response.
> >
> > > The contents of StatusData are technically arbitrary, but they're
actually
> > > pretty well-defined. We could tighten them up to prevent key
collisions with
> > > whatever we pick; or we could put the arbitrary data into its own
dict?
> >
> > Agreed that we should be careful about what we put in StatusData.
How about
> > namespacing the keys along the lines of "category.item" to help
avoid
> > collisions?

> Depends what we mean by StatusData -- the stuff that gets set by the
agent, or
> the stuff we return over status? For the latter, I think it may be
better to
> separate the explicitly-set-by-agent bits from the
also-relevant-to-status bits.

I haven't been making a distinction - these changes currently let the
StatusData-as-set-by-the-agent straight out - but I guess we probably
should to ensur...

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations map[string]RelationStatus
On 2014/06/03 11:57:09, menn0 wrote:
> > Hmm... yeah, I think it probably would. I also find myself wondering
whether
> we
> > should actually be representing the endpoint data as well, lest
clients have
> to
> > mess around parsing keys? Don't let this block you, it's addressable
in a
> > followup, but take a quick look at the AddRelation API: what we
chose there
> > *may* have bearing on what we should expose here.

> Funnily enough, my initial implementation did include endpoint data in
> RelationStatus but I removed it because the data was mostly repeating
what was
> already in the relation name. I'm happy to put this back in though.

> Looking at the AddRelation API, it returns a map[string]charm.Relation
where the
> key is the service name. I'm not sure that it makes sense for
RelationStatus to
> represent endpoints in a map - a slice probably makes more sense.

> How about each RelationStatus holds a slice of EndpointStatus structs
which have
> a subset of what AddRelation returns? The EndpointStatus fields would
be
> something like:

> ServiceName string
> Interface string
> Role charm.RelationRole (string)

> How does that sound?

SGTM, but let's also record the relation scope (it should match across
the endpoints, so I think it's relation-level status).

Also, please see if you can think of a nice way to show which (if any)
is the subordinate in a given relation.(It's potentially subtle: if foo
is subordinate to bar via some locally-scoped relation, it's only
subordinate in the context of that relation: it can have a perfectly
good global relation with some other service.) This information is
encoded elsewhere in status output, though, so it's optional to some
degree; but it might be a cleaner way of doing some things, so bear it
in mind.

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
On 2014/06/03 11:57:09, menn0 wrote:
> On 2014/05/28 08:52:38, fwereade wrote:
> > On 2014/05/26 21:47:43, menn0 wrote:
> > > On 2014/05/26 06:45:34, fwereade wrote:
> > > > Were we going to add the components of the "(error: blam)" stuff
into
> > > StatusData
> > > > (or something similar)?
> > >
> > > Yes that's coming (in a separate branch). This branch is just
about getting
> > the
> > > supporting infrastructure in place.
> >
> > Great, sorry for the confusion, I fear you will have to get used to
me asking
> > stupid questions to which I should already know the answers :).
> >
> > > The test data here isn't realistic. This test is just checking
that
> StatusData
> > > is making it where it should the status response.
> > >
> > > > The contents of StatusData are technically arbitrary, but
they're actually
> ...