Merge lp:~axwalk/juju-core/sanity-check-constraints into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

all: introduce environs.Prechecker

This introduces a new interface, environs.Prechecker,
which is an optional interface that an environs.Environ
may implement.

Prechecker has two methods: PrecheckInstance and
PrecheckContainer. These methods will check if
constraints can possibly be met by the environment.
They will be used to check if an environment is capable
of creating an instance with the specified series and
constraints, or container with the specified type.

If the methods return nil, that does not necessarily
mean that the constraints are satisfied; if a non-nil
error is returned, then it means that the constraints
are definitely not satisfiable.

The local, ec2, azure and openstack Environ implementatons
will now disallow creating containers. When the work to
support addressability is done, this needs to be changed.
Currently, maas lets everything through.

The interface is not currently used anywhere. This will
be done in a followup.

https://codereview.appspot.com/13632046/

all: introduce environs.Prechecker

This introduces a new interface, environs.Prechecker,
which is an optional interface that an environs.Environ
may implement.

Prechecker has two methods: PrecheckInstance and
PrecheckContainer. These methods will check if
constraints can possibly be met by the environment.
They will be used to check if an environment is capable
of creating an instance with the specified series and
constraints, or container with the specified type.

If the methods return nil, that does not necessarily
mean that the constraints are satisfied; if a non-nil
error is returned, then it means that the constraints
are definitely not satisfiable.

The local, ec2, azure and openstack Environ implementatons
will now disallow creating containers. When the work to
support addressability is done, this needs to be changed.
Currently, maas lets everything through.

The interface is not currently used anywhere. This will
be done in a followup.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Reviewers: mp+185015_code.launchpad.net,

Message:
Please take a look.

Description:
all: implements Environ.SanityCheckConstraints

This introduces a new method on environs.Environ,
SanityCheckConstraints. This method will check
if constraints can possibly be met by the environment.
This will be used to check if an environment is
capable of creating an instance with the specified
constraints.

If the method returns nil, that does not necessarily
mean that the constraints are satisfied; if a non-nil
error is returned, then it means that the constraints
are definitely not satisfiable.

The ec2, azure and openstack Environ implementatons
will now disallow creating containers. When the work
to support addressability is done, this needs to be
changed. Currently, maas and local let everything
through.

cmd/juju is updated so that add-machine, and
deploy/add-unit --to call the new SanityCheckConstraints
method.

https://code.launchpad.net/~axwalk/juju-core/sanity-check-constraints/+merge/185015

(do not edit description out of merge proposal)

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

Affected files (+166, -25 lines):
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/juju/addmachine_test.go
   M cmd/juju/addunit_test.go
   M cmd/juju/deploy_test.go
   M environs/interface.go
   M juju/conn.go
   M provider/azure/environ.go
   M provider/azure/environ_test.go
   M provider/dummy/environs.go
   M provider/ec2/ec2.go
   M provider/ec2/local_test.go
   M provider/local/environ.go
   M provider/maas/environ.go
   M provider/openstack/local_test.go
   M provider/openstack/provider.go

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[John A Meinel (jameinel)]

Because this is being added to the environs interface we should have a
test added in environs/jujutest/livetests.go Or possibly just
".../tests.go" but I'd like to see the former. That gives us an
interface-level test against all things that implement Environs. (Except
Azure, because it hasn't been hooked into the live testing
infrastructure yet.)

Offhand this could just be that constraints that we will definitely
support (like all values of nil) are checked to not return an error. And
maybe testing an LXC sort of constraint and test that either it succeeds
(err == nil) or it fails in a known way.

Given the functions you've written, something like:

func (s *LiveSuite) TestSanityCheckConstraints(c *gc.C) {
     var cons constraints.Value
     c.Check(s.Env.SanityCheckConstraints(cons), gc.IsNil)
     cons.Container = new(instance.ContainerType)
     *cons.Container = instance.LXC
     err := s.Env.SanityCheckConstraints(cons)
     // If err is nil, that is fine, some providers support containers
     if err != nil {
         // But for ones that don't, they should have a standard syntax
for erroring
         c.Check(err, gc.ErrorMatches, ".*provider does not support
containers")
     }
}

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go#newcode112
cmd/juju/addmachine_test.go:112: dummy.SanityCheckConstraintsError =
errors.New("computer says no")
You don't clean up after setting this value, which will give errors in
strange ways for other tests (I think).
You need something like:

defer func() { dummy.SanityCheckConstraints = nil }

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go
File cmd/juju/addunit_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go#newcode143
cmd/juju/addunit_test.go:143: dummy.SanityCheckConstraintsError =
errors.New("computer says no")
Similarly here, you should be resetting the error when the test
finishes.

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go#newcode250
cmd/juju/deploy_test.go:250: dummy.SanityCheckConstraintsError =
stderrors.New("computer says no")
and here

Maybe turn SanityCheckConstraintsError into a function that returns a
cleanup func, rather than reproducing that in every test that wants to
override it.

So you would have:

cleanup := dummy.SetSanityCheckConstraintsError(errors.New("stuff"))
defer cleanup()

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode342
juju/conn.go:342: }
My first thought on reading this was that you don't need to validate
containerType if you got "3/lxc/2" because that machine must already
exist.
But looking closer, I'm a bit confused by the logic here. It looks like
you get really weird behavior if you do: lxc:2:3

It actually looks like you could do: lxc:2:lxc:3 to create a new lxc on
machine 2/lxc/3, thoug...

[William Reade (fwereade)]

General +1 to jam's comments.

It looks to me like deploy/add-unit and add-machine go through different
code paths that both involve manipulating placement directives. That
doesn't necessarily need to be fixed this branch, but it must be
addressed in a followup; it's hard to trust that they're still doing the
same things in the same ways.

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

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine.go#newcode106
cmd/juju/addmachine.go:106: if err =
conn.Environ.SanityCheckConstraints(checkConstraints); err != nil {
We really shouldn't use conn.Environ. We should probably unexport it,
even.

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go#newcode112
cmd/juju/addmachine_test.go:112: dummy.SanityCheckConstraintsError =
errors.New("computer says no")
On 2013/09/11 12:05:59, jameinel wrote:
> You don't clean up after setting this value, which will give errors in
strange
> ways for other tests (I think).
> You need something like:

> defer func() { dummy.SanityCheckConstraints = nil }

Oddly enough I just approved rog's jc.Set, which works in exactly this
situation.

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode329
juju/conn.go:329: machineId = strings.Join(specParts[1:], "/")
Yeah, I'm pretty sure we don't want this. :s later in the string should
not be turned into /s.

And I remain really nervous about having this logic right here,
regardless... can we break it out a little, and write it in terms of
"placement directives", in which the bit before the first : determines
the handler for the bit afterwards? And thus have an explicit lxc
handler, ready to add the others?

I do appreciate it's just a code move, but even if you don't fix it
today please be sure to document that it's a good-enough-for-now hack at
the above functionality.

https://codereview.appspot.com/13632046/diff/4001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/13632046/diff/4001/provider/local/environ.go#newcode100
provider/local/environ.go:100: return nil
On 2013/09/11 12:05:59, jameinel wrote:
> I don't think we actually support nesting LXC's in the local provider.
So we
> should probably check and return an error here, rather than allowing
it.
+1

https://codereview.appspot.com/13632046/

[William Reade (fwereade)]

Hmm, further to my previous comments... we shouldn't be calling
SanityCheckConstraints *quite* so early; it'll want to happen in
statecmd (or wherever the implementation is currently shared by the API)
rather than ever doing it directly in the command.

https://codereview.appspot.com/13632046/

[William Reade (fwereade)]

On 2013/09/11 16:17:54, fwereade wrote:
> Hmm, further to my previous comments... we shouldn't be calling
> SanityCheckConstraints *quite* so early; it'll want to happen in
statecmd (or
> wherever the implementation is currently shared by the API) rather
than ever
> doing it directly in the command.

And... hmm. I think it's a bit broken for nested containers -- the
environ may not support a particular container type, but it may be able
to host a different sort of container that can. We don't support nesting
today but I would prefer to avoid making assumptions based on this state
of affairs.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

On 2013/09/11 12:05:59, jameinel wrote:
> Because this is being added to the environs interface we should have a
test
> added in environs/jujutest/livetests.go Or possibly just
".../tests.go" but I'd
> like to see the former. That gives us an interface-level test against
all things
> that implement Environs. (Except Azure, because it hasn't been hooked
into the
> live testing infrastructure yet.)

Thanks, I wasn't familiar with this common test code. Done.

> Offhand this could just be that constraints that we will definitely
support
> (like all values of nil) are checked to not return an error. And maybe
testing
> an LXC sort of constraint and test that either it succeeds (err ==
nil) or it
> fails in a known way.

> Given the functions you've written, something like:

> func (s *LiveSuite) TestSanityCheckConstraints(c *gc.C) {
> var cons constraints.Value
> c.Check(s.Env.SanityCheckConstraints(cons), gc.IsNil)
> cons.Container = new(instance.ContainerType)
> *cons.Container = instance.LXC
> err := s.Env.SanityCheckConstraints(cons)
> // If err is nil, that is fine, some providers support containers
> if err != nil {
> // But for ones that don't, they should have a standard syntax
for
> erroring
> c.Check(err, gc.ErrorMatches, ".*provider does not support
containers")
> }
> }

Thanks, added that in. I tidied up the imports while I was at it.

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go
> File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go#newcode112
> cmd/juju/addmachine_test.go:112: dummy.SanityCheckConstraintsError =
> errors.New("computer says no")
> You don't clean up after setting this value, which will give errors in
strange
> ways for other tests (I think).
> You need something like:

> defer func() { dummy.SanityCheckConstraints = nil }

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go
> File cmd/juju/addunit_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go#newcode143
> cmd/juju/addunit_test.go:143: dummy.SanityCheckConstraintsError =
> errors.New("computer says no")
> Similarly here, you should be resetting the error when the test
finishes.

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go
> File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go#newcode250
> cmd/juju/deploy_test.go:250: dummy.SanityCheckConstraintsError =
> stderrors.New("computer says no")
> and here

> Maybe turn SanityCheckConstraintsError into a function that returns a
cleanup
> func, rather than reproducing that in every test that wants to
override it.

> So you would have:

> cleanup := dummy.SetSanityCheckConstraintsError(errors.New("stuff"))
> defer cleanup()

> https://codereview.appspot.com/13632046/diff/4001/juju/conn.go
> File juju/conn.go (right):

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode342
> juju/conn.go:342: }
> My first thought on reading this was that you don't need to validate
> containerType if you got...

[Andrew Wilkins (axwalk)]

On 2013/09/11 16:17:54, fwereade wrote:
> Hmm, further to my previous comments... we shouldn't be calling
> SanityCheckConstraints *quite* so early; it'll want to happen in
statecmd (or
> wherever the implementation is currently shared by the API) rather
than ever
> doing it directly in the command.

That would be good, but add-machine, add-unit and deploy all currently
go directly to state. State doesn't work with Environs.
I can't see a better place for these to go right now.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

On 2013/09/11 16:20:46, fwereade wrote:
> On 2013/09/11 16:17:54, fwereade wrote:
> > Hmm, further to my previous comments... we shouldn't be calling
> > SanityCheckConstraints *quite* so early; it'll want to happen in
statecmd (or
> > wherever the implementation is currently shared by the API) rather
than ever
> > doing it directly in the command.

> And... hmm. I think it's a bit broken for nested containers -- the
environ may
> not support a particular container type, but it may be able to host a
different
> sort of container that can. We don't support nesting today but I would
prefer to
> avoid making assumptions based on this state of affairs.

SanityCheckConstraints isn't saying "this environment can't create
container X in top-level machines", it's saying, nowhere in this
environment can container X exist (e.g. due to lack of addressability).
To handle that scenario, we need the second piece that thumper
mentioned: something on the Instance interface that says whether or not
that instance supports creating a container of type X.

I feel like I might be missing something here.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

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

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine.go#newcode106
cmd/juju/addmachine.go:106: if err =
conn.Environ.SanityCheckConstraints(checkConstraints); err != nil {
On 2013/09/11 14:43:35, fwereade wrote:
> We really shouldn't use conn.Environ. We should probably unexport it,
even.

Sigh, sorry (again). Will fix.

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go#newcode112
cmd/juju/addmachine_test.go:112: dummy.SanityCheckConstraintsError =
errors.New("computer says no")
On 2013/09/11 12:05:59, jameinel wrote:
> You don't clean up after setting this value, which will give errors in
strange
> ways for other tests (I think).
> You need something like:

> defer func() { dummy.SanityCheckConstraints = nil }

I do, it's just not obvious from here. It's cleaned up in dummy.Reset,
which gets called in the JujuConnSuite's TearDownTest. This is similar
to how dummy.Listen is used.

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode329
juju/conn.go:329: machineId = strings.Join(specParts[1:], "/")
On 2013/09/11 14:43:35, fwereade wrote:
> Yeah, I'm pretty sure we don't want this. :s later in the string
should not be
> turned into /s.

> And I remain really nervous about having this logic right here,
regardless...
> can we break it out a little, and write it in terms of "placement
directives",
> in which the bit before the first : determines the handler for the bit
> afterwards? And thus have an explicit lxc handler, ready to add the
others?

> I do appreciate it's just a code move, but even if you don't fix it
today please
> be sure to document that it's a good-enough-for-now hack at the above
> functionality.

I have added a comment stating this is due for refactoring into
placement directives, and changes in logic should consider that first.

When I've got my current work done, I'll have a look into it.

https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode342
juju/conn.go:342: }
On 2013/09/11 12:05:59, jameinel wrote:
> My first thought on reading this was that you don't need to validate
> containerType if you got "3/lxc/2" because that machine must already
exist.
> But looking closer, I'm a bit confused by the logic here. It looks
like you get
> really weird behavior if you do: lxc:2:3

> It actually looks like you could do: lxc:2:lxc:3 to create a new lxc
on machine
> 2/lxc/3, though if we really want to support that, we'd use the syntax
> lxc:2/lxc/3
> (I don't really understand why we have the strings.Join(specParts[1:])

> Anyway, I realize you just moved the code. Though I don't fully know
why you had
> to move it.

I didn't have to, I just die a little bit inside when I see code in
loops that doesn't need to be.

https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test.go
File provider/azure/environ_test.go (right):...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[William Reade (fwereade)]

OK, I'm sorry I didn't express this well before, but it only just
clicked: this is happening at the wrong level. See
state/state.go:343-347 -- the constraints you're checking do not match
the constraints we actually use.

But... threading the environ through to the place we actually need to
use it may be a problem, because state knows naught of the environ; we
can stick SanityCheckConstraints behind an interface, ofc, but it's
still not entirely clear how to get it on target (pass one in AddMachine
and AddMachineWithConstraints but not InjectMachine, maybe? somehow set
a SanityChecker on the state object itself? neither prospect fills me
with glee).

But I don't think we can usefully do it at this level, so this approach
does not LGTM as is.

FWIW, I think that we may be missing a trick with the
constraints-specificity here. Series is also sanely preflightable; I'm
wondering whether we should do something like:

package instance

type Preflighter interface {
     Preflight(series string, cons constraints.Value)
}

...and implementations thereof on the environs -- which should be
entirely uncontroversial -- and get that landed so we can focus
separately on the problem of applying it in the right place. Sane?

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

On 2013/09/13 08:17:48, fwereade wrote:
> OK, I'm sorry I didn't express this well before, but it only just
clicked: this
> is happening at the wrong level. See state/state.go:343-347 -- the
constraints
> you're checking do not match the constraints we actually use.

Ah, I see now. Thanks for explaining.

> But... threading the environ through to the place we actually need to
use it may
> be a problem, because state knows naught of the environ; we can stick
> SanityCheckConstraints behind an interface, ofc, but it's still not
entirely
> clear how to get it on target (pass one in AddMachine and
> AddMachineWithConstraints but not InjectMachine, maybe? somehow set a
> SanityChecker on the state object itself? neither prospect fills me
with glee).

> But I don't think we can usefully do it at this level, so this
approach does not
> LGTM as is.

> FWIW, I think that we may be missing a trick with the
constraints-specificity
> here. Series is also sanely preflightable; I'm wondering whether we
should do
> something like:

> package instance

> type Preflighter interface {
> Preflight(series string, cons constraints.Value)
> }

Interesting name! That's a bit more memorable than "sanity check".

> ...and implementations thereof on the environs -- which should be
entirely
> uncontroversial -- and get that landed so we can focus separately on
the problem
> of applying it in the right place. Sane?

Yes, that sounds like a fine approach. I'll back out the juju/conn &
cmd/juju bits while pondering how best to do it.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[Tim Penhey (thumper)]

On 2013/09/13 08:35:28, axw1 wrote:
> On 2013/09/13 08:17:48, fwereade wrote:

> > package instance
> >
> > type Preflighter interface {
> > Preflight(series string, cons constraints.Value)
> > }

> Interesting name! That's a bit more memorable than "sanity check".

Yes it is an interesting name, but I'm -1 on it as it is neither in the
problem nor solution domain.

I'll go through and review now and see if a better name comes to mind.

https://codereview.appspot.com/13632046/

[Tim Penhey (thumper)]

If we consider what the preflight check is for, it is to validate or vet
the series and constraints for machine creation.

If we look through a thesaurus here, other ideas:
   vet, probe, examine, inspect, precheck, review

ProbeMachineCreation
PrecheckCreateMachine
VetCreateMachine
ReviewMachineCreation

A few ideas there.

https://codereview.appspot.com/13632046/diff/26001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/interface.go#newcode146
environs/interface.go:146: Preflight(inst instance.Instance, series
string, cons constraints.Value) error
This is a small thing, but if there are two parameters that are always
sent and one that is sometimes sent, I have a preference that the
sometimes sent one is last.

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetests.go#newcode151
environs/jujutest/livetests.go:151: // But for ones that don't, they
should have a standard error format.
I think we should go one step further than a standard error format, and
have a standard error type.

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

https://codereview.appspot.com/13632046/diff/26001/provider/azure/environ.go#newcode144
provider/azure/environ.go:144: if cons.Container != nil {
instance.NONE is a valid container constraint, so we should probably
allow that too.

https://codereview.appspot.com/13632046/diff/26001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/13632046/diff/26001/provider/maas/environ.go#newcode1
provider/maas/environ.go:1: // Copyright 2013 Canonical Ltd.
No Preflight equivalent check here?

https://codereview.appspot.com/13632046/

[Tim Penhey (thumper)]

review: Needs Information

[Andrew Wilkins (axwalk)]

I have renamed the interface to Prechecker, and created separate methods
for checking machine and container creation.

https://codereview.appspot.com/13632046/diff/26001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/interface.go#newcode146
environs/interface.go:146: Preflight(inst instance.Instance, series
string, cons constraints.Value) error
On 2013/09/18 23:04:08, thumper wrote:
> This is a small thing, but if there are two parameters that are always
sent and
> one that is sometimes sent, I have a preference that the sometimes
sent one is
> last.

As discussed on IRC, I'm going to split it out into two methods: one for
prechecking machine creation, one for container creation.

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetests.go#newcode151
environs/jujutest/livetests.go:151: // But for ones that don't, they
should have a standard error format.
On 2013/09/18 23:04:08, thumper wrote:
> I think we should go one step further than a standard error format,
and have a
> standard error type.

Done. Added "containersUnsupportedError" and friends to
juju-core/errors, and added a jc.Satisfies check here.

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

https://codereview.appspot.com/13632046/diff/26001/provider/azure/environ.go#newcode144
provider/azure/environ.go:144: if cons.Container != nil {
On 2013/09/18 23:04:08, thumper wrote:
> instance.NONE is a valid container constraint, so we should probably
allow that
> too.

Done.

https://codereview.appspot.com/13632046/diff/26001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/13632046/diff/26001/provider/maas/environ.go#newcode1
provider/maas/environ.go:1: // Copyright 2013 Canonical Ltd.
On 2013/09/18 23:04:08, thumper wrote:
> No Preflight equivalent check here?

Is there anything that's currently unsupported?

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[Tim Penhey (thumper)]

On 2013/09/19 02:44:05, axw wrote:
> Please take a look.

LGTM, although I do wonder if we should wait for fwereade to override
his not.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

On 2013/09/20 03:19:24, thumper wrote:
> On 2013/09/19 02:44:05, axw wrote:
> > Please take a look.

> LGTM, although I do wonder if we should wait for fwereade to override
his not.

Thanks. I will wait. There's a followup needed for this to be turned on,
anyway.

https://codereview.appspot.com/13632046/

[William Reade (fwereade)]

Sorry for the delay... my eyes somehow slid right past it several times,
I think.

Assuming this is still intended for use by state, to check final
constraints for sanity before creating the mongo documents, I think my
criticisms below are on-target. If it's intended for something else, in
light of your discussions with thumper, I'm still pretty uncertain about
using a constraints value to deliver the container-type information...
but I could possibly be convinced that it's useful for the environ to
know what instance is being asked about.

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode136
environs/interface.go:136: // Prechecker's methods are best effort, and
not guaranteed to eliminated
s/eliminated/eliminate/

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode144
environs/interface.go:144: PrecheckCreateMachine(series string, cons
constraints.Value) error
I don't think the environ should be talking machines, it should be
talking instances. I know it's called before CreateMachine, but it feels
to me more like it's a heuristic for whether *StartInstance* will work
and that that's what it should be named after.

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode152
environs/interface.go:152: PrecheckCreateContainer(series string, cons
constraints.Value, host instance.Instance) error
I wasn't there for the talks with thumper, so maybe the plan already
addresses this, but if this is meant to be used inside the state package
you'll need an instance.Id rather than an Instance (). I think using an
Id is kinda nice anyway, because you clearly don't always need an actual
Instance -- many providers just say "no".

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_test.go
File provider/local/environ_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_test.go#newcode62
provider/local/environ_test.go:62: err =
prechecker.PrecheckCreateContainer("precise", cons, inst)
This nil instance doesn't sit quite right with me. Not enough to block,
but enough to whine a little.

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_test.go#newcode240
provider/openstack/local_test.go:240: err =
prechecker.PrecheckCreateContainer("precise", cons, inst)
Hmm. I'm thinking... what does the environ particularly know about what
*host* will be able to run a container? By passing it, we kinda imply
that the constraints will be checked against the host's hardware... is
that sensible? Can we maybe drop the instance param entirely? Anything
machine-specific that we can discover should surely be stored in state
anyway -- eg the hardware characteristics that we record at provisioning
time (and against which we do indeed do the aforementioned checking, I
think).

I guess the environment *is* in a position to know about available
series, because the environment sup...

[Andrew Wilkins (axwalk)]

Thanks for the review.

I've removed the series from PrecheckContainer, too. Let me know if you
think that's actually needed (it certainly won't be checked in my
initial cut).

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode136
environs/interface.go:136: // Prechecker's methods are best effort, and
not guaranteed to eliminated
On 2013/09/23 14:36:11, fwereade wrote:
> s/eliminated/eliminate/

Done.

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode144
environs/interface.go:144: PrecheckCreateMachine(series string, cons
constraints.Value) error
On 2013/09/23 14:36:11, fwereade wrote:
> I don't think the environ should be talking machines, it should be
talking
> instances. I know it's called before CreateMachine, but it feels to me
more like
> it's a heuristic for whether *StartInstance* will work and that that's
what it
> should be named after.

Fair enough. PrecheckInstance/PrecheckContainer SGTM.

https://codereview.appspot.com/13632046/diff/36001/environs/interface.go#newcode152
environs/interface.go:152: PrecheckCreateContainer(series string, cons
constraints.Value, host instance.Instance) error
On 2013/09/23 14:36:11, fwereade wrote:
> I wasn't there for the talks with thumper, so maybe the plan already
addresses
> this, but if this is meant to be used inside the state package you'll
need an
> instance.Id rather than an Instance (). I think using an Id is kinda
nice
> anyway, because you clearly don't always need an actual Instance --
many
> providers just say "no".

Oops, of course, thanks. Doesn't matter for now, as I'll remove the host
parameter altogether.

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_test.go
File provider/local/environ_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_test.go#newcode62
provider/local/environ_test.go:62: err =
prechecker.PrecheckCreateContainer("precise", cons, inst)
On 2013/09/23 14:36:11, fwereade wrote:
> This nil instance doesn't sit quite right with me. Not enough to
block, but
> enough to whine a little.

I agree. I figured the amount of work to get a real instance wasn't
worthwhile. It's moot now, since the parameter is disappearing.

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_test.go#newcode240
provider/openstack/local_test.go:240: err =
prechecker.PrecheckCreateContainer("precise", cons, inst)
On 2013/09/23 14:36:11, fwereade wrote:
> Hmm. I'm thinking... what does the environ particularly know about
what *host*
> will be able to run a container? By passing it, we kinda imply that
the
> constraints will be checked against the host's hardware... is that
sensible? Can
> we maybe drop the instance param entirely? Anything machine-specific
that we can
> discover should surely be stored in state anyway -- eg the hardware
> characteristics that we record at provisioning time (and against which
w...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[William Reade (fwereade)]

LGTM, thanks.

https://codereview.appspot.com/13632046/diff/49001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13632046/diff/49001/environs/interface.go#newcode152
environs/interface.go:152: PrecheckContainer(series string, kind
instance.ContainerType) error
I'm easy wrt series. I think we will one day want to check it, but our
best effort doe not yet include that; I worry a tiny bit that we'll also
want to be checking arch (people might have a legitimate reason to ask
for an i386 kvm machine...) but that's a bit less certain and not worth
the churn. Let it stand as it is.

https://codereview.appspot.com/13632046/diff/49001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/13632046/diff/49001/errors/errors.go#newcode127
errors/errors.go:127: return
containersUnsupportedError{&errorWrapper{Err: err, Msg: msg}}
Hey, are these tested?

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Thank you. I'll add direct tests for the coreerrors changes.

https://codereview.appspot.com/13632046/diff/49001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/13632046/diff/49001/errors/errors.go#newcode127
errors/errors.go:127: return
containersUnsupportedError{&errorWrapper{Err: err, Msg: msg}}
On 2013/09/26 07:37:53, fwereade wrote:
> Hey, are these tested?

Tested in jujutest/livetests.go.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13632046/

[Andrew Wilkins (axwalk)]

On 2013/09/26 08:49:40, axw wrote:
> Please take a look.

Added some tests, and fixed a bug in an existing error constructor ;)

Will land now.

https://codereview.appspot.com/13632046/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/sanity-check-constraints into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.712s
ok launchpad.net/juju-core/agent/tools 0.252s
ok launchpad.net/juju-core/bzr 6.322s
ok launchpad.net/juju-core/cert 3.633s
ok launchpad.net/juju-core/charm 0.524s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.037s
ok launchpad.net/juju-core/cmd 0.213s
? 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 155.940s
ok launchpad.net/juju-core/cmd/jujud 39.680s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.891s
ok launchpad.net/juju-core/constraints 0.025s
ok launchpad.net/juju-core/container/lxc 0.273s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.321s
ok launchpad.net/juju-core/environs 2.135s
ok launchpad.net/juju-core/environs/bootstrap 5.312s
ok launchpad.net/juju-core/environs/cloudinit 0.426s
ok launchpad.net/juju-core/environs/config 0.716s
ok launchpad.net/juju-core/environs/configstore 0.031s
ok launchpad.net/juju-core/environs/filestorage 0.030s
ok launchpad.net/juju-core/environs/httpstorage 0.756s
ok launchpad.net/juju-core/environs/imagemetadata 0.516s
ok launchpad.net/juju-core/environs/instances 0.062s
ok launchpad.net/juju-core/environs/jujutest 0.278s
ok launchpad.net/juju-core/environs/manual 4.458s
ok launchpad.net/juju-core/environs/simplestreams 0.347s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.624s
ok launchpad.net/juju-core/environs/storage 0.234s
ok launchpad.net/juju-core/environs/sync 26.036s
ok launchpad.net/juju-core/environs/testing 0.209s
ok launchpad.net/juju-core/environs/tools 11.633s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.022s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 20.351s
ok launchpad.net/juju-core/juju/osenv 0.038s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.030s
ok launchpad.net/juju-core/log/syslog 0.037s
ok launchpad.net/juju-core/names 0.024s
ok launchpad.net/juju-core/provider 0.298s
? launchpad.net/juju-core/provider/all [no test files]
FAIL launchpad.net/juju-core/provider/azure [build failed]
ok launchpad.net/juju-core/provider/dummy 20.498s
FAIL launchpad.net/juju-core/provider/ec2 [build failed]
ok launchpad.net/juju-core/provider/ec2/httpstorage 0.211s
ok launchpad.net/juju-core/provider/local 1.528s
ok launchpad.net/juju-core/provider/maas 3.557s
FAIL launchpad.net/juju-core/provider/openstack [build failed]
ok launchpad.net/juju-core/rpc 0.084s
ok launchpad.net/juju-core/rpc/jsoncodec 0.031s
? launchpad.net/juju-core/rpc/rpcreflect [no test files]
ok launchpad.net/juju-core/sch...

[Go Bot (go-bot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'environs/interface.go'
--- environs/interface.go	2013-09-25 17:30:09 +0000
+++ environs/interface.go	2013-09-26 12:28:55 +0000
@@ -88,6 +88,28 @@
 	Config() *config.Config
 }
 
+// Prechecker is an optional interface that an Environ may implement,
+// in order to support pre-flight checking of instance/container creation.
+//
+// Prechecker's methods are best effort, and not guaranteed to eliminate
+// all invalid parameters. If a precheck method returns nil, it is not
+// guaranteed that the constraints are valid; if a non-nil error is
+// returned, then the constraints are definitely invalid.
+type Prechecker interface {
+	// PrecheckInstance performs a preflight check on the specified
+	// series and constraints, ensuring that they are possibly valid for
+	// creating an instance in this environment.
+	PrecheckInstance(series string, cons constraints.Value) error
+
+	// PrecheckContainer performs a preflight check on the container type,
+	// ensuring that the environment is possibly capable of creating a
+	// container of the specified type and series.
+	//
+	// The container type must be a valid ContainerType as specified
+	// in the instance package, and != instance.NONE.
+	PrecheckContainer(series string, kind instance.ContainerType) error
+}
+
 // An Environ represents a juju environment as specified
 // in the environments.yaml file.
 //

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2013-09-24 16:07:20 +0000
+++ environs/jujutest/livetests.go	2013-09-26 12:28:55 +0000
@@ -23,6 +23,7 @@
 	"launchpad.net/juju-core/environs/sync"
 	envtesting "launchpad.net/juju-core/environs/testing"
 	envtools "launchpad.net/juju-core/environs/tools"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
@@ -153,6 +154,27 @@
 	t.Env = nil
 }
 
+func (t *LiveTests) TestPrechecker(c *gc.C) {
+	// Providers may implement Prechecker. If they do, then they should
+	// return nil for empty constraints (excluding the null provider).
+	prechecker, ok := t.Env.(environs.Prechecker)
+	if !ok {
+		return
+	}
+
+	const series = "precise"
+	var cons constraints.Value
+	c.Check(prechecker.PrecheckInstance(series, cons), gc.IsNil)
+
+	err := prechecker.PrecheckContainer(series, instance.LXC)
+	// If err is nil, that is fine, some providers support containers.
+	if err != nil {
+		// But for ones that don't, they should have a standard error format.
+		c.Check(err, gc.ErrorMatches, ".*provider does not support containers")
+		c.Check(err, jc.Satisfies, coreerrors.IsContainersUnsupported)
+	}
+}
+
 // TestStartStop is similar to Tests.TestStartStop except
 // that it does not assume a pristine environment.
 func (t *LiveTests) TestStartStop(c *gc.C) {

=== modified file 'errors/errors.go'
--- errors/errors.go	2013-08-13 19:07:35 +0000
+++ errors/errors.go	2013-09-26 12:28:55 +0000
@@ -30,8 +30,8 @@
 	*errorWrapper
 }
 
-// IsNotFoundError is satisfied by errors created by this package representing resources that can't
-// be found.
+// IsNotFoundError is satisfied by errors created by this package representing
+// resources that can't be found.
 func IsNotFoundError(err error) bool {
 	if _, ok := err.(notFoundError); ok {
 		return true
@@ -51,19 +51,21 @@
 	}
 }
 
-// NewNotFoundError returns a new error wrapping err that satisfies IsNotFoundError().
+// NewNotFoundError returns a new error wrapping err that satisfies
+// IsNotFoundError().
 func NewNotFoundError(err error, msg string) error {
 	return notFoundError{&errorWrapper{Err: err, Msg: msg}}
 }
 
 // unauthorizedError represents the error that an operation is unauthorized.
-// Use IsUnauthorized() to determine if the error was related to authorization failure.
+// Use IsUnauthorized() to determine if the error was related to authorization
+// failure.
 type unauthorizedError struct {
 	*errorWrapper
 }
 
-// IsUnauthorizedError is satisfied by errors created by this package representing
-// authorization failures.
+// IsUnauthorizedError is satisfied by errors created by this package
+// representing authorization failures.
 func IsUnauthorizedError(err error) bool {
 	_, ok := err.(unauthorizedError)
 	return ok
@@ -78,19 +80,20 @@
 	}
 }
 
-// NewUnauthorizedError returns an error which wraps err and satisfies IsUnauthorized().
+// NewUnauthorizedError returns an error which wraps err and satisfies
+// IsUnauthorized().
 func NewUnauthorizedError(err error, msg string) error {
-	return unauthorizedError{&errorWrapper{Msg: msg}}
+	return unauthorizedError{&errorWrapper{Err: err, Msg: msg}}
 }
 
-// notBootstrappedError indicates that the environment can't be used because it hasn't been
-// bootstrapped yet.
+// notBootstrappedError indicates that the environment can't be used because it
+// hasn't been bootstrapped yet.
 type notBootstrappedError struct {
 	*errorWrapper
 }
 
-// IsNotBootstrapped is satisfied by errors created by this package representing an environment
-// that isn't bootstrapped.
+// IsNotBootstrapped is satisfied by errors created by this package
+// representing an environment that isn't bootstrapped.
 func IsNotBootstrapped(err error) bool {
 	if _, ok := err.(notBootstrappedError); ok {
 		return true
@@ -98,7 +101,28 @@
 	return false
 }
 
-// NewNotBootstrappedError returns a new error which wraps err and satisfies IsNotBootstrapped().
+// NewNotBootstrappedError returns a new error which wraps err and satisfies
+// IsNotBootstrapped().
 func NewNotBootstrappedError(err error, msg string) error {
 	return notBootstrappedError{&errorWrapper{Err: err, Msg: msg}}
 }
+
+// containersUnsupportedError indicates that the environment does not support
+// creation of containers.
+type containersUnsupportedError struct {
+	*errorWrapper
+}
+
+// IsContainersUnsupported is satisfied by errors, created by this package,
+// representing an attempt to create a container that failed on account of
+// containers not being supported by the environment.
+func IsContainersUnsupported(err error) bool {
+	_, ok := err.(containersUnsupportedError)
+	return ok
+}
+
+// NewContainersUnsupportedError returns a new error which wraps err and
+// satisfies IsContainersUnsupported().
+func NewContainersUnsupported(err error, msg string) error {
+	return containersUnsupportedError{&errorWrapper{Err: err, Msg: msg}}
+}

=== added file 'errors/errors_test.go'
--- errors/errors_test.go	1970-01-01 00:00:00 +0000
+++ errors/errors_test.go	2013-09-26 12:28:55 +0000
@@ -0,0 +1,107 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package errors_test
+
+import (
+	stderrors "errors"
+	"reflect"
+	"runtime"
+	"testing"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/errors"
+	jc "launchpad.net/juju-core/testing/checkers"
+)
+
+type errorsSuite struct{}
+
+var _ = gc.Suite(&errorsSuite{})
+
+func Test(t *testing.T) {
+	gc.TestingT(t)
+}
+
+type errorSatisfier struct {
+	f func(error) bool
+}
+
+func (s *errorSatisfier) String() string {
+	value := reflect.ValueOf(s.f)
+	f := runtime.FuncForPC(value.Pointer())
+	return f.Name()
+}
+
+func (*errorsSuite) TestNotFoundError(c *gc.C) {
+	isNotFoundError := &errorSatisfier{errors.IsNotFoundError}
+	isUnauthorizedError := &errorSatisfier{errors.IsUnauthorizedError}
+	isNotBootstrapped := &errorSatisfier{errors.IsNotBootstrapped}
+	isContainersUnsupported := &errorSatisfier{errors.IsContainersUnsupported}
+	satisfiers := []*errorSatisfier{
+		isNotFoundError,
+		isUnauthorizedError,
+		isNotBootstrapped,
+		isContainersUnsupported,
+	}
+
+	// make some errors, and record the errorSatsifier
+	// that should satisfy the error.
+	type errorTest struct {
+		err       error
+		message   string
+		satisfier *errorSatisfier
+	}
+	errorTests := []errorTest{{
+		errors.NotFoundf("woop %d", 123),
+		"woop 123 not found",
+		isNotFoundError,
+	}, {
+		errors.NewNotFoundError(stderrors.New("woo"), "msg"),
+		"msg: woo",
+		isNotFoundError,
+	}, {
+		errors.NewNotFoundError(stderrors.New("woo"), ""),
+		"woo",
+		isNotFoundError,
+	}, {
+		errors.NewNotFoundError(nil, "msg"),
+		"msg",
+		isNotFoundError,
+	}, {
+		errors.NewNotFoundError(nil, ""),
+		"",
+		isNotFoundError,
+	}, {
+		errors.Unauthorizedf("woo %s", "hoo"),
+		"woo hoo",
+		isUnauthorizedError,
+	}, {
+		errors.NewUnauthorizedError(stderrors.New("hoo"), "woo"),
+		"woo: hoo",
+		isUnauthorizedError,
+	}, {
+		errors.NewNotBootstrappedError(stderrors.New("hoo"), "woo"),
+		"woo: hoo",
+		isNotBootstrapped,
+	}, {
+		errors.NewContainersUnsupported(stderrors.New("hoo"), "woo"),
+		"woo: hoo",
+		isContainersUnsupported,
+	}}
+
+	for i, t := range errorTests {
+		c.Logf("test #%d: %v", i, t.err)
+		c.Assert(t.err, gc.ErrorMatches, t.message)
+		c.Assert(t.err, jc.Satisfies, t.satisfier.f)
+		for _, satisfier := range satisfiers {
+			// Not using jc.Satisfier here, because it doesn't give
+			// a nice string representation of the function. Also,
+			// you can't take the address of a func, but you can
+			// store it and take the address of the struct.
+			if satisfier != t.satisfier && satisfier.f(t.err) {
+				c.Errorf("%#v satisfies %v", t.err, satisfier)
+			}
+		}
+	}
+}

=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go	2013-09-25 11:19:35 +0000
+++ provider/azure/environ.go	2013-09-26 12:28:55 +0000
@@ -20,6 +20,7 @@
 	"launchpad.net/juju-core/environs/simplestreams"
 	"launchpad.net/juju-core/environs/storage"
 	envtools "launchpad.net/juju-core/environs/tools"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider"
 	"launchpad.net/juju-core/state"
@@ -137,6 +138,18 @@
 	return key, nil
 }
 
+// PrecheckInstance is specified in the environs.Prechecker interface.
+func (*azureEnviron) PrecheckInstance(series string, cons constraints.Value) error {
+	return nil
+}
+
+// PrecheckContainer is specified in the environs.Prechecker interface.
+func (*azureEnviron) PrecheckContainer(series string, kind instance.ContainerType) error {
+	// This check can either go away or be relaxed when the azure
+	// provider manages container addressibility.
+	return coreerrors.NewContainersUnsupported(nil, "azure provider does not support containers")
+}
+
 // Name is specified in the Environ interface.
 func (env *azureEnviron) Name() string {
 	return env.name

=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go	2013-09-24 09:42:10 +0000
+++ provider/azure/environ_test.go	2013-09-26 12:28:55 +0000
@@ -89,6 +89,15 @@
 	c.Check(env.Name(), gc.Equals, env.name)
 }
 
+func (*environSuite) TestPrecheck(c *gc.C) {
+	env := azureEnviron{name: "foo"}
+	var cons constraints.Value
+	err := env.PrecheckInstance("saucy", cons)
+	c.Check(err, gc.IsNil)
+	err = env.PrecheckContainer("saucy", instance.LXC)
+	c.Check(err, gc.ErrorMatches, "azure provider does not support containers")
+}
+
 func (*environSuite) TestConfigReturnsConfig(c *gc.C) {
 	cfg := new(config.Config)
 	ecfg := azureEnvironConfig{Config: cfg}

=== modified file 'provider/ec2/ec2.go'
--- provider/ec2/ec2.go	2013-09-25 17:30:09 +0000
+++ provider/ec2/ec2.go	2013-09-26 12:28:55 +0000
@@ -25,6 +25,7 @@
 	"launchpad.net/juju-core/environs/simplestreams"
 	"launchpad.net/juju-core/environs/storage"
 	envtools "launchpad.net/juju-core/environs/tools"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider"
 	"launchpad.net/juju-core/state"
@@ -307,6 +308,18 @@
 	return s3
 }
 
+// PrecheckInstance is specified in the environs.Prechecker interface.
+func (e *environ) PrecheckInstance(series string, cons constraints.Value) error {
+	return nil
+}
+
+// PrecheckContainer is specified in the environs.Prechecker interface.
+func (e *environ) PrecheckContainer(series string, kind instance.ContainerType) error {
+	// This check can either go away or be relaxed when the ec2
+	// provider manages container addressibility.
+	return coreerrors.NewContainersUnsupported(nil, "ec2 provider does not support containers")
+}
+
 func (e *environ) Name() string {
 	return e.name
 }

=== modified file 'provider/ec2/local_test.go'
--- provider/ec2/local_test.go	2013-09-24 10:08:33 +0000
+++ provider/ec2/local_test.go	2013-09-26 12:28:55 +0000
@@ -190,6 +190,17 @@
 	t.srv.stopServer(c)
 }
 
+func (t *localServerSuite) TestPrecheck(c *gc.C) {
+	env := t.Prepare(c)
+	prechecker, ok := env.(environs.Prechecker)
+	c.Assert(ok, jc.IsTrue)
+	var cons constraints.Value
+	err := prechecker.PrecheckInstance("precise", cons)
+	c.Check(err, gc.IsNil)
+	err = prechecker.PrecheckContainer("precise", instance.LXC)
+	c.Check(err, gc.ErrorMatches, "ec2 provider does not support containers")
+}
+
 func (t *localServerSuite) TestBootstrapInstanceUserDataAndState(c *gc.C) {
 	env := t.Prepare(c)
 	envtesting.UploadFakeTools(c, env.Storage())

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-09-24 09:42:10 +0000
+++ provider/local/environ.go	2013-09-26 12:28:55 +0000
@@ -24,6 +24,7 @@
 	"launchpad.net/juju-core/environs/filestorage"
 	"launchpad.net/juju-core/environs/httpstorage"
 	"launchpad.net/juju-core/environs/storage"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/names"
@@ -97,6 +98,18 @@
 	return nil
 }
 
+// PrecheckInstance is specified in the environs.Prechecker interface.
+func (*localEnviron) PrecheckInstance(series string, cons constraints.Value) error {
+	return nil
+}
+
+// PrecheckContainer is specified in the environs.Prechecker interface.
+func (*localEnviron) PrecheckContainer(series string, kind instance.ContainerType) error {
+	// This check can either go away or be relaxed when the local
+	// provider can do nested containers.
+	return coreerrors.NewContainersUnsupported(nil, "local provider does not support nested containers")
+}
+
 // Bootstrap is specified in the Environ interface.
 func (env *localEnviron) Bootstrap(cons constraints.Value, possibleTools tools.List, machineID string) error {
 	if !env.config.runningAsRoot {

=== modified file 'provider/local/environ_test.go'
--- provider/local/environ_test.go	2013-09-23 17:55:12 +0000
+++ provider/local/environ_test.go	2013-09-26 12:28:55 +0000
@@ -10,9 +10,13 @@
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/jujutest"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider/local"
+	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type environSuite struct {
@@ -42,6 +46,21 @@
 	c.Assert(environ.PublicStorage(), gc.NotNil)
 }
 
+func (s *environSuite) TestPrecheck(c *gc.C) {
+	testConfig := minimalConfig(c)
+	environ, err := local.Provider.Open(testConfig)
+	c.Assert(err, gc.IsNil)
+	var cons constraints.Value
+	prechecker, ok := environ.(environs.Prechecker)
+	c.Assert(ok, jc.IsTrue)
+
+	err = prechecker.PrecheckInstance("precise", cons)
+	c.Check(err, gc.IsNil)
+
+	err = prechecker.PrecheckContainer("precise", instance.LXC)
+	c.Check(err, gc.ErrorMatches, "local provider does not support nested containers")
+}
+
 type localJujuTestSuite struct {
 	baseProviderSuite
 	jujutest.Tests

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2013-09-24 09:42:10 +0000
+++ provider/maas/environ.go	2013-09-26 12:28:55 +0000
@@ -70,6 +70,7 @@
 	return env, nil
 }
 
+// Name is specified in the Environ interface.
 func (env *maasEnviron) Name() string {
 	return env.name
 }

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2013-09-25 12:22:10 +0000
+++ provider/openstack/local_test.go	2013-09-26 12:28:55 +0000
@@ -244,6 +244,17 @@
 	s.LoggingSuite.TearDownTest(c)
 }
 
+func (s *localServerSuite) TestPrecheck(c *gc.C) {
+	var cons constraints.Value
+	env := s.Prepare(c)
+	prechecker, ok := env.(environs.Prechecker)
+	c.Assert(ok, jc.IsTrue)
+	err := prechecker.PrecheckInstance("precise", cons)
+	c.Check(err, gc.IsNil)
+	err = prechecker.PrecheckContainer("precise", instance.LXC)
+	c.Check(err, gc.ErrorMatches, "openstack provider does not support containers")
+}
+
 // If the bootstrap node is configured to require a public IP address,
 // bootstrapping fails if an address cannot be allocated.
 func (s *localServerSuite) TestBootstrapFailsWhenPublicIPError(c *gc.C) {

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2013-09-26 07:47:47 +0000
+++ provider/openstack/provider.go	2013-09-26 12:28:55 +0000
@@ -30,6 +30,7 @@
 	"launchpad.net/juju-core/environs/simplestreams"
 	"launchpad.net/juju-core/environs/storage"
 	envtools "launchpad.net/juju-core/environs/tools"
+	coreerrors "launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/provider"
@@ -395,6 +396,18 @@
 	return nova
 }
 
+// PrecheckInstance is specified in the environs.Prechecker interface.
+func (*environ) PrecheckInstance(series string, cons constraints.Value) error {
+	return nil
+}
+
+// PrecheckContainer is specified in the environs.Prechecker interface.
+func (*environ) PrecheckContainer(series string, kind instance.ContainerType) error {
+	// This check can either go away or be relaxed when the openstack
+	// provider manages container addressibility.
+	return coreerrors.NewContainersUnsupported(nil, "openstack provider does not support containers")
+}
+
 func (e *environ) Name() string {
 	return e.name
 }