Code review comment for lp:~fwereade/pyjuju/no-private-cloud-constraints

Reviewers: mp+101592_code.launchpad.net,

Message:
Please take a look.

Description:
Disable constraints on private clouds

Private clouds (defined as those with an ec2-uri config setting) now
expose
no constraints at all; everything in the ec2 provider that uses
constraints
now knows not to assume that any keys are present other than
provider-type
and ubuntu-series.

https://code.launchpad.net/~fwereade/juju/no-private-cloud-constraints/+merge/101592

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/providers/ec2/__init__.py
   M juju/providers/ec2/launch.py
   M juju/providers/ec2/tests/common.py
   M juju/providers/ec2/tests/test_launch.py
   M juju/providers/ec2/tests/test_provider.py
   M juju/providers/ec2/tests/test_utils.py
   M juju/providers/ec2/utils.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/providers/ec2/__init__.py
=== modified file 'juju/providers/ec2/__init__.py'
--- juju/providers/ec2/__init__.py 2012-04-11 00:09:41 +0000
+++ juju/providers/ec2/__init__.py 2012-04-11 15:47:12 +0000
@@ -74,8 +74,7 @@
      def get_constraint_set(self):
          """Return the set of constraints that are valid for this
provider."""
          cs = yield super(MachineProvider, self).get_constraint_set()
- if 1: # These keys still need to be valid (instance-type and
ec2-zone)
- #if self.using_amazon:
+ if self.using_amazon:
              # Expose EC2 instance types/zones on AWS itelf, not private
clouds.
              cs.register_generics(INSTANCE_TYPES.keys())
              cs.register("ec2-zone", converter=convert_zone)

Index: juju/providers/ec2/launch.py
=== modified file 'juju/providers/ec2/launch.py'
--- juju/providers/ec2/launch.py 2012-03-27 22:46:44 +0000
+++ juju/providers/ec2/launch.py 2012-04-11 15:47:12 +0000
@@ -34,7 +34,7 @@
              "$(curl http://169.254.169.254/1.0/meta-data/instance-id)")
          user_data = cloud_init.render()

- availability_zone = self._constraints["ec2-zone"]
+ availability_zone = self._constraints.get("ec2-zone")
          if availability_zone is not None:
              region = self._provider.config.get("region", DEFAULT_REGION)
              availability_zone = region + availability_zone

Index: juju/providers/ec2/tests/common.py
=== modified file 'juju/providers/ec2/tests/common.py'
--- juju/providers/ec2/tests/common.py 2012-04-11 00:09:41 +0000
+++ juju/providers/ec2/tests/common.py 2012-04-11 15:47:12 +0000
@@ -101,12 +101,13 @@
          get_public_key(MATCH(match_config))
          self.mocker.result("zebra")

- get_ami_args = get_ami_args or (
- "splendid", "amd64", "us-east-1", False, False)
- get_ami = self.mocker.replace(
- "juju.providers.ec2.utils.get_current_ami")
- get_ami(*get_ami_args)
- self.mocker.result(succeed(ami_name))
+ if get_ami_args is not None:
+ get_ami_args = get_ami_args or (
+ "splendid", "amd64", "us-east-1", False, False)
+ get_ami = self.mocker.replace(
+ "juju.providers.ec2.utils.get_current_ami")
+ get_ami(*get_ami_args)
+ self.mocker.result(succeed(ami_name))

      def _mock_create_group(self):
          group_name = "juju-%s" % self.env_name

Index: juju/providers/ec2/tests/test_launch.py
=== modified file 'juju/providers/ec2/tests/test_launch.py'
--- juju/providers/ec2/tests/test_launch.py 2012-04-11 00:18:49 +0000
+++ juju/providers/ec2/tests/test_launch.py 2012-04-11 15:47:12 +0000
@@ -88,6 +88,35 @@
          d.addCallback(verify_result)
          return d

+ def test_provider_launch_private_cloud_constraints(self):
+ """
+ When running on a private cloud (ie when the provider registers no
+ constraints) we should only require default-image-id in config, and
+ fall back to an instance-type of m1.small if default-instance-type
is
+ not present.
+ """
+ self.ec2.describe_security_groups()
+ self.mocker.result(succeed([]))
+ self._mock_create_group()
+ self._mock_create_machine_group("1")
+ self._mock_launch_utils(get_ami_args=None)
+ self._mock_get_zookeeper_hosts()
+ self._mock_launch(self.get_instance("i-foobar"), "ami-tango")
+ self.mocker.replay()
+
+ def verify_result(result):
+ (machine,) = result
+ self.assert_machine(machine, "i-foobar", "")
+ provider = self.get_provider()
+ provider.config["default-image-id"] = "ami-tango"
+ d = provider.start_machine({
+ "machine-id": "1",
+ "constraints": {
+ "provider-type": "ec2",
+ "ubuntu-series": "splendid"}})
+ d.addCallback(verify_result)
+ return d
+
      @inlineCallbacks
      def test_provider_launch_requires_constraints(self):
          self.mocker.replay()

Index: juju/providers/ec2/tests/test_provider.py
=== modified file 'juju/providers/ec2/tests/test_provider.py'
--- juju/providers/ec2/tests/test_provider.py 2012-04-11 00:18:49 +0000
+++ juju/providers/ec2/tests/test_provider.py 2012-04-11 15:47:12 +0000
@@ -254,9 +254,7 @@
          self.assertFalse(b.can_satisfy(a))

      @inlineCallbacks
- def xtest_non_amazon_constraints(self):
- # Disabled because the ec2 provider requires these keys
(instance-type
- # and ec2-zone)
+ def test_non_amazon_constraints(self):
          provider = MachineProvider("some-non-ec2-env", {
              "ec2-uri": "blah", "secret-key": "foobar", "access-key": "bar"})
          cs = yield provider.get_constraint_set()

Index: juju/providers/ec2/tests/test_utils.py
=== modified file 'juju/providers/ec2/tests/test_utils.py'
--- juju/providers/ec2/tests/test_utils.py 2012-04-11 08:19:44 +0000
+++ juju/providers/ec2/tests/test_utils.py 2012-04-11 15:47:12 +0000
@@ -4,7 +4,7 @@
  from twisted.internet.defer import fail, succeed, inlineCallbacks
  from twisted.web.error import Error

-from juju.errors import ProviderError
+from juju.errors import ProviderError, UnknownConstraintError
  from juju.lib.testing import TestCase
  from juju.lib.mocker import MATCH
  from juju.providers import ec2
@@ -13,6 +13,7 @@

  from .common import get_constraints

+from juju.providers.ec2 import MachineProvider
  from juju.providers.ec2.utils import VerifyingContextFactory

@@ -275,7 +276,27 @@

      @inlineCallbacks
      def test_config_override(self):
+ """If config contains default-instance-type, that should always be
used
+ regardless of constraints (it's only a valid key when in legacy
+ deployments, in which constraint entry is disabled, or on private
+ clouds (which themselves register no constraints))."""
          constraints = yield get_constraints([])
          instance_type = get_instance_type(
              {"default-instance-type": "whatever-they-typed"}, constraints)
          self.assertEquals(instance_type, "whatever-they-typed")
+
+ @inlineCallbacks
+ def test_config_fallback(self):
+ """In a private cloud, a missing default-instance-id should fall
back
+ to m1.small."""
+ provider = MachineProvider("", {
+ "access-key": "foo", "secret-key": "bar",
+ "ec2-uri": "http://example.com"})
+ cs = yield provider.get_constraint_set()
+
+ # Verify that this is indeed a ConstraintSet without instance-type
+ self.assertRaises(
+ UnknownConstraintError, cs.parse, ["instance-type=c1.medium"])
+ constraints = cs.parse([])
+ instance_type = get_instance_type({}, constraints)
+ self.assertEquals(instance_type, "m1.small")

Index: juju/providers/ec2/utils.py
=== modified file 'juju/providers/ec2/utils.py'
--- juju/providers/ec2/utils.py 2012-04-11 08:36:43 +0000
+++ juju/providers/ec2/utils.py 2012-04-11 15:47:12 +0000
@@ -143,6 +143,8 @@
      instance_type = config.get("default-instance-type")
      if instance_type is not None:
          return instance_type
+ if "instance-type" not in constraints:
+ return "m1.small"
      instance_type = constraints["instance-type"]
      if instance_type is not None:
          return instance_type
@@ -161,7 +163,7 @@
      image_id = config.get("default-image-id")
      if image_id is None:
          series = constraints["ubuntu-series"] or config["default-series"]
- arch = constraints["arch"] or "amd64"
+ arch = constraints.get("arch") or "amd64"
          region = config.get("region", DEFAULT_REGION)
          hvm = INSTANCE_TYPES[instance_type].hvm
          ssl_verify = config.get("ssl-hostname-verification", False)