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

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