Implement AssignClean and AssignCleanEmpty

Implement AssignClean and AssignCleanEmpty

This branch removes the AssignUnused policy and instead provides
an implementation for AssignClean and AssignEmpty.
The tests were refactored so that all the existing AssignUnused tests
were added to a suite and run using the same code for the clean and
clean/empty cases.

https://code.launchpad.net/~wallyworld/juju-core/reuse-assign-policy/+merge/172452

(do not edit description out of merge proposal)

[wallyworld, 2013-07-02 01:07:38]

Please take a look.

[thumper, 2013-07-02 02:11:25]

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode481
state/assign_test.go:481: _, err := s.State.AddMachine("series",
state.JobManageEnviron) // bootstrap machine
the bootstrap machine has JobHostUnits now too

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode527
state/assign_test.go:527: } else {
You could drop the else clause, the scope and the panic.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode533
state/assign_test.go:533: func (s *assignCleanSuite) assertMachineEmpty(c *C,
machine *state.Machine, empty bool) {
I would so prefer:
  assertMachineEmpty
and
  assertMachineNotEmpty
rather than a bool flag.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode545
state/assign_test.go:545: _, err := s.State.AddMachine("series",
state.JobManageEnviron) // bootstrap machine
I do wonder if we should go and add some methods to the ConnSuite:

  AddMachine() // uses "series" and JobHostUnits
  AddBootstrapMachine()
  AddMachineWithSeries(series string)

etc.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode549
state/assign_test.go:549: service1, err := s.State.AddService("mysql",
s.AddTestingCharm(c, "mysql"))
Also testing methods for this...

  ConnSuite.AddService(name string)
    - the function does the hoop juping.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode578
state/assign_test.go:578: c.Assert(cleanEmptyMachine.Clean(), Equals, true)
IsTrue

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode619
state/assign_test.go:619: c.Assert(err, IsNil)
I'd just combine these lines...

  c.Assert(m.EnsureDead(), IsNil)
  c.Assert(m.Remove(), IsNil)

etc.

https://codereview.appspot.com/10854043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10854043/diff/1/state/unit.go#newcode873
state/unit.go:873: // This method does not take constraints into consideration
when choosing a
I think there is a little more to the machine 0.  We shouldn't ever operate on
zero itself, but JobHostEnviron.  These machines *shouldn't* get assigned units
that aren't in containers (unless someone "hulk smashes" it on with
--force-machine).  The same should be said for any machine that has a
JobHostEnviron job.

https://codereview.appspot.com/10854043/diff/1/state/unit.go#newcode926
state/unit.go:926: context += " machine"
two other questions here:
 * where would you hook in the constraint checking
 * how does this perform in parallel?  Do we need to care?

[wallyworld, 2013-07-02 05:30:48]

Please take a look.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode527
state/assign_test.go:527: } else {
On 2013/07/02 02:11:26, thumper wrote:
> You could drop the else clause, the scope and the panic.

Done.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode533
state/assign_test.go:533: func (s *assignCleanSuite) assertMachineEmpty(c *C,
machine *state.Machine, empty bool) {
On 2013/07/02 02:11:26, thumper wrote:
> I would so prefer:
>   assertMachineEmpty
> and
>   assertMachineNotEmpty
> rather than a bool flag.

Done.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode545
state/assign_test.go:545: _, err := s.State.AddMachine("series",
state.JobManageEnviron) // bootstrap machine
On 2013/07/02 02:11:26, thumper wrote:
> I do wonder if we should go and add some methods to the ConnSuite:
> 
>   AddMachine() // uses "series" and JobHostUnits
>   AddBootstrapMachine()
>   AddMachineWithSeries(series string)
> 
> etc.

Sounds like a plan. In a followup branch. Since other test suites can benefit.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode549
state/assign_test.go:549: service1, err := s.State.AddService("mysql",
s.AddTestingCharm(c, "mysql"))
On 2013/07/02 02:11:26, thumper wrote:
> Also testing methods for this...
> 
>   ConnSuite.AddService(name string)
>     - the function does the hoop juping.

Ditto

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode578
state/assign_test.go:578: c.Assert(cleanEmptyMachine.Clean(), Equals, true)
On 2013/07/02 02:11:26, thumper wrote:
> IsTrue

Done.

https://codereview.appspot.com/10854043/diff/1/state/assign_test.go#newcode619
state/assign_test.go:619: c.Assert(err, IsNil)
On 2013/07/02 02:11:26, thumper wrote:
> I'd just combine these lines...
> 
>   c.Assert(m.EnsureDead(), IsNil)
>   c.Assert(m.Remove(), IsNil)
> 
> etc.

Done.

https://codereview.appspot.com/10854043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10854043/diff/1/state/unit.go#newcode873
state/unit.go:873: // This method does not take constraints into consideration
when choosing a
On 2013/07/02 02:11:26, thumper wrote:
> I think there is a little more to the machine 0.  We shouldn't ever operate on
> zero itself, but JobHostEnviron.  These machines *shouldn't* get assigned
units
> that aren't in containers (unless someone "hulk smashes" it on with
> --force-machine).  The same should be said for any machine that has a
> JobHostEnviron job.

This is cut and paste. Referring to machine 0 as special is a legacy of some of
the original juju implementation decisions ie bootstrap node = machine 0. I can
tweak the comment.

https://codereview.appspot.com/10854043/diff/1/state/unit.go#newcode926
state/unit.go:926: context += " machine"
On 2013/07/02 02:11:26, thumper wrote:
> two other questions here:
>  * where would you hook in the constraint checking
>  * how does this perform in parallel?  Do we need to care?

Constraints checking will be in the next branch. I haven't got the specific
implementation details 00% sorted yet.
Mongo sucks because it doesn't support cross collection queries. So you are
forced to load in one collection, extract the ids, and do a second query. With
no transaction protection. There's potentially a microscopic window where a
container is added to a machine after the machinesWithContainers array is
populated. Perhaps I could rework the downstream logic to add an assert if the
machine is required to be empty. Would you be ok for this to be in a followup
branch so I can get this branch and the next one landed (no-one can use the
AssignClean and AssignCleanEmpty policies yet anyway)

[fwereade, 2013-07-03 10:43:56]

I think I'd rather see this as a single assignment policy for now... and I don't
think it's correct to useJobManageEnviron. JobHostUnits should control whether
we can start units; I guess we should have JobHostContainers to control whether
we run alternative provisioners.

Would it be wicked to avoid machine 0 by preemptively marking it dirty, rather
than redefining job meanings?

https://codereview.appspot.com/10854043/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10854043/diff/7001/state/unit.go#newcode914
state/unit.go:914: {"_id", D{{"$nin", machinesWithContainers}}},
Can't we just include this in the query?

sel := D{{"life", Alive}, ...}
if requireEmpty {
    sel = append(sel, D{{"$or", []D{
        {{"children", D{{"$size", 0}}}},
        {{"children", D{{"$exists", false}}}},
    }}})
}
query := u.st.machines.Find(sel)

[wallyworld, 2013-07-03 12:10:49]

If it were to be a single policy, which one? Clean or CleanEmpty? I think both
have reasons for existing and are useful to have. 

The tests which use JobManageEnviron when starting a "bootstrap" machine predate
this branch. I'd like to be able to land this and do any cleanup separately.
Preemptively making the bootstrap machine dirty is interesting. I think we need
a broader discussion outside the scope of this branch on how to manage the
bootstrap node moving forward.

https://codereview.appspot.com/10854043/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10854043/diff/7001/state/unit.go#newcode914
state/unit.go:914: {"_id", D{{"$nin", machinesWithContainers}}},
On 2013/07/03 10:43:57, fwereade wrote:
> Can't we just include this in the query?
> 
> sel := D{{"life", Alive}, ...}
> if requireEmpty {
>     sel = append(sel, D{{"$or", []D{
>         {{"children", D{{"$size", 0}}}},
>         {{"children", D{{"$exists", false}}}},
>     }}})
> }
> query := u.st.machines.Find(sel)

The children attribute lives on the containerRefs collection, not the machines
collection. And Mongo doesn't support cross collection queries/joins. So it
needs to be done in code in 2 steps sadly :-(

[thumper, 2013-07-08 02:40:52]

On 2013/07/03 12:10:49, wallyworld wrote:
> If it were to be a single policy, which one? Clean or CleanEmpty? I think both
> have reasons for existing and are useful to have. 
> 
> The tests which use JobManageEnviron when starting a "bootstrap" machine
predate
> this branch. I'd like to be able to land this and do any cleanup separately.
> Preemptively making the bootstrap machine dirty is interesting. I think we
need
> a broader discussion outside the scope of this branch on how to manage the
> bootstrap node moving forward.
> 
> https://codereview.appspot.com/10854043/diff/7001/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/10854043/diff/7001/state/unit.go#newcode914
> state/unit.go:914: {"_id", D{{"$nin", machinesWithContainers}}},
> On 2013/07/03 10:43:57, fwereade wrote:
> > Can't we just include this in the query?
> > 
> > sel := D{{"life", Alive}, ...}
> > if requireEmpty {
> >     sel = append(sel, D{{"$or", []D{
> >         {{"children", D{{"$size", 0}}}},
> >         {{"children", D{{"$exists", false}}}},
> >     }}})
> > }
> > query := u.st.machines.Find(sel)
> 
> The children attribute lives on the containerRefs collection, not the machines
> collection. And Mongo doesn't support cross collection queries/joins. So it
> needs to be done in code in 2 steps sadly :-(

Personally I see value for two different policies at this stage - LGTM and see

[dave_cheney.net, 2013-07-08 03:16:02]

LGTM. I have no position on the number of policies, that is between you and your
maker.

https://codereview.appspot.com/10854043/diff/7001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/10854043/diff/7001/state/assign_test.go#newcode12
state/assign_test.go:12: . "launchpad.net/juju-core/testing/checkers"
wasn't there some discussion about this style ? I thought they were going to be
imported as gc and jc

https://codereview.appspot.com/10854043/diff/7001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10854043/diff/7001/state/state.go#newcode1133
state/state.go:1133: panic(fmt.Errorf("unknown unit assignment policy: %q",
policy))
Why panic, you can return an error here