Merge lp://staging/~dimitern/juju-core/040-machine-api-calls-needed-by-machiner into lp://staging/~juju/juju-core/trunk

Reviewers: mp+164920_code.launchpad.net,

Message:
Please take a look.

Description:
state/api: Machine API needed by machiner

This introduces several new API calls, needed by the
worker/machiner to handle machines through the API:
* Life
* EnsureDead
* SetAgentAlive
* SetStatus

In the course of this some refactoring of the API
server was in order:
* Introduced presence.Pinger support
* Watchers and pingers are now both handled as resources
   which have a Stop() method
* Added a couple of root-level permission checking calls:
- authOwner(), taking an entity with a Tag() method and returning
   true if the authenticated user's tag matches the entity's tag
- authEnvironManager() returning true if the authenticated user
   matches the environment manager (machine with JobManageEnviron)

Permission checks are performed to ensure only agents (or managers)
can access the new machine API calls.

https://code.launchpad.net/~dimitern/juju-core/040-machine-api-calls-needed-by-machiner/+merge/164920

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/api/apiclient.go
   M state/api/params/params.go
   M state/apiserver/api_test.go
   M state/apiserver/apiserver.go
   M state/apiserver/error.go

LGTM with the error code fix and some other minor suggestions.

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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode352
state/api/apiclient.go:352: // a HasAssignedUnitsError.
i think you need a new error code for this.
you'll need to add a new code (say CodeHasAssignedUnits) to the error
codes in api/error.go, and add some code to translate from
*state.HasAssignUnitsError to CodeHasAssignedUnits in
apiserver.serverError.

plus an associated test and changing the comment above appropriately.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode370
state/api/apiclient.go:370: type Pinger struct {
doc comment please.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode375
state/api/apiclient.go:375: func (p *Pinger) Stop() error {
ditto

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

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode32
state/api/params/params.go:32: // SetStatus holds the parameters for
making the SetStatus call for a machine or unit.
// SetStatus holds the parameters for Machine.SetStatus
// and Unit.SetStatus.
?

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode352
state/api/params/params.go:352: Life string
please change this to be of type Life.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode77
state/apiserver/api_test.go:77: // 1, because it's managing the
environment.
// Machine 0 is allowed because it is an environment manager.
?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode90
state/apiserver/api_test.go:90: // Machine 0 needs to set the status of
a machine when provisioning.
ditto?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode287
state/apiserver/api_test.go:287: }
you can stop the pinger here (and you should check the error return too)

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode320
state/apiserver/api_test.go:320: setDefaultStatus(c, m)
rather than setting a default status here, i think you should find out
the previous status and set it back.
you should also check the error when doing so.

that way you won't need to call setDefaultStatus in setupScenario, which
is then free to add other machines with different statuses if necessary.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode692
state/apiserver/api_test.go:692: func setDefaultStatus(c *C, entity
setStatuser) {
as implied above, i don't think this is necessary.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode246
state/apiserver/apiserver.go:246: // authOwner returns true only if the
authenticated user's tag
// a...

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode268
state/apiserver/apiserver.go:268: ew := w.r.(*state.EntityWatcher)
On 2013/05/21 15:44:01, TheMue wrote:
> Hmm, "ew", "w", "r", not very self-documenting variable names. Using
one char
> for the type is OK, but the other ones?

that's perhaps a fair point. it was more obvious when "w" meant
"watcher" both times.

perhaps we could rename srvResource.r to srvResource.resource.

https://codereview.appspot.com/9614043/

LGTM, some rambling comments, not all of them actionable, so ignore as
appropriate.

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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode335
state/api/apiclient.go:335: // returns the started pinger.
I find this name and comment slightly confusing. The call esentially
registers the machine over the api and creates a heartbeat object?

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

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode40
state/api/params/params.go:40: type Life string
This is where I want a type system that lets you describe the set of
valid values in something other than a comment. :)

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode317
state/apiserver/api_test.go:317: return func() {}, err
That's a lot of this line in these test helpers... at least func() {} is
short-ish...

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1026
state/apiserver/api_test.go:1026: c.Assert(string(life), Equals,
"alive")
Wouldn't asserting (life, Equals, params.Life("alive")) make more sense?
Or does that mess with test readability more than it helps with type
verification?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1053
state/apiserver/api_test.go:1053: c.Assert(life, Equals, state.Alive)
Hm, and this one asserts against a variable, not a string... which is
because it's a different type, an int enum representing the same
values...

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode528
state/apiserver/apiserver.go:528: // SetAgentAlive signals that the
agent for machine m is alive.
Okay, I'm still confused by this, even seeing the other side. The
registration seems a pretty important side-effect that should be
mentioned.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode737
state/apiserver/apiserver.go:737: rs.rs = make(map[string]*srvResource)
Assuming the compiler/tests will catch any of the mechanical rename
issues,and that somebody understands the variables.

https://codereview.appspot.com/9614043/

Please take a look.

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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode352
state/api/apiclient.go:352: // a HasAssignedUnitsError.
On 2013/05/21 15:37:20, rog wrote:
> i think you need a new error code for this.
> you'll need to add a new code (say CodeHasAssignedUnits) to the error
codes in
> api/error.go, and add some code to translate from
*state.HasAssignUnitsError to
> CodeHasAssignedUnits in apiserver.serverError.

> plus an associated test and changing the comment above appropriately.

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode370
state/api/apiclient.go:370: type Pinger struct {
On 2013/05/21 15:44:01, TheMue wrote:
> +1

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode375
state/api/apiclient.go:375: func (p *Pinger) Stop() error {
On 2013/05/21 15:37:20, rog wrote:
> ditto

Done.

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

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode32
state/api/params/params.go:32: // SetStatus holds the parameters for
making the SetStatus call for a machine or unit.
On 2013/05/21 15:37:20, rog wrote:
> // SetStatus holds the parameters for Machine.SetStatus
> // and Unit.SetStatus.
> ?

Done.

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode352
state/api/params/params.go:352: Life string
On 2013/05/21 15:37:20, rog wrote:
> please change this to be of type Life.

Done.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode77
state/apiserver/api_test.go:77: // 1, because it's managing the
environment.
On 2013/05/21 15:37:20, rog wrote:
> // Machine 0 is allowed because it is an environment manager.
> ?

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode90
state/apiserver/api_test.go:90: // Machine 0 needs to set the status of
a machine when provisioning.
On 2013/05/21 15:37:20, rog wrote:
> ditto?

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode287
state/apiserver/api_test.go:287: }
On 2013/05/21 15:37:20, rog wrote:
> you can stop the pinger here (and you should check the error return
too)

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode320
state/apiserver/api_test.go:320: setDefaultStatus(c, m)
On 2013/05/21 15:37:20, rog wrote:
> rather than setting a default status here, i think you should find out
the
> previous status and set it back.
> you should also check the error when doing so.

> that way you won't need to call setDefaultStatus in setupScenario,
which is then
> free to add other machines with different statuses if necessary.

As discussed setDefaultStatus must stay (we have to move status from
pending in setupScenario, because we cannot set the status back to
Pending, which is the default), but ...

LGTM with a separate error code test.

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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode335
state/api/apiclient.go:335: // returns the started pinger.
On 2013/05/21 16:19:41, gz wrote:
> I find this name and comment slightly confusing. The call esentially
registers
> the machine over the api and creates a heartbeat object?

yeah. that's the original name in state. if we're going to change it,
that should be done in a different CL. i don't think it's worth changing
now though - feels a bit bikesheddy - although perhaps the comment could
use some improvement.

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

https://codereview.appspot.com/9614043/diff/3002/state/apiserver/api_test.go#newcode304
state/apiserver/api_test.go:304: c.Check(api.ErrCode(err), Equals,
api.CodeHasAssignedUnits)
this isn't the right place for the errcode check, i think.
there should be a separate test that checks the err code.

https://codereview.appspot.com/9614043/diff/3002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9614043/diff/3002/state/machine.go#newcode210
state/machine.go:210: func IsHasAssignedUnitsError(err error) bool {
is has :-)

https://codereview.appspot.com/9614043/diff/3002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/9614043/diff/3002/state/presence/presence.go#newcode416
state/presence/presence.go:416: // Pinger periodically reports that a
specific key is alive, so that
I think this can remain as it was. As long as it's a full sentence, the
rule isn't hard and fast.

https://codereview.appspot.com/9614043/

Thanks gz, here are some answers to the best of my understanding.

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

https://codereview.appspot.com/9614043/diff/1/state/api/apiclient.go#newcode335
state/api/apiclient.go:335: // returns the started pinger.
On 2013/05/21 16:19:41, gz wrote:
> I find this name and comment slightly confusing. The call esentially
registers
> the machine over the api and creates a heartbeat object?

No, there's no registering involved (the machine is already in state and
we just access it through the API). The heartbeat part is essentially
right - or in state parlance a presence pinger.

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

https://codereview.appspot.com/9614043/diff/1/state/api/params/params.go#newcode40
state/api/params/params.go:40: type Life string
On 2013/05/21 16:19:41, gz wrote:
> This is where I want a type system that lets you describe the set of
valid
> values in something other than a comment. :)

We already have constants for all these values in state/life.go, but
when marshalled through JSON using we rather use strings, rather than
having to define those constants again in JS client-side.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode317
state/apiserver/api_test.go:317: return func() {}, err
On 2013/05/21 16:19:41, gz wrote:
> That's a lot of this line in these test helpers... at least func() {}
is
> short-ish...

I agree it could be improved in general with some helpers.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1026
state/apiserver/api_test.go:1026: c.Assert(string(life), Equals,
"alive")
On 2013/05/21 16:19:41, gz wrote:
> Wouldn't asserting (life, Equals, params.Life("alive")) make more
sense? Or does
> that mess with test readability more than it helps with type
verification?

It probably due to my profound laziness when typing :) But I agree the
second form is probably better. If you insist I'll change it, otherwise
let's live with it.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#newcode1053
state/apiserver/api_test.go:1053: c.Assert(life, Equals, state.Alive)
On 2013/05/21 16:19:41, gz wrote:
> Hm, and this one asserts against a variable, not a string... which is
because
> it's a different type, an int enum representing the same values...

Yeah, see the comment above about JSON and marshalling. Also it's a bit
a matter of preference.

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

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#newcode528
state/apiserver/apiserver.go:528: // SetAgentAlive signals that the
agent for machine m is alive.
On 2013/05/21 16:19:41, gz wrote:
> Okay, I'm still confused by this, even seeing the other side. The
registration
> seems a pretty important side-effect that should be mentioned.

The registration here is only rel...

*** Submitted:

state/api: Machine API needed by machiner

This introduces several new API calls, needed by the
worker/machiner to handle machines through the API:
* Life
* EnsureDead
* SetAgentAlive
* SetStatus

In the course of this some refactoring of the API
server was in order:
* Introduced presence.Pinger support
* Watchers and pingers are now both handled as resources
   which have a Stop() method
* Added a couple of root-level permission checking calls:
- authOwner(), taking an entity with a Tag() method and returning
   true if the authenticated user's tag matches the entity's tag
- authEnvironManager() returning true if the authenticated user
   matches the environment manager (machine with JobManageEnviron)

Permission checks are performed to ensure only agents (or managers)
can access the new machine API calls.

A few other drive-by changes done as well as suggested.

R=rog, TheMue, gz
CC=
https://codereview.appspot.com/9614043

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

https://codereview.appspot.com/9614043/diff/3002/state/apiserver/api_test.go#newcode304
state/apiserver/api_test.go:304: c.Check(api.ErrCode(err), Equals,
api.CodeHasAssignedUnits)
On 2013/05/21 16:52:33, rog wrote:
> this isn't the right place for the errcode check, i think.
> there should be a separate test that checks the err code.

Done.

https://codereview.appspot.com/9614043/diff/3002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9614043/diff/3002/state/machine.go#newcode210
state/machine.go:210: func IsHasAssignedUnitsError(err error) bool {
On 2013/05/21 16:52:33, rog wrote:
> is has :-)

Well.. it has has :)

https://codereview.appspot.com/9614043/diff/3002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/9614043/diff/3002/state/presence/presence.go#newcode416
state/presence/presence.go:416: // Pinger periodically reports that a
specific key is alive, so that
On 2013/05/21 16:52:33, rog wrote:
> I think this can remain as it was. As long as it's a full sentence,
the rule
> isn't hard and fast.

Nah, let's keep to our defined standards.
A lot more stupid cases come to mind.

https://codereview.appspot.com/9614043/