state/machine: new agent API

state/machine: new agent API

As discussed, for testing reasons it makes sense to
place APIs for a given agent alongside one another,
so this CL puts the machiner API and the machine agent
API alongside one another.

Client-side machine agent API in a subsequent CL.

https://code.launchpad.net/~rogpeppe/juju-core/312-alt-api-jobs/+merge/170135

Requires: https://code.launchpad.net/~rogpeppe/juju-core/311-alt-juju-bootstrap-state-change-password-1.5/+merge/170128

(do not edit description out of merge proposal)

[rog, 2013-06-18 16:59:53]

Please take a look.

[jameinel, 2013-06-19 10:04:28]

LGTM

I'm fine with the rename, and the new APIs look really good to me.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:33: 
This looks good to me. I like the refactoring with each piece making sense.

[Danilo, 2013-06-19 10:50:26]

LGTM (only a few minor comments mostly relating to your code comments).

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:20: // for the given
This comment seems incomplete.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:49: // this check means we can only ever return
a single object.
I can't claim to know exactly when we would want a bulk API call for something
like this, but the bulk call would be there to reduce the potential network
latency as well (if you are doing API calls over a very slow connection, it's
easier to do a single call, and then do all the per-machine authorization checks
server side and simply return the values). Thus, I wouldn't call it "utter
madness" no matter how ugly the implementation may end up being.

I do agree we should not worry about this while our API server is on the same
node as the API consumers, but I'd at least refrain from such absolute
statements of madness. :)

Anyway, I am not even exactly sure what is 'this' you refer to as 'having a bulk
API call for'.  There is a GetMachines() bulk call, and if I understand it
correctly, that behaves as I describe above (i.e. all the checks happen
server-side).

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent_test.go:68: }},
Machines "0" and "42" seem to fail in exactly the same way: anything special for
each of them (not clear from the context of the diff)?  I generally like to see
test cases be minimal as well, so if they do not exhibit any different
behaviour, I'd only choose one for testing.

Not a big deal, but if there are some differences, it'd be good to point them
out with a comment.

[dimitern, 2013-06-19 13:40:52]

A few comments

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test....
state/apiserver/login_test.go:64: // Note that Open does not log in if the tag
and password
this *is* crack - Open shouldn't work like that just for the sake of testing
logins. Another argument towards taking the login step out of the apiserver RPC
infrastructure. Just a rant otherwise, nothing to do now..

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:39: result, err := api.getMachine(id)
I though we agreed to have arrays-of-structs as bulk ops results, rather than
several disjoint slices ([]error separate from []Machines)

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent_test.go:73: err := s.machine1.Destroy()
a test with a non-existent machine id like "42" is equally good and leads less
code imo - you're not testing state.Machine lifecycle state here.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/com...
File state/apiserver/machine/common_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/com...
state/apiserver/machine/common_test.go:45: type fakeAuthorizer struct {
I'd like to see a way to tweak the flags of the authorizer once created. That
way it'll enable us to write simpler test cases which require different
authorization gates (like TestGetMachineAsManagerFails etc)

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
state/apiserver/machine/machiner.go:13: type MachinerAPI struct {
Why the rename, why the "API" suffix? It's well known and unambiguous already,
because it's in its own package. Putting the machine agent stuff in machiner
apiserver subpackage seems wrong - what's the benefit?

[rog, 2013-06-19 15:30:01]

Thanks for the on-holiday review!

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test....
state/apiserver/login_test.go:64: // Note that Open does not log in if the tag
and password
On 2013/06/19 13:40:52, dimitern wrote:
> this *is* crack - Open shouldn't work like that just for the sake of testing
> logins. Another argument towards taking the login step out of the apiserver
RPC
> infrastructure. Just a rant otherwise, nothing to do now..

I can't get worked up about it. Any API client connecting directly
to the websocket can connect without logging in, so it doesn't seem
too awful to be able to do it in Go.
I thought about api.OpenWithoutLoggingIn but it didn't seem
worth it.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:20: // for the given
On 2013/06/19 10:50:27, Danilo wrote:
> This comment seems incomplete.

Done.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:39: result, err := api.getMachine(id)
On 2013/06/19 13:40:52, dimitern wrote:
> I though we agreed to have arrays-of-structs as bulk ops results, rather than
> several disjoint slices ([]error separate from []Machines)

that's what i'm doing.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:49: // this check means we can only ever return
a single object.
On 2013/06/19 10:50:27, Danilo wrote:
> I can't claim to know exactly when we would want a bulk API call for something
> like this, but the bulk call would be there to reduce the potential network
> latency as well (if you are doing API calls over a very slow connection, it's
> easier to do a single call, and then do all the per-machine authorization
checks
> server side and simply return the values). Thus, I wouldn't call it "utter
> madness" no matter how ugly the implementation may end up being.
> 
> I do agree we should not worry about this while our API server is on the same
> node as the API consumers, but I'd at least refrain from such absolute
> statements of madness. :)
> 
> Anyway, I am not even exactly sure what is 'this' you refer to as 'having a
bulk
> API call for'.  There is a GetMachines() bulk call, and if I understand it
> correctly, that behaves as I describe above (i.e. all the checks happen
> server-side).

As discussed online, this API call can never be used to obtain more than one
machine, so there are no possible way of reducing latency.
Comment left in place.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent_test.go:68: }},
On 2013/06/19 10:50:27, Danilo wrote:
> Machines "0" and "42" seem to fail in exactly the same way: anything special
for
> each of them (not clear from the context of the diff)?  I generally like to
see
> test cases be minimal as well, so if they do not exhibit any different
> behaviour, I'd only choose one for testing.
> 
> Not a big deal, but if there are some differences, it'd be good to point them
> out with a comment.

I've got them as separate test cases because I'm checking that we get the same
error when a machine isn't there vs when you don't have permission to access the
machine.

Note that the machiner tests don't do quite the same - I intend to change that
behaviour to align with this, but didn't want to clutter this CL even more.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent_test.go:73: err := s.machine1.Destroy()
On 2013/06/19 13:40:52, dimitern wrote:
> a test with a non-existent machine id like "42" is equally good and leads less
> code imo - you're not testing state.Machine lifecycle state here.

I wanted to use the machiner instance in the agentSuite rather than
tweaking the authorizer. You're right it would be about 3 lines shorter
that way, but I don't think it's that important. I may change
it in a later CL.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/com...
File state/apiserver/machine/common_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/com...
state/apiserver/machine/common_test.go:45: type fakeAuthorizer struct {
On 2013/06/19 13:40:52, dimitern wrote:
> I'd like to see a way to tweak the flags of the authorizer once created. That
> way it'll enable us to write simpler test cases which require different
> authorization gates (like TestGetMachineAsManagerFails etc)

Ok, sorry - it looked to me as if the original code was expecting
by-value functionality, otherwise I wouldn't have expected
it to copy the s.authorizer field before modifying it.

BTW even with things as they are below, we can still make
a fakeAuthorizer with tweakable flags:

auth := &fakeAuthorizer{...}
api := &MachinerAPI{st, auth}
... some call
auth.tag = "foo"
... some other call

It might be best to create mutable authorizers only for tests that
explicitly require them.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
state/apiserver/machine/machiner.go:13: type MachinerAPI struct {
On 2013/06/19 13:40:52, dimitern wrote:
> Why the rename, why the "API" suffix? It's well known and unambiguous already,
> because it's in its own package. Putting the machine agent stuff in machiner
> apiserver subpackage seems wrong - what's the benefit? 

There's substantially shared test infrastructure between the two
APIs. Rather than proliferating packages (one for each API and another
for the shared testing infrastructure), I think it makes sense to
put all the APIs that are unique to a given client inside the
same package to avoid that proliferation. I don't think it will
make it any harder to move workers between agents.

As for the API suffix, I *think* it's nice to have a common
"API" suffix for all types that implement a portion
of the API. In the relatively near future, I expect
to have each client provide an entire server root and all associated
types, so we don't need to have a single srvRoot type that
caters for all clients. I hope the API suffixes will start
to be more obviously a good thing then.

[fwereade, 2013-06-21 08:28:34]

LGTM, despite grumping about authentication model, which is unquestionably
orthogonal to this change. Let's land this.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/login_test....
state/apiserver/login_test.go:64: // Note that Open does not log in if the tag
and password
On 2013/06/19 15:30:01, rog wrote:
> On 2013/06/19 13:40:52, dimitern wrote:
> > this *is* crack - Open shouldn't work like that just for the sake of testing
> > logins. Another argument towards taking the login step out of the apiserver
> RPC
> > infrastructure. Just a rant otherwise, nothing to do now..
> 
> I can't get worked up about it. Any API client connecting directly
> to the websocket can connect without logging in, so it doesn't seem
> too awful to be able to do it in Go.
> I thought about api.OpenWithoutLoggingIn but it didn't seem
> worth it.

Roger, it's awful that we can do that at all, whatever the client. It'll be less
of a concern once access to the rest of the API is actually gated behind
authentication, and we can present only the appropriate API to each connected
client, rather than having every method have to take all possibilities into
account; but this is all still by way of mitigating the impact of a
fundamentally bad idea that's predicated on an inappropriate prioritization of
SLOC over separation of concerns (not to mention clarity of security model).

We're not saying to change it now -- we have other priorities -- but I think we
are a little worried that you apparently don't see any problems with the current
approach. It's extremely wearing to have to repeatedly explain that *yes*
codebases do go bad when shortcuts like this are taken, *yes* this is a real
thing we've seen far too often before, and *no* we do not wish to repeat those
known mistakes just in case the dice land differently this time.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:49: // this check means we can only ever return
a single object.
On 2013/06/19 15:30:01, rog wrote:
> On 2013/06/19 10:50:27, Danilo wrote:
> > I can't claim to know exactly when we would want a bulk API call for
something
> > like this, but the bulk call would be there to reduce the potential network
> > latency as well (if you are doing API calls over a very slow connection,
it's
> > easier to do a single call, and then do all the per-machine authorization
> checks
> > server side and simply return the values). Thus, I wouldn't call it "utter
> > madness" no matter how ugly the implementation may end up being.
> > 
> > I do agree we should not worry about this while our API server is on the
same
> > node as the API consumers, but I'd at least refrain from such absolute
> > statements of madness. :)
> > 
> > Anyway, I am not even exactly sure what is 'this' you refer to as 'having a
> bulk
> > API call for'.  There is a GetMachines() bulk call, and if I understand it
> > correctly, that behaves as I describe above (i.e. all the checks happen
> > server-side).
> 
> As discussed online, this API call can never be used to obtain more than one
> machine, so there are no possible way of reducing latency.
> Comment left in place.

So, no machine agent will ever need information about a different machine, and
this code will be set in stone forever? That's a pretty bold prediction...

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/mac...
state/apiserver/machine/machiner.go:13: type MachinerAPI struct {
On 2013/06/19 15:30:01, rog wrote:
> On 2013/06/19 13:40:52, dimitern wrote:
> > Why the rename, why the "API" suffix? It's well known and unambiguous
already,
> > because it's in its own package. Putting the machine agent stuff in machiner
> > apiserver subpackage seems wrong - what's the benefit? 
> 
> There's substantially shared test infrastructure between the two
> APIs. Rather than proliferating packages (one for each API and another
> for the shared testing infrastructure), I think it makes sense to
> put all the APIs that are unique to a given client inside the
> same package to avoid that proliferation. I don't think it will
> make it any harder to move workers between agents.
> 
> As for the API suffix, I *think* it's nice to have a common
> "API" suffix for all types that implement a portion
> of the API. In the relatively near future, I expect
> to have each client provide an entire server root and all associated
> types, so we don't need to have a single srvRoot type that
> caters for all clients. I hope the API suffixes will start
> to be more obviously a good thing then.

Hmm. I kinda feel like this is the worst of both worlds. I'd be comfortable with
either (1) one facade per package, with an extra package for common test
infrastructure, or (2) one facades package that exported them all and tested
them all in one package. Adding the arbitrary
where-is-this-responsibility-currently-vested subdivision seems unhelpful to me;
but I'm ok to roll with it for now on the strength of your assurances that it's
easy to change (which it is -- at least we don't have to change what we expose
to rearrange these).

[fwereade, 2013-06-21 08:31:43]

https://codereview.appspot.com/10398043/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/machine.go#newcode41
state/machine.go:41: JobManageState:   params.JobManageState,
FWIW, on reflection, I'd be fine with the following jobs:

JobHostUnits
JobHostMachines (or is this implicit?)
JobManageEnviron
JobManageState
JobServeAPI
JobServeState

...but that's a suggestion for a followup really.

[rog, 2013-06-21 08:51:39]

submitting as https://codereview.appspot.com/10384046/
to avoid the prereq.

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/10398043/diff/2001/state/apiserver/machine/age...
state/apiserver/machine/agent.go:49: // this check means we can only ever return
a single object.
On 2013/06/21 08:28:34, fwereade wrote:
> On 2013/06/19 15:30:01, rog wrote:
> > On 2013/06/19 10:50:27, Danilo wrote:
> > > I can't claim to know exactly when we would want a bulk API call for
> something
> > > like this, but the bulk call would be there to reduce the potential
network
> > > latency as well (if you are doing API calls over a very slow connection,
> it's
> > > easier to do a single call, and then do all the per-machine authorization
> > checks
> > > server side and simply return the values). Thus, I wouldn't call it "utter
> > > madness" no matter how ugly the implementation may end up being.
> > > 
> > > I do agree we should not worry about this while our API server is on the
> same
> > > node as the API consumers, but I'd at least refrain from such absolute
> > > statements of madness. :)
> > > 
> > > Anyway, I am not even exactly sure what is 'this' you refer to as 'having
a
> > bulk
> > > API call for'.  There is a GetMachines() bulk call, and if I understand it
> > > correctly, that behaves as I describe above (i.e. all the checks happen
> > > server-side).
> > 
> > As discussed online, this API call can never be used to obtain more than one
> > machine, so there are no possible way of reducing latency.
> > Comment left in place.
> 
> So, no machine agent will ever need information about a different machine, and
> this code will be set in stone forever? That's a pretty bold prediction...

If we ever find that the machine agent is getting information about more than
one machine in a way that is made significantly more efficient by the use of a
bulk API call, I will eat my hat.

[rog, 2013-06-25 06:10:25]

merged as https://codereview.appspot.com/10384046/