state/api{,server}: more info from FullStatus API

state/api{,server}: more info from FullStatus API

Machines and units now have a StatusData map in the FullStatus API
response which holds extra status information that a client may choose
to use when displaying status. This was already being created but
wasn't through the client API.

The FullStatus response now also includes a map of relation id to
relation data (currently just the relation key). This can be used to
present a user friendly relation name to users (e.g. the StatusData
dict contains the relation id when a relation hook fails)

These new bits of data will be used to display the relation name in
the "juju status" output when a relation hook fails.

The client API test scenario has been extended to include a case of a
unit with an error with extra 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)

[menn0, 2014-05-18 23:32:26]

Please take a look.

[fwereade, 2014-05-19 07:46:24]

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_tes...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/1/state/apiserver/client/api_tes...
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.

[menn0, 2014-05-26 05:54:36]

Please take a look.

[fwereade, 2014-05-26 06:45:35]

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#newcod...
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...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api...
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/sta...
File state/apiserver/client/status.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/sta...
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.

[menn0, 2014-05-26 21:47:44]

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#newcod...
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...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api...
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?

[fwereade, 2014-05-28 08:52:38]

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#newcod...
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...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api...
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.

[menn0, 2014-06-03 11:57:09]

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#newcod...
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...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api...
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
ensure that internal-only data doesn't leak out. I don't see many places in the
code that currently make use of StatusData. I'll whitelist the allowed keys or
something.

[fwereade, 2014-06-03 14:02:36]

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#newcod...
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...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api...
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
> > > > 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
> ensure that internal-only data doesn't leak out. I don't see many places in
the
> code that currently make use of StatusData. I'll whitelist the allowed keys or
> something.

SGTM. We can loosen it up at that end as we need to; and if we make a point of
only *taking* specific keys at this end, those changes won't pollute this side
of the code (ie ninja-change the API) until someone explicitly includes them in
status output.