New assignment policy is AssignCleanEmpty

New assignment policy is AssignCleanEmpty

Add container support to AssignClean(Empty) policies and make
the default policy AssignCleanEmpty. This allows manual deployment
of charms into existing clean and empty containers/instances, with
the fallback that a new container/instance is created if none are 
available. The system behaves the same as previously if a simple
bootstrap and deploy is performed, but if add-machine is used to
create some containers/instances, then those are no longer ignored.

https://code.launchpad.net/~wallyworld/juju-core/clean-policy-container/+merge/173633

(do not edit description out of merge proposal)

[wallyworld, 2013-07-09 02:56:45]

Please take a look.

[wallyworld, 2013-07-11 02:26:55]

Please take a look.

[fwereade, 2013-07-11 03:44:37]

There's an awful lot of container-specificity here, and they're apparently being
paid attention to *instead of* the other constraints. I really don't think it's
ok to privilege them like this even if they do already exist.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode924
state/unit.go:924: }
This feels like the wrong level to do this unpacking. We pass cons down to
assignToNewMachine... if it's paying attention to constraints, can't it do this
bit itself? Having two ways to specify container type in the same params
confuses me quite hard.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode979
state/unit.go:979: }
Why are we still doing this? Sorry to make you repeat yourself, I just can't
remember which CL I suggested an alternative in and therefore don't know why it
didn't happen.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode1016
state/unit.go:1016: // Get the service constraints to see if the charm is to be
deployed into a container.
unit constraints :)

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode1026
state/unit.go:1026: query, err := u.findCleanMachineQuery(requireEmpty,
containerType)
I really don't think it's ok for us to start paying attention *only* to the
container constraint here. If the container constraint were one of many I would
feel more comfortable overlooking it, but it's feeling really pretty central to
this CL. Why don't we pass all the constraints down?

[wallyworld, 2013-07-11 04:05:34]

Container constraints are the only ones supported in the mp. The others (mem
etc) are coming real soon now.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode924
state/unit.go:924: }
On 2013/07/11 03:44:37, fwereade wrote:
> This feels like the wrong level to do this unpacking. We pass cons down to
> assignToNewMachine... if it's paying attention to constraints, can't it do
this
> bit itself? Having two ways to specify container type in the same params
> confuses me quite hard.

The container in constraints can be "" (don't care), lxc etc (use this one), or
none (must use a machine). The container in the params is the actual container
to use when creating a new machine. The semantics are quite different.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode979
state/unit.go:979: }
On 2013/07/11 03:44:37, fwereade wrote:
> Why are we still doing this? Sorry to make you repeat yourself, I just can't
> remember which CL I suggested an alternative in and therefore don't know why
it
> didn't happen.

The data is stored in 2 documents and mongo doesn't support cross document
queries.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode1016
state/unit.go:1016: // Get the service constraints to see if the charm is to be
deployed into a container.
On 2013/07/11 03:44:37, fwereade wrote:
> unit constraints :)

Will fix.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode1026
state/unit.go:1026: query, err := u.findCleanMachineQuery(requireEmpty,
containerType)
On 2013/07/11 03:44:37, fwereade wrote:
> I really don't think it's ok for us to start paying attention *only* to the
> container constraint here. If the container constraint were one of many I
would
> feel more comfortable overlooking it, but it's feeling really pretty central
to
> this CL. Why don't we pass all the constraints down?

I could pass them all down. Right now, only container is supported. The rest
will be supported real soon now.

[fwereade, 2013-07-11 04:19:49]

OK, LGTM, so long as we're dealing with *constraints* and the use of the
container stuff is just because it's easiest to integrate and doesn't depend on
possibly-missing instance info.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode979
state/unit.go:979: }
On 2013/07/11 04:05:34, wallyworld wrote:
> On 2013/07/11 03:44:37, fwereade wrote:
> > Why are we still doing this? Sorry to make you repeat yourself, I just can't
> > remember which CL I suggested an alternative in and therefore don't know why
> it
> > didn't happen.
> 
> The data is stored in 2 documents and mongo doesn't support cross document
> queries.

Oh, hell, thanks.

https://codereview.appspot.com/11019044/diff/4001/state/unit.go#newcode1026
state/unit.go:1026: query, err := u.findCleanMachineQuery(requireEmpty,
containerType)
On 2013/07/11 04:05:34, wallyworld wrote:
> On 2013/07/11 03:44:37, fwereade wrote:
> > I really don't think it's ok for us to start paying attention *only* to the
> > container constraint here. If the container constraint were one of many I
> would
> > feel more comfortable overlooking it, but it's feeling really pretty central
> to
> > this CL. Why don't we pass all the constraints down?
> 
> I could pass them all down. Right now, only container is supported. The rest
> will be supported real soon now.

That would make me a lot more comfortable, because at least then we're building
it for the cases we know we'll need. I just don't want to make container
constraints the driving force for anything until we know for sure that they're
wanted.

[wallyworld, 2013-07-11 05:05:38]

I've addressed the issues raised.

[wallyworld, 2013-07-11 05:06:55]

Please take a look.

[dimitern, 2013-07-11 20:25:51]

LGTM with a few suggestions.

https://codereview.appspot.com/11019044/diff/11001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/11019044/diff/11001/juju/conn.go#newcode301
juju/conn.go:301: // Hard code for now till we implement a different approach.
clarification?

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:724: // Add a state management machine which can host jobs
and check it is not chosen.
s/jobs/units/

Also, JobManageEnviron does not imply state management (just it runs a
provisioner, etc.), JobManageState does that (running API server and mongo).

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:729: c.Assert(err, ErrorMatches, `all eligible machines in
use`)
s/`/"/

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:752: c.Assert(err, ErrorMatches, s.errorMessage(`cannot
assign unit "wordpress/0" to %s machine.*: unit not found`))
"to %s machine"? how's that working? isn't it supposed to be replaced by some
string?

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:880: c.Assert(hostMachine.Clean(), IsTrue)
wow, i didn't know there's a IsTrue checker!

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:899: unit, err := s.wordpress.AddUnit()
please pull this block out in a helper func with appropriate args, it awfully
twisted to read all these 2*(i+1)+3 magics. Thus, you can even get rid of the
loop and just call it twice.

https://codereview.appspot.com/11019044/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode802
state/unit.go:802: func (u *Unit) assignToNewMachine(params *AddMachineParams,
cons constraints.Value) (err error) {
why do you need a named return arg?

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode878
state/unit.go:878: if cons.Container == nil || *cons.Container == "" ||
*cons.Container == instance.NONE {
there should be a helper for this check; it seems we might need it in a few
places.

noContainer(cons) bool

(or maybe as a method on constrains.Value?)

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode895
state/unit.go:895: return u.AssignToNewMachine()
could there be a race here? like another unit agent jumping in and taking your
newly create container away to assign another unit to it?

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode925
state/unit.go:925: if cons.Container != nil && *cons.Container != "" &&
*cons.Container != instance.NONE {
...like here (noContainer, see above).

[wallyworld, 2013-07-12 02:27:28]

Please take a look.

https://codereview.appspot.com/11019044/diff/11001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/11019044/diff/11001/juju/conn.go#newcode301
juju/conn.go:301: // Hard code for now till we implement a different approach.
On 2013/07/11 20:25:51, dimitern wrote:
> clarification?

The final approach to use needs to be decided. It may be constraints based, it
may not.

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:724: // Add a state management machine which can host jobs
and check it is not chosen.
On 2013/07/11 20:25:51, dimitern wrote:
> s/jobs/units/
> 
> Also, JobManageEnviron does not imply state management (just it runs a
> provisioner, etc.), JobManageState does that (running API server and mongo).

Done.

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:729: c.Assert(err, ErrorMatches, `all eligible machines in
use`)
On 2013/07/11 20:25:51, dimitern wrote:
> s/`/"/

Done.

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:752: c.Assert(err, ErrorMatches, s.errorMessage(`cannot
assign unit "wordpress/0" to %s machine.*: unit not found`))
On 2013/07/11 20:25:51, dimitern wrote:
> "to %s machine"? how's that working? isn't it supposed to be replaced by some
> string?

s.errorMessage() fills in the context

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:880: c.Assert(hostMachine.Clean(), IsTrue)
On 2013/07/11 20:25:51, dimitern wrote:
> wow, i didn't know there's a IsTrue checker!

Recent addition - Tim did it.

https://codereview.appspot.com/11019044/diff/11001/state/assign_test.go#newco...
state/assign_test.go:899: unit, err := s.wordpress.AddUnit()
On 2013/07/11 20:25:51, dimitern wrote:
> please pull this block out in a helper func with appropriate args, it awfully
> twisted to read all these 2*(i+1)+3 magics. Thus, you can even get rid of the
> loop and just call it twice.

Done.

https://codereview.appspot.com/11019044/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode802
state/unit.go:802: func (u *Unit) assignToNewMachine(params *AddMachineParams,
cons constraints.Value) (err error) {
On 2013/07/11 20:25:51, dimitern wrote:
> why do you need a named return arg?

Just following what the other exiting methods on this file do (ie consistency)

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode878
state/unit.go:878: if cons.Container == nil || *cons.Container == "" ||
*cons.Container == instance.NONE {
On 2013/07/11 20:25:51, dimitern wrote:
> there should be a helper for this check; it seems we might need it in a few
> places.
> 
> noContainer(cons) bool
> 
> (or maybe as a method on constrains.Value?)

Done.

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode895
state/unit.go:895: return u.AssignToNewMachine()
On 2013/07/11 20:25:51, dimitern wrote:
> could there be a race here? like another unit agent jumping in and taking your
> newly create container away to assign another unit to it?
Yes, I was going to address this in a follow up branch but I've added the logic
to this branch.

https://codereview.appspot.com/11019044/diff/11001/state/unit.go#newcode925
state/unit.go:925: if cons.Container != nil && *cons.Container != "" &&
*cons.Container != instance.NONE {
On 2013/07/11 20:25:51, dimitern wrote:
> ...like here (noContainer, see above).

Done.

[wallyworld, 2013-07-12 03:42:20]

Please take a look.

[dimitern, 2013-07-12 07:22:02]

Thanks, just a fix a small typo.

https://codereview.appspot.com/11019044/diff/20001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/11019044/diff/20001/constraints/constraints.go...
constraints/constraints.go:218: // HasContainer returns true is the
constraints.Value specifies a container.
s/is/if/