Unit agents know their assigned machine

Unit agents know their assigned machine

The unit agent conf file has a new value called
MACHINE_TAG added when the unit is deployed. The
value is the tag of the machine on which the 
unit is deployed.

To allow new tools to run on older deployments 
without the updated agent conf file, a new Uniter
API is provided which allows the unit agent to
look up the assigned machine over the API.
The API will be removed when it is possible
to have all current unit agent config files
upgraded.

This functionality is used by a downstream
branch which reworks the uniter upgrade worker.

https://code.launchpad.net/~wallyworld/juju-core/unit-agent-knows-assigned-machine/+merge/205291

Requires: https://code.launchpad.net/~wallyworld/juju-core/machiner-access-for-uniter/+merge/205290

(do not edit description out of merge proposal)

[wallyworld, 2014-02-07 07:24:13]

Please take a look.

[dimitern, 2014-02-10 08:55:04]

Very good direction, but I have some concerns about backward compatibility with
<1.18 API servers.

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

https://codereview.appspot.com/60930043/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:456: err := u.st.caller.Call(uniter, "",
"GetAssignedMachine", args, &results)
We need to be careful here for CodeNotImplemented errors, if this is run against
an older API server. What's the appropriate action to take when we can't read it
from the API server?

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

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:1004: if canAccess(entity.Tag) {
To reduce the code nesting, you could define a:

func (u *UniterAPI) getOneAssignedMachine(tag string) (string, error) {
    unit, err := u.getUnit(tag)
    if err != nil {
        return "", err
    }
    machineId, err := unit.AssignedMachineId()
    if err != nil {
        return "", err
    }
    return machineId, nil
}

And then reduce the following block to:

machineId, err := u.getOneAssignedMachine(entity.Tag)
if err == nil {
    reslt.Results[i].Result = names.MachineTag(machineId)
}

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter_...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:1441: {Tag: "unit-mysql-0"},
I'd like to see a subsequent test in this method that first unassigns
wordpress/0 and then calls GetAssignedMachine again with args =
params.Entities{Entities: []params.Entity{{Tag: "unit-wordpress-0"}}}, so that
the returned error is checked to match apiservertesting.NotAssignedError(..).

[wallyworld, 2014-02-11 02:39:12]

Please take a look.

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

https://codereview.appspot.com/60930043/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:456: err := u.st.caller.Call(uniter, "",
"GetAssignedMachine", args, &results)
On 2014/02/10 08:55:04, dimitern wrote:
> We need to be careful here for CodeNotImplemented errors, if this is run
against
> an older API server. What's the appropriate action to take when we can't read
it
> from the API server?

When we upgrade, is the API server upgraded first? If so then, everything is ok.
If not, then I don't think there's much choice other than to let the error
propagate and cause the caller (the unit agent) to be restarted so it can try
again. This call needs to succeed before the new, upgraded unit agent can come
up. So I think letting it keep retrying is ok here?

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

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:1004: if canAccess(entity.Tag) {
On 2014/02/10 08:55:04, dimitern wrote:
> To reduce the code nesting, you could define a:
> 
> func (u *UniterAPI) getOneAssignedMachine(tag string) (string, error) {
>     unit, err := u.getUnit(tag)
>     if err != nil {
>         return "", err
>     }
>     machineId, err := unit.AssignedMachineId()
>     if err != nil {
>         return "", err
>     }
>     return machineId, nil
> }
> 
> And then reduce the following block to:
> 
> machineId, err := u.getOneAssignedMachine(entity.Tag)
> if err == nil {
>     reslt.Results[i].Result = names.MachineTag(machineId)
> }

Done.

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter_...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/60930043/diff/1/state/apiserver/uniter/uniter_...
state/apiserver/uniter/uniter_test.go:1441: {Tag: "unit-mysql-0"},
On 2014/02/10 08:55:04, dimitern wrote:
> I'd like to see a subsequent test in this method that first unassigns
> wordpress/0 and then calls GetAssignedMachine again with args =
> params.Entities{Entities: []params.Entity{{Tag: "unit-wordpress-0"}}}, so that
> the returned error is checked to match apiservertesting.NotAssignedError(..).

Done.

[dimitern, 2014-02-11 09:52:19]

LGTM

https://codereview.appspot.com/60930043/diff/20001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/60930043/diff/20001/agent/agent.go#newcode34
agent/agent.go:34: BootstrapJobs    = "BOOTSTRAP_JOBS"
Where is this coming from? Update the CL description?

[wallyworld, 2014-02-11 10:28:03]

https://codereview.appspot.com/60930043/diff/20001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/60930043/diff/20001/agent/agent.go#newcode34
agent/agent.go:34: BootstrapJobs    = "BOOTSTRAP_JOBS"
On 2014/02/11 09:52:19, dimitern wrote:
> Where is this coming from? Update the CL description?

I merged trunk and shiteveld can't handle such merges properly so it gets
confused. Launchpad has the correct diff.

[fwereade, 2014-02-14 09:09:32]

On 2014/02/11 10:28:03, wallyworld wrote:
> https://codereview.appspot.com/60930043/diff/20001/agent/agent.go
> File agent/agent.go (right):
> 
> https://codereview.appspot.com/60930043/diff/20001/agent/agent.go#newcode34
> agent/agent.go:34: BootstrapJobs    = "BOOTSTRAP_JOBS"
> On 2014/02/11 09:52:19, dimitern wrote:
> > Where is this coming from? Update the CL description?
> 
> I merged trunk and shiteveld can't handle such merges properly so it gets
> confused. Launchpad has the correct diff.

as prereq, not lgtm