Wire up prechecker

Wire up prechecker

This change is to set a Prechecker in
the CLI, and in cmd/jujud. The Environ
itself is a Prechecker, and that is
what is assigned to the state object.

State now calls the prechecker when
decoding to create instance/container
machine entries in state. The prechecker
calls are elided for "injected" machines.

Environ embeds the Prechecker interface.
All Environ implementations now embed
environs.EnvironBase, which implements
defaults for an environs.Environ. Currently
it only implements Prechecker, by
embedding a NilPrechecker (i.e. one that
allows everything through).

All Environs, apart from MAAS, disallow
containers. The null provider disallows
everything (unless done "manually").

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

(do not edit description out of merge proposal)

[fwereade, 2013-09-27 10:59:22]

Thoughts:

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

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker, here to
avoid the dependency.
Better I think to define it in state, and document (in code) that appropriate
Environs implement it. environs already knows about state, not vice versa, so we
may as well just define the interface in the place that uses it. After all, it
really *doesn't* have to be just an environ that satsisfies that interface: we'd
surely want the state tests to be written with a suitable test implementation,
not of a whole environ.

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode302
state/state.go:302: }
/me strokes chin

Leaving aside the fact that we'll need lots of locking, and that makes me a bit
suspicious, this is kinda wrong; I'm worried that we would not in general be in
a position to track changes to the environment, and are therefore in some danger
of screwing up by using out-of-date environments.

But my spidey-sense is tingling wrt the provisioner. That too needs an
up-to-date environment, and we've tied ourselves in frightful knots over it;
and, really, I don't think we need to at all. An Environ is meant to be
goroutine-safe, after all, so it might be reasonable to get a single Environ per
(authorized) agent and run a task that keeps it updated via the API by
validating changes and calling SetConfig; we can then use that single task to
keep all the environs in sync.

(I'm thinking of a Task that also implements several one-method interfaces --
WaitPrechecker, WaitInstanceBroker, WaitPortOpenCloseThing -- that all share an
implementation (return the single Environ held by the task once it's available);
and which just watches the API and does the whole SetConfig lark.)

Can anyone think of a reason not to do this?

[axw1, 2013-09-29 16:20:44]

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

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker, here to
avoid the dependency.
On 2013/09/27 10:59:22, fwereade wrote:
> Better I think to define it in state, and document (in code) that appropriate
> Environs implement it. environs already knows about state, not vice versa, so
we
> may as well just define the interface in the place that uses it. After all, it
> really *doesn't* have to be just an environ that satsisfies that interface:
we'd
> surely want the state tests to be written with a suitable test implementation,
> not of a whole environ.

Fair enough, I'll do that. Do you think it'd be worthwhile adding something like
below to environs/interface.go?

// Prechecker is an optional Environ interface. See state.Prechecker for
details.
type Prechecker interface {
    state.Prechecker
}

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode302
state/state.go:302: }
On 2013/09/27 10:59:22, fwereade wrote:
> /me strokes chin
> 
> Leaving aside the fact that we'll need lots of locking, and that makes me a
bit
> suspicious, this is kinda wrong; I'm worried that we would not in general be
in
> a position to track changes to the environment, and are therefore in some
danger
> of screwing up by using out-of-date environments.
> 
> But my spidey-sense is tingling wrt the provisioner. That too needs an
> up-to-date environment, and we've tied ourselves in frightful knots over it;
> and, really, I don't think we need to at all. An Environ is meant to be
> goroutine-safe, after all, so it might be reasonable to get a single Environ
per
> (authorized) agent and run a task that keeps it updated via the API by
> validating changes and calling SetConfig; we can then use that single task to
> keep all the environs in sync.
> 
> (I'm thinking of a Task that also implements several one-method interfaces --
> WaitPrechecker, WaitInstanceBroker, WaitPortOpenCloseThing -- that all share
an
> implementation (return the single Environ held by the task once it's
available);
> and which just watches the API and does the whole SetConfig lark.)
> 
> Can anyone think of a reason not to do this?

That sounds sensible to me, though I don't know the finer points of the machine
agent.

I lbox proposed with -wip, so this wouldn't have circulated; "anyone" is just me
at the moment.

[axw, 2013-10-01 08:33:01]

Please take a look.

[fwereade, 2013-10-01 10:24:32]

I feel like we may be converging on something... let me know what you think

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

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker, here to
avoid the dependency.
On 2013/09/29 16:20:44, axw1 wrote:
> On 2013/09/27 10:59:22, fwereade wrote:
> > Better I think to define it in state, and document (in code) that
appropriate
> > Environs implement it. environs already knows about state, not vice versa,
so
> we
> > may as well just define the interface in the place that uses it. After all,
it
> > really *doesn't* have to be just an environ that satsisfies that interface:
> we'd
> > surely want the state tests to be written with a suitable test
implementation,
> > not of a whole environ.
> 
> Fair enough, I'll do that. Do you think it'd be worthwhile adding something
like
> below to environs/interface.go?
> 
> // Prechecker is an optional Environ interface. See state.Prechecker for
> details.
> type Prechecker interface {
>     state.Prechecker
> }

I think rog and nate have convinced me that these optional interfaces may do
more harm than good -- we can add state.Prechecker directly to the Environ
interface, and just supply an embeddable do-nothing prechecker that people can
implement to begin with.

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:98: conn.State.SetPrechecker(nil)
This feels like a timebomb to me. What happens when this functionality moves
into the API server? One manually provisioned machine will disable prechecking
for everything.

I think we should probably be skipping prechecks on injected machines anyway.

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go#newco...
environs/interface.go:105: type Environ interface {
yeah, it seems sane to stick state.Prechecker in here and embed a shared
nil-returning implementation in the various implementations. Reasonable?

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go#newcode89
juju/conn.go:89: if prechecker, ok := environ.(state.Prechecker); ok {
It's that old wrong-environ problem again, I'm afraid. I'd reuse that utility
function from jujud myself -- I think it's basically State.EnablePrechecking, or
something like that, with the complication that state doesn't know about
environs and so we can't implement it in-package.

Hmm:

type PrecheckerSource interface {
    GetPrechecker(st *State) (Prechecker, error)
}

how often do we directly Open state? often enough that supplying one of those
would be an inconvenience? Because if we could do that, we could drop the
locking in here, I think, and rely on safe/efficient external implementations of
GetPrechecker.

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

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode70
state/state.go:70: precheckerMutex  sync.Mutex
RWMutex might be appropriate here?

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode233
state/state.go:233: // we are creating a new machine instance (not a container).
we should be locking these bits as well, surely?

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go#newcode...
state/state_test.go:1951: params.ParentId = ""
where did params come from?

[axw1, 2013-10-02 09:18:49]

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go#newc...
cmd/juju/addmachine.go:98: conn.State.SetPrechecker(nil)
On 2013/10/01 10:24:33, fwereade wrote:
> This feels like a timebomb to me. What happens when this functionality moves
> into the API server? One manually provisioned machine will disable prechecking
> for everything.
> 
> I think we should probably be skipping prechecks on injected machines anyway.

Removed, and updated state to not call PrecheckInstance for injected machines.

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go#newco...
environs/interface.go:105: type Environ interface {
On 2013/10/01 10:24:33, fwereade wrote:
> yeah, it seems sane to stick state.Prechecker in here and embed a shared
> nil-returning implementation in the various implementations. Reasonable?

Very. I've created a new file, base.go, which contains a default implementation.

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go#newcode89
juju/conn.go:89: if prechecker, ok := environ.(state.Prechecker); ok {
On 2013/10/01 10:24:33, fwereade wrote:
> It's that old wrong-environ problem again, I'm afraid.

So, my intention here was that the user would later update the environ. But
actually, thinking about it some more, this is just wrong. NewConn shouldn't
call SetPrechecker. If the user wants to do that, they can.

The one user that perhaps should SetPrechecker is NewConnFromName. Is there any
reason why we don't have it always set the environ's config from state?

> I'd reuse that utility
> function from jujud myself -- I think it's basically State.EnablePrechecking,
or
> something like that, with the complication that state doesn't know about
> environs and so we can't implement it in-package.
> 
> Hmm:
> 
> type PrecheckerSource interface {
>     GetPrechecker(st *State) (Prechecker, error)
> }
> 
> how often do we directly Open state? often enough that supplying one of those
> would be an inconvenience? Because if we could do that, we could drop the
> locking in here, I think, and rely on safe/efficient external implementations
of
> GetPrechecker.

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

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode70
state/state.go:70: precheckerMutex  sync.Mutex
On 2013/10/01 10:24:33, fwereade wrote:
> RWMutex might be appropriate here?

Done.

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode233
state/state.go:233: // we are creating a new machine instance (not a container).
On 2013/10/01 10:24:33, fwereade wrote:
> we should be locking these bits as well, surely?

Indeed, thanks for picking that up. Done.

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go#newcode...
state/state_test.go:1951: params.ParentId = ""
On 2013/10/01 10:24:33, fwereade wrote:
> where did params come from?

Derp. I split the test above; thought I ran the tests, but clearly did not.

[axw, 2013-10-02 09:28:26]

Please take a look.

[axw, 2013-10-04 03:40:57]

Please take a look.

[axw1, 2013-10-04 03:52:38]

On 2013/10/04 03:40:57, axw wrote:
> Please take a look.

Updated juju/conn.go to call SetPrechecker again, but with a fresh Environ whose
config is loaded from state.

I'll start on a new CL now for adding a task that updates an Environ, and
encapsulates worker.WaitForEnviron.

[fwereade, 2013-10-08 07:30:42]

Sorry this one has hung around so long, but I think it's basically there. Last
round?

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
Good point in your comment that I think was around here. Let's use this to set
the environ on the Conn itself -- I can't think of a good reason not to tbh.

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go#...
provider/azure/environ.go:83: var _ state.Prechecker = (*azureEnviron)(nil)
Don't think we need this now it's in the Environ interface

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go#newcode69
provider/ec2/ec2.go:69: var _ state.Prechecker = (*environ)(nil)
ditto

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go...
provider/ec2/local_test.go:196: prechecker, ok := env.(state.Prechecker)
unnecessary again?

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go#n...
provider/null/environ.go:165: func (*nullEnviron) PrecheckContainer(series
string, kind instance.ContainerType) error {
Hmm. Unless we can give containers their own addresses, we might need to error
out here too.

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_t...
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_t...
provider/openstack/local_test.go:251: prechecker, ok := env.(state.Prechecker)
again not necessary I think

https://codereview.appspot.com/14032043/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/19001/state/state.go#newcode236
state/state.go:236: // in which case we do not want to precheck it.
Is this really the best way to get it? Seems a little bit roundabout. I'd prefer
to use InstanceId, really, it seems a bit clearer to me.

[axw, 2013-10-08 09:52:33]

Thanks for the review.

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
On 2013/10/08 07:30:42, fwereade wrote:
> Good point in your comment that I think was around here. Let's use this to set
> the environ on the Conn itself -- I can't think of a good reason not to tbh.

Done. It's slightly more complicated than getting the env from state: it needs
to have the admin-secret and ca-private-key copied across from the input
environ.

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go#...
provider/azure/environ.go:83: var _ state.Prechecker = (*azureEnviron)(nil)
On 2013/10/08 07:30:42, fwereade wrote:
> Don't think we need this now it's in the Environ interface

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go#newcode69
provider/ec2/ec2.go:69: var _ state.Prechecker = (*environ)(nil)
On 2013/10/08 07:30:42, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go...
provider/ec2/local_test.go:196: prechecker, ok := env.(state.Prechecker)
On 2013/10/08 07:30:42, fwereade wrote:
> unnecessary again?

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go#n...
provider/null/environ.go:165: func (*nullEnviron) PrecheckContainer(series
string, kind instance.ContainerType) error {
On 2013/10/08 07:30:42, fwereade wrote:
> Hmm. Unless we can give containers their own addresses, we might need to error
> out here too.

Done.

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_t...
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_t...
provider/openstack/local_test.go:251: prechecker, ok := env.(state.Prechecker)
On 2013/10/08 07:30:42, fwereade wrote:
> again not necessary I think

Done.

https://codereview.appspot.com/14032043/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/19001/state/state.go#newcode236
state/state.go:236: // in which case we do not want to precheck it.
On 2013/10/08 07:30:42, fwereade wrote:
> Is this really the best way to get it? Seems a little bit roundabout. I'd
prefer
> to use InstanceId, really, it seems a bit clearer to me.

I think it is. InstanceId is deprecated on machineDoc (and it's inside metadata,
but it's nil...)

[axw, 2013-10-08 09:56:34]

Please take a look.

[fwereade, 2013-10-09 08:46:30]

Apart from the extra attrs, this seems fine -- but they could use a spot more
investigation.

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
On 2013/10/08 09:52:34, axw wrote:
> On 2013/10/08 07:30:42, fwereade wrote:
> > Good point in your comment that I think was around here. Let's use this to
set
> > the environ on the Conn itself -- I can't think of a good reason not to tbh.
> 
> Done. It's slightly more complicated than getting the env from state: it needs
> to have the admin-secret and ca-private-key copied across from the input
> environ.

/me has a suspicious. Shouldn't we do it after updating secrets?

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

https://codereview.appspot.com/14032043/diff/30001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:245: // TODO(axw) 2013-10-01 bug #pending
needs a bug# now :)

https://codereview.appspot.com/14032043/diff/30001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/30001/juju/conn.go#newcode106
juju/conn.go:106: "ca-private-key": string(caPrivateKey),
Yeah, I don't *think* we should need these.

[axw, 2013-10-09 09:54:16]

Please take a look.

[fwereade, 2013-11-07 09:44:52]

Sorry, this lingered so long that I've lost context, so the comments below may
be a bit dumb. On the upside, I've heard surprisingly few screams about its
absence, so I feel we're a bit more free to think about the approach a bit more.

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

https://codereview.appspot.com/14032043/diff/37001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:249: environ, err := getStateEnviron(st)
Remind me -- how does this work when the environ config is incomplete? Don't we
still need a state worker then?

https://codereview.appspot.com/14032043/diff/37001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/37001/state/state.go#newcode257
state/state.go:257: }
OK, Kapil's concerns are getting to me a little bit. Gut feeling: is it
sane/easy/reasonable to allow container=lxc (which we *really* should name
isolation=lxc) constraints to create unroutable containers, but to restrict
add-machine so it only allows addressable ones?

(if we implemented lxc:X handling with a pseudo-provider maybe *that* could be
the bit responsible for container checking?)

[axw, 2013-11-08 07:25:28]

May want to hold off until synchronous bootstrap now, given the oversight on
getting an environ.

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

https://codereview.appspot.com/14032043/diff/37001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:249: environ, err := getStateEnviron(st)
On 2013/11/07 09:44:53, fwereade wrote:
> Remind me -- how does this work when the environ config is incomplete? Don't
we
> still need a state worker then?

Well, oops, I don't think it does work.

I must have coded this against a version of the manual provider that couldn't
have an incomplete config (there was a time when it had defaults for
everything).

This would change after synchronous bootstrap, since the agent will never have
an incomplete config. Doesn't work for now, though.

https://codereview.appspot.com/14032043/diff/37001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/37001/state/state.go#newcode257
state/state.go:257: }
On 2013/11/07 09:44:53, fwereade wrote:
> OK, Kapil's concerns are getting to me a little bit. Gut feeling: is it
> sane/easy/reasonable to allow container=lxc (which we *really* should name
> isolation=lxc) constraints to create unroutable containers, but to restrict
> add-machine so it only allows addressable ones?

It's probably doable, but that sounds a bit hackish to me. I'd rather just be
explicit about requiring (at worst) an unroutable container.
 
> (if we implemented lxc:X handling with a pseudo-provider maybe *that* could be
> the bit responsible for container checking?)