Some initial thoughts added. I assume William is ok with the design?
Seems ok to me. 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?


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})
What about 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?
Can't we inspect the scope to see if it's a valid container type?

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"},
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?

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 {
I know we check for nil elsewhere, but maybe we should have it here as
well to be defensive and avoid a npe?

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

https://codereview.appspot.com/85040046/diff/1/state/addmachine.go#newcode85
state/addmachine.go:85: func (st *State)
AddMachineWithPlacement(template MachineTemplate, placement
*instance.Placement) (*Machine, error) {
We need a test for this method? I can see it's tested elsewhere outside
this package so maybe that's sufficient, but in general it's nice to
have at least one local test to cover all the corner cases, and the
external tests can be a little more general to test how things are wired
up.

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
So we are retaining ContainerType and ParentId but add-machine does not
set them anymore?

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")
We have started returning perm denied if a machine is not found.
We should have tests for genuine perm denied cases as well?

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: })
I think we should query state to see that the actual request was
properly fulfilled

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#newcode1036
state/machine.go:1036: func (m *Machine) Placement() *instance.Placement
{
We should have a test for this, I think there are tests for other
similar methods?

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

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