mstate: added unit status

mstate: added unit status

Added the status methods for Unit in mstate. It relies
on AgentAlive() which itself relies on the presence. So
currently the alive regarding methods are mocked, the
according part in the test for unit state is commented.
Both has to be changed with the availability of
presence.

https://code.launchpad.net/~themue/juju-core/go-mstate-unit-status/+merge/122500

(do not edit description out of merge proposal)

[TheMue, 2012-09-03 11:58:37]

Please take a look.

[TheMue, 2012-09-03 12:01:56]

Please take a look.

[aram, 2012-09-04 12:36:31]

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode225
mstate/unit.go:225: return true, nil
panic("unimplemented")

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode231
mstate/unit.go:231: return nil
panic("unimplemented")

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode239
mstate/unit.go:239: return nil, nil
panic("unimplemented")

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode449
mstate/unit.go:449: config, err = readConfigNode(u.st, "u/"+u.Name())
Is using "u/"+u.Name() wise?

[TheMue, 2012-09-04 13:05:39]

Some notes, change to new mstate presence is in progress.

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode225
mstate/unit.go:225: return true, nil
On 2012/09/04 12:36:32, aram wrote:
> panic("unimplemented")

So far done it w/o panic to allow the tests to be as unchanged as possible.

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode231
mstate/unit.go:231: return nil
On 2012/09/04 12:36:32, aram wrote:
> panic("unimplemented")

Ditto.

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode239
mstate/unit.go:239: return nil, nil
On 2012/09/04 12:36:32, aram wrote:
> panic("unimplemented")

Ditto, but her panic would work too as I can't use it this way. But right at
this moment I'm changing to the new mstate presence.

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode449
mstate/unit.go:449: config, err = readConfigNode(u.st, "u/"+u.Name())
On 2012/09/04 12:36:32, aram wrote:
> Is using "u/"+u.Name() wise?

Motivation is to make it similar to service to keep the code homogenous. Do you
see a special problem in the context of the unit?

[aram, 2012-09-04 13:10:25]

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode225
mstate/unit.go:225: return true, nil
But the tests have this commented out, don't they?

Of course, since this is WIP, and not ready for integration it would be
pointless to do this change, I just mentioned it IFF you'd have submitted it
without presence.

https://codereview.appspot.com/6494073/diff/3001/mstate/unit.go#newcode449
mstate/unit.go:449: config, err = readConfigNode(u.st, "u/"+u.Name())
Unit names have a slash in them.

[TheMue, 2012-09-04 16:21:19]

Please take a look.

[niemeyer, 2012-09-04 19:32:28]

Nice to see how well this things are fitting in as we progress.

A few comments:

https://codereview.appspot.com/6494073/diff/6001/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/open.go#newcode33
mstate/open.go:33: agents:     presencedb.C("agents"),
I think we should have only a single collection named "presence", and a single
watcher instance for it. No matter what kind of presence we end up having in the
future besides "agents", we'll never want to watch a second collection as it has
a high cost.

It's fine to keep the instance as st.agentsw if you prefer that, though, as we
can trivially change that if we find a reason down the road.

https://codereview.appspot.com/6494073/diff/6001/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/state.go#newcode28
mstate/state.go:28: type entity interface {
Nothing is using this interface, so we can drop it for now.

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode110
mstate/unit.go:110: // entityKey returns the database key of the unit.
Hmmm.. sorry for changing this now, but can we please have this as:

// globalKey returns the global database key for the unit.

I think this expresses more clearly the intent.

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode226
mstate/unit.go:226: u.st.agentsw.ForceRefresh()
That doesn't look right. Imagine we have a pool of 1000 units for which we're
verifying their status. Would it make sense to refresh liveness information for
the whole environment for every single one of them?

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode237
mstate/unit.go:237: for {
There are a few comments here:

- We need to observe the Dying of the presence watcher, which unfortunately
doesn't exist yet.

- I don't think we need to loop. If the agent is currently alive, we can return
immediately. If it is not, we can return on the next event.

- The above statement is false, and that's a bug in the presence Watcher API.
presence.Alive needs to return an error result, which is only set in case it
needs to unblock due to the fact the watcher is dying. That's the only
circumstance in which it can return a false after a false.

Would you mind to fix the first and the third points in a separate CL?

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode260
mstate/unit.go:260: u.st.agentsw.ForceRefresh()
Similar to the previous comment. While it'd be less of an issue here given the
use cases, I still don't see a reason to be forcefully refreshing that knowledge
in the implementation.

[TheMue, 2012-09-04 19:47:44]

Most comments make sense and I will change it tomorrow morning. Only two
remarks:

1. If we have one collection for all presences, which is fine for me, we
possibly need better keys to avoid conflicts between different types of presence
watchers.

2. The non-usage of u.st.agentsw.ForceRefresh() leads to a problem in the tests.
AgentAlive() then showed the wrong results after SetAgentAlive() or killing the
pinger. So I thought the usage of ForceRefresh() would be needed.

[niemeyer, 2012-09-04 20:13:44]

On 2012/09/04 19:47:44, TheMue wrote:
> Most comments make sense and I will change it tomorrow morning. Only two
> remarks:
> 
> 1. If we have one collection for all presences, which is fine for me, we
> possibly need better keys to avoid conflicts between different types of
presence
> watchers.

Isn't that exactly what globalKey gives us? It's up to use to preserve
uniqueness.

> 2. The non-usage of u.st.agentsw.ForceRefresh() leads to a problem in the
tests.
> AgentAlive() then showed the wrong results after SetAgentAlive() or killing
the
> pinger. So I thought the usage of ForceRefresh() would be needed.

I'd be curious to see the specific places in which tests are failing, since
perhaps I'm missing something relevant in the context.

For the moment, it sounds sensible to attempt to:

1) Introduce state.ForcePresenceRefresh()

2) Add the call to strategic places in tests

I'm almost willing to claim that the tests were already broken, in fact, and
that wasn't noticed just because the refresh happened fast enough with
ZooKeeper, but I won't claim that as true before knowing what really breaks.

[TheMue, 2012-09-04 20:20:57]

> Isn't that exactly what globalKey gives us? It's up to use to preserve
> uniqueness.

So far it gives us one per entity (first is unit, next machine). That's ok as
long as we watch only one thing, e.g. the agent. I only see troubles if we have
to watch something else for the same entity. But think I'm only guessing too
much. Let's postpone it until the problem really exists.

> For the moment, it sounds sensible to attempt to:
> 
> 1) Introduce state.ForcePresenceRefresh()
> 
> 2) Add the call to strategic places in tests

OK, will do so.

[niemeyer, 2012-09-04 20:26:56]

On 2012/09/04 20:20:57, TheMue wrote:
> > Isn't that exactly what globalKey gives us? It's up to use to preserve
> > uniqueness.
> 
> So far it gives us one per entity (first is unit, next machine). That's ok as
> long as we watch only one thing, e.g. the agent. I only see troubles if we
have
> to watch something else for the same entity. But think I'm only guessing too
> much. Let's postpone it until the problem really exists.

The key is a free-form string. We put whatever we want on it.

> OK, will do so.

Thanks a lot!

[TheMue, 2012-09-05 08:26:19]

Please take a look.

https://codereview.appspot.com/6494073/diff/6001/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/open.go#newcode33
mstate/open.go:33: agents:     presencedb.C("agents"),
On 2012/09/04 19:32:29, niemeyer wrote:
> I think we should have only a single collection named "presence", and a single
> watcher instance for it. No matter what kind of presence we end up having in
the
> future besides "agents", we'll never want to watch a second collection as it
has
> a high cost.
> 
> It's fine to keep the instance as st.agentsw if you prefer that, though, as we
> can trivially change that if we find a reason down the road.

Done and renamed it to its intention: presence.

https://codereview.appspot.com/6494073/diff/6001/mstate/state.go
File mstate/state.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/state.go#newcode28
mstate/state.go:28: type entity interface {
On 2012/09/04 19:32:29, niemeyer wrote:
> Nothing is using this interface, so we can drop it for now.

Done.

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode110
mstate/unit.go:110: // entityKey returns the database key of the unit.
On 2012/09/04 19:32:29, niemeyer wrote:
> Hmmm.. sorry for changing this now, but can we please have this as:
> 
> // globalKey returns the global database key for the unit.
> 
> I think this expresses more clearly the intent.

Done. What I meant is that using this key in the single presence collection
currently for agents is fine. But once we introduce a new unit related feature
which presence has to be watched this one key isn't enough. But as already said
yesterday that's premature thinking. ;)

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode226
mstate/unit.go:226: u.st.agentsw.ForceRefresh()
On 2012/09/04 19:32:29, niemeyer wrote:
> That doesn't look right. Imagine we have a pool of 1000 units for which we're
> verifying their status. Would it make sense to refresh liveness information
for
> the whole environment for every single one of them?

Done like discussed.

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode237
mstate/unit.go:237: for {
On 2012/09/04 19:32:29, niemeyer wrote:
> There are a few comments here:
> 
> - We need to observe the Dying of the presence watcher, which unfortunately
> doesn't exist yet.
> 
> - I don't think we need to loop. If the agent is currently alive, we can
return
> immediately. If it is not, we can return on the next event.
> 
> - The above statement is false, and that's a bug in the presence Watcher API.
> presence.Alive needs to return an error result, which is only set in case it
> needs to unblock due to the fact the watcher is dying. That's the only
> circumstance in which it can return a false after a false.
> 
> Would you mind to fix the first and the third points in a separate CL?

Done. There may be an issue with the timeout as both selects may need e.g.
timeout -1ms, so the wait in total would last almost 2 x timeout. But that's
only theoretical as I expect the initial change to answer almost immediately.

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode260
mstate/unit.go:260: u.st.agentsw.ForceRefresh()
On 2012/09/04 19:32:29, niemeyer wrote:
> Similar to the previous comment. While it'd be less of an issue here given the
> use cases, I still don't see a reason to be forcefully refreshing that
knowledge
> in the implementation.

Done.

[niemeyer, 2012-09-05 21:47:05]

LGTM

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode237
mstate/unit.go:237: for {
On 2012/09/05 08:26:20, TheMue wrote:
> On 2012/09/04 19:32:29, niemeyer wrote:
> > There are a few comments here:
> > 
> > - We need to observe the Dying of the presence watcher, which unfortunately
> > doesn't exist yet.
> > 
> > - I don't think we need to loop. If the agent is currently alive, we can
> return
> > immediately. If it is not, we can return on the next event.
> > 
> > - The above statement is false, and that's a bug in the presence Watcher
API.
> > presence.Alive needs to return an error result, which is only set in case it
> > needs to unblock due to the fact the watcher is dying. That's the only
> > circumstance in which it can return a false after a false.
> > 
> > Would you mind to fix the first and the third points in a separate CL?
> 
> Done. There may be an issue with the timeout as both selects may need e.g.
> timeout -1ms, so the wait in total would last almost 2 x timeout. But that's
> only theoretical as I expect the initial change to answer almost immediately.

Indeed. You just haven't actually Done neither of the two changes suggested in
the comment I've made above, but that's fine. This is a fantastic start, and we
can sort those out when you return.

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go#newcode234
mstate/unit.go:234: // Initial check.
There are two tabs at the end of this line which should be trimmed before
merging.

[TheMue, 2012-09-06 16:57:01]

*** Submitted:

mstate: added unit status

Added the status methods for Unit in mstate. It relies
on AgentAlive() which itself relies on the presence. So
currently the alive regarding methods are mocked, the
according part in the test for unit state is commented.
Both has to be changed with the availability of
presence.

R=aram, niemeyer
CC=
https://codereview.appspot.com/6494073

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/6001/mstate/unit.go#newcode237
mstate/unit.go:237: for {
On 2012/09/05 21:47:05, niemeyer wrote:
> On 2012/09/05 08:26:20, TheMue wrote:
> > On 2012/09/04 19:32:29, niemeyer wrote:
> > > There are a few comments here:
> > > 
> > > - We need to observe the Dying of the presence watcher, which
unfortunately
> > > doesn't exist yet.
> > > 
> > > - I don't think we need to loop. If the agent is currently alive, we can
> > return
> > > immediately. If it is not, we can return on the next event.
> > > 
> > > - The above statement is false, and that's a bug in the presence Watcher
> API.
> > > presence.Alive needs to return an error result, which is only set in case
it
> > > needs to unblock due to the fact the watcher is dying. That's the only
> > > circumstance in which it can return a false after a false.
> > > 
> > > Would you mind to fix the first and the third points in a separate CL?
> > 
> > Done. There may be an issue with the timeout as both selects may need e.g.
> > timeout -1ms, so the wait in total would last almost 2 x timeout. But that's
> > only theoretical as I expect the initial change to answer almost
immediately.
> 
> Indeed. You just haven't actually Done neither of the two changes suggested in
> the comment I've made above, but that's fine. This is a fantastic start, and
we
> can sort those out when you return.

Eh, I hope you mean the CL with the watcher dying and the agent error? The break
of the loop into two selects had been done in the following proposal, you've
seen it there.

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go#newcode234
mstate/unit.go:234: // Initial check.
On 2012/09/05 21:47:05, niemeyer wrote:
> There are two tabs at the end of this line which should be trimmed before
> merging.

Done. Interesting, how did you find them? Thought they would be removed with go
fmt. So will take more care in future.

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go#newcode249
mstate/unit.go:249: panic(fmt.Sprintf("unexpected alive status of unit %q", u))
Also applied your remark of the machine CL here.

https://codereview.appspot.com/6494073/diff/3003/mstate/unit.go#newcode258
mstate/unit.go:258: // started pinger.
Ditto.