state/api: Machine API needed by machiner

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.

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

(do not edit description out of merge proposal)

[dimitern, 2013-05-21 15:11:37]

Please take a look.

[rog, 2013-05-21 15:37:20]

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#newc...
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#newc...
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#new...
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#new...
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#new...
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#new...
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#new...
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#ne...
state/apiserver/apiserver.go:246: // authOwner returns true only if the
authenticated user's tag
// authOwner returns whether the authenticated user's tag
// matches the given entity's tag.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:253: // authEnvironManager returns true only if the
authenticated user is
// authEnvironManager returns whether the authenticated
// user is a machine with running the ManageEnviron job.

[TheMue, 2013-05-21 15:44:01]

Mostly LGTM, only minor comments additional to Rogers.

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#newcode370
state/api/apiclient.go:370: type Pinger struct {
+1

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#ne...
state/apiserver/apiserver.go:214: func (r *srvRoot) AllWatcher(id string)
(srvClientAllWatcher, error) {
Would like to see a doc comment here, just to be consistent.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:268: ew := w.r.(*state.EntityWatcher)
Hmm, "ew", "w", "r", not very self-documenting variable names. Using one char
for the type is OK, but the other ones?

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:570: if !m.root.authOwner(m.m) &&
!m.root.authEnvironManager() {
Elegant.

[rog, 2013-05-21 16:05:14]

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#ne...
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.

[gz, 2013-05-21 16:19:40]

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#newc...
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#new...
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#new...
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#new...
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#ne...
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#ne...
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.

[dimitern, 2013-05-21 16:41:25]

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#newc...
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#newc...
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#new...
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#new...
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#new...
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#new...
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 I added reading and resetting the original status and info.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:692: func setDefaultStatus(c *C, entity setStatuser)
{
On 2013/05/21 15:37:20, rog wrote:
> as implied above, i don't think this is necessary.

See above.

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#ne...
state/apiserver/apiserver.go:214: func (r *srvRoot) AllWatcher(id string)
(srvClientAllWatcher, error) {
On 2013/05/21 15:44:01, TheMue wrote:
> Would like to see a doc comment here, just to be consistent.

Me too :)
Ok, I added it, even though it wasn't part of my changes.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:246: // authOwner returns true only if the
authenticated user's tag
On 2013/05/21 15:37:20, rog wrote:
> // authOwner returns whether the authenticated user's tag
> // matches the given entity's tag.

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:253: // authEnvironManager returns true only if the
authenticated user is
On 2013/05/21 15:37:20, rog wrote:
> // authEnvironManager returns whether the authenticated
> // user is a machine with running the ManageEnviron job.

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:268: ew := w.r.(*state.EntityWatcher)
On 2013/05/21 16:05:14, rog wrote:
> 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.

Done.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
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?

I don't like it either.. Anyway I changed some names to be more descriptive.

[rog, 2013-05-21 16:52:33]

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#...
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#n...
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.

[dimitern, 2013-05-21 16:52:53]

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#newc...
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#new...
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#new...
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#new...
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#ne...
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 relevant to the internals of the API server - it
does not affect how it's used externally. The registration is necessary to keep
track of all per-client connection resources, and has nothing to do with the
machine itself.

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:570: if !m.root.authOwner(m.m) &&
!m.root.authEnvironManager() {
On 2013/05/21 15:44:01, TheMue wrote:
> Elegant.

Cheers :)

https://codereview.appspot.com/9614043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:737: rs.rs = make(map[string]*srvResource)
On 2013/05/21 16:19:41, gz wrote:
> Assuming the compiler/tests will catch any of the mechanical rename issues,and
> that somebody understands the variables.

I changed them a bit - see the next patch set, should be better now.

[dimitern, 2013-05-21 17:26:27]

*** 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#...
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#n...
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.