Merge lp:~fwereade/pyjuju/no-private-cloud-constraints into lp:~fwereade/pyjuju/constraints-compatibility

[William Reade]

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

[William Reade (fwereade)]

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.r...

[Kapil Thangavelu (hazmat)]

unfortunate but nesc. lgtm.

https://codereview.appspot.com/6003046/

[William Reade (fwereade)]

*** Submitted:

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.

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

https://codereview.appspot.com/6003046/

=== 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:55:30 +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)
 
=== 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:55:30 +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
 
=== 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:55:30 +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
 
=== 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:55:30 +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()
 
=== 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:55:30 +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()
 
=== 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:55:30 +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")
 
=== 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:55:30 +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)