state/api: New client API watchers; api.State

state/api: New client API watchers; api.State

Implemented two API client-side watchers:
* LifecycleWatcher
* EnvironConfigWatcher

Also introducing a top-level api.State object to
provide API access to state.* calls. It's only
accessible by agents.
Implemented methods:
* State.WatchMachines (using LifecycleWatcher)
* State.WatchEnvironConfig (using EnvironConfigWatcher)
These two can be called only by the environment manager
(for now, if we need we'll relax this restriction later).
Also, Machine.EnsureDead now can be called both by the
owning agent and the environment manager.

https://code.launchpad.net/~dimitern/juju-core/043-api-client-new-watchers/+merge/165603

(do not edit description out of merge proposal)

[dimitern, 2013-05-24 12:50:57]

Please take a look.

[mue, 2013-05-24 13:16:55]

LGTM, only again one comment.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode599
state/api/apiclient.go:599: w.commonWatcher.init()
Hmm, doing it this way, with internal panics, init() also could be the first
instruction of commonLoop().

[rog, 2013-05-24 13:35:13]

LGTM with a few suggestions below.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode602
state/api/apiclient.go:602: // Watch calls Next internally at the server-side,
so we expect
this isn't quite right. how about:

// The first watch call returns the initial result
// value which we try to send immediately.
?

(and below)

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode659
state/api/apiclient.go:659: w.newResult = func() interface{} { return
new(params.EnvironConfigWatchResults) }
split body onto a new line for consistency with w.call below?

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode677
state/api/apiclient.go:677: // We have received changes, so send them out.
this comment is a bit misleading - we're making them available to be sent, not
actually sending them.

i tried to think of a better comment, but honestly, i think the code is clear
enough without.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode681
state/api/apiclient.go:681: log.Errorf("state/api: error reading environ config:
%v", err)
s/config:/config from watcher:/ ?

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:154: // State.WatchServices(): id of the created
LifecycleWatcher and the
i find this a bit hard to parse.

how about something like this? (and similar for EnvironConfigWatchResults)

// LifecycleWatchResults holds the results of API calls
// that watch the lifecycle of a set of objects.
// It is used both for the initial Watch request
// and for subsequent Next requests.
type LifecycleWatchResults struct {
    // LifeCycleWatcherId holds the id of the newly
    // created watcher. It will be empty for a Next
    // request.
    LifecycleWatcherId string

    // Ids holds the list of entity ids.
    // For a Watch request, it holds all entity ids being
    // watched; for a Next request, it holds the ids of those
    // that have changed.
    Ids []string
}

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

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1334: for i := 0; i < len(stMachines); i++ {
for i := range stMachines {

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1429: // Setup state - add entites to watch.
s/Setup/Set up/
s/entites/entities/

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

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:72: func (s *srvState) WatchMachines()
(params.LifecycleWatchResults, error) {
these two methods should be further down the file, perhaps just before the
srvMachine methods.

also, i'm not sure we want to allow any agent to watch all machines - perhaps
only an EnvironManager machine?

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:91: watcher :=
s.root.srv.state.WatchEnvironConfig()
// TODO restrict access to environment manager machines only.

?

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:92: // To save an extra round-trip to call Next
after Watch, we check
// The watcher always sends an initial value on the channel,
// so we send that as the result of the watch request.
// This saves the client a round trip.
?

[dimitern, 2013-05-24 14:07:44]

*** Submitted:

state/api: New client API watchers; api.State

Implemented two API client-side watchers:
* LifecycleWatcher
* EnvironConfigWatcher

Also introducing a top-level api.State object to
provide API access to state.* calls. It's only
accessible by agents.
Implemented methods:
* State.WatchMachines (using LifecycleWatcher)
* State.WatchEnvironConfig (using EnvironConfigWatcher)
These two can be called only by the environment manager
(for now, if we need we'll relax this restriction later).
Also, Machine.EnsureDead now can be called both by the
owning agent and the environment manager.

R=mue, rog
CC=
https://codereview.appspot.com/9714044

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode599
state/api/apiclient.go:599: w.commonWatcher.init()
On 2013/05/24 13:16:55, mue wrote:
> Hmm, doing it this way, with internal panics, init() also could be the first
> instruction of commonLoop().

Calling init() in commonLoop() is not quite the same, because it's needs to be
called before the go routine spins up. This causes timeouts in tests. I'll leave
it as is if you don't mind.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode602
state/api/apiclient.go:602: // Watch calls Next internally at the server-side,
so we expect
On 2013/05/24 13:35:13, rog wrote:
> this isn't quite right. how about:
> 
> // The first watch call returns the initial result
> // value which we try to send immediately.
> ?
> 
> (and below)

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode659
state/api/apiclient.go:659: w.newResult = func() interface{} { return
new(params.EnvironConfigWatchResults) }
On 2013/05/24 13:35:13, rog wrote:
> split body onto a new line for consistency with w.call below?

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode677
state/api/apiclient.go:677: // We have received changes, so send them out.
On 2013/05/24 13:35:13, rog wrote:
> this comment is a bit misleading - we're making them available to be sent, not
> actually sending them.
> 
> i tried to think of a better comment, but honestly, i think the code is clear
> enough without.

Removed.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode681
state/api/apiclient.go:681: log.Errorf("state/api: error reading environ config:
%v", err)
On 2013/05/24 13:35:13, rog wrote:
> s/config:/config from watcher:/ ?

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:154: // State.WatchServices(): id of the created
LifecycleWatcher and the
On 2013/05/24 13:35:13, rog wrote:
> i find this a bit hard to parse.
> 
> how about something like this? (and similar for EnvironConfigWatchResults)
> 
> // LifecycleWatchResults holds the results of API calls
> // that watch the lifecycle of a set of objects.
> // It is used both for the initial Watch request
> // and for subsequent Next requests.
> type LifecycleWatchResults struct {
>     // LifeCycleWatcherId holds the id of the newly
>     // created watcher. It will be empty for a Next
>     // request.
>     LifecycleWatcherId string
> 
>     // Ids holds the list of entity ids.
>     // For a Watch request, it holds all entity ids being
>     // watched; for a Next request, it holds the ids of those
>     // that have changed.
>     Ids []string
> }

Done. Thanks for the suggestion! I tried rephrasing this several times, but
didn't like it.

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

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1334: for i := 0; i < len(stMachines); i++ {
On 2013/05/24 13:35:13, rog wrote:
> for i := range stMachines {

Done.

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1429: // Setup state - add entites to watch.
On 2013/05/24 13:35:13, rog wrote:
> s/Setup/Set up/
> s/entites/entities/

Done.

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

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:72: func (s *srvState) WatchMachines()
(params.LifecycleWatchResults, error) {
On 2013/05/24 13:35:13, rog wrote:
> these two methods should be further down the file, perhaps just before the
> srvMachine methods.
> 
> also, i'm not sure we want to allow any agent to watch all machines - perhaps
> only an EnvironManager machine?

Done.

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:91: watcher :=
s.root.srv.state.WatchEnvironConfig()
On 2013/05/24 13:35:13, rog wrote:
> // TODO restrict access to environment manager machines only.
> 
> ?

No need, I added the authEnvironManager check anyway.

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:92: // To save an extra round-trip to call Next
after Watch, we check
On 2013/05/24 13:35:13, rog wrote:
> // The watcher always sends an initial value on the channel,
> // so we send that as the result of the watch request.
> // This saves the client a round trip.
> ?

Done. Also updated the other places.