Add dirty flag to machine state

Add dirty flag to machine state

When choosing an unused machine to assign a unit to, a check was being made
to see if there were any principal units for the machine. If machines had units
assigned and then unassigned, the machine would appear clean since number of
principal unit goes to 0 but the machine should still be considered dirty.

The machine state has a "clean" flag (boolean). Machines are created with the 
clean flag set to true. It is set false when a machine has a unit assigned, 
and not reset when units are unassigned. The assign to unused machine logic 
is updated to look at the clean flag.

https://code.launchpad.net/~wallyworld/juju-core/add-machine-dirty-flag/+merge/165954

(do not edit description out of merge proposal)

[wallyworld, 2013-05-28 02:52:33]

Please take a look.

[fwereade, 2013-05-28 11:59:40]

NOT LGTM as it is; we need to talk about advanceLifecycle.

https://codereview.appspot.com/9820043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode224
state/machine.go:224: // changed from underneath us by another process.
FWIW, this is not normal practice, and I would prefer to remain consistent; see
doc/hacking-state.txt for more stuff that might be relevant when working in this
package.

I don't understand your concerns about the retries -- we should talk about this
live.

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode524
state/machine.go:524: func (m *Machine) SetClean(isClean bool) (err error) {
I'm not sure this operation is sensible -- what's the use case for pretending
that a dirty machine is clean again?

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

https://codereview.appspot.com/9820043/diff/1/state/unit.go#newcode745
state/unit.go:745: err = m.Refresh()
Please don't refresh objects that other things might have references to. I'm +1
on setting the fields we know to have changed, but not really happy to pay
additional roundtrips that weren't requested.

https://codereview.appspot.com/9820043/diff/1/state/unit.go#newcode845
state/unit.go:845: // is clean if it has never had any principal units assigned
to it.
This is an incompatible change; machines that once had units assigned but don't
any more won't be marked as dirty. Can't see a clear way around this without
major-version-upgrade work, which we'll need for this containerization stuff
regardless. Thoughts?

[wallyworld, 2013-05-29 04:19:37]

https://codereview.appspot.com/9820043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode224
state/machine.go:224: // changed from underneath us by another process.
On 2013/05/28 11:59:40, fwereade wrote:
> FWIW, this is not normal practice, and I would prefer to remain consistent;
see
> doc/hacking-state.txt for more stuff that might be relevant when working in
this
> package.
> 
> I don't understand your concerns about the retries -- we should talk about
this
> live.

The specific issue relates to this bit of the code:

if len(m.doc.Principals) != 0 {
	return &HasAssignedUnitsError{
		MachineId: m.doc.Id,
		UnitNames: m.doc.Principals,
	}
}

If something unassigns all units from a machine, this in-memory object doesn't
get updated with that info and the above causes a HasAssignedUnitsError to break
some tests. Adding the Refresh() fixed it. I figured instead of up to 3 tries at
doing the work, the cost of a simple Refresh() up front to guarantee only a
single attempt is required would be minimal overhead. I'll take another look at
it.

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode524
state/machine.go:524: func (m *Machine) SetClean(isClean bool) (err error) {
On 2013/05/28 11:59:40, fwereade wrote:
> I'm not sure this operation is sensible -- what's the use case for pretending
> that a dirty machine is clean again?

When a machine has containers added to it, if those containers are all removed,
the machine becomes clean again. Since containers are separate domain entities,
I thought we would need an API for the business logic to use when required.
Perhaps it's just sufficient to update the machine's state in the db? But then
the dirty attribute of the in-memory object would need to be changed too and so
this method does that all-in-one.

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

https://codereview.appspot.com/9820043/diff/1/state/unit.go#newcode745
state/unit.go:745: err = m.Refresh()
On 2013/05/28 11:59:40, fwereade wrote:
> Please don't refresh objects that other things might have references to. I'm
+1
> on setting the fields we know to have changed, but not really happy to pay
> additional roundtrips that weren't requested.

ok

https://codereview.appspot.com/9820043/diff/1/state/unit.go#newcode845
state/unit.go:845: // is clean if it has never had any principal units assigned
to it.
On 2013/05/28 11:59:40, fwereade wrote:
> This is an incompatible change; machines that once had units assigned but
don't
> any more won't be marked as dirty. Can't see a clear way around this without
> major-version-upgrade work, which we'll need for this containerization stuff
> regardless. Thoughts?

Good point. We could saw the logic so that the new attribute is "clean" which
defaults to false. ie pessimistically assume all machines are dirty unless
explicitly told otherwise. I think I was going to do it this way for the reason
you mention but somehow made the new attribute be "dirty" instead. Not sure why
now.

[fwereade, 2013-05-30 15:01:06]

A few responses:

https://codereview.appspot.com/9820043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode224
state/machine.go:224: // changed from underneath us by another process.
On 2013/05/29 04:19:38, wallyworld wrote:
> On 2013/05/28 11:59:40, fwereade wrote:
> > FWIW, this is not normal practice, and I would prefer to remain consistent;
> see
> > doc/hacking-state.txt for more stuff that might be relevant when working in
> this
> > package.
> > 
> > I don't understand your concerns about the retries -- we should talk about
> this
> > live.
> 
> The specific issue relates to this bit of the code:
> 
> if len(m.doc.Principals) != 0 {
> 	return &HasAssignedUnitsError{
> 		MachineId: m.doc.Id,
> 		UnitNames: m.doc.Principals,
> 	}
> }
> 
> If something unassigns all units from a machine, this in-memory object doesn't
> get updated with that info and the above causes a HasAssignedUnitsError to
break
> some tests. Adding the Refresh() fixed it. I figured instead of up to 3 tries
at
> doing the work, the cost of a simple Refresh() up front to guarantee only a
> single attempt is required would be minimal overhead. I'll take another look
at
> it.

I'd prefer to maintain the optimistic style, really, given that in most cases
the machine actually *will* be fresh (but it's never guaranteed to be fresh, and
I don't want to leak whoops-you'd-better-refresh errors out of state except in
the most trying circumstances -- that's what ErrExcessiveContention is for).

That bit of code is, I think, correct: it signifies that the latest known
in-memory state indicates that it's not worth even trying to advance lifecycle.
If latest in-memory state indicates that it's reasonable to try, we run a
transaction; if that fails, *then* we refresh state to find out what we were
wrong about.

I hadn't spotted that you correctly avoid modifying the receiver, though, thanks
for preserving that and sorry I accused of not doing so ;p.

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode524
state/machine.go:524: func (m *Machine) SetClean(isClean bool) (err error) {
On 2013/05/29 04:19:38, wallyworld wrote:
> On 2013/05/28 11:59:40, fwereade wrote:
> > I'm not sure this operation is sensible -- what's the use case for
pretending
> > that a dirty machine is clean again?
> 
> When a machine has containers added to it, if those containers are all
removed,
> the machine becomes clean again. Since containers are separate domain
entities,
> I thought we would need an API for the business logic to use when required.
> Perhaps it's just sufficient to update the machine's state in the db? But then
> the dirty attribute of the in-memory object would need to be changed too and
so
> this method does that all-in-one. 

The dirty field *must* be modified in the same transaction that causes the
machine to become dirty... or, if we're just removing the final container, the
one that makes it become clean again.

But, hmm, unit-dirty and container-dirty are (I think) separate concepts. A
machine that has ever had a unit assigned directly is surely dirty forever; a
machine that just has a *container* assigned can become clean again (and should
still be available for further container assignments, even if not for unit
assignments). And in that case we need to track those two states separately.
Right?

(side note: I suspect there will be use cases that involve uncontained units
alongside contained ones... maybe we should kick that back with "--force-machine
if you must, but that's not something we want to support directly")

Regardless, all this method does is provide an opportunity for fools and enemies
to mess with db state and break expected guarantees... if we later decide we
want to explicitly mark a machine clean, we can do that when the time comes, but
I don't see an immediate need.

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

https://codereview.appspot.com/9820043/diff/1/state/unit.go#newcode845
state/unit.go:845: // is clean if it has never had any principal units assigned
to it.
On 2013/05/29 04:19:38, wallyworld wrote:
> On 2013/05/28 11:59:40, fwereade wrote:
> > This is an incompatible change; machines that once had units assigned but
> don't
> > any more won't be marked as dirty. Can't see a clear way around this without
> > major-version-upgrade work, which we'll need for this containerization stuff
> > regardless. Thoughts?
> 
> Good point. We could saw the logic so that the new attribute is "clean" which
> defaults to false. ie pessimistically assume all machines are dirty unless
> explicitly told otherwise. I think I was going to do it this way for the
reason
> you mention but somehow made the new attribute be "dirty" instead. Not sure
why
> now.

+1, thanks.

[wallyworld, 2013-05-31 04:13:53]

Please take a look.

https://codereview.appspot.com/9820043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode224
state/machine.go:224: // changed from underneath us by another process.
On 2013/05/30 15:01:07, fwereade wrote:
> On 2013/05/29 04:19:38, wallyworld wrote:
> > On 2013/05/28 11:59:40, fwereade wrote:
> > > FWIW, this is not normal practice, and I would prefer to remain
consistent;
> > see
> > > doc/hacking-state.txt for more stuff that might be relevant when working
in
> > this
> > > package.
> > > 
> > > I don't understand your concerns about the retries -- we should talk about
> > this
> > > live.
> > 
> > The specific issue relates to this bit of the code:
> > 
> > if len(m.doc.Principals) != 0 {
> > 	return &HasAssignedUnitsError{
> > 		MachineId: m.doc.Id,
> > 		UnitNames: m.doc.Principals,
> > 	}
> > }
> > 
> > If something unassigns all units from a machine, this in-memory object
doesn't
> > get updated with that info and the above causes a HasAssignedUnitsError to
> break
> > some tests. Adding the Refresh() fixed it. I figured instead of up to 3
tries
> at
> > doing the work, the cost of a simple Refresh() up front to guarantee only a
> > single attempt is required would be minimal overhead. I'll take another look
> at
> > it.
> 
> I'd prefer to maintain the optimistic style, really, given that in most cases
> the machine actually *will* be fresh (but it's never guaranteed to be fresh,
and
> I don't want to leak whoops-you'd-better-refresh errors out of state except in
> the most trying circumstances -- that's what ErrExcessiveContention is for).
> 
> That bit of code is, I think, correct: it signifies that the latest known
> in-memory state indicates that it's not worth even trying to advance
lifecycle.
> If latest in-memory state indicates that it's reasonable to try, we run a
> transaction; if that fails, *then* we refresh state to find out what we were
> wrong about.
> 
> I hadn't spotted that you correctly avoid modifying the receiver, though,
thanks
> for preserving that and sorry I accused of not doing so ;p.

Done.

https://codereview.appspot.com/9820043/diff/1/state/machine.go#newcode524
state/machine.go:524: func (m *Machine) SetClean(isClean bool) (err error) {
On 2013/05/30 15:01:07, fwereade wrote:
> On 2013/05/29 04:19:38, wallyworld wrote:
> > On 2013/05/28 11:59:40, fwereade wrote:
> > > I'm not sure this operation is sensible -- what's the use case for
> pretending
> > > that a dirty machine is clean again?
> > 
> > When a machine has containers added to it, if those containers are all
> removed,
> > the machine becomes clean again. Since containers are separate domain
> entities,
> > I thought we would need an API for the business logic to use when required.
> > Perhaps it's just sufficient to update the machine's state in the db? But
then
> > the dirty attribute of the in-memory object would need to be changed too and
> so
> > this method does that all-in-one. 
> 
> The dirty field *must* be modified in the same transaction that causes the
> machine to become dirty... or, if we're just removing the final container, the
> one that makes it become clean again.
> 
> But, hmm, unit-dirty and container-dirty are (I think) separate concepts. A
> machine that has ever had a unit assigned directly is surely dirty forever; a
> machine that just has a *container* assigned can become clean again (and
should
> still be available for further container assignments, even if not for unit
> assignments). And in that case we need to track those two states separately.
> Right?
> 
> (side note: I suspect there will be use cases that involve uncontained units
> alongside contained ones... maybe we should kick that back with
"--force-machine
> if you must, but that's not something we want to support directly")
> 
> Regardless, all this method does is provide an opportunity for fools and
enemies
> to mess with db state and break expected guarantees... if we later decide we
> want to explicitly mark a machine clean, we can do that when the time comes,
but
> I don't see an immediate need. 

Ok, it's gone.

[fwereade, 2013-06-04 11:29:48]

LGTM as far as it goes; I would also support a rename from "unused" to "clean";
but I will not be entirely comfortable reactivating this code without taking
constraints into consideration.

[thumper, 2013-06-04 22:38:42]

Just a query about running this with existing state that doesn't have the Clean
flag.

https://codereview.appspot.com/9820043/diff/8001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/9820043/diff/8001/state/machine.go#newcode66
state/machine.go:66: Clean        bool
How does this work with existing state documents? Do we need schema changes to
say it is optional with a default?

[wallyworld, 2013-06-05 00:19:43]

On 2013/06/04 22:38:42, thumper wrote:
> Just a query about running this with existing state that doesn't have the
Clean
> flag.
> 
> https://codereview.appspot.com/9820043/diff/8001/state/machine.go
> File state/machine.go (right):
> 
> https://codereview.appspot.com/9820043/diff/8001/state/machine.go#newcode66
> state/machine.go:66: Clean        bool
> How does this work with existing state documents? Do we need schema changes to
> say it is optional with a default?

The reason why I made the flag "Clean" is that for existing schemas it's value
will be false, which is what we want - assume existing machines are dirty.

[thumper, 2013-06-07 00:54:45]

On 2013/06/05 00:19:43, wallyworld wrote:
> On 2013/06/04 22:38:42, thumper wrote:
> > Just a query about running this with existing state that doesn't have the
> Clean
> > flag.
> > 
> > https://codereview.appspot.com/9820043/diff/8001/state/machine.go
> > File state/machine.go (right):
> > 
> > https://codereview.appspot.com/9820043/diff/8001/state/machine.go#newcode66
> > state/machine.go:66: Clean        bool
> > How does this work with existing state documents? Do we need schema changes
to
> > say it is optional with a default?
> 
> The reason why I made the flag "Clean" is that for existing schemas it's value
> will be false, which is what we want - assume existing machines are dirty.

LGTM

[wallyworld, 2013-06-10 10:35:15]

*** Submitted:

Add dirty flag to machine state

When choosing an unused machine to assign a unit to, a check was being made
to see if there were any principal units for the machine. If machines had units
assigned and then unassigned, the machine would appear clean since number of
principal unit goes to 0 but the machine should still be considered dirty.

The machine state has a "clean" flag (boolean). Machines are created with the 
clean flag set to true. It is set false when a machine has a unit assigned, 
and not reset when units are unassigned. The assign to unused machine logic 
is updated to look at the clean flag.

R=fwereade, thumper
CC=
https://codereview.appspot.com/9820043