all: introduce environs.Prechecker

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://code.launchpad.net/~axwalk/juju-core/sanity-check-constraints/+merge/185015

(do not edit description out of merge proposal)

[axw, 2013-09-11 10:56:45]

Please take a look.

[axw, 2013-09-11 11:00:32]

Please take a look.

[jameinel, 2013-09-11 12:05:59]

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...
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#ne...
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#new...
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, 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.

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

https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test...
provider/azure/environ_test.go:94: *cons.Container = instance.LXC
It is really unfortunate that it has to be written this way.
But from what I can tell, you can't take the address of a const, and you can't
use &instance.ContainerType("lxc")

You can do it as:

container := instance.LXC
cons.Container = &container

I don't know if that is actually cleaner, though. (I'm just generally not a big
fan of new())

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#n...
provider/local/environ.go:100: return nil
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.

[fwereade, 2013-09-11 14:43:35]

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#newc...
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...
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#n...
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

[fwereade, 2013-09-11 16:17:54]

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.

[fwereade, 2013-09-11 16:20:46]

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.

[axw1, 2013-09-12 05:22:43]

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...
> 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#ne...
> 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#new...
> 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, 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.
> 
>
https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test.go
> File provider/azure/environ_test.go (right):
> 
>
https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test...
> provider/azure/environ_test.go:94: *cons.Container = instance.LXC
> It is really unfortunate that it has to be written this way.
> But from what I can tell, you can't take the address of a const, and you can't
> use &instance.ContainerType("lxc")
> 
> You can do it as:
> 
> container := instance.LXC
> cons.Container = &container
> 
> I don't know if that is actually cleaner, though. (I'm just generally not a
big
> fan of new())
> 
> 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#n...
> provider/local/environ.go:100: return nil
> 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.

[axw1, 2013-09-12 06:06:14]

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.

[axw1, 2013-09-12 06:12:14]

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.

[axw1, 2013-09-12 06:21:15]

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#newc...
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...
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):

https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test...
provider/azure/environ_test.go:94: *cons.Container = instance.LXC
On 2013/09/11 12:05:59, jameinel wrote:
> It is really unfortunate that it has to be written this way.
> But from what I can tell, you can't take the address of a const, and you can't
> use &instance.ContainerType("lxc")
> 
> You can do it as:
> 
> container := instance.LXC
> cons.Container = &container
> 
> I don't know if that is actually cleaner, though. (I'm just generally not a
big
> fan of new())

Six of one... I've changed it. I don't mind either way.

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#n...
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.

Done.

[axw, 2013-09-12 06:24:00]

Please take a look.

[fwereade, 2013-09-13 08:17:48]

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?

[axw1, 2013-09-13 08:35:28]

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.

[axw, 2013-09-17 07:11:47]

Please take a look.

[axw, 2013-09-18 10:20:10]

Please take a look.

[thumper, 2013-09-18 22:37:26]

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.

[thumper, 2013-09-18 23:04:07]

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#newc...
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/livetest...
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetest...
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#...
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#n...
provider/maas/environ.go:1: // Copyright 2013 Canonical Ltd.
No Preflight equivalent check here?

[axw1, 2013-09-19 02:41:05]

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#newc...
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/livetest...
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/13632046/diff/26001/environs/jujutest/livetest...
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#...
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#n...
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?

[axw, 2013-09-19 02:44:05]

Please take a look.

[thumper, 2013-09-20 03:19:24]

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.

[axw1, 2013-09-20 03:54:37]

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.

[fwereade, 2013-09-23 14:36:10]

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#newc...
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#newc...
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#newc...
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_tes...
File provider/local/environ_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_tes...
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_t...
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_t...
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 supplies the simplestreams data for container images
(hmm. does it? it should...) so, regardless, it's conceptually the right
component to ask.

Containerization kind is also important, but I don't think it should be
expressed as part of a constraints value, especially not wrt the environment. A
container *may* be created due to a container constraint, but it might just as
easily be done due to an "add-machine lxc:0", which wouldn't be caught here. And
since we can already check the constraints against the host's hardware in state,
I think we can drop cons entirely. And so I think I'm still -1 on passing the
host up at all. So, as a discussion point, how would you feel about something
more like:

type Prechecker interface{
    PrecheckInstance(series string, cons constraints.Value) error
    PrecheckContainer(series string, kind instance.ContainerType) error
}

..?

[axw1, 2013-09-24 04:16:10]

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#newc...
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#newc...
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#newc...
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_tes...
File provider/local/environ_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/local/environ_tes...
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_t...
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/13632046/diff/36001/provider/openstack/local_t...
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 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 supplies the simplestreams data for container images
> (hmm. does it? it should...) so, regardless, it's conceptually the right
> component to ask.
> 
> Containerization kind is also important, but I don't think it should be
> expressed as part of a constraints value, especially not wrt the environment.
A
> container *may* be created due to a container constraint, but it might just as
> easily be done due to an "add-machine lxc:0", which wouldn't be caught here.
And
> since we can already check the constraints against the host's hardware in
state,
> I think we can drop cons entirely. And so I think I'm still -1 on passing the
> host up at all. So, as a discussion point, how would you feel about something
> more like:
> 
> type Prechecker interface{
>     PrecheckInstance(series string, cons constraints.Value) error
>     PrecheckContainer(series string, kind instance.ContainerType) error
> }
> 
> ..?

That sounds reasonable. If we find an environment that does need to interrogate
a host for its capabilities, then we can always change this later. That's
hypothetical at the moment.

Is series even important in PrecheckContainer? Maybe we should go further, and
making it PrecheckContainer(instance.ContainerType).

By the way, "add-machine lxc:0" was caught with my original changes, as I
injected the container-type into the constraints. That's maybe a little abusive,
though.

[axw, 2013-09-24 05:08:49]

Please take a look.

[axw, 2013-09-24 06:32:55]

Please take a look.

[fwereade, 2013-09-26 07:37:53]

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#newc...
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?

[axw1, 2013-09-26 07:41:03]

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.

[axw, 2013-09-26 08:49:40]

Please take a look.

[axw1, 2013-09-26 08:55:50]

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.