Introduce instance.Placement

Introduce instance.Placement

This change introduces a new Placement
structure in the instance package that
encapsulates machine-id, container, and
(new) environment-specific placement
directives.

The add-machine subcommand is modified
to pass this new Placement structure
through the AddMachine client API, which
calls a new state method AddMachineWithPlacement.

Machine-id and container placement do the
same thing as before, while new environment-
specific placement structures are stored in
state. A new state policy is added to allow
an Environ to validate placement.

The provisioner API and worker are updated
to pass Placement through to StartInstance.

Follow-ups to come:
 - update MAAS provider to pick maas-name out
   the Placement.
 - update ec2, OpenStack (and MAAS?) to pick
   out availbility-zone from Placement.
 - update add-unit and deploy subcommands to
   use Placement; this will involve modifications
   to state unit assignment to choose existing
   matching machines (e.g. for maas-name).

https://code.launchpad.net/~axwalk/juju-core/addmachine-placement/+merge/214761

(do not edit description out of merge proposal)

[axw, 2014-04-08 13:47:23]

Please take a look.

[wallyworld, 2014-04-09 04:52:18]

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#new...
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#new...
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#new...
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/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/85040046/diff/1/state/api/provisioner/provisio...
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/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/85040046/diff/1/state/apiserver/provisioner/pr...
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...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/85040046/diff/1/worker/provisioner/provisioner...
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.

[axw, 2014-04-09 05:20:22]

> 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#new...
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#new...
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#new...
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/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/85040046/diff/1/state/apiserver/provisioner/pr...
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...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/85040046/diff/1/worker/provisioner/provisioner...
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.

[jameinel, 2014-04-09 09:58:21]

I'm a little concerned that we're changing an API.

How will we handle trying to send Placement to and older server? (Remember that
newer client should play nice when connecting to older servers, just as newer
servers should play nice when older clients talk to them.)

[axw, 2014-04-11 08:42:28]

On 2014/04/09 09:58:21, jameinel wrote:
> I'm a little concerned that we're changing an API.
> 
> How will we handle trying to send Placement to and older server? (Remember
that
> newer client should play nice when connecting to older servers, just as newer
> servers should play nice when older clients talk to them.)

Yes, we will have to continue passing ContainerType/ParentId.

Actually we can't really reuse the API, because if we connect to an old API
server it'll just ignore the Placement field. I'll create a new API method.

[fwereade, 2014-04-11 12:27:11]

Coming along nicely -- let me know what you think about the following.

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

And the machine id encodes this info anyway, doesn't it

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
I'm not clear on how this is distinct from Prechecker -- isn't placement just
another StartInstance parameter?

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

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) {
Hmm. Problem coming: environments named after a container type will act in a
surprising way. Can you think of any way around this?

https://codereview.appspot.com/85040046/diff/1/instance/placement.go#newcode63
instance/placement.go:63: return &Placement{Scope: directive}, nil
I'm not following this case. `add-machine --to kvm`? I think we need a target
machine for this to make sense.

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

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
s/Parse/MustParse/

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#newc...
provider/azure/environ.go:1179: return fmt.Errorf("unknown placement directive:
%s", p)
Do we want a whole Placement here, or should we maybe just handle the directive
alone?

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#new...
provider/dummy/environs.go:548: case "valid":
"valid" scope would surely not be valid, unless the dummy environ's name were
itself "valid"? And I'm not sure that it's worth passing down a value that
always has to be a particular value... this STM to indicate that it's just
ValidatePlacement(string) -- or, better yet, we just add a `placement string` to
StartInstance and precheck that too.

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

https://codereview.appspot.com/85040046/diff/1/state/addmachine.go#oldcode238
state/addmachine.go:238: if err := st.precheckInstance(template.Series,
template.Constraints); err != nil {
Yeah, I think we should definitely just pass the placement directive in here.

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#newcode101
state/addmachine.go:101: return st.AddOneMachine(template)
Hmm, I sort of see why you went this way, but I'm not 100% sure that we
shouldn't have the Placement exported from MachineTemplate. How painful would it
be to rework the structure to accommodate that?

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#new...
state/api/params/params.go:107: ContainerType instance.ContainerType
On 2014/04/09 05:20:22, axw wrote:
> 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.

Except we actually have to carry it on LTS timescales.

https://codereview.appspot.com/85040046/diff/1/state/api/params/params.go#new...
state/api/params/params.go:705: // PlacementResult contains the result of the
Placement provisioner API call.
This is internal, so I'm less bothered, but for future reference: we really want
to be making few-but-detailed API calls. Ideally we'd get all the info necessary
to provision a machine in one shot, and then (as I think we do now?) report all
the information back to state in one shot.

And later we'd be able to make all these calls in bulk, too, but at least we're
generally writing things in such a way as to accommodate that :).

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

https://codereview.appspot.com/85040046/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:330: c.Assert(err, gc.ErrorMatches,
"machine 1 not found")
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)

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....
state/apiserver/client/client.go:630: return
c.api.state.AddMachineWithPlacement(template, placement)
Based on this, I reckon placement certainly ought to be part of MachineTemplate.

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

https://codereview.appspot.com/85040046/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1674: apiParams[2].Placement =
instance.MustParsePlacement("invalid:uhoh")
dummy:invalid

https://codereview.appspot.com/85040046/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1675: apiParams[3].Placement =
instance.MustParsePlacement("valid:hooray")
dummy:valid

?

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"`
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?)

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

https://codereview.appspot.com/85040046/diff/1/state/policy.go#newcode150
state/policy.go:150: func (st *State) validatePlacement(p *instance.Placement)
error {
In particular, grabbing and constructing and environ twice in a row, to
separately check constraints and placement, feels wrong. More db hits and
(probably) more provider API calls for essentially the same op.

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

https://codereview.appspot.com/85040046/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:428: machinePlacement, err :=
machine.Placement()
On 2014/04/09 05:20:22, axw wrote:
> 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.

Glad we're all aligned :).

[axw, 2014-04-17 06:13:46]

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#new...
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#newc...
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/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/85040046/diff/1/state/api/provisioner/provisio...
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....
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.

[fwereade, 2014-04-18 09:15:51]

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

https://codereview.appspot.com/85040046/diff/20001/environs/jujutest/livetest...
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...
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#n...
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#newcod...
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#newcod...
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...
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/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/cli...
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/provisi...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisi...
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.

[axw, 2014-04-22 07:35:29]

Please take a look.

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#new...
cmd/juju/addmachine.go:123: // message.
On 2014/04/18 09:15:52, fwereade wrote:
> 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...

Done. Sorry, misunderstood a previous comment about maintaining use of
ContainerType/ParentId. I now think you were referring to machine templates.

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...
instance/placement_test.go:16: var parsePlacementTests = []struct {
On 2014/04/18 09:15:52, fwereade wrote:
> 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
> }

Sure, I just copied the structure from instance_test.go.

Updated. I don't like munging the slice literal into the range, as it puts the
assigned variables too far away from the loop body.

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#n...
provider/maas/environ.go:173: // TODO(axw) handle maas-name placement directive
On 2014/04/18 09:15:52, fwereade wrote:
> deserves a bug#

Done.

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#newcod...
state/api/client.go:230: // This exists for backwards compatibility; remove in
1.21 (client only).
On 2014/04/18 09:15:52, fwereade wrote:
> 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

I'm talking about the client code only, and then only from 1.21 when we no
longer require new-client-to-1.18-server compat.

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go#newcod...
state/api/client.go:246: err := c.call("AddMachinesV2", args, results)
On 2014/04/18 09:15:52, fwereade wrote:
> grump, rage. jam, *please* get me some API versioning.

I didn't want to do this either.

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...
state/api/params/params.go:714: Results []PlacementResult
On 2014/04/18 09:15:52, fwereade wrote:
> 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.

Done.

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

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1702: apiParams[0].Placement =
instance.MustParsePlacement("lxc")
On 2014/04/18 09:15:52, fwereade wrote:
> 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.

I'm not a fan, because if we want to use non-numeric directives for containers
in the future, then we have some arbitrary string ("new") to work around. The
empty string is always going to be a special case, and I think makes sense in
this context (empty string == give me an LXC container on no specific machine).

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

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisi...
worker/provisioner/provisioner_task.go:467: machinePlacement, err :=
machine.Placement()
On 2014/04/18 09:15:52, fwereade wrote:
> 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.

I have created a new API method, ProvisioningInfo, and removed the old
Constraints and RequestedNetworks ones. Series is still in use in one other
place.

[axw, 2014-04-22 09:43:23]

Please take a look.

[axw, 2014-04-22 10:08:01]

Please take a look.

[wallyworld, 2014-04-23 04:47:59]

LGTM with the error tweak as discussed

[axw, 2014-04-23 05:55:32]

Please take a look.

[axw, 2014-04-23 06:43:39]

Please take a look.