Merge lp://staging/~wallyworld/juju-core/move-assignment-policy into lp://staging/~juju/juju-core/trunk

Reviewers: mp+165983_code.launchpad.net,

Message:
Please take a look.

Description:
Move assignment policy to global env config

Previoysly, each provider defined it's own AssignmentPolicy,
which was hard coded to AssignNew. Now, it's been made an
env setting with default AssignNew. With containers, it will be
possible to add a unit to an existing machine (if not dirty) so
this work is a step in that direction.

https://code.launchpad.net/~wallyworld/juju-core/move-assignment-policy/+merge/165983

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9824043/

Affected files:
   A [revision details]
   M environs/config/config.go
   M environs/config/config_test.go
   M environs/dummy/environs.go
   M environs/ec2/ec2.go
   M environs/ec2/local_test.go
   M environs/interface.go
   M environs/maas/environ.go
   M environs/maas/environ_test.go
   M environs/openstack/local_test.go
   M environs/openstack/provider.go
   M juju/conn.go
   M state/assign_test.go
   M state/bench_test.go
   M state/state.go
   M state/unit.go

I agree with the principle that the assignment policy is not based on
the provider (ec2 vs openstack) but rather config (user preference).
I'm a little concerned if this is only loaded locally, but I think the
environments.yaml is serialized up to the mongodb, so my concerns may be
misplaced.

You have a TODO which you removed, though you didn't actually do what it
said. However it might be relevant.

Otherwise LGTM.

https://codereview.appspot.com/9824043/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/9824043/diff/1/juju/conn.go#newcode294
juju/conn.go:294: policy :=
conn.Environ.Config().DefaultAssignmentPolicy()
It seems you've moved the policy to environment config rather than
state. Or is this Environ object instantiated from the state?

The essential difference is whether you need to 'juju set' to get new
behavior, or whether just editing ~/.juju/environments.yaml will cause
the client to behave differently.

https://codereview.appspot.com/9824043/

NOT LGTM, we should have a talk about how we implement this. I presume
there's some plan to allow an alternative way of specifying policies for
services (or individual units? -1 on that) but I'm not sure that a new
path is going to be superior to a constraint approach.

Also, bad luck hitting the conn.Environ timebomb.

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode47
environs/config/config.go:47: AssignUnused AssignmentPolicy = "unused"
A thought: should this be "clean" rather than "unused"?

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode382
environs/config/config.go:382: "default-assignment-policy": AssignNew,
How is this "default"? Surely it's just assignment-policy, unless
there's an alternative means of specifying it?

Anyway, I'm really not sure it should go in the env config. How would
you feel about making it a constraint?

(this opens up the default-constraints-in-env-config question again, I
am aware, but it feels like the right time to have that discussion)

https://codereview.appspot.com/9824043/diff/1/environs/interface.go
File environs/interface.go (left):

https://codereview.appspot.com/9824043/diff/1/environs/interface.go#oldcode216
environs/interface.go:216: AssignmentPolicy() state.AssignmentPolicy
Nice to trim Environ a little.

https://codereview.appspot.com/9824043/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/9824043/diff/1/juju/conn.go#newcode294
juju/conn.go:294: policy :=
conn.Environ.Config().DefaultAssignmentPolicy()
On 2013/05/28 11:37:20, jameinel wrote:
> It seems you've moved the policy to environment config rather than
state. Or is
> this Environ object instantiated from the state?

> The essential difference is whether you need to 'juju set' to get new
behavior,
> or whether just editing ~/.juju/environments.yaml will cause the
client to
> behave differently.

This is definitely wrong.

/me feels a little bit smug that the parallel non-matching environ
configs are causing exactly the confusion he predicted. It's less
satisfying than one might imagine.

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

https://codereview.appspot.com/9824043/diff/1/state/state.go#newcode944
state/state.go:944: case config.AssignLocal:
This'll need some rearrangement if it's a constraint... but I haven't
yet seen anything to make me think it shouldn't be.

https://codereview.appspot.com/9824043/

Sorry for not being clear that the end goal is to use constraints for
the assignment policy. This was spelled out in Tim's pre-impl email.

The goal of this branch is to remove the hard coded policy values from
the providers. I envisioned that with constraints, there needs to be a
default value defined somewhere for when the constraint is not
specified. So I added that to the env, and use that to keep the system
working as before until constraints are written. And having it as an env
setting allows us as developers to actually use a different policy aside
from AssignNew so we can prototype and experiment etc.

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode47
environs/config/config.go:47: AssignUnused AssignmentPolicy = "unused"
On 2013/05/28 12:16:48, fwereade wrote:
> A thought: should this be "clean" rather than "unused"?

This code is not new - it is copied directly from the state package. It
is moved here to avoid circular reference issues. I guess "clean" may be
a better name for it now given the new work we are doing. I'll get a
consensus from others before I make any change.

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode382
environs/config/config.go:382: "default-assignment-policy": AssignNew,
On 2013/05/28 12:16:48, fwereade wrote:
> How is this "default"? Surely it's just assignment-policy, unless
there's an
> alternative means of specifying it?

> Anyway, I'm really not sure it should go in the env config. How would
you feel
> about making it a constraint?

> (this opens up the default-constraints-in-env-config question again, I
am aware,
> but it feels like the right time to have that discussion)

Perhaps my covering letter did not clearly enough described the reason
for this branch. The assignment policy *will* be a constraint when this
work is ultimately finished (this branch is an intermediate step).
However, there still needs to be a default behaviour for when the
constraint is not specified. Right now, each provider hard codes an
assignment policy. It shouldn't be done at the provider level. So what
we are doing here via the default-assignment-policy setting, is removing
the provider attribute and allowing the system to behave sensibly when
no constraint is specified (since that code is not written yet). I could
hard code juju to always use AssignNew until the constraints are done,
but why wouldn't we want to allow the user the option to specify a
default in the env? I wasn't aware this had been previously discussed.
With the code in this branch, the default-assignment-policy is totally
optional to include in the env; if it's not there, AssignNew will be
used. But, the user can change that to AssignUnused if they choose to. I
haven't included it in the boilerplate config, so one currently needs to
know what one is doing to mess with it, but it sure will make things
easier for dev and testing.

https://codereview.appspot.com/9824043/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/9824043/diff/1/juju/conn.go#newcode294
j...

A few more comments.

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode47
environs/config/config.go:47: AssignUnused AssignmentPolicy = "unused"
On 2013/05/29 03:57:11, wallyworld wrote:
> On 2013/05/28 12:16:48, fwereade wrote:
> > A thought: should this be "clean" rather than "unused"?

> This code is not new - it is copied directly from the state package.
It is moved
> here to avoid circular reference issues. I guess "clean" may be a
better name
> for it now given the new work we are doing. I'll get a consensus from
others
> before I make any change.

I'm getting a bit antsy about this actually. We need to think very
carefully about how the assignment-policy and container-kind (sp?)
constraints will interact... just reactivating "unused" (which is kinda
broken anyway, what with the way it ignores constraints) doesn't feel
smart.

Would you please sketch out and circulate an approximate plan for how
these things will work together before you go ahead with this branch?

(I wouldn't object to a much smaller branch that dropped
AssignmentPolicy *entirely* and hardcoded the AssignNew behaviour
directly into state -- I really don't think we'd lose much by doing so,
except bugs, and we'd gain a clean state to do something sensible -- but
I'm not mandating that; if you think there's value to be extracted from
the existing AssignUnused code I could accept that).

https://codereview.appspot.com/9824043/diff/1/environs/config/config.go#newcode382
environs/config/config.go:382: "default-assignment-policy": AssignNew,
On 2013/05/29 03:57:11, wallyworld wrote:
> On 2013/05/28 12:16:48, fwereade wrote:
> > How is this "default"? Surely it's just assignment-policy, unless
there's an
> > alternative means of specifying it?
> >
> > Anyway, I'm really not sure it should go in the env config. How
would you feel
> > about making it a constraint?
> >
> > (this opens up the default-constraints-in-env-config question again,
I am
> aware,
> > but it feels like the right time to have that discussion)

> Perhaps my covering letter did not clearly enough described the reason
for this
> branch. The assignment policy *will* be a constraint when this work is
> ultimately finished (this branch is an intermediate step). However,
there still
> needs to be a default behaviour for when the constraint is not
specified. Right
> now, each provider hard codes an assignment policy. It shouldn't be
done at the
> provider level. So what we are doing here via the
default-assignment-policy
> setting, is removing the provider attribute and allowing the system to
behave
> sensibly when no constraint is specified (since that code is not
written yet). I
> could hard code juju to always use AssignNew until the constraints are
done, but
> why wouldn't we want to allow the user the option to specify a default
in the
> env? I wasn't aware this had been previously discussed. With the code
in this
> branch, the default-assignment-policy is totally optional to include
in the env;
> if it's not there, AssignNew will be used. But, the user can change
that...

I've removed all the code that put assignment policy in the environment
config. All this branch does now is get rid of the AssignmentPolicy()
methods on each provider and use a hard coded value (AssignNew). The
next branch will start implementing a constraints based approach.

https://codereview.appspot.com/9824043/

Please take a look.

https://codereview.appspot.com/9824043/

On 2013/05/31 04:44:26, wallyworld wrote:
> Please take a look.

LGTM trivial in its current form.

https://codereview.appspot.com/9824043/

LGTM

https://codereview.appspot.com/9824043/

On 2013/05/31 13:10:21, jameinel wrote:
> LGTM

Just make sure to update the cover description since the "Move
assignment policy to global env config" doesn't match anymore.

https://codereview.appspot.com/9824043/

*** Submitted:

Move assignment policy to global env config

Previoysly, each provider defined it's own AssignmentPolicy,
which was hard coded to AssignNew. Now, it's been made an
env setting with default AssignNew. With containers, it will be
possible to add a unit to an existing machine (if not dirty) so
this work is a step in that direction.

R=jameinel, fwereade
CC=
https://codereview.appspot.com/9824043

https://codereview.appspot.com/9824043/