deployer: Replace state calls with API calls

deployer: Replace state calls with API calls

This is the last step needed to enable the API
connection for the deployer worker. Several other
things needed to be moved around and reused (in
state, api/deployer).

Contains a workaround to get the correct API and
State addresses, which needs to be fixed properly
once we store machine addresses in state and can
get the public addresses of all machines with
JobManageState.

https://code.launchpad.net/~dimitern/juju-core/080-deployer-uses-api/+merge/176937

(do not edit description out of merge proposal)

[dimitern, 2013-07-25 13:32:07]

Please take a look.

[dimitern, 2013-07-25 13:35:00]

Please take a look.

[fwereade, 2013-07-25 14:59:17]

What did we miss that caused us to need CanDeploy?

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/agent.go#newcode242
cmd/jujud/agent.go:242: return deployer.NewSimpleContext(dataDir, caCert, st),
nil
Now we have access to BackingState in the, I think you might be able shear off a
whole chunk of needless complexity around here, and just patch out one or two
judiciously chosen vars. Not 100% sure, but bear it in mind please.

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:28: const numberSnippet = "(0|[1-9][0-9]*)"
Can we put these in some standard place instead of duplicating them please? I'm
sure there are some like this in state...

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:152: // take the addresses of all machines
with JobManageState.
Please create a bug and put the number in here.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go
File worker/deployer/deployer.go (left):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go...
worker/deployer/deployer.go:106: }
Can we not drop this whole block, apart from getting `life`? I think that the
Unauthorized errors cover everything here, don't they?

[rog, 2013-07-25 15:07:40]

Great direction, thanks, but that error handling needs sorting out, and a few
other thoughts.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go#ne...
cmd/jujud/deploy_test.go:92: c.Check(err, IsNil)
i think you want to return if the check fails here.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go#ne...
cmd/jujud/deploy_test.go:94: c.Check(err, IsNil)
ditto

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:186: // TODO(dimitern): Once all workers moved over to
using
the ManageState job will never be an API job :-)

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:240: // Deployer moved into APIWorker now.
// implemented in APIWorker

?
(the comment will otherwise become dated very shortly)

https://codereview.appspot.com/11800045/diff/6001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/11800045/diff/6001/errors/errors.go#newcode71
errors/errors.go:71: type NotAssignedError struct {

This doesn't seem like the right place for this error, which is very
state-specific and contains errors which potentially apply all across juju.

Moreover, AFAICS the reason this is renamed here is so that it can be used by
clients of the CanDeploy call but:
a) CanDeploy can't return a NotAssignedError
and
b) even if it did, it wouldn't turn out to be a NotAssignedError
on the API client side (instead it would have an CodeNotAssigned error code).

I'd leave this where it was before.

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:39: func IsUnitName(name string) bool {
do we really want to reimplement these functions in every api
package that needs them. i suggest that some functions that
might be useful across several workers and don't actually
make any API calls might be implemented inside state/api
itself.

alternatively we could have another (leaf) package for this
kind of thing (state/names ?)

https://codereview.appspot.com/11800045/diff/6001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/assign_test.go#newcod...
state/assign_test.go:142: c.Assert(err, ErrorMatches, `"unit-wordpress-0" is not
assigned to a machine`)

I'm slightly concerned that this may change the error messages presented to a
user - "wordpress/0" is what they're used to seeing; "unit-wordpress-0" is not.

Perhaps that's not an issue, but worth considering the issue, i think.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go...
worker/deployer/deployer.go:106: if responsible, err = unit.CanDeploy();
errors.IsNotAssigned(err) {
This is wrong, I think. CanDeploy cannot return a NotAssigned error.

I think you want to do:

if responsible, err = unit.CanDeploy(); err != nil {
    return err
}
if !responsible {
    logger.Warningf...
}

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
File worker/deployer/deployer_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:52: // Login as the machine agent of the
created machine.
s/Login/Log in/

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:107: s.waitFor(c, isRemoved(s.BackingState,
u0.Name()))
i think you can leave this as s.State

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:152: s.waitFor(c, isRemoved(s.BackingState,
u1.Name()))
double ditto

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go#n...
worker/deployer/simple.go:105: logger.Debugf("state addresses: %v", stateAddrs)
%q perhaps?

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go#n...
worker/deployer/simple.go:111: logger.Debugf("API addresses: %v", apiAddrs)
ditto

[rog, 2013-07-25 15:34:44]

one other remark

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:68: unitName := state.UnitNameFromTag(tag)

A discussion with William caused me to look at this.
Could I suggest something like this instead:

	getAuthFunc := func() (common.AuthFunc, error) {
		// Get all units of the machine and cache them.
		thisMachineTag := authorizer.GetAuthTag()
		if units, err := getAllUnits(st, thisMachineTag); err != nil {
			return nil, err
		}
		// Then we just check if the unit is already known.
		return func(tag string) bool {
			for _, unit := range units {
				if state.UnitTag(unit) == tag {
					return true
				}
			}
			return false
		}, nil
	}

?

In particular, the tag that we're authorizing is not necessarily a unit tag so
calling UnitNameFromTag on it seems wrong. The other change is just, in my view,
removing premature optimisation. Alternatively rather than adding each unit
explicitly to the set, call set.NewStrings(knownUnits...) instead.

[dimitern, 2013-07-26 11:33:22]

Please take a look.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/agent.go#newcode242
cmd/jujud/agent.go:242: return deployer.NewSimpleContext(dataDir, caCert, st),
nil
On 2013/07/25 14:59:17, fwereade wrote:
> Now we have access to BackingState in the, I think you might be able shear off
a
> whole chunk of needless complexity around here, and just patch out one or two
> judiciously chosen vars. Not 100% sure, but bear it in mind please.

Not sure what you mean, can you expand please?

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go#ne...
cmd/jujud/deploy_test.go:92: c.Check(err, IsNil)
On 2013/07/25 15:07:40, rog wrote:
> i think you want to return if the check fails here.

Done.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/deploy_test.go#ne...
cmd/jujud/deploy_test.go:94: c.Check(err, IsNil)
On 2013/07/25 15:07:40, rog wrote:
> ditto

Done.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:186: // TODO(dimitern): Once all workers moved over to
using
On 2013/07/25 15:07:40, rog wrote:
> the ManageState job will never be an API job :-)

Updated the comment.

https://codereview.appspot.com/11800045/diff/6001/cmd/jujud/machine.go#newcod...
cmd/jujud/machine.go:240: // Deployer moved into APIWorker now.
On 2013/07/25 15:07:40, rog wrote:
> // implemented in APIWorker
> 
> ?
> (the comment will otherwise become dated very shortly)

Done.

https://codereview.appspot.com/11800045/diff/6001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/11800045/diff/6001/errors/errors.go#newcode71
errors/errors.go:71: type NotAssignedError struct {
On 2013/07/25 15:07:40, rog wrote:
> 
> This doesn't seem like the right place for this error, which is very
> state-specific and contains errors which potentially apply all across juju.
> 
> Moreover, AFAICS the reason this is renamed here is so that it can be used by
> clients of the CanDeploy call but:
> a) CanDeploy can't return a NotAssignedError
> and
> b) even if it did, it wouldn't turn out to be a NotAssignedError
> on the API client side (instead it would have an CodeNotAssigned error code).
> 
> I'd leave this where it was before.

Done.

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:28: const numberSnippet = "(0|[1-9][0-9]*)"
On 2013/07/25 14:59:17, fwereade wrote:
> Can we put these in some standard place instead of duplicating them please?
I'm
> sure there are some like this in state...

See the comment below.

https://codereview.appspot.com/11800045/diff/6001/state/api/deployer/unit.go#...
state/api/deployer/unit.go:39: func IsUnitName(name string) bool {
On 2013/07/25 15:07:40, rog wrote:
> do we really want to reimplement these functions in every api
> package that needs them. i suggest that some functions that
> might be useful across several workers and don't actually
> make any API calls might be implemented inside state/api
> itself.
> 
> alternatively we could have another (leaf) package for this
> kind of thing (state/names ?)

I'd like to do that in a follow up: moving all of these things common to state
and api in state/names/ and reuse them, including constants, regexpes, simple
conversion calls etc.

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:68: unitName := state.UnitNameFromTag(tag)
On 2013/07/25 15:34:45, rog wrote:
> 
> A discussion with William caused me to look at this.
> Could I suggest something like this instead:
> 
> 	getAuthFunc := func() (common.AuthFunc, error) {
> 		// Get all units of the machine and cache them.
> 		thisMachineTag := authorizer.GetAuthTag()
> 		if units, err := getAllUnits(st, thisMachineTag); err != nil {
> 			return nil, err
> 		}
> 		// Then we just check if the unit is already known.
> 		return func(tag string) bool {
> 			for _, unit := range units {
> 				if state.UnitTag(unit) == tag {
> 					return true
> 				}
> 			}
> 			return false
> 		}, nil
> 	}
> 
> ?
> 
> In particular, the tag that we're authorizing is not necessarily a unit tag so
> calling UnitNameFromTag on it seems wrong. The other change is just, in my
view,
> removing premature optimisation. Alternatively rather than adding each unit
> explicitly to the set, call set.NewStrings(knownUnits...) instead.

Done.

https://codereview.appspot.com/11800045/diff/6001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer.go:152: // take the addresses of all machines
with JobManageState.
On 2013/07/25 14:59:17, fwereade wrote:
> Please create a bug and put the number in here.

The code is not landed yet, so I'll create a bug and add the number in a follow
up, while cleaning up the duplicated common state and api calls like UnitTag,
regexpes, etc.

https://codereview.appspot.com/11800045/diff/6001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/state/assign_test.go#newcod...
state/assign_test.go:142: c.Assert(err, ErrorMatches, `"unit-wordpress-0" is not
assigned to a machine`)
On 2013/07/25 15:07:40, rog wrote:
> 
> I'm slightly concerned that this may change the error messages presented to a
> user - "wordpress/0" is what they're used to seeing; "unit-wordpress-0" is
not.
> 
> Perhaps that's not an issue, but worth considering the issue, i think.

This is no longer an issue, because I reverted the NotAssignedError back to
state.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go
File worker/deployer/deployer.go (left):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go...
worker/deployer/deployer.go:106: }
On 2013/07/25 14:59:17, fwereade wrote:
> Can we not drop this whole block, apart from getting `life`? I think that the
> Unauthorized errors cover everything here, don't they?

Done.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer.go...
worker/deployer/deployer.go:106: if responsible, err = unit.CanDeploy();
errors.IsNotAssigned(err) {
On 2013/07/25 15:07:40, rog wrote:
> This is wrong, I think. CanDeploy cannot return a NotAssigned error.
> 
> I think you want to do:
> 
> if responsible, err = unit.CanDeploy(); err != nil {
>     return err
> }
> if !responsible {
>     logger.Warningf...
> }

CanDeploy removed.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
File worker/deployer/deployer_test.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:52: // Login as the machine agent of the
created machine.
On 2013/07/25 15:07:40, rog wrote:
> s/Login/Log in/

Done.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:107: s.waitFor(c, isRemoved(s.BackingState,
u0.Name()))
On 2013/07/25 15:07:40, rog wrote:
> i think you can leave this as s.State

No, if I do the test still succeeds, but it takes 10s (LongWait) more.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/deployer_te...
worker/deployer/deployer_test.go:152: s.waitFor(c, isRemoved(s.BackingState,
u1.Name()))
On 2013/07/25 15:07:40, rog wrote:
> double ditto

See above.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go#n...
worker/deployer/simple.go:105: logger.Debugf("state addresses: %v", stateAddrs)
On 2013/07/25 15:07:40, rog wrote:
> %q perhaps?

Done.

https://codereview.appspot.com/11800045/diff/6001/worker/deployer/simple.go#n...
worker/deployer/simple.go:111: logger.Debugf("API addresses: %v", apiAddrs)
On 2013/07/25 15:07:40, rog wrote:
> ditto

Done.

[fwereade, 2013-07-26 13:04:37]

LGTM, but my questions may indicate the need for a followup.

I'd be +1 on a state/names leaf package I think, we can always move it if it
turns out to be misguided.

Maybe juju/names, and include the charm bits? Maybe not?

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go#n...
cmd/jujud/deploy_test.go:95: stateAddrs, err := dst.StateAddresses()
This might be worth tweaking -- if we always want CACert and StateAddresses,
might be smarter to get them all at once in a single api call.

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:188: // JobManageState.
How about being explicit here?

case params.JobManageEnviron:
case params.JobManageState:
default:
    // error

https://codereview.appspot.com/11800045/diff/22001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/22001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:147: // BUG,FIXME: This is temporary, until
the Addresser worker lands and we can
bug# x3

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.g...
worker/deployer/deployer.go:85: return errors.IsNotFoundError(err) ||
params.ErrCode(err) == params.CodeUnauthorized
This is weird. Do we translate NotFound but not Unauthorized? If so, why?
They're both errors-package-level errors, right?

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.g...
worker/deployer/deployer.go:176: machine, err := d.st.Machine(d.machineTag)
Hmm. Does Machine have any associated data? Do we need an api hit to get an
object?

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/simple.go#...
worker/deployer/simple.go:211: if !apideployer.IsUnitName(unitName) {
If this is a straight move from state, I'd rather leave it where it is this CL
and just fix up the other ones in the next one.

[dimitern, 2013-07-26 15:21:10]

Please take a look.

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go#n...
cmd/jujud/deploy_test.go:95: stateAddrs, err := dst.StateAddresses()
On 2013/07/26 13:04:37, fwereade wrote:
> This might be worth tweaking -- if we always want CACert and StateAddresses,
> might be smarter to get them all at once in a single api call.

It was originally like that, but Roger suggested it's unnecessary to incur
round-trips if we need them separately.

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:188: // JobManageState.
On 2013/07/26 13:04:37, fwereade wrote:
> How about being explicit here?
> 
> case params.JobManageEnviron:
> case params.JobManageState:
> default:
>     // error

Done.

https://codereview.appspot.com/11800045/diff/22001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/22001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:147: // BUG,FIXME: This is temporary, until
the Addresser worker lands and we can
On 2013/07/26 13:04:37, fwereade wrote:
> bug# x3

Done.

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.g...
worker/deployer/deployer.go:85: return errors.IsNotFoundError(err) ||
params.ErrCode(err) == params.CodeUnauthorized
On 2013/07/26 13:04:37, fwereade wrote:
> This is weird. Do we translate NotFound but not Unauthorized? If so, why?
> They're both errors-package-level errors, right?

It seems the UnauthorizedError in errors is a different one, because changing
the code to use IsUnauthorized error causes the tests to fail.

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/deployer.g...
worker/deployer/deployer.go:176: machine, err := d.st.Machine(d.machineTag)
On 2013/07/26 13:04:37, fwereade wrote:
> Hmm. Does Machine have any associated data? Do we need an api hit to get an
> object?

It's not an API hit, it just returns a proxy object that implements WatchUnits.

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/11800045/diff/22001/worker/deployer/simple.go#...
worker/deployer/simple.go:211: if !apideployer.IsUnitName(unitName) {
On 2013/07/26 13:04:37, fwereade wrote:
> If this is a straight move from state, I'd rather leave it where it is this CL
> and just fix up the other ones in the next one.

Done.

[rog, 2013-07-26 15:25:19]

LGTM thanks!

[fwereade, 2013-07-26 15:31:15]

LGTM. Am I missing something wrt the separate-calls argument?

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/11800045/diff/22001/cmd/jujud/deploy_test.go#n...
cmd/jujud/deploy_test.go:95: stateAddrs, err := dst.StateAddresses()
On 2013/07/26 15:21:11, dimitern wrote:
> On 2013/07/26 13:04:37, fwereade wrote:
> > This might be worth tweaking -- if we always want CACert and StateAddresses,
> > might be smarter to get them all at once in a single api call.
> 
> It was originally like that, but Roger suggested it's unnecessary to incur
> round-trips if we need them separately.

Isn't that leaking api implementation details into the interface? The api surely
needs to be designed for the client -- which *does* need all that information at
the same time -- not for the convenience of the backend. API implementation
problems are easily addressed when compared to API interface problems...

https://codereview.appspot.com/11800045/diff/31001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/11800045/diff/31001/worker/deployer/simple.go#...
worker/deployer/simple.go:85: tag := apideployer.UnitTag(unitName)
May as well keep state.UnitTag in place? Assuming it'll be refactored as would
apideployer.UnitTag in the followup anyway?

https://codereview.appspot.com/11800045/diff/31001/worker/deployer/simple.go#...
worker/deployer/simple.go:105: logger.Debugf("state addresses: %q", stateAddrs)
got state addresses?