Merge lp:~jimbaker/pyjuju/juju-status-changes-spec into lp:~juju/pyjuju/docs

[Jim Baker]

Fix `juju status` bug when working with multiple relations for a service.

Fix a bug in `juju status` status for the scenario that a service
has multiple established relations to consuming clients, each
using the same relation name from the provider's perspective. This
fix does introduce a backwards breaking change.

https://codereview.appspot.com/5837051/

[Jim Baker (jimbaker)]

Reviewers: mp+97738_code.launchpad.net,

Message:
Please take a look.

Description:
Fix `juju status` bug when working with multiple relations for a
service.

Fix a bug in `juju status` status for the scenario that a service
has multiple established relations to consuming clients, each
using the same relation name from the provider's perspective. This
fix does introduce a backwards breaking change.

https://code.launchpad.net/~jimbaker/juju/juju-status-changes-spec/+merge/97738

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A source/drafts/juju-status-changes.rst

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: <email address hidden>
+New revision: <email address hidden>

Index: source/drafts/juju-status-changes.rst
=== added file 'source/drafts/juju-status-changes.rst'
--- source/drafts/juju-status-changes.rst 1970-01-01 00:00:00 +0000
+++ source/drafts/juju-status-changes.rst 2012-03-15 19:45:16 +0000
@@ -0,0 +1,26 @@
+Relation information in ``juju status``
+---------------------------------------
+
+For each service and each service unit, `juju status` currently
+outputs the ``relations`` key, which maps relation names to a service
+and a the state of the relation unit lifecycle. This is a bug in Juju:
+currently in `juju status`, the implementation will collect status
+information such that only the *last* relation iterated over for a
+service or unit is recorded for a given relation name.
+
+To fix this bug, the following backwards breaking changes are made:
+
+ * For the relations key for each service, a list of dictionaries
+ including the relation name and service name is output. Example::
+
+ relations: - {name: db, service: blog1}
+ - {name: db, service: blog2}
+
+ * For the relations key for each service unit, a list of dictionaries
+ including the relation name and relation workflow state is
+ output::
+
+ relations: - {name: db, service: blog1, state: up}
+ - {name: db, service: blog2, state: down}
+
+Note that the relation id is not visible in this output.

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode17
source/drafts/juju-status-changes.rst:17: - {name: db, service: blog2}
Rather than having a list of dictionaries, I suggest a dictionary of
lists:

     relations:
         db: [blog1, blog2]

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode24
source/drafts/juju-status-changes.rst:24: - {name: db, service: blog2,
state: down}
I wonder about this one, though. Showing a "down" relation here is not
very useful. We already have the information of which relations exist at
the service level. I suggest we display information in the same format
as for the service level, but only ever show those relations that are
up. So, for the above example:

     relations:
         db: [blog1]

https://codereview.appspot.com/5837051/

[Kapil Thangavelu (hazmat)]

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode17
source/drafts/juju-status-changes.rst:17: - {name: db, service: blog2}
On 2012/03/15 21:28:00, niemeyer wrote:
> Rather than having a list of dictionaries, I suggest a dictionary of
lists:

> relations:
> db: [blog1, blog2]

much nicer and succinct

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode24
source/drafts/juju-status-changes.rst:24: - {name: db, service: blog2,
state: down}
Its useful i think to explicitly state the instance of a particular
relation is down rather than relying on a user inferring against the
absence of a value that there is a problem.

On 2012/03/15 21:28:00, niemeyer wrote:
> I wonder about this one, though. Showing a "down" relation here is not
very
> useful. We already have the information of which relations exist at
the service
> level. I suggest we display information in the same format as for the
service
> level, but only ever show those relations that are up. So, for the
above
> example:

> relations:
> db: [blog1]

https://codereview.appspot.com/5837051/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode24
source/drafts/juju-status-changes.rst:24: - {name: db, service: blog2,
state: down}
On 2012/03/16 01:30:59, hazmat wrote:
> Its useful i think to explicitly state the instance of a particular
relation is
> down rather than relying on a user inferring against the absence of a
value
> that there is a problem.

If the goal is to make things clear and obvious, having a massive amount
of information for each relation won't help.

What about splitting up, then:

   relations:
       db: [blog1]
   relations-pending:
       db: [blog2]

https://codereview.appspot.com/5837051/

[Kapil Thangavelu (hazmat)]

On 2012/03/16 02:53:42, niemeyer wrote:

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst
> File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode24
> source/drafts/juju-status-changes.rst:24: - {name: db, service: blog2,
state:
> down}
> On 2012/03/16 01:30:59, hazmat wrote:
> > Its useful i think to explicitly state the instance of a particular
relation
> is
> > down rather than relying on a user inferring against the absence of
a value
> > that there is a problem.

> If the goal is to make things clear and obvious, having a massive
amount of
> information for each relation won't help.

> What about splitting up, then:

> relations:
> db: [blog1]
> relations-pending:
> db: [blog2]

that sounds reasonable.. it would be relations-down.. pending isn't
valid from the unit's perspective, it either knows about and its up or
down, i mean we could infer, but its a very transient state, effectively
the time between the add-relation and a watch firing.

also i wonder if it makes sense to only use list format for relations
that are server oriented, for client relations its only a singleton by
name. it does add minor ambiguity to the parsing as interface role isn't
available in status. we should probably run this by the list before it
lands, as its going to break the extant status parsers.

https://codereview.appspot.com/5837051/

[Gustavo Niemeyer (niemeyer)]

On Fri, Mar 16, 2012 at 00:03, <email address hidden> wrote:

> that sounds reasonable.. it would be relations-down.. pending isn't
> valid from the unit's perspective, it either knows about and its up or
> down, i mean we could infer, but its a very transient state, effectively
> the time between the add-relation and a watch firing.

So what's a down relation, and why does it matter? My understanding is
that while the agent doesn't recognize the relation it's down. For a
user, "pending" feels like a more understandable term for that.

> also i wonder if it makes sense to only use list format for relations
> that are server oriented, for client relations its only a singleton by
> name. it does add minor ambiguity to the parsing as interface role isn't
> available in status. we should probably run this by the list before it
> lands, as its going to break the extant status parsers.

I think we over that quite a few times already. :-) Both requires and
provides may have multiple endpoints.

--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[Kapil Thangavelu (hazmat)]

On Thu, Mar 15, 2012 at 11:19 PM, Gustavo Niemeyer <
<email address hidden>> wrote:

> On Fri, Mar 16, 2012 at 00:03, <email address hidden> wrote:
>
> > that sounds reasonable.. it would be relations-down.. pending isn't
> > valid from the unit's perspective, it either knows about and its up or
> > down, i mean we could infer, but its a very transient state, effectively
> > the time between the add-relation and a watch firing.
>
> So what's a down relation, and why does it matter? My understanding is
> that while the agent doesn't recognize the relation it's down. For a
> user, "pending" feels like a more understandable term for that.
>

down represents an error state that needs user resolution.

> I think we over that quite a few times already. :-) Both requires and
> provides may have multiple endpoints.
>

ack.

[Gustavo Niemeyer (niemeyer)]

On Fri, Mar 16, 2012 at 10:24, Kapil Thangavelu <email address hidden> wrote:
> down represents an error state that needs user resolution.

Can you please expand a bit on that? We already have an error state
saying that a unit has problems. What's the benefit/reason why we tag
a specific relation as down?

--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[Jim Baker (jimbaker)]

Please take a look.

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode17
source/drafts/juju-status-changes.rst:17: - {name: db, service: blog2}
On 2012/03/15 21:28:00, niemeyer wrote:
> Rather than having a list of dictionaries, I suggest a dictionary of
lists:

> relations:
> db: [blog1, blog2]

Done.

https://codereview.appspot.com/5837051/diff/1/source/drafts/juju-status-changes.rst#newcode24
source/drafts/juju-status-changes.rst:24: - {name: db, service: blog2,
state: down}
Updated, along with a generalization to show relations-<state name> if
nonempty.

https://codereview.appspot.com/5837051/

[lp:~jimbaker/pyjuju/juju-status-changes-spec]

7. By Jim Baker on 2012-03-19

Addressed review points, along with an expanded example

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/5837051/diff/9001/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/9001/source/drafts/juju-status-changes.rst#newcode41
source/drafts/juju-status-changes.rst:41: db: [blog2]
This doesn't reflect the latest agreement. Please check out the email
from Kapil to the list.

I actually wonder.. do we need this spec now? It was good to spark the
debate and reach an agreement, but I'm personally happy with Kapil's
email alone now.

https://codereview.appspot.com/5837051/

[Jim Baker (jimbaker)]

https://codereview.appspot.com/5837051/diff/9001/source/drafts/juju-status-changes.rst
File source/drafts/juju-status-changes.rst (right):

https://codereview.appspot.com/5837051/diff/9001/source/drafts/juju-status-changes.rst#newcode41
source/drafts/juju-status-changes.rst:41: db: [blog2]
Weird, my email client (Thunderbird) had not synced this Friday email
sometime this morning, but it's there now. I'm happy with Kapil's email
as well.

On 2012/03/19 22:23:14, niemeyer wrote:
> This doesn't reflect the latest agreement. Please check out the email
from Kapil
> to the list.

> I actually wonder.. do we need this spec now? It was good to spark the
debate
> and reach an agreement, but I'm personally happy with Kapil's email
alone now.

https://codereview.appspot.com/5837051/

[Kapil Thangavelu (hazmat)]

it would be nice to have this as documentation, could you update with the contents of that email and merge. thanks.

review: Approve

[Mark Mims (mark-mims)]

merged into lp:juju/docs instead

=== added file 'source/drafts/juju-status-changes.rst'
--- source/drafts/juju-status-changes.rst	1970-01-01 00:00:00 +0000
+++ source/drafts/juju-status-changes.rst	2012-03-19 21:12:18 +0000
@@ -0,0 +1,51 @@
+Relation information in ``juju status``
+---------------------------------------
+
+For each service and each service unit, `juju status` currently
+outputs the ``relations`` key, which maps relation names to a service
+and the state of the relation unit lifecycle. This is a bug in Juju:
+currently in `juju status`, the implementation will collect status
+information such that only the *last* relation iterated over for a
+service or unit is recorded for a given relation name.
+
+To fix this bug, the following backwards breaking changes are made:
+
+ * For the ``relations`` key for each service, a list of the
+   corresponding services for each relation name is output. For
+   example, if the ``mysql5`` service has consuming services
+   ``blog1``, ``blog2``, ``blog3``, and ``blog4``, then this could be
+   a subset of the YAML output::
+
+     services:
+       mysql5:
+         relations:
+           db: [blog1, blog2, blog3, blog4]
+
+ * Instead of using just a ``relations`` key for each service unit to
+   display the status of its relation, this functionality is split:
+   ``relations`` has for its value a dictionary mapping relation name
+   to the list of services that are in the ``up`` state for that
+   relation name. This dictionary may be empty. In addition, the
+   ``relations-<state name>`` key is added for a similar dictionary of
+   relation names mapped to services in that state; this key is only
+   added if the dictionary is nonempty. For example, this could be a
+   subset of output for the ``mysql5/0`` service unit::
+
+     units:
+       mysql5/0:
+         relations:
+           db: [blog1]
+	 relations-error:
+           db: [blog3, blog4]
+	 relations-pending:
+           db: [blog2]
+
+   Later after ``blog3`` is no longer pending and the errors for
+   ``blog3`` and ``blog4`` have been resolved, this could be the
+   output::
+
+     units:
+       mysql5/0:
+         relations: [blog1, blog2, blog3, blog4]
+
+Note that the relation id is not visible in this output.