Move assignment policy to global env config

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)

[wallyworld, 2013-05-28 08:09:41]

Please take a look.

[jameinel, 2013-05-28 11:37:20]

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.

[fwereade, 2013-05-28 12:16:48]

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

[wallyworld, 2013-05-29 03:57:11]

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#newco...
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#newco...
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
juju/conn.go:294: policy := conn.Environ.Config().DefaultAssignmentPolicy()
On 2013/05/28 12:16:48, fwereade wrote:
> 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.

The answer both reviewers' questions:

this is temporary until constraints are written. Because the policy is no longer
hard coded against the provider, I had to get it from somewhere else (for now).
Even with constraints, when they are written, I'd love to understand any
objections to allowing a default constraint value to be specified in env. If a
constraint is not specified, the value to use has to come from somewhere. I
thought environments.yaml was a reasonable choice since it allows flexibility
for dev and experimentation etc.

There appeared to be a number of common env attributes, applicable to all
providers, so I stuck this new one there too. Hence to conn.Environ.Config()
call to get these.

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:
On 2013/05/28 12:16:48, fwereade wrote:
> 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.

It definitely will be a constraint, but just not yet. This branch is merely a
step along the way. This branch removes the hard coded policy values from the
providers.

[fwereade, 2013-05-30 15:32:58]

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

The forces in play around constraints-in-env-config are that (1) we already
store them somewhere else in state and (2) there's already an uncomfortable
fusion of two ideas around env constraints: that they represent both the
constraints used for the bootstrap node *and* the fallback constraints for
individual services. If we make changes here, I want to be sure we don't muddy
the waters further (we can *change*, sure, so long as we communicate what and
why; but we can't just layer stuff on top of what we have without a very good
reason).

If we were to leave this alone, and implement it as a constraint with possible
values of (say):

new: always create a new top-level machine
existing: always stick it on an existing one
clean: put it on an existing machine that's never been used (fall back to new)
empty: put it on an existing machine that's not currently in use (fall back to
new)

...we'd be able to specify the env default directly via `bootstrap
--constraints` (or set-constraints); if the user didn't specify it there, we
could inject a suitable one from the provider. In the case where the constraint
was explicitly unset, I'd be fine with a global juju-level fallback of "new".

The only interesting question remains "is it sane to store the assignment-policy
constraint on a machine?" and I'd be willing to accept an answer of "no, but I
don't care: the environ won't pay attention to that anyway and it might be
interesting for debugging".

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/29 03:57:11, wallyworld wrote:
> There appeared to be a number of common env attributes, applicable to all
> providers, so I stuck this new one there too. Hence to conn.Environ.Config()
> call to get these. 

Nah, there's a subtlety: conn.Environ.Config() is arbitrarily out of date, but
conn.State.EnvironConfig() is current(-enough-to-be-reasonable); you should
never use conn.Environ.Config() when conn.State is available. (Yes, this is a
sociopathic API design.)

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:
On 2013/05/29 03:57:11, wallyworld wrote:
> On 2013/05/28 12:16:48, fwereade wrote:
> > 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.
> 
> It definitely will be a constraint, but just not yet. This branch is merely a
> step along the way. This branch removes the hard coded policy values from the
> providers.

As suggested elsewhere, how about straight-up trashing all that code and just
replacing this bit with "AssignToNewMachine()"?

Also... this isn't your problem, but... this method is kinda crazy. Does any
reader know why it's in state at all?

[wallyworld, 2013-05-31 04:42:46]

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.

[wallyworld, 2013-05-31 04:44:26]

Please take a look.

[fwereade, 2013-05-31 09:02:43]

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

LGTM trivial in its current form.

[jameinel, 2013-05-31 13:10:21]

LGTM

[jameinel, 2013-05-31 13:11:12]

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.

[wallyworld, 2013-06-10 10:42:49]

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