state/api: Machiner API facade

state/api: Machiner API facade

Implemented Machiner facade providing API
methods with bulk operations support, but
only the subset of state calls needed by
the machiner worker.

Removed domain objects like Machine and Unit
both server-side and client-side. Instead,
the machiner facade at server-side provides
all the API calls needed by the machiner.
In a follow-up at the client-side there will
be another Machiner facade and lightweight
MachinerMachine wrapper preserving the state.Machine
interface, while using the facade to do the
actual calls.
Thus the machiner facade effectively implements
the state.State object, but for the machiner.

Also removed the Pinger at both sides and some
obsolete state/api/params types.

Added internal server-side tests for the machiner.

https://code.launchpad.net/~dimitern/juju-core/052-api-machiner-facade/+merge/167211

(do not edit description out of merge proposal)

[dimitern, 2013-06-04 07:42:54]

Please take a look.

[fwereade, 2013-06-04 08:37:29]

A few thoughts.

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

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode15
state/api/machiner.go:15: type MachinerMachine struct {
This is nasty, but ok for now on the understanding that we'll be getting an
api/machiner package in an imminent followup.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode51
state/api/machiner.go:51: return result.Errors[0]
Check length first please :).

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode55
state/api/machiner.go:55: func (mm *MachinerMachine) Refresh() error {
We should either remove this entirely, or grab Life() and cache it internally,
for actual interface compatibility. I think I favour the latter...

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode65
state/api/machiner.go:65: func (mm *MachinerMachine) Life() (params.Life, error)
{
...because this isn't compatible either.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode74
state/api/machiner.go:74: if err := result.Machines[0].Error; err != nil {
Hmm, should we be converting NotFound into Dead here?

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

https://codereview.appspot.com/9896046/diff/1/state/api/state.go#newcode27
state/api/state.go:27: // Just verify we're allowed to access it.
Not convinced there's much value to doing this.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:101: if err := conn.Serve(srv.root, serverError);
err != nil {
Not clear on the impact of this change, but it smells a bit. Isn't
connection-specific auth accessed through root? If that's so, surely we can't
share it?

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/login_test.go#n...
state/apiserver/login_test.go:34: func (s *suite) TestBadLogin(c *C) {
Can we please keep test suites in their own files, instead of sharing one across
the whole package? I know this is pre-existing, but I'd really appreciate a fix,
because I have literally no idea what a *suite is or does.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/login_test.go#n...
state/apiserver/login_test.go:67: c.Assert(api.ErrCode(err), Equals,
api.CodeUnauthorized, Commentf("error %#v", err))
Hold on... so we can open an API connection using a struct that includes a name
and password, but we don't actually use those fields at all? The surprising call
in st.Machiner makes more sense now... but I'm still not sure it's a good idea.

Probably out of scope, but please TODO this craziness at source.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:29: Id: machine.Id(),
This seems like an awful lot of work to turn a list of ids into a list of ids.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:50: if !m.root.authOwner(machine) &&
!m.root.authEnvironManager() {
Drop all the authEnvironManager checks -- the machiner interface should surely
only be accessible to the given machine, right? (*for now*, that is -- IMO this
feature of the current codebase is not sufficient justification to unbulk these
ops)

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:63: // TODO (dimitern) implement this once the
watchers can handle bulk ops
I don't think this is a matter of making watchers handle bulk ops, is it? It's
just a matter of returning N watcher ids in response to N machine ids.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner_test.g...
state/apiserver/machiner_test.go:73: c.Assert(err, ErrorMatches,
api.CodeBadVersion)
Ah! I wondered about CodeBadVersion. Maybe double-check its docs, and see
whether they could be improved (or whether I'm just dense).

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner_test.g...
state/apiserver/machiner_test.go:88: c.Assert(result.Machines[0].Machine.Id,
Equals, "1")
This'll need to change given the auth changes.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/perm_test.go
File state/apiserver/perm_test.go (left):

https://codereview.appspot.com/9896046/diff/1/state/apiserver/perm_test.go#ol...
state/apiserver/perm_test.go:584: }
wooooot!

[dimitern, 2013-06-04 11:06:17]

Please take a look.

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

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode15
state/api/machiner.go:15: type MachinerMachine struct {
On 2013/06/04 08:37:29, fwereade wrote:
> This is nasty, but ok for now on the understanding that we'll be getting an
> api/machiner package in an imminent followup.

Yes, as agreed.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode51
state/api/machiner.go:51: return result.Errors[0]
On 2013/06/04 08:37:29, fwereade wrote:
> Check length first please :).

Done.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode55
state/api/machiner.go:55: func (mm *MachinerMachine) Refresh() error {
On 2013/06/04 08:37:29, fwereade wrote:
> We should either remove this entirely, or grab Life() and cache it internally,
> for actual interface compatibility. I think I favour the latter...

Good point, done.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode65
state/api/machiner.go:65: func (mm *MachinerMachine) Life() (params.Life, error)
{
On 2013/06/04 08:37:29, fwereade wrote:
> ...because this isn't compatible either.

Changed to be compatible.

https://codereview.appspot.com/9896046/diff/1/state/api/machiner.go#newcode74
state/api/machiner.go:74: if err := result.Machines[0].Error; err != nil {
On 2013/06/04 08:37:29, fwereade wrote:
> Hmm, should we be converting NotFound into Dead here?

Life is now cached, so no errors to return.

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

https://codereview.appspot.com/9896046/diff/1/state/api/state.go#newcode27
state/api/state.go:27: // Just verify we're allowed to access it.
On 2013/06/04 08:37:29, fwereade wrote:
> Not convinced there's much value to doing this.

We need this to guarantee "not logged in"/"permission denied" (you're not an
agent) is reported as expected.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:101: if err := conn.Serve(srv.root, serverError);
err != nil {
On 2013/06/04 08:37:29, fwereade wrote:
> Not clear on the impact of this change, but it smells a bit. Isn't
> connection-specific auth accessed through root? If that's so, surely we can't
> share it?

Good catch! Reverted and changed as agreed - machiner gets a *state.State and
Authorizer interface, so this is no longer needed.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/login_test.go#n...
state/apiserver/login_test.go:34: func (s *suite) TestBadLogin(c *C) {
On 2013/06/04 08:37:29, fwereade wrote:
> Can we please keep test suites in their own files, instead of sharing one
across
> the whole package? I know this is pre-existing, but I'd really appreciate a
fix,
> because I have literally no idea what a *suite is or does.

Let's do that in a follow-up please.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/login_test.go#n...
state/apiserver/login_test.go:67: c.Assert(api.ErrCode(err), Equals,
api.CodeUnauthorized, Commentf("error %#v", err))
On 2013/06/04 08:37:29, fwereade wrote:
> Hold on... so we can open an API connection using a struct that includes a
name
> and password, but we don't actually use those fields at all? The surprising
call
> in st.Machiner makes more sense now... but I'm still not sure it's a good
idea.
> 
> Probably out of scope, but please TODO this craziness at source.

Added TODO in the test to describe better what's going on.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:29: Id: machine.Id(),
On 2013/06/04 08:37:29, fwereade wrote:
> This seems like an awful lot of work to turn a list of ids into a list of ids.

Removed; now using Life instead internally.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:50: if !m.root.authOwner(machine) &&
!m.root.authEnvironManager() {
On 2013/06/04 08:37:29, fwereade wrote:
> Drop all the authEnvironManager checks -- the machiner interface should surely
> only be accessible to the given machine, right? (*for now*, that is -- IMO
this
> feature of the current codebase is not sufficient justification to unbulk
these
> ops)

Done.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner.go#new...
state/apiserver/machiner.go:63: // TODO (dimitern) implement this once the
watchers can handle bulk ops
On 2013/06/04 08:37:29, fwereade wrote:
> I don't think this is a matter of making watchers handle bulk ops, is it? It's
> just a matter of returning N watcher ids in response to N machine ids.

Right! But I'd still prefer to do it in a follow-up.

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

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner_test.g...
state/apiserver/machiner_test.go:73: c.Assert(err, ErrorMatches,
api.CodeBadVersion)
On 2013/06/04 08:37:29, fwereade wrote:
> Ah! I wondered about CodeBadVersion. Maybe double-check its docs, and see
> whether they could be improved (or whether I'm just dense).

Changed to "API version not available" as suggested online.

https://codereview.appspot.com/9896046/diff/1/state/apiserver/machiner_test.g...
state/apiserver/machiner_test.go:88: c.Assert(result.Machines[0].Machine.Id,
Equals, "1")
On 2013/06/04 08:37:29, fwereade wrote:
> This'll need to change given the auth changes.

It's gone now.

[dimitern, 2013-06-04 11:16:41]

Please take a look.

[fwereade, 2013-06-04 11:59:09]

LGTM modulo a quick live chat re comments -- some might appreciate a fix this
CL, but none is a dealbreaker (and stripping out the client code for reproposal
in a new CL with tests, as agreed).

https://codereview.appspot.com/9896046/diff/11001/state/api/machiner.go
File state/api/machiner.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/machiner.go#newco...
state/api/machiner.go:35: return "", fmt.Errorf("expected server response, got
nothing")
expected one result, got <N>?

https://codereview.appspot.com/9896046/diff/11001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/state.go#newcode27
state/api/state.go:27: // Just verify we're allowed to access it.
OK, and this is all because of the Login behaviour that lets us have an open
conn without actually being authorized, right? Rog, would you remind us what the
benefit of this behaviour is?

(Hmm, is it even necessary, even then? Wouldn't the Authorizer used in the
Machiner methods be able to barf the same error regardless?)

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go
File state/apiserver/machiner.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:26: machine, err := m.st.Machine(arg.Id)
These could in general be smooshed inside the if; YMMV.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:28: // Allow only for the owner agent or the
environment manager
s/or the environment manager//

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:90: }
There are some *very* similar methods here, but maybe not quite similar enough
to factor out. Bah.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
File state/apiserver/machiner_test.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:51: setDefaultPassword(c, s.machine0)
s/0/1/

...but consider whether we need password at all, now we have actual unit tests
here and have dropped all the login stuff.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:92: // ...machine 1 is fine thought.
s/ght/gh/

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:135: c.Assert(s.machine1.Life(), Equals,
state.Dead)
Is it overkill to also check that EnsureDead works on a Dead machine, and on a
missing one? (which I don't think it currently does... and I *think* it should,
although I guess it could be argued that it's irrelevant in context...
thoughts?)

[mue, 2013-06-04 12:25:53]

LGTM, only the already know comments and some smaller additional ones.

https://codereview.appspot.com/9896046/diff/11001/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/apierror.go#newco...
state/api/apierror.go:28: CodeBadVersion          = "API version not available"
Would prefer "not supported".

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

https://codereview.appspot.com/9896046/diff/11001/state/api/params/params.go#...
state/api/params/params.go:80: Error *Error
Nothing important, but I prefer the errors here as the last member of the
struct. Like the last value returned when calling a function. But again, not
really important.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/apierror.go
File state/apiserver/apierror.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/apierror.go...
state/apiserver/apierror.go:16: errBadVersion     = stderrors.New("API version
not available")
Here again "not supported".

[dimitern, 2013-06-04 13:10:14]

*** Submitted:

state/api: Machiner API facade

Implemented Machiner facade providing API
methods with bulk operations support, but
only the subset of state calls needed by
the machiner worker.

Removed domain objects like Machine and Unit
both server-side and client-side. Instead,
the machiner facade at server-side provides
all the API calls needed by the machiner.
In a follow-up at the client-side there will
be another Machiner facade and lightweight
MachinerMachine wrapper preserving the state.Machine
interface, while using the facade to do the
actual calls.
Thus the machiner facade effectively implements
the state.State object, but for the machiner.

Also removed the Pinger at both sides and some
obsolete state/api/params types.

Added internal server-side tests for the machiner.

R=fwereade, mue
CC=
https://codereview.appspot.com/9896046

https://codereview.appspot.com/9896046/diff/11001/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/apierror.go#newco...
state/api/apierror.go:28: CodeBadVersion          = "API version not available"
On 2013/06/04 12:25:54, mue wrote:
> Would prefer "not supported".

Good point, done.

https://codereview.appspot.com/9896046/diff/11001/state/api/machiner.go
File state/api/machiner.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/machiner.go#newco...
state/api/machiner.go:35: return "", fmt.Errorf("expected server response, got
nothing")
On 2013/06/04 11:59:09, fwereade wrote:
> expected one result, got <N>?

As agreed, will remove the client-side machiner from this CL and propose it +
tests in a follow-up, including this suggestion.

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

https://codereview.appspot.com/9896046/diff/11001/state/api/params/params.go#...
state/api/params/params.go:80: Error *Error
On 2013/06/04 12:25:54, mue wrote:
> Nothing important, but I prefer the errors here as the last member of the
> struct. Like the last value returned when calling a function. But again, not
> really important.

Done.

https://codereview.appspot.com/9896046/diff/11001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/api/state.go#newcode27
state/api/state.go:27: // Just verify we're allowed to access it.
On 2013/06/04 11:59:09, fwereade wrote:
> OK, and this is all because of the Login behaviour that lets us have an open
> conn without actually being authorized, right? Rog, would you remind us what
the
> benefit of this behaviour is?
> 
> (Hmm, is it even necessary, even then? Wouldn't the Authorizer used in the
> Machiner methods be able to barf the same error regardless?)

You'll get "not logged in" from the first method you call, but I thought it's
nicer to get this right away here.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/apierror.go
File state/apiserver/apierror.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/apierror.go...
state/apiserver/apierror.go:16: errBadVersion     = stderrors.New("API version
not available")
On 2013/06/04 12:25:54, mue wrote:
> Here again "not supported".

Done.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go
File state/apiserver/machiner.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:26: machine, err := m.st.Machine(arg.Id)
On 2013/06/04 11:59:09, fwereade wrote:
> These could in general be smooshed inside the if; YMMV.

Not really, because err is reused across all cases.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:28: // Allow only for the owner agent or the
environment manager
On 2013/06/04 11:59:09, fwereade wrote:
> s/or the environment manager//

Done.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner.go...
state/apiserver/machiner.go:90: }
On 2013/06/04 11:59:09, fwereade wrote:
> There are some *very* similar methods here, but maybe not quite similar enough
> to factor out. Bah.

I actually tried having a helper taking a list of ids and a callback taking
index and *state.Machine, returning []error. But actually using it makes the
code less readable :/

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
File state/apiserver/machiner_test.go (right):

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:51: setDefaultPassword(c, s.machine0)
On 2013/06/04 11:59:09, fwereade wrote:
> s/0/1/
> 
> ...but consider whether we need password at all, now we have actual unit tests
> here and have dropped all the login stuff.

Good point, will remove password setting.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:92: // ...machine 1 is fine thought.
On 2013/06/04 11:59:09, fwereade wrote:
> s/ght/gh/

Done.

https://codereview.appspot.com/9896046/diff/11001/state/apiserver/machiner_te...
state/apiserver/machiner_test.go:135: c.Assert(s.machine1.Life(), Equals,
state.Dead)
On 2013/06/04 11:59:09, fwereade wrote:
> Is it overkill to also check that EnsureDead works on a Dead machine, and on a
> missing one? (which I don't think it currently does... and I *think* it
should,
> although I guess it could be argued that it's irrelevant in context...
> thoughts?)

The 42 test actually proves it doesn't work on missing machines. If it's
absolutely a required behavior I can add a special case. But again, think of the
client-side - maybe we can fake it out there instead.
Also, will add a test for a Dead machine.