Merge lp:~fwereade/pyjuju/env-constraints into lp:~fwereade/pyjuju/shadow-trunk-1204

[William Reade]

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://codereview.appspot.com/5957043/

[William Reade (fwereade)]

Reviewers: mp+99852_code.launchpad.net,

Message:
Please take a look.

Description:
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)

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

Affected files:
   A [revision details]
   M juju/agents/tests/test_provision.py
   M juju/control/bootstrap.py
   M juju/control/constraints_get.py
   M juju/control/constraints_set.py
   M juju/control/deploy.py
   M juju/control/initialize.py
   M juju/control/tests/test_add_unit.py
   M juju/control/tests/test_bootstrap.py
   M juju/control/tests/test_constraints_get.py
   M juju/control/tests/test_constraints_set.py
   M juju/control/tests/test_deploy.py
   M juju/control/tests/test_initialize.py
   M juju/control/utils.py
   M juju/environment/tests/test_config.py
   M juju/providers/common/base.py
   M juju/providers/common/bootstrap.py
   M juju/providers/common/cloudinit.py
   M juju/providers/common/launch.py
   M juju/providers/common/tests/data/cloud_init_bootstrap
   M juju/providers/common/tests/data/cloud_init_bootstrap_zookeepers
   M juju/providers/common/tests/data/cloud_init_distro
   M juju/providers/common/tests/data/cloud_init_ppa
   M juju/providers/common/tests/test_bootstrap.py
   M juju/providers/common/tests/test_cloudinit.py
   M juju/providers/dummy.py
   M juju/providers/ec2/tests/common.py
   M juju/providers/ec2/tests/data/bootstrap_cloud_init
   M juju/providers/ec2/tests/test_bootstrap.py
   M juju/providers/local/__init__.py
   M juju/providers/local/tests/test_provider.py
   M juju/providers/orchestra/tests/data/bootstrap_user_data
   M juju/providers/orchestra/tests/test_bootstrap.py
   M juju/providers/tests/test_dummy.py
   M juju/state/environment.py
   M juju/state/initialize.py
   M juju/state/machine.py
   M juju/state/service.py
   M juju/state/tests/test_environment.py
   M juju/state/tests/test_initialize.py
   M juju/state/tests/test_service.py

[lp:~fwereade/pyjuju/env-constraints]

515. By William Reade on 2012-03-29

improved constraints-related docstrings in juju.state

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/5957043/

[Kapil Thangavelu (hazmat)]

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#newcode51
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#newcode90
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_bootstrap.py
File juju/control/tests/test_bootstrap.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstrap.py#newcode7
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_initialize.py
File juju/control/tests/test_initialize.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initialize.py#newcode41
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_provision.py
File juju/agents/tests/test_provision.py (right):

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provision.py#newcode56
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.py#newcode57
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.py#newcode90
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...

[William Reade (fwereade)]

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

https://codereview.appspot.com/5957043/diff/3001/juju/agents/tests/test_provision.py#newcode56
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.py#newcode57
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.py#newcode90
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#newcode49
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.

https://codereview.appspot.com/5957043/

[lp:~fwereade/pyjuju/env-constraints]

516. By William Reade on 2012-03-30

address review points

[William Reade (fwereade)]

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#newcode51
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#newcode90
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_bootstrap.py
File juju/control/tests/test_bootstrap.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_bootstrap.py#newcode7
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_initialize.py
File juju/control/tests/test_initialize.py (right):

https://codereview.appspot.com/5957043/diff/1/juju/control/tests/test_initialize.py#newcode41
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.

https://codereview.appspot.com/5957043/

[lp:~fwereade/pyjuju/env-constraints]

517. By William Reade on 2012-03-30

address missed review points

[William Reade (fwereade)]

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

https://codereview.appspot.com/5957043/