Implement environment constraints

Implement environment constraints

In user-visible terms:

* bootstrap now accepts --constraints;
* set-constraints now works without --service;
* get-constraints now accepts empty args and outputs env constraints.

In developer terms:

* EnvironmentStateManager has new methods for get/set constraints;
* A new /constraints node is used to store environment constraints (if you
  can think of a better way to do this, bearing in mind that we can't create
  /environment until we've got data for the PA in there, please tell me);
* MachineProvider.bootstrap now requires a Constraints;
* CloudInit now needs a Constraints when setting up a master;
* juju-admin initialize now accept --constraints-data;
* get-constraints and set-constraints take an apparent long way round to
  construct their constraints (they sync environents.yaml so that a provider
  can be created from it by EnvironmentStateManager -- but ESM is used
  internally in a number of places, and the fact that we have a provider
  already available at the juju.control level is not so helpful);
* an awful lot of the diff is tedious test fixing in juju.providers.

https://code.launchpad.net/~fwereade/juju/env-constraints/+merge/99852

(do not edit description out of merge proposal)

[fwereade, 2012-03-29 02:30:31]

Please take a look.

[fwereade, 2012-03-29 15:43:28]

Please take a look.

[hazmat, 2012-03-29 19:52:52]

LGTM, some minors on docs.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py...
juju/control/constraints_get.py:51: yield esm.set_config_state(env_config,
environment.name)
so why are we syncing env on get-constraints.. the constraints are already
actualized on the object being inspected. if its a legacy environment we should
just log warning and exit.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py...
juju/control/constraints_set.py:90: yield esm.set_config_state(env_config,
environment.name)
Some comments on the reasoning for this here and in get-constraints would be
good.

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstr...
File juju/control/tests/test_bootstrap.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstr...
juju/control/tests/test_bootstrap.py:7: from juju.lib.mocker import ANY
not needed.

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initial...
File juju/control/tests/test_initialize.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initial...
juju/control/tests/test_initialize.py:41: def test_bad_data(self):
general comment, its really nice to have doc strings on tests for other devs,
even if its just a single line to express intent, and assuming the test name
itself isn't obvious. ie test_bad_constraints_data or
this is a test that the admin initialize exits 1 on bad constraints data.

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provi...
File juju/agents/tests/test_provision.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provi...
juju/agents/tests/test_provision.py:56: reactor.callLater(0.3,
self.push_default_config, False)
nitpick, please use the with_constraints=False invocation, its a bit easier to
grok what the value is doing without hunting for the method def.

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get...
juju/control/constraints_get.py:57: entity = yield msm.get_machine_state(name)
what do you think an info log message describing the entity for which the
constraints are being retrieved.

ie. log.info("Fetching constraints for service %s", entity)
log.info("Fetching constriants for environment")

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set...
juju/control/constraints_set.py:90: yield esm.set_config_state(env_config,
environment.name)
i'd appreciate if you could document the rationale for syncing as applying to
constraints, perhaps in an internals doc, referenced by the relevant code
points. This will make it much easier to keep the code concerns in mind when the
sync goes away.

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py
File juju/state/environment.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py#ne...
juju/state/environment.py:49: provider =
config.get_default().get_machine_provider()
hmm.. this is a red flag, ie the only callers here should be privileged things
(cli or provisioning agent), a doc string on the method would be good.

[fwereade, 2012-03-30 08:15:38]

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provi...
File juju/agents/tests/test_provision.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provi...
juju/agents/tests/test_provision.py:56: reactor.callLater(0.3,
self.push_default_config, False)
On 2012/03/29 19:52:53, hazmat wrote:
> nitpick, please use the with_constraints=False invocation, its a bit easier to
> grok what the value is doing without hunting for the method def.

Done.

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_get...
juju/control/constraints_get.py:57: entity = yield msm.get_machine_state(name)
On 2012/03/29 19:52:53, hazmat wrote:
> what do you think an info log message describing the entity for which the
> constraints are being retrieved.
> 
> ie. log.info("Fetching constraints for service %s", entity)
> log.info("Fetching constriants for environment")

Sounds good, at least it'll be clear which entity failed if anything goes wrong.
Done.

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/control/constraints_set...
juju/control/constraints_set.py:90: yield esm.set_config_state(env_config,
environment.name)
On 2012/03/29 19:52:53, hazmat wrote:
> i'd appreciate if you could document the rationale for syncing as applying to
> constraints, perhaps in an internals doc, referenced by the relevant code
> points. This will make it much easier to keep the code concerns in mind when
the
> sync goes away.

This is addressed in
https://code.launchpad.net/%7Efwereade/juju/sync-env-when-required/+merge/99941
(in which I centralise the syncing behaviour).

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py
File juju/state/environment.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/state/environment.py#ne...
juju/state/environment.py:49: provider =
config.get_default().get_machine_provider()
On 2012/03/29 19:52:53, hazmat wrote:
> hmm.. this is a red flag, ie the only callers here should be privileged things
> (cli or provisioning agent), a doc string on the method would be good.

Done.

[fwereade, 2012-03-30 08:57:22]

Sorry, didn't see the comments on patch set 1.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py
File juju/control/constraints_get.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_get.py...
juju/control/constraints_get.py:51: yield esm.set_config_state(env_config,
environment.name)
On 2012/03/29 19:52:53, hazmat wrote:
> so why are we syncing env on get-constraints.. the constraints are already
> actualized on the object being inspected. if its a legacy environment we
should
> just log warning and exit.

Addressed in later comments & branches.

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py
File juju/control/constraints_set.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/constraints_set.py...
juju/control/constraints_set.py:90: yield esm.set_config_state(env_config,
environment.name)
On 2012/03/29 19:52:53, hazmat wrote:
> Some comments on the reasoning for this here and in get-constraints would be
> good.

Addressed in later comments and branches

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstr...
File juju/control/tests/test_bootstrap.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstr...
juju/control/tests/test_bootstrap.py:7: from juju.lib.mocker import ANY
On 2012/03/29 19:52:53, hazmat wrote:
> not needed.

Done.

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initial...
File juju/control/tests/test_initialize.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initial...
juju/control/tests/test_initialize.py:41: def test_bad_data(self):
On 2012/03/29 19:52:53, hazmat wrote:
> general comment, its really nice to have doc strings on tests for other devs,
> even if its just a single line to express intent, and assuming the test name
> itself isn't obvious. ie test_bad_constraints_data or
> this is a test that the admin initialize exits 1 on bad constraints data.

Done.

[fwereade, 2012-03-30 09:13:44]

*** Submitted:

Implement environment constraints

In user-visible terms:

* bootstrap now accepts --constraints;
* set-constraints now works without --service;
* get-constraints now accepts empty args and outputs env constraints.

In developer terms:

* EnvironmentStateManager has new methods for get/set constraints;
* A new /constraints node is used to store environment constraints (if you
  can think of a better way to do this, bearing in mind that we can't create
  /environment until we've got data for the PA in there, please tell me);
* MachineProvider.bootstrap now requires a Constraints;
* CloudInit now needs a Constraints when setting up a master;
* juju-admin initialize now accept --constraints-data;
* get-constraints and set-constraints take an apparent long way round to
  construct their constraints (they sync environents.yaml so that a provider
  can be created from it by EnvironmentStateManager -- but ESM is used
  internally in a number of places, and the fact that we have a provider
  already available at the juju.control level is not so helpful);
* an awful lot of the diff is tedious test fixing in juju.providers.

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