state/apiserver: Implement Machiner.Watch

state/apiserver: Implement Machiner.Watch

This implements the last API call needed by the
machiner worker - Machiner.Watch, at server-side.
Client-side implementation will follow up.

https://code.launchpad.net/~dimitern/juju-core/057-api-machiner-watch/+merge/167573

Requires: https://code.launchpad.net/~dimitern/juju-core/056-api-machiner-client-subpackage/+merge/167489

(do not edit description out of merge proposal)

[dimitern, 2013-06-05 15:43:57]

Please take a look.

[thumper, 2013-06-06 04:32:43]

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

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:54: return result, nil
While I don't have a problem with this, I have been told that it is idiomatic Go
to return nil for an empty slice rather than a real slice backed by an empty
array.

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:66: if _, ok := <-watcher.Changes(); !ok {
Do we really want to discard the initial changes?

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner_test.go:52: func (frr fakeResourceRegistry)
Register(resource common.Resource) string {
Why not call it "registry" instead of "frr".  How do you pronounce "frr"?  All
variable names should be pronounceable, otherwise how do you talk about it?

[dimitern, 2013-06-06 10:01:26]

Please take a look.

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

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:54: return result, nil
On 2013/06/06 04:32:43, thumper wrote:
> While I don't have a problem with this, I have been told that it is idiomatic
Go
> to return nil for an empty slice rather than a real slice backed by an empty
> array.

This is as agreed: the API results of bulk operations will return the same
number of errors/results as the length of the arguments passed.

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:66: if _, ok := <-watcher.Changes(); !ok {
On 2013/06/06 04:32:43, thumper wrote:
> Do we really want to discard the initial changes?

This is more to check if the channel is sending changes, and since the
EntityWatcher returns empty structs we don't lose any information I think.
Anyway, this is how Roger originally implemented it, maybe he can answer better.

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner_test.go:52: func (frr fakeResourceRegistry)
Register(resource common.Resource) string {
On 2013/06/06 04:32:43, thumper wrote:
> Why not call it "registry" instead of "frr".  How do you pronounce "frr"?  All
> variable names should be pronounceable, otherwise how do you talk about it?

Good point, will do.

[jameinel, 2013-06-06 11:13:59]

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner_test.go:52: func (frr fakeResourceRegistry)
Register(resource common.Resource) string {
On 2013/06/06 04:32:43, thumper wrote:
> Why not call it "registry" instead of "frr".  How do you pronounce "frr"?  All
> variable names should be pronounceable, otherwise how do you talk about it?

Eff Arr Arr, but otherwise I agree an actual word is nicer.

[fwereade, 2013-06-06 17:04:38]

Looks sane, just one quibble.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:65: // for initial changes.
I *think* I see what's going on here... the client will automatically send out
the guaranteed initial event, right? I don't love it, honestly... it feels like
an unnecessary distinction between entity watchers and other watchers at the API
level -- some will send initial events and some won't -- and I'm not convinced
the saving is worth the mental load and loss of clarity.

If we're not currently constrained by a client-side watcher implementation,
let's at least start by being consistent with our general plans for watchers; we
can optimize it if and when it becomes an issue.

[dimitern, 2013-06-07 11:47:54]

Please take a look.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:65: // for initial changes.
On 2013/06/06 17:04:38, fwereade wrote:
> I *think* I see what's going on here... the client will automatically send out
> the guaranteed initial event, right? I don't love it, honestly... it feels
like
> an unnecessary distinction between entity watchers and other watchers at the
API
> level -- some will send initial events and some won't -- and I'm not convinced
> the saving is worth the mental load and loss of clarity.
> 
> If we're not currently constrained by a client-side watcher implementation,
> let's at least start by being consistent with our general plans for watchers;
we
> can optimize it if and when it becomes an issue.

Will remove this from here and the client-side implementation will change
accordingly.

[Danilo, 2013-06-07 12:37:37]

Hey Dimiter: this generally LGTM. I have a bunch of stylistic comments, but
nothing that is too critical in regard to the features you are implementing. 
The only thing that bothers me a bit is the logger change, especially since that
seems not to belong in this branch.

https://codereview.appspot.com/9937045/diff/12001/cmd/logging.go
File cmd/logging.go (right):

https://codereview.appspot.com/9937045/diff/12001/cmd/logging.go#newcode55
cmd/logging.go:55: if l.Debug {
Does this mean we do not modify the logging level when verbose flag is set
anymore? Why is it so? Does it make sense to keep the verbose option then (or is
it there simply for backwards compatibility)?

https://codereview.appspot.com/9937045/diff/12001/cmd/logging.go#newcode57
cmd/logging.go:57: logger.SetLogLevel(loggo.DEBUG)
Does this not remove the handling of l.Verbose option? Does it mean that we do
not support 'verbose' anymore, but only 'debug'?

https://codereview.appspot.com/9937045/diff/12001/cmd/logging_test.go
File cmd/logging_test.go (left):

https://codereview.appspot.com/9937045/diff/12001/cmd/logging_test.go#oldcode43
cmd/logging_test.go:43: func (s *LogSuite) TestVerboseSetsLogLevel(c *C) {
It might still be a good idea to keep this test as long as option is there, but
modify it to just show that option is accepted and acts as a no-op on the log
level.

https://codereview.appspot.com/9937045/diff/12001/cmd/logging_test.go#oldcode53
cmd/logging_test.go:53: func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
I suspect we'd definitely want this test to stay in.

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

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/api_test.go...
state/apiserver/api_test.go:110: RetryDelay: 0,
At least to me it's not immediately obvious what does 'retry delay' of zero do. 
Is it worth a comment or is it just my lack of familiarity with our state code?
:)

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
state/apiserver/machiner/machiner.go:60: }
Is there a place where we are actively documenting API design decisions like
returning empty slice instead of nil (the question Tim raised: I see you
responded that it's the design we are taking, but how does the next person know
that other than by trying to mimic what you have done)?

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
state/apiserver/machiner/machiner_test.go:222: s.assertError(c,
result.Results[2].Error, api.CodeNotFound, "machine 42 not found")
I generally prefer tests for each different case to be split into a separate
test method. This makes it easier to see where the problem is if multiple tests
start failing (you don't have to fix the first one and only then see that the
next one has failed). It also helps with readability of tests.  I know that it
sometimes leads to slower test suite and a bit duplicated code, but I think the
readability and "specialization" mitigate that completely.  It's just an opinion
though, and I am not going to block on this.

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/resource.go
File state/apiserver/resource.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/resource.go...
state/apiserver/resource.go:66: return sr.id
I know this is not your code, but I can imagine very funny bugs with code that
uses variables named both 'sr' and 'rs' in the same method: hopefully Go's type
checking helps with that. :)

https://codereview.appspot.com/9937045/diff/12001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/open.go#newcode55
state/open.go:55: // unsucssful connection attempts.
"unsuccessful" :)

However, not even this explains the meaning of 0: does that mean no retries? Or
retry as soon as it times out? Or something else entirely? (Yes, I can read the
code but that's what comments are there for)

[dimitern, 2013-06-07 12:42:58]

I'm sorry that's a case of a merge gone wrong! I'll strip the bits that somehow
got there from trunk and reverted and will repropose. (Note to self: keep your
pipelines shorter next time :)

[rog, 2013-06-07 13:04:36]

oops some comments i hadn't got around to publishing, sorry
(plus one more recent one)

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

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:49: func (m *Machiner) Watch(args
params.Machines) (params.MachinesWatchResults, error) {
bulk operations for creating machine watchers are approaching ludicrous, given
that we can't possibly have bulk operations for the actual Next operations.
not to mention the fact that if we want to watch several machines at once, we
already have specific watcher types for that.

i know i seem to have lost this argument, but... sigh.

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:66: if _, ok := <-watcher.Changes(); !ok {
On 2013/06/06 10:01:26, dimitern wrote:
> On 2013/06/06 04:32:43, thumper wrote:
> > Do we really want to discard the initial changes?
> 
> This is more to check if the channel is sending changes, and since the
> EntityWatcher returns empty structs we don't lose any information I think.
> Anyway, this is how Roger originally implemented it, maybe he can answer
better.

as dimiter says, there is no information in the initial change to lose.
if there was some information, we'd return it with the initial Watch request.

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner_test.go:52: func (frr fakeResourceRegistry)
Register(resource common.Resource) string {
On 2013/06/06 11:13:59, jameinel wrote:
> On 2013/06/06 04:32:43, thumper wrote:
> > Why not call it "registry" instead of "frr".  How do you pronounce "frr"? 
All
> > variable names should be pronounceable, otherwise how do you talk about it?
> 
> Eff Arr Arr, but otherwise I agree an actual word is nicer.

"r" works fine for me too. receiver names need not be descriptive.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:19: // Resource represents the interface
provided by state watchers.
// Resource represents any resource that should be
// cleaned up when an API connection terminates.
// The Stop method will be called when that happens.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:24: // ResourceRegistry interface defines a
way to register resources.
// ResourceRegistry is an interface that allows the
// registration of resources that will be cleaned
// up when an API connection terminates.
// It is typically implemented by an API server.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:26: Register(resource Resource) string
// Register registers the given resource.
// It returns a unique identifier for the resource
// which can then be used in subsequent API
// requests to refer to the resource.

shouldn't we have a corresponding GetResource
method too?

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:16: resourceRegistry
common.ResourceRegistry
s/resourceRegistry/resources/ ?

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:21: func New(st *state.State,
resourceRegistry common.ResourceRegistry, authorizer common.Authorizer)
*Machiner {
s/resourceRegistry/resources/ ?

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:65: // for initial changes.
On 2013/06/06 17:04:38, fwereade wrote:
> I *think* I see what's going on here... the client will automatically send out
> the guaranteed initial event, right? I don't love it, honestly... it feels
like
> an unnecessary distinction between entity watchers and other watchers at the
API
> level -- some will send initial events and some won't -- and I'm not convinced
> the saving is worth the mental load and loss of clarity.

There should be no such distinction between watchers at the API level.
The idea is that *all* watchers send the initial data with the
first Watch request. The only thing that's special about the
entity watcher is that there is no actual data to include in the Watch
reply.

AFAIK all watchers in state send an initial event.

Please can we leave this as is?

[dimitern, 2013-06-07 13:56:37]

Please take a look.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
File state/apiserver/common/interfaces.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:19: // Resource represents the interface
provided by state watchers.
On 2013/06/07 13:04:37, rog wrote:
> // Resource represents any resource that should be
> // cleaned up when an API connection terminates.
> // The Stop method will be called when that happens.

Done.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:24: // ResourceRegistry interface defines a
way to register resources.
On 2013/06/07 13:04:37, rog wrote:
> // ResourceRegistry is an interface that allows the
> // registration of resources that will be cleaned
> // up when an API connection terminates.
> // It is typically implemented by an API server.

Done.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/common/inter...
state/apiserver/common/interfaces.go:26: Register(resource Resource) string
On 2013/06/07 13:04:37, rog wrote:
> // Register registers the given resource.
> // It returns a unique identifier for the resource
> // which can then be used in subsequent API
> // requests to refer to the resource.
> 
> shouldn't we have a corresponding GetResource
> method too?

Changed.
We don't currently need a GetResource for the facades. If need arises, we'll add
it later.

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/6001/state/apiserver/machiner/mac...
state/apiserver/machiner/machiner.go:16: resourceRegistry
common.ResourceRegistry
On 2013/06/07 13:04:37, rog wrote:
> s/resourceRegistry/resources/ ?

I started liking longer, more descriptive names :)

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
File state/apiserver/machiner/machiner.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
state/apiserver/machiner/machiner.go:60: }
On 2013/06/07 12:37:37, Danilo wrote:
> Is there a place where we are actively documenting API design decisions like
> returning empty slice instead of nil (the question Tim raised: I see you
> responded that it's the design we are taking, but how does the next person
know
> that other than by trying to mimic what you have done)?

It was part of the API Bulk Operations email I sent on juju-dev and reflects
what was discussed. But you're right, we should have it documented somewhere
clearly.

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/machiner/ma...
state/apiserver/machiner/machiner_test.go:222: s.assertError(c,
result.Results[2].Error, api.CodeNotFound, "machine 42 not found")
On 2013/06/07 12:37:37, Danilo wrote:
> I generally prefer tests for each different case to be split into a separate
> test method. This makes it easier to see where the problem is if multiple
tests
> start failing (you don't have to fix the first one and only then see that the
> next one has failed). It also helps with readability of tests.  I know that it
> sometimes leads to slower test suite and a bit duplicated code, but I think
the
> readability and "specialization" mitigate that completely.  It's just an
opinion
> though, and I am not going to block on this.

Well, the idea of this test is to test bulk operations as well. And due to how
the authorization checks are implemented, it's convenient to test all 3 cases in
a single test (i.e. you cannot test for success for a machine other than the one
the machiner is responsible for - machine 1 here, and the "not found" and "perm
denied" errors could be split). But I see your point and we might try doing that
in the future, at the expense of some code duplication in tests.

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/resource.go
File state/apiserver/resource.go (right):

https://codereview.appspot.com/9937045/diff/12001/state/apiserver/resource.go...
state/apiserver/resource.go:66: return sr.id
On 2013/06/07 12:37:37, Danilo wrote:
> I know this is not your code, but I can imagine very funny bugs with code that
> uses variables named both 'sr' and 'rs' in the same method: hopefully Go's
type
> checking helps with that. :)

Not really, doesn't help with readability, just the compiler's happy :) But I
totally agree and try to change names as I encounter cases like this.

[fwereade, 2013-06-07 14:07:03]

Coming along very nicely, code looks solid, but I'd like to see direct tests
that the returned watcher id is valid and works properly in Next calls.

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

https://codereview.appspot.com/9937045/diff/1/state/apiserver/machiner/machin...
state/apiserver/machiner/machiner.go:66: if _, ok := <-watcher.Changes(); !ok {
On 2013/06/07 13:04:37, rog wrote:
> On 2013/06/06 10:01:26, dimitern wrote:
> > On 2013/06/06 04:32:43, thumper wrote:
> > > Do we really want to discard the initial changes?
> > 
> > This is more to check if the channel is sending changes, and since the
> > EntityWatcher returns empty structs we don't lose any information I think.
> > Anyway, this is how Roger originally implemented it, maybe he can answer
> better.
> 
> as dimiter says, there is no information in the initial change to lose.
> if there was some information, we'd return it with the initial Watch request.

OK, so all client watchers can be expected to send an event on in response to
the first Watch call, which will hold any data (like initial-members lists) from
the underlying watcher's initial event? As long as we follow some clear
convention, I'm happy; that one SGTM. Sorry i hadn't picked up on it.

https://codereview.appspot.com/9937045/diff/21001/state/apiserver/machiner/ma...
File state/apiserver/machiner/machiner_test.go (right):

https://codereview.appspot.com/9937045/diff/21001/state/apiserver/machiner/ma...
state/apiserver/machiner/machiner_test.go:224: // Just verify the resource was
registered and stop it.
Do we have direct tests for the results of Next() calls passing in the returned
watcher id? Feels like pretty important behaviour to unit test...