Merge lp:~gz/juju-core/057-api-machiner-watch into lp:~go-bot/juju-core/trunk

[Martin Packman]

state/apiserver: Implement Machiner.Watch

Server-side implementation of the final API call needed by the machiner.

R=fwereade, rogpeppe, wallyworld

state/apiserver: Implement Machiner.Watch

Adopting Dimiter's branch, with merge conflicts on trunk resolved.

See <https://codereview.appspot.com/9937045/> for earlier discussion.

As I understand it, William would like a couple more tests, but all
other comments were addressed.

https://codereview.appspot.com/10439043/

[Martin Packman (gz)]

Reviewers: mp+170586_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver: Implement Machiner.Watch

Adopting Dimiter's branch, with merge conflicts on trunk resolved.

See <https://codereview.appspot.com/9937045/> for earlier discussion.

As I understand it, William would like a couple more tests, but all
other comments were addressed.

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

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/api/params/params.go
   M state/apiserver/client.go
   M state/apiserver/common/interfaces.go
   M state/apiserver/machine/machiner.go
   M state/apiserver/machine/machiner_test.go
   M state/apiserver/resource.go
   M state/apiserver/root.go

[Roger Peppe (rogpeppe)]

LGTM with a couple of trivial suggestions.

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

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner.go#newcode15
state/apiserver/machine/machiner.go:15: resourceRegistry
common.ResourceRegistry
s/resourceRegistry/resources/ ?

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

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner_test.go#newcode19
state/apiserver/machine/machiner_test.go:19: resourcesRegistry
fakeResourceRegistry
s/resourcesRegistry/resources/ ?

https://codereview.appspot.com/10439043/

[Roger Peppe (rogpeppe)]

one thought:

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#newcode70
state/api/params/params.go:70: type MachinesWatchResults struct {
i think this should probably be EntityWatchResults, as anything that
returns an entity watcher will want to use the same type.

https://codereview.appspot.com/10439043/

[Martin Packman (gz)]

Please take a look.

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#newcode70
state/api/params/params.go:70: type MachinesWatchResults struct {
On 2013/06/20 15:29:34, rog wrote:
> i think this should probably be EntityWatchResults, as anything that
returns an
> entity watcher will want to use the same type.

Done, have left MachinesWatchResults named as currently. I'm also not
crazy about the construction of the array being done in the apiserver
module having to know about the array/etc from params to construct one
of a suitable length, but this is all old territory and let's not go
there.

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

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner.go#newcode15
state/apiserver/machine/machiner.go:15: resourceRegistry
common.ResourceRegistry
On 2013/06/20 13:37:26, rog wrote:
> s/resourceRegistry/resources/ ?

Done.

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

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner_test.go#newcode19
state/apiserver/machine/machiner_test.go:19: resourcesRegistry
fakeResourceRegistry
On 2013/06/20 13:37:26, rog wrote:
> s/resourcesRegistry/resources/ ?

Done.

https://codereview.appspot.com/10439043/

[Roger Peppe (rogpeppe)]

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#newcode70
state/api/params/params.go:70: type MachinesWatchResults struct {
On 2013/06/20 16:01:30, gz wrote:
> On 2013/06/20 15:29:34, rog wrote:
> > i think this should probably be EntityWatchResults, as anything that
returns
> an
> > entity watcher will want to use the same type.

> Done, have left MachinesWatchResults named as currently. I'm also not
crazy
> about the construction of the array being done in the apiserver module
having to
> know about the array/etc from params to construct one of a suitable
length, but
> this is all old territory and let's not go there.

I think that EntityWatchResults is probably right actually.
Any bulk call that watches a set of entities will be able to use it, and
that seems reasonable to me.

I'm not sure what you mean about the array construction. Surely we can't
hide the fact that there's a slice here?

https://codereview.appspot.com/10439043/

[Martin Packman (gz)]

On 2013/06/20 16:13:25, rog wrote:

> I think that EntityWatchResults is probably right actually.
> Any bulk call that watches a set of entities will be able to use it,
and that
> seems reasonable to me.

> I'm not sure what you mean about the array construction. Surely we
can't hide
> the fact that there's a slice here?

I think it depends how you view that outer struct. Per the comment, I
was envisioning it as a "thing you pass to a particular api call to get
results", in which case you want one per api call, even if there happens
to be overlap. Also, you then perhaps don't want the caller having to
know all the details of the struct in order to create one and pass it
in. Maybe that's the wrong way of looking at it?

https://codereview.appspot.com/10439043/

[Ian Booth (wallyworld)]

LGTM. I have no real view on the array question as I'm not fully across
the nuances of it.

https://codereview.appspot.com/10439043/

[Roger Peppe (rogpeppe)]

On 20 June 2013 17:30, <email address hidden> wrote:
> On 2013/06/20 16:13:25, rog wrote:
>
>> I think that EntityWatchResults is probably right actually.
>> Any bulk call that watches a set of entities will be able to use it,
>
> and that
>>
>> seems reasonable to me.
>
>
>> I'm not sure what you mean about the array construction. Surely we
>
> can't hide
>>
>> the fact that there's a slice here?
>
>
> I think it depends how you view that outer struct. Per the comment, I
> was envisioning it as a "thing you pass to a particular api call to get
> results", in which case you want one per api call, even if there happens
> to be overlap. Also, you then perhaps don't want the caller having to
> know all the details of the struct in order to create one and pass it
> in. Maybe that's the wrong way of looking at it?

It's really just the return value of an API call. It needs a separate type
if the API call is returning something that no other API call returns,
but otherwise it's fine to share.

If we later need to add some other stuff to the return value for
a particular API call that shares a return type with another,
it's easy to change the return type - it's not part of the wire
contract and the return type can be changed, and a the other
stuff added, with no more compatibility
issues than adding a field to a non-shared return type.

In a way I think it's nice to share when possible as it's less lines
of code and easier to see the commonality (also easier to
write generic helper functions).

BTW the caller doesn't have to know the details of the struct
to pass it in. They do of course have to know the details of
the struct in order to deal with the results though.

[William Reade (fwereade)]

On 2013/06/21 06:31:34, rog wrote:
> It's really just the return value of an API call. It needs a separate
type
> if the API call is returning something that no other API call returns,
> but otherwise it's fine to share.

+1

https://codereview.appspot.com/10439043/

[William Reade (fwereade)]

Commented where I think we need better testing. Otherwise LGTM.

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go#newcode173
state/apiserver/machine/machiner_test.go:173:
Can we perhaps call Next with the valid watcher's id and see what we
get?

https://codereview.appspot.com/10439043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go#newcode173
state/apiserver/machine/machiner_test.go:173:
On 2013/06/21 10:22:48, fwereade wrote:
> Can we perhaps call Next with the valid watcher's id and see what we
get?

I've had a crack at doing something with the watcher, if you can give me
more of a hint as to what we want I can improve the check.

https://codereview.appspot.com/10439043/

[William Reade (fwereade)]

On 2013/06/24 12:56:02, gz wrote:
> Please take a look.

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go
> File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go#newcode173
> state/apiserver/machine/machiner_test.go:173:
> On 2013/06/21 10:22:48, fwereade wrote:
> > Can we perhaps call Next with the valid watcher's id and see what we
get?

> I've had a crack at doing something with the watcher, if you can give
me more of
> a hint as to what we want I can improve the check.

LGTM, as discussed, but please read the first event off Changes() before
returning from Watch(); then do some basic verification of the watcher's
state with something like NotifyWatcherC.

https://codereview.appspot.com/10439043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/10439043/

[Roger Peppe (rogpeppe)]

LGTM

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/machiner_test.go
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/machiner_test.go#newcode180
state/apiserver/machine/machiner_test.go:180: err = resource.Stop()
s/=/:=

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/machiner_test.go#newcode186
state/apiserver/machine/machiner_test.go:186: // Should use helpers from
state/watcher_test.go when generalised
we've got loads of places that read from a channel without a timeout. if
we generalise the code, we'll look for and change lots of them, so i
don't think this comment is really necessary.

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/machiner_test.go#newcode188
state/apiserver/machine/machiner_test.go:188: case _, ok := <-channel:
this isn't actually checking that the watcher is watching the correct
machine, but i'm ok if we leave that for the client-side test.

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/machiner_test.go#newcode191
state/apiserver/machine/machiner_test.go:191: case <-time.After(50 *
time.Millisecond):
2-5 seconds would be better. this is the worst case, and we want to be
resilient to slow machines.

https://codereview.appspot.com/10439043/

[Go Bot (go-bot)]

No approved revision specified.

[Go Bot (go-bot)]

The attempt to merge lp:~gz/juju-core/057-api-machiner-watch into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/bzr 7.360s
ok launchpad.net/juju-core/cert 4.845s
ok launchpad.net/juju-core/charm 0.553s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.008s
ok launchpad.net/juju-core/cmd 0.227s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 129.914s

----------------------------------------------------------------------
PANIC: agent.go:0: MachineSuite.TearDownTest

... Panic: Cannot drop MongoDB database juju: local error: bad record MAC (PC=0x41176C)

/usr/lib/go/src/pkg/runtime/proc.c:1443
  in panic
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:221
  in MgoReset
/home/tarmac/trees/src/launchpad.net/juju-core/environs/dummy/environs.go:218
  in environState.destroy
/home/tarmac/trees/src/launchpad.net/juju-core/environs/dummy/environs.go:189
  in Reset
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:224
  in JujuConnSuite.tearDownConn
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:132
  in JujuConnSuite.TearDownTest

----------------------------------------------------------------------
PANIC: machine_test.go:155: MachineSuite.TestHostUnits

[LOG] 82.19123 INFO juju environs/testing: uploading FAKE tools 1.11.1-precise-amd64
[LOG] 82.19131 INFO juju environs: reading tools with major version 1
[LOG] 82.19133 DEBUG juju environs/tools: reading v1.* tools
[LOG] 82.19135 INFO juju environs: falling back to public bucket
[LOG] 82.19137 DEBUG juju environs/tools: reading v1.* tools
[LOG] 82.19141 DEBUG juju environs/tools: found 1.11.1-precise-amd64
[LOG] 82.19143 INFO juju environs: filtering tools by series: precise
[LOG] 82.19145 INFO juju environs: filtering tools by version: 1.11.1
[LOG] 82.19148 INFO juju environs/dummy: would pick tools from 1.11.1-precise-amd64
[LOG] 82.21977 INFO juju state: opening state; mongo addresses: ["localhost:57176"]; entity ""
[LOG] 82.22346 INFO juju state: connection established
[LOG] 82.28876 INFO juju state: initializing environment
[LOG] 82.31655 INFO juju state/api: listening on "localhost:0"
[LOG] 82.34608 INFO juju state: opening state; mongo addresses: ["localhost:57176"]; entity ""
[LOG] 82.34955 INFO juju state: connection established
[LOG] 82.35024 INFO juju juju: authorization error while connecting to state server; retrying
[LOG] 82.35030 INFO juju state: opening state; mongo addresses: ["localhost:57176"]; entity ""
[LOG] 82.35341 INFO juju state: connection established
[LOG] 82.40812 INFO juju state/api: dialing "wss://127.0.0.1:50277/"
[LOG] 82.41266 INFO juju state/api: connection established
[LOG] 82.41286 DEBUG juju rpc/jsoncodec: <- {"RequestId":1,"Type":"Admin","Request":"Login","Params":{"AuthTag":"user-admin","Password":"dummy-secret"}}
[LOG] 82.41335 DEBUG juju rpc/jsoncodec: -> {"RequestId":1,"Response":{}}
[LOG] 82.41971 DEBUG juju rpc/jsoncodec: <- {"RequestId":2,"Typ...

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2013-06-20 16:59:05 +0000
+++ state/api/params/params.go	2013-06-25 11:18:26 +0000
@@ -60,6 +60,17 @@
 	Machines []MachineSetStatus
 }
 
+// EntityWatchResult holds an EntityWatcher id and an error (if any).
+type EntityWatchResult struct {
+	EntityWatcherId string
+	Error           *Error
+}
+
+// MachinesWatchResults holds the parameters for making a Machiner.Watch call.
+type MachinesWatchResults struct {
+	Results []EntityWatchResult
+}
+
 // AddRelation holds the parameters for making the AddRelation call.
 // The endpoints specified are unordered.
 type AddRelation struct {

=== modified file 'state/apiserver/client.go'
--- state/apiserver/client.go	2013-06-12 18:19:17 +0000
+++ state/apiserver/client.go	2013-06-25 11:18:26 +0000
@@ -37,7 +37,7 @@
 func (c *srvClient) WatchAll() (params.AllWatcherId, error) {
 	w := c.root.srv.state.Watch()
 	return params.AllWatcherId{
-		AllWatcherId: c.root.resources.register(w).id,
+		AllWatcherId: c.root.resources.Register(w),
 	}, nil
 }
 

=== modified file 'state/apiserver/common/interfaces.go'
--- state/apiserver/common/interfaces.go	2013-06-22 08:49:38 +0000
+++ state/apiserver/common/interfaces.go	2013-06-25 11:18:26 +0000
@@ -27,3 +27,20 @@
 	// a machine running the environment manager job.
 	AuthEnvironManager() bool
 }
+
+// Resource represents any resource that should be cleaned up when an
+// API connection terminates. The Stop method will be called when
+// that happens.
+type Resource interface {
+	Stop() error
+}
+
+// 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.
+type ResourceRegistry interface {
+	// 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.
+	Register(resource Resource) string
+}

=== modified file 'state/apiserver/machine/machiner.go'
--- state/apiserver/machine/machiner.go	2013-06-22 08:49:38 +0000
+++ state/apiserver/machine/machiner.go	2013-06-25 11:18:26 +0000
@@ -9,18 +9,19 @@
 	"launchpad.net/juju-core/state/apiserver/common"
 )
 
-// Machiner implements the API used by the machiner worker.
+// MachinerAPI implements the API used by the machiner worker.
 type MachinerAPI struct {
-	st   *state.State
-	auth common.Authorizer
+	st        *state.State
+	resources common.ResourceRegistry
+	auth      common.Authorizer
 }
 
 // NewMachinerAPI creates a new instance of the Machiner API.
-func NewMachinerAPI(st *state.State, authorizer common.Authorizer) (*MachinerAPI, error) {
+func NewMachinerAPI(st *state.State, resources common.ResourceRegistry, authorizer common.Authorizer) (*MachinerAPI, error) {
 	if !authorizer.AuthMachineAgent() {
 		return nil, common.ErrPerm
 	}
-	return &MachinerAPI{st, authorizer}, nil
+	return &MachinerAPI{st, resources, authorizer}, nil
 }
 
 // SetStatus sets the status of each given machine.
@@ -47,9 +48,28 @@
 }
 
 // Watch starts an EntityWatcher for each given machine.
-//func (m *MachinerAPI) Watch(args params.Machines) (params.MachinerWatchResults, error) {
-// TODO (dimitern) implement this once the watchers can handle bulk ops
-//}
+func (m *MachinerAPI) Watch(args params.Machines) (params.MachinesWatchResults, error) {
+	result := params.MachinesWatchResults{
+		Results: make([]params.EntityWatchResult, len(args.Ids)),
+	}
+	if len(args.Ids) == 0 {
+		return result, nil
+	}
+	for i, id := range args.Ids {
+		machine, err := m.st.Machine(id)
+		if err == nil {
+			// Allow only for the owner agent.
+			if !m.auth.AuthOwner(machine.Tag()) {
+				err = common.ErrPerm
+			} else {
+				watcher := machine.Watch()
+				result.Results[i].EntityWatcherId = m.resources.Register(watcher)
+			}
+		}
+		result.Results[i].Error = common.ServerError(err)
+	}
+	return result, nil
+}
 
 // Life returns the lifecycle state of each given machine.
 func (m *MachinerAPI) Life(args params.Machines) (params.MachinesLifeResults, error) {

=== modified file 'state/apiserver/machine/machiner_test.go'
--- state/apiserver/machine/machiner_test.go	2013-06-22 08:49:38 +0000
+++ state/apiserver/machine/machiner_test.go	2013-06-25 11:18:26 +0000
@@ -8,22 +8,41 @@
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/state/apiserver/machine"
+	"strconv"
+	"time"
 )
 
 type machinerSuite struct {
 	commonSuite
-	machiner *machine.MachinerAPI
+
+	resources fakeResourceRegistry
+	machiner  *machine.MachinerAPI
 }
 
 var _ = Suite(&machinerSuite{})
 
+// fakeResourceRegistry implements the common.ResourceRegistry interface.
+type fakeResourceRegistry map[string]common.Resource
+
+func (registry fakeResourceRegistry) Register(resource common.Resource) string {
+	id := strconv.Itoa(len(registry))
+	registry[id] = resource
+	return id
+}
+
 func (s *machinerSuite) SetUpTest(c *C) {
 	s.commonSuite.SetUpTest(c)
 
+	// Create the resource registry separately to track invocations to
+	// Register.
+	s.resources = make(fakeResourceRegistry)
+
 	// Create a machiner API for machine 1.
 	machiner, err := machine.NewMachinerAPI(
 		s.State,
+		s.resources,
 		s.authorizer,
 	)
 	c.Assert(err, IsNil)
@@ -39,7 +58,7 @@
 func (s *machinerSuite) TestMachinerFailsWithNonMachineAgentUser(c *C) {
 	anAuthorizer := s.authorizer
 	anAuthorizer.machineAgent = false
-	aMachiner, err := machine.NewMachinerAPI(s.State, anAuthorizer)
+	aMachiner, err := machine.NewMachinerAPI(s.State, s.resources, anAuthorizer)
 	c.Assert(err, NotNil)
 	c.Assert(aMachiner, IsNil)
 	c.Assert(err, ErrorMatches, "permission denied")
@@ -130,3 +149,37 @@
 	c.Assert(err, IsNil)
 	c.Assert(s.machine1.Life(), Equals, state.Dead)
 }
+
+func (s *machinerSuite) TestWatch(c *C) {
+	c.Assert(s.resources, HasLen, 0)
+
+	args := params.Machines{
+		Ids: []string{"1", "0", "42"},
+	}
+	result, err := s.machiner.Watch(args)
+	c.Assert(err, IsNil)
+	c.Assert(result.Results, HasLen, 3)
+	c.Assert(result.Results[0].Error, IsNil)
+	s.assertError(c, result.Results[1].Error, api.CodeUnauthorized, "permission denied")
+	s.assertError(c, result.Results[2].Error, api.CodeNotFound, "machine 42 not found")
+
+	// Verify the resource was registered and stop when done
+	c.Assert(s.resources, HasLen, 1)
+	c.Assert(result.Results[0].EntityWatcherId, Equals, "0")
+	resource := s.resources["0"]
+	defer func() {
+		err := resource.Stop()
+		c.Assert(err, IsNil)
+	}()
+
+	// Check that the watcher returns an initial event
+	channel := resource.(*state.EntityWatcher).Changes()
+	// Should use helpers from state/watcher_test.go when generalised
+	select {
+	case _, ok := <-channel:
+		c.Assert(ok, Equals, true)
+		// The value is just an empty struct currently
+	case <-time.After(50 * time.Millisecond):
+		c.Fatal("timeout waiting for entity watcher")
+	}
+}

=== modified file 'state/apiserver/resource.go'
--- state/apiserver/resource.go	2013-05-24 19:03:39 +0000
+++ state/apiserver/resource.go	2013-06-25 11:18:26 +0000
@@ -5,15 +5,11 @@
 
 import (
 	"launchpad.net/juju-core/log"
+	"launchpad.net/juju-core/state/apiserver/common"
 	"strconv"
 	"sync"
 )
 
-// resource represents the interface provided by state watchers and pingers.
-type resource interface {
-	Stop() error
-}
-
 // resources holds all the resources for a connection.
 type resources struct {
 	mu    sync.Mutex
@@ -25,7 +21,7 @@
 // Stop RPC method for all resources.
 type srvResource struct {
 	rs       *resources
-	resource resource
+	resource common.Resource
 	id       string
 }
 
@@ -55,9 +51,9 @@
 	return rs.rs[id]
 }
 
-// register records the given watcher and returns
-// a srvResource instance for it.
-func (rs *resources) register(r resource) *srvResource {
+// Register records the given watcher and returns
+// its id.
+func (rs *resources) Register(r common.Resource) string {
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
 	rs.maxId++
@@ -67,7 +63,7 @@
 		resource: r,
 	}
 	rs.rs[sr.id] = sr
-	return sr
+	return sr.id
 }
 
 func (rs *resources) stopAll() {

=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2013-06-22 08:49:38 +0000
+++ state/apiserver/root.go	2013-06-25 11:18:26 +0000
@@ -62,14 +62,15 @@
 	return nil
 }
 
-// Machiner returns an object that provides access to the Machiner API.
-// The id argument is reserved for future use and must currently
-// be empty.
+// Machiner returns an object that provides access to the Machiner API
+// facade. The id argument is reserved for future use and currently
+// needs to be empty.
 func (r *srvRoot) Machiner(id string) (*machine.MachinerAPI, error) {
 	if id != "" {
+		// Safeguard id for possible future use.
 		return nil, common.ErrBadId
 	}
-	return machine.NewMachinerAPI(r.srv.state, r)
+	return machine.NewMachinerAPI(r.srv.state, r.resources, r)
 }
 
 // MachineAgent returns an object that provides access to the machine