Looking very nice, only a couple of details that maybe demand
thought/discussion.


https://codereview.appspot.com/85040046/diff/1/instance/placement.go
File instance/placement.go (right):

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode54
instance/placement.go:54: if (scope == MachineScope ||
isContainerType(scope)) && !names.IsMachine(value) {
On 2014/04/17 06:13:46, axw wrote:
> On 2014/04/11 12:27:11, fwereade wrote:
> > Hmm. Problem coming: environments named after a container type will
act in a
> > surprising way. Can you think of any way around this?

> Our current placement directives currently define the lxc, kvm, ssh
and
> machine-id special cases. Can't do anything about this without
breaking that
> existing format.

> In the initial implementation we won't be able to handle environment
placement
> directives for an environment named "lxc" or "kvm". We *could* add
special
> casing so that it looks for "env:lxc:" or something, but really I
don't think
> it's worth it.

Yeah, I was more thinking that we ought to disallow "lxc" and "kvm" as
env names, but can't see how we can get there from here.

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode63
instance/placement.go:63: return &Placement{Scope: directive}, nil
On 2014/04/17 06:13:46, axw wrote:
> On 2014/04/11 12:27:11, fwereade wrote:
> > I'm not following this case. `add-machine --to kvm`? I think we need
a target
> > machine for this to make sense.

> It means "add a kvm container on a new machine"; see the bottom of
> state/apiserver.Client.addOneMachine.

Huh. I guess it was always like this, and I completely missed it.

Still not quite sure I like it, but let's not change things completely
arbitrarily :).

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

https://codereview.appspot.com/85040046/diff/20001/cmd/juju/addmachine.go#newcode123
cmd/juju/addmachine.go:123: // message.
This feels a little bit off -- if we're not incompatible, we should
surely send the info in placement form? I think we should do this
tweaking *after* getting a NotImplemented...

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

https://codereview.appspot.com/85040046/diff/20001/environs/jujutest/livetests.go#newcode166
environs/jujutest/livetests.go:166: err :=
prechecker.PrecheckInstance("precise", constraints.Value{}, placement)
<3

https://codereview.appspot.com/85040046/diff/20001/instance/placement_test.go
File instance/placement_test.go (right):

https://codereview.appspot.com/85040046/diff/20001/instance/placement_test.go#newcode16
instance/placement_test.go:16: var parsePlacementTests = []struct {
FWIW, I really like it when test tables are defined inside the funcs
that use them, instead of cluttering up the package globals ;). It's a
bit surprising at first, but the structure is actually quite clean:

for i, test := range []struct{
     // fields
}{{
     // test
}, {
     // test
}} {
     // code
}

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

https://codereview.appspot.com/85040046/diff/20001/provider/maas/environ.go#newcode173
provider/maas/environ.go:173: // TODO(axw) handle maas-name placement
directive
deserves a bug#

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go#newcode230
state/api/client.go:230: // This exists for backwards compatibility;
remove in 1.21 (client only).
are we sure we can drop this? I fear we'll have to deal with the
possibility of a 1.18 client *or* server pretty-much-forever

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go#newcode246
state/api/client.go:246: err := c.call("AddMachinesV2", args, results)
grump, rage. jam, *please* get me some API versioning.

https://codereview.appspot.com/85040046/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/api/params/params.go#newcode714
state/api/params/params.go:714: Results []PlacementResult
This feels like it should be all together in a single call that extracts
all the info we need for a given machine -- ie we should be able to grab
series+constraints+placement (and I guess instance-group) for N machines
in a single call.

Maybe out of scope here, but I feel obliged to bang the
chatty-APIs-are-bad drum at every opportunity.

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/client_test.go#newcode1702
state/apiserver/client/client_test.go:1702: apiParams[0].Placement =
instance.MustParsePlacement("lxc")
A thought -- can we at least make what's happening explicit at the API
level? parse "lxc" to "lxc:new", or something?

If this is stupid, disregard.

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisioner_task.go#newcode467
worker/provisioner/provisioner_task.go:467: machinePlacement, err :=
machine.Placement()
I have a half-suspicion that we can actually safely tack Placement onto
some existing API call -- if we talk to an old state server we don't get
placement, but we wouldn't anyway (and hopefully we won't talk to old
state servers at all, anyway, soon enough. Crazy?

It's really just that people tend to follow existing conventions,
whether or not they're good ones, and every chatty API call normalises
the behaviour in people's minds.

https://codereview.appspot.com/85040046/