Wire up prechecker

Wire up prechecker

We introduce the concept of a policy
into state: state.State is given a
state.Policy interface when opened,
which is consulted during certain
operations to validate or modify
behaviour.

The Policy interface contains only
one method, Prechecker, which is called
to obtain a Prechecker. The Prechecker,
if non-nil, is used to ensure that an
instance with the specified parameters
can potentially be instantiated.

There is currently only one implementation
of Prechecker, and that is the manual
provider's Environ. The manual provider
thus prevents add-machine without ssh-
placement.

This CL supersedes https://codereview.appspot.com/14032043/
The primary differences are:
 - up-to-date with latest codebase
 - introduction of Policy, and removal of
   raciness/lack of environ config updates
 - policy is not set on State in juju.NewConn
   methods; prechecking simply will not occur for
   old environments
 - Environ does not embed Prechecker, and there
   is no new EnvironBase struct; this may be
   proposed as a follow-up
 - there is no container prechecking, as otherwise
   we have no lightweight means of deploying charms
   that do not require addressability

https://code.launchpad.net/~axwalk/juju-core/wire-up-prechecker-take2/+merge/205700

(do not edit description out of merge proposal)

[axw, 2014-02-11 06:26:20]

Please take a look.

[fwereade, 2014-02-14 09:28:53]

On 2014/02/11 06:26:20, axw wrote:
> Please take a look.

Couple of preliminary comments: I'm not actually sure it's correct to disable
containers anywhere, because they still have value as proxy charms: kapil in
particular doesn't want to waste an instance on an ELB charm, and hulk-smash is
a pretty weak alternative.

There's definitely value in prechecking environ instances though.

[axw, 2014-02-14 09:39:44]

On 2014/02/14 09:28:53, fwereade wrote:
> On 2014/02/11 06:26:20, axw wrote:
> > Please take a look.
> 
> Couple of preliminary comments: I'm not actually sure it's correct to disable
> containers anywhere, because they still have value as proxy charms: kapil in
> particular doesn't want to waste an instance on an ELB charm, and hulk-smash
is
> a pretty weak alternative.

Agreed that hulk smash is a crappy alternative, and I know the use case is
there; I was hoping to address it when the time came (this is all server side).

It doesn't seem ideal to allow containers for everyone, letting users shoot
themselves, to enable ELB, proxies, whatever. It'd be nice to have something
inside the charm that indicates what it requires (or doesn't require?) so that
we know to let those special cases through. OTOH, are users likely to
"add-machine lxc" if they don't know what they're doing?

In any case, there's work to be done. I can change it to allow containers to
begin with.

> There's definitely value in prechecking environ instances though.

[fwereade, 2014-02-17 09:47:03]

On 2014/02/14 09:39:44, axw wrote:
> It doesn't seem ideal to allow containers for everyone, letting users shoot
> themselves, to enable ELB, proxies, whatever. It'd be nice to have something
> inside the charm that indicates what it requires (or doesn't require?) so that
> we know to let those special cases through. OTOH, are users likely to
> "add-machine lxc" if they don't know what they're doing?

Yeah, I think a charm flag (i-dont-need-an-address? ;p) is probably the best
approach. Once we have one of those we can do it all internally, and expose a
cmdline flag for add-machine, for those users who really do know what they're
doing.

> In any case, there's work to be done. I can change it to allow containers to
> begin with.

Ideal, thanks.

> > There's definitely value in prechecking environ instances though.

[fwereade, 2014-02-17 10:00:17]

possible approach question, please discuss with waigani:

the issue with setting a prechecker, even if it's watched, is raciness. I was
willing to handwave it a bit before, but now we have waigani's use case which
requires that we get *less* racy wrt environ config... how about setting not a
prechecker, but something like a getter: eg `func(map[string]interface{})
(Prechecker, error)`? This is irrelevant for your purposes really [0] but
critically important for waigani's -- and the only difference is in the type
that needs to be returned (whether it has SetConfig or Precheck). That'd need a
bit of finesse but it'd let you use basically the same implementation for
basically the same task -- *and* mean that you don't need to do the watching
followup (at the cost of an extra db hits when prechecking -- worth it, I
think).

*and* we wouldn't need to mess around with locks in state :).

[0] you *could* use it to assert that the environ hasn't changed since you
checked validity of the upcoming op, but the value of that is somewhat limited I
think

[axw, 2014-02-17 12:26:29]

On 2014/02/17 10:00:17, fwereade wrote:
> possible approach question, please discuss with waigani:
> 
> the issue with setting a prechecker, even if it's watched, is raciness. I was
> willing to handwave it a bit before, but now we have waigani's use case which
> requires that we get *less* racy wrt environ config... how about setting not a
> prechecker, but something like a getter: eg `func(map[string]interface{})
> (Prechecker, error)`? This is irrelevant for your purposes really [0] but
> critically important for waigani's -- and the only difference is in the type
> that needs to be returned (whether it has SetConfig or Precheck). That'd need
a
> bit of finesse but it'd let you use basically the same implementation for
> basically the same task -- *and* mean that you don't need to do the watching
> followup (at the cost of an extra db hits when prechecking -- worth it, I
> think).

I might just create one getter type that can do Precheck or Validate, but
otherwise I like this alternative. I'll look into implementing this, and discuss
with waigani in our standup tomorrow.

> *and* we wouldn't need to mess around with locks in state :).
> 
> [0] you *could* use it to assert that the environ hasn't changed since you
> checked validity of the upcoming op, but the value of that is somewhat limited
I
> think

Yeah, I don't think we need to go that far.

[axw, 2014-02-18 05:56:11]

Please take a look.

[fwereade, 2014-02-18 15:30:18]

Looking very nice, basically just quibbles. And I'm not quite clear about the
distinction between a nil policy and a PolicyBase{} -- what exactly governs
which you use in a given situation?

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:121: }, state.DefaultDialOpts(), state.Policy(nil))
I'm wondering whether it might be good to have a package that implements
environStatePolicy, and includes a state.Open wrapper that always passes the
environ policy in. I'm not sure I can see a use case for alternative policies at
the moment, and if one does show up I think it'll be pretty easy to add.

As it is I feel it's a bit icky having the nil policies everywhere -- I think we
probably want all our states to work the same in-test and out. Open to
counterarguments though.

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/statepolicy.go
File cmd/jujud/statepolicy.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/statepolicy.go#n...
cmd/jujud/statepolicy.go:25: return p, nil
I don't really like the nil-return-without-error. Can we have a specific sort of
NotImplemented error that we handle explicitly in precheckInstance, please?

https://codereview.appspot.com/61520045/diff/20001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/61520045/diff/20001/juju/conn.go#newcode57
juju/conn.go:57: policy := state.PolicyBase{}
This bit doesn't have to use the mooted wrapper, ofc.

*but* I don't quite understand what the benefit is of having a null policy here.
What breaks down if we use a real one?

https://codereview.appspot.com/61520045/diff/20001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/61520045/diff/20001/state/policy.go#newcode17
state/policy.go:17: // for any of the interfaces, but should only return
I think you a word.

https://codereview.appspot.com/61520045/diff/20001/state/policy.go#newcode43
state/policy.go:43: return nil, nil
yeah, `nil, nil` scares me. Please change it :).

[axw, 2014-02-19 07:08:45]

Please take a look.

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:121: }, state.DefaultDialOpts(), state.Policy(nil))
On 2014/02/18 15:30:18, fwereade wrote:
> I'm wondering whether it might be good to have a package that implements
> environStatePolicy, and includes a state.Open wrapper that always passes the
> environ policy in. I'm not sure I can see a use case for alternative policies
at
> the moment, and if one does show up I think it'll be pretty easy to add.

In any case, juju-core/agent can't use it directly. The policy implementation
requires environs, which depends on agent (which seems a bit unfortunate.)

I'll just move the policy implementation into environs. The wrappers aren't
really all that useful since they can't be used by both production and testing
code.

> As it is I feel it's a bit icky having the nil policies everywhere -- I think
we
> probably want all our states to work the same in-test and out. Open to
> counterarguments though.

No problems. I'd prefer to keep the ones in juju-core/state self-contained
though.

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/statepolicy.go
File cmd/jujud/statepolicy.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/statepolicy.go#n...
cmd/jujud/statepolicy.go:25: return p, nil
On 2014/02/18 15:30:18, fwereade wrote:
> I don't really like the nil-return-without-error. Can we have a specific sort
of
> NotImplemented error that we handle explicitly in precheckInstance, please?

Done.

https://codereview.appspot.com/61520045/diff/20001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/61520045/diff/20001/juju/conn.go#newcode57
juju/conn.go:57: policy := state.PolicyBase{}
On 2014/02/18 15:30:18, fwereade wrote:
> This bit doesn't have to use the mooted wrapper, ofc.
> 
> *but* I don't quite understand what the benefit is of having a null policy
here.
> What breaks down if we use a real one?

Nothing actually, this is basically a holdover from when Environ had to be
updated.

I'll move the policy implementation to its own package as suggested and update
this.

https://codereview.appspot.com/61520045/diff/20001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/61520045/diff/20001/state/policy.go#newcode17
state/policy.go:17: // for any of the interfaces, but should only return
On 2014/02/18 15:30:18, fwereade wrote:
> I think you a word.

Done.

https://codereview.appspot.com/61520045/diff/20001/state/policy.go#newcode43
state/policy.go:43: return nil, nil
On 2014/02/18 15:30:18, fwereade wrote:
> yeah, `nil, nil` scares me. Please change it :).

Done. Must return an error that satisfies errors.IsNotImplementedError.

[fwereade, 2014-02-19 13:01:39]

LGTM, thanks

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:121: }, state.DefaultDialOpts(), state.Policy(nil))
On 2014/02/19 07:08:45, axw wrote:
> In any case, juju-core/agent can't use it directly. The policy implementation
> requires environs, which depends on agent (which seems a bit unfortunate.)

Grar. Navigate the twisty passages as you must, then. Cheers.

> No problems. I'd prefer to keep the ones in juju-core/state self-contained
> though.

SGTM

https://codereview.appspot.com/61520045/diff/40001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/61520045/diff/40001/state/policy.go#newcode58
state/policy.go:58: logger.Debugf("policy returned nil prechecker, ignoring")
I think this is really bad behaviour on the policy's part; probably bad enough
to justify erroring out of the AddMachine, because the implementation is crazy
enough that all bets should be considered to be off.

[axw, 2014-02-20 08:14:22]

Please take a look.

https://codereview.appspot.com/61520045/diff/40001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/61520045/diff/40001/state/policy.go#newcode58
state/policy.go:58: logger.Debugf("policy returned nil prechecker, ignoring")
On 2014/02/19 13:01:39, fwereade wrote:
> I think this is really bad behaviour on the policy's part; probably bad enough
> to justify erroring out of the AddMachine, because the implementation is crazy
> enough that all bets should be considered to be off.

Done.