> I assume William is ok with the design?

I ran the basic idea by William, but not all the details. I asked for a
review last night, but I think he was busy reviewing other things.

> Maybe we can ultimately remove container constraints? My main concern
is about backwards compatibility of clients invoking add machine. Do we
support 1.18 add-machine commands?

Yeah I think we need to keep it for backwards compatibility. The client
code should continue to pass ContainerType and ParentId to the API
server; when we get to 1.22, then we can use Placement exclusively.


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 04:52:18, wallyworld wrote:
> What about backwards compatibility?

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

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

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

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

https://codereview.appspot.com/85040046/diff/1/provider/dummy/environs.go#newcode546
provider/dummy/environs.go:546: func (*environ) ValidatePlacement(p
*instance.Placement) error {
On 2014/04/09 04:52:18, wallyworld wrote:
> I know we check for nil elsewhere, but maybe we should have it here as
well to
> be defensive and avoid a npe?

I suppose so. Alternatively, make it a non-pointer.

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

https://codereview.appspot.com/85040046/diff/1/state/api/params/params.go#newcode107
state/api/params/params.go:107: ContainerType instance.ContainerType
On 2014/04/09 04:52:18, wallyworld wrote:
> So we are retaining ContainerType and ParentId but add-machine does
not set them
> anymore?

Yes, to support old clients. This deserves a DEPRECATED comment, due for
removal in 1.22.

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

https://codereview.appspot.com/85040046/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode756
state/apiserver/provisioner/provisioner_test.go:756: })
On 2014/04/09 04:52:18, wallyworld wrote:
> I think we should query state to see that the actual request was
properly
> fulfilled

I don't understand. What request? The placement is in state; how would
the API server have gotten the correct value if it didn't go to state?

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

https://codereview.appspot.com/85040046/diff/1/worker/provisioner/provisioner_task.go#newcode428
worker/provisioner/provisioner_task.go:428: machinePlacement, err :=
machine.Placement()
On 2014/04/09 04:52:18, wallyworld wrote:
> Not for this branch per se, but this is very chatty. There's now 3 api
calls to
> machine to get series, constraints and placement. And probably config.
It really
> needs to be munged into 1 api call.

Agreed.

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