Please take a look.


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

https://codereview.appspot.com/85040046/diff/1/cmd/juju/addmachine.go#newcode127
cmd/juju/addmachine.go:127: results, err :=
client.AddMachines([]params.AddMachineParams{machineParams})
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > What about backwards compatibility?

> Yeah, I suppose we will need to continue setting ContainerType and
ParentId for
> backwards compatibility.

Done.

https://codereview.appspot.com/85040046/diff/1/cmd/juju/addmachine.go#newcode137
cmd/juju/addmachine.go:137: // TODO(axw) figure out how to convey
whether it's a container?
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > Can't we inspect the scope to see if it's a valid container type?

> Yes indeed. I had previously implemented things differently, but with
the
> current implementation it's trivial to do so.

Done.

https://codereview.appspot.com/85040046/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/85040046/diff/1/environs/interface.go#newcode160
environs/interface.go:160: state.PlacementValidator
On 2014/04/11 12:27:11, fwereade wrote:
> I'm not clear on how this is distinct from Prechecker -- isn't
placement just
> another StartInstance parameter?

My intention was for PlacementValidator to be used for validating
add-unit/deploy placement too. I suppose those can just be handled by a
unit-placement policy.

> Food for thought and maybe future implementation: should the
prechecker perhaps
> return something that implements InstanceBroker? that'd keep
what-we-ask-for and
> what-we-check in sync...

Sounds like a decent idea, but I'd rather keep this CL to a minimum.

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/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.

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode63
instance/placement.go:63: return &Placement{Scope: directive}, nil
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.

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode66
instance/placement.go:66: return &Placement{Value: directive}, nil
On 2014/04/11 12:27:11, fwereade wrote:
> I think I'd prefer an explict ErrScopeUnclear, and explicit handling
thereof in
> the client.

Done.

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode69
instance/placement.go:69: // ParsePlacement attempts to parse the
specified string and create a
On 2014/04/11 12:27:11, fwereade wrote:
> s/Parse/MustParse/

Done.

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

https://codereview.appspot.com/85040046/diff/1/instance/placement_test.go#newcode25
instance/placement_test.go:25: expect: &instance.Placement{Scope:
instance.MachineScope, Value: "0"},
On 2014/04/09 05:20:22, axw wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > Perhaps to reduce the verbosity, the test struct can have
expectScope and
> > expectDirective and the test would use those individual values to
construct a
> > Placement to compare with?

> Sure. There's a special case: empty scope and value == nil placement.

Done.

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

https://codereview.appspot.com/85040046/diff/1/provider/azure/environ.go#newcode1179
provider/azure/environ.go:1179: return fmt.Errorf("unknown placement
directive: %s", p)
On 2014/04/11 12:27:11, fwereade wrote:
> Do we want a whole Placement here, or should we maybe just handle the
directive
> alone?

Just the directive I suppose. I was thinking we might want to handle
provider-type vs. environment name, but it's probably not a sensible
distinction.

https://codereview.appspot.com/85040046/diff/1/state/api/provisioner/provisioner_test.go
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/85040046/diff/1/state/api/provisioner/provisioner_test.go#newcode330
state/api/provisioner/provisioner_test.go:330: c.Assert(err,
gc.ErrorMatches, "machine 1 not found")
On 2014/04/11 12:27:11, fwereade wrote:
> On 2014/04/09 04:52:18, wallyworld wrote:
> > We have started returning perm denied if a machine is not found.
> > We should have tests for genuine perm denied cases as well?

> The distinction is whether or not the client ought to be able to
access it. But
> we also have a slight tendency to be lazy, and to ErrPerm in *all*
cases of
> NotFound; I don't entirely love this, but often it simplifies all the
related
> code enough that it's a win regardless.

> In particular, though, I think we should ErrPerm for containers not on
the
> machine running this provisioner. (ideally we'd ErrPerm for *all*
containers in
> the case of an environ-provisioner, but I don;t think we're capable of
making
> that distinction)

Placement follows the same authorization rules as other provisioner API,
which is essentially what you've described.

Added some tests to apiserver/provisioner.

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

https://codereview.appspot.com/85040046/diff/1/state/apiserver/client/client.go#newcode630
state/apiserver/client/client.go:630: return
c.api.state.AddMachineWithPlacement(template, placement)
On 2014/04/11 12:27:11, fwereade wrote:
> Based on this, I reckon placement certainly ought to be part of
MachineTemplate.

Done.

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

https://codereview.appspot.com/85040046/diff/1/state/machine.go#newcode118
state/machine.go:118: Placement *instance.Placement `bson:",omitempty"`
On 2014/04/11 12:27:11, fwereade wrote:
> Hmm. I think scope is redundant in here, too. If it's a container,
it's encoded
> in the id; if it's not, it's *currently* implicit -- but long-term, it
would
> probably be best to make it an explicit "Environ"... except our
terminology is
> all messed up, because of the implicit 1:1 between juju environments
and
> envirns.Environ objects. Grr.

> So: I'd most like to see machines explicitly tagged with their
environ, but I'm
> not sure it's a good idea. Shall we run with provider-directive-only
for now?
> (and skip it for containers, because it's already been handled?)

Okay.

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