Providers are now directly responsible for their own constraints

Providers are now directly responsible for their own constraints

This is really just a rearrangement of code; nothing fundamental has
actually changed, except that Constraints.from_strs is entirely gone; and
trying to construct Constraints(None, data) will no longer produce a viable
specimen.

https://code.launchpad.net/~fwereade/juju/actual-provider-constraints/+merge/99655

Requires: https://code.launchpad.net/~fwereade/juju/retire-direct-constraints-creation/+merge/99651

(do not edit description out of merge proposal)

[fwereade, 2012-03-28 02:05:37]

Please take a look.

[fwereade, 2012-03-28 08:44:26]

Please take a look.

[fwereade, 2012-03-28 09:15:33]

Please take a look.

[hazmat, 2012-03-28 17:29:04]

LGTM, thanks for working on this.

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#...
juju/machine/constraints.py:170: if data[name] is None:
Doesn't this value need to be kept to properly compose constraints?

ie say we had env constraints with an instance-type=m1.large, and cli
constraints for the service with instance-type=any... we'll end up using the env
constraints, i guess i can see reasoning both ways, but any at that point means
use default.. which doesn't sound quite right.

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py
File juju/providers/ec2/utils.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py#...
juju/providers/ec2/utils.py:57: if s not in _PLAUSIBLE_ZONES:
nitpick, any reason to just not lower the string here, ie. be accepting of
nominally valid inputs.

[fwereade, 2012-03-28 17:58:11]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#...
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 17:29:04, hazmat wrote:
> Doesn't this value need to be kept to properly compose constraints?
> 
> ie say we had env constraints with an instance-type=m1.large, and cli
> constraints for the service with instance-type=any... we'll end up using the
env
> constraints, i guess i can see reasoning both ways, but any at that point
means
> use default.. which doesn't sound quite right.

I think you've misunderstood (...or maybe I have ;)).

instance-type=m1.large overwritten with instance-type=any will actually give you
an amd64 t1.micro; the cpu/mem constraints at the m1.large level are cleared by
dint of being next to an explicitly-set m1.large, and then the m1.large is also
cleared out by the instance-type=any.

The kicker is cpu=10 mem=8G overwritten with instance-type=any, which IMO should
absolutely retain the cpu=10 mem=8G (and won't, if the later
instance-type-unsetting also unsets everything it conflicts with -- which is
what this change fixes).

Incidentally, "any" only means "use default" when the default is None; "" always
means "use default".

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py
File juju/providers/ec2/utils.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py#...
juju/providers/ec2/utils.py:57: if s not in _PLAUSIBLE_ZONES:
On 2012/03/28 17:29:04, hazmat wrote:
> nitpick, any reason to just not lower the string here, ie. be accepting of
> nominally valid inputs. 

Eh, why not :). Done.

[hazmat, 2012-03-28 18:07:39]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#...
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 17:58:11, fwereade wrote:
> On 2012/03/28 17:29:04, hazmat wrote:
> > Doesn't this value need to be kept to properly compose constraints?
> > 
> > ie say we had env constraints with an instance-type=m1.large, and cli
> > constraints for the service with instance-type=any... we'll end up using the
> env
> > constraints, i guess i can see reasoning both ways, but any at that point
> means
> > use default.. which doesn't sound quite right.
> 
> I think you've misunderstood (...or maybe I have ;)).
> 
> instance-type=m1.large overwritten with instance-type=any will actually give
you
> an amd64 t1.micro; the cpu/mem constraints at the m1.large level are cleared
by
> dint of being next to an explicitly-set m1.large, and then the m1.large is
also
> cleared out by the instance-type=any.
> 
> The kicker is cpu=10 mem=8G overwritten with instance-type=any, which IMO
should
> absolutely retain the cpu=10 mem=8G (and won't, if the later
> instance-type-unsetting also unsets everything it conflicts with -- which is
> what this change fixes).
> 
> Incidentally, "any" only means "use default" when the default is None; ""
always
> means "use default".

if the any value isn't stored then when composing constraints from different
levels, there isn't any distinguishing between something which was set and
something that was set to 'any'. granted we don't compose multi-level
constraints right now, but given env constraints, this would need changing to
keep the recorded value for comparision afaics.

[fwereade, 2012-03-28 18:16:03]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#...
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 18:07:39, hazmat wrote:
> On 2012/03/28 17:58:11, fwereade wrote:
> > On 2012/03/28 17:29:04, hazmat wrote:
> > > Doesn't this value need to be kept to properly compose constraints?
> > > 
> > > ie say we had env constraints with an instance-type=m1.large, and cli
> > > constraints for the service with instance-type=any... we'll end up using
the
> > env
> > > constraints, i guess i can see reasoning both ways, but any at that point
> > means
> > > use default.. which doesn't sound quite right.
> > 
> > I think you've misunderstood (...or maybe I have ;)).
> > 
> > instance-type=m1.large overwritten with instance-type=any will actually give
> you
> > an amd64 t1.micro; the cpu/mem constraints at the m1.large level are cleared
> by
> > dint of being next to an explicitly-set m1.large, and then the m1.large is
> also
> > cleared out by the instance-type=any.
> > 
> > The kicker is cpu=10 mem=8G overwritten with instance-type=any, which IMO
> should
> > absolutely retain the cpu=10 mem=8G (and won't, if the later
> > instance-type-unsetting also unsets everything it conflicts with -- which is
> > what this change fixes).
> > 
> > Incidentally, "any" only means "use default" when the default is None; ""
> always
> > means "use default".
> 
> if the any value isn't stored then when composing constraints from different
> levels, there isn't any distinguishing between something which was set and
> something that was set to 'any'. granted we don't compose multi-level
> constraints right now, but given env constraints, this would need changing to
> keep the recorded value for comparision afaics. 

For the record, I was unclear: this loop only deals with masking out conflicts,
the unset value still gets stored below.

[fwereade, 2012-03-28 18:57:14]

*** Submitted:

Providers are now directly responsible for their own constraints

This is really just a rearrangement of code; nothing fundamental has
actually changed, except that Constraints.from_strs is entirely gone; and
trying to construct Constraints(None, data) will no longer produce a viable
specimen.

R=hazmat
CC=
https://codereview.appspot.com/5939046