Merge lp:~hazmat/pyjuju/sans-ami into lp:pyjuju

[Kapil Thangavelu]

This branch (re) enables usage of standard ubuntu amis for ensemble machines, utilizing the ensemble ppa as the default mechanism for installing ensemble, as an added bonus this removes the extraneous zk instances on service unit machines. It also preserves the ability to install ensemble from a public lp branch

[Benjamin Saller (bcsaller)]

Overall, fine, I think point 0 is important and where we want to be in terms of installing ensemble on top of 'clean' APIs. I think the point I'm trying to make in [3] about configuration is also important. The rest is kinda nitpicky.

[0]

I think in general we need to support 3 options for the ensemble code.
    a. distro's packages (which will be the 11.10 default)
    b. a ppa's packages (tip will be be the default for most of our initial consumers)
    c. a lp branch (the developers common case)

I think its a minor refactoring to support all 3 and will be well worth while to get this right.

47 + # These should be in the deb
48 + launch_scripts.extend([
49 + "sudo mkdir -p /var/lib/ensemble",
50 + "sudo mkdir -p /var/log/ensemble"])
51 +

[1] This _should_ be in the deb.

123 class EC2UtilsTest(TestCase):
124 @@ -28,7 +28,7 @@
125 a LookupError is raised.
126 """
127 page = self.mocker.replace("twisted.web.client.getPage")
128 - page(SAMPLE_URI)
129 + page(IMAGE_URI_TEMPLATE % "lucid")
130 self.mocker.result(succeed(""))
131 self.mocker.replay()
132 d = get_current_ami(ubuntu_release_name="lucid")

[2] Our consumers are already plotting about 11.10. Even in our test cases we might benefit from removing release names to avoid feeling of bit rot as things move forward. Every 6 months these references look older and older. release-1, release-2 etc might make as much sense. I think we should also remove actual release names from the codebase like get_current_ami.

[3] related to the last note but I think ensemble.providers.ec2.utils.get_current_ami should be renamed and retargeted based on a default from the environment file and remove the default disto assumption from here. This is something that will have to generally be in sync with what happens on orchestra and lxc providers in the future (even if they don't actually use an AMI). When someone is ready to transition their dev or production deployment environment to a new Ubuntu rev series it should default to a single decision they have to take.

review: Needs Fixing

[Kapil Thangavelu (hazmat)]

[0]. I think we're a little early to support static packages atm, we're not upstream on any release we're actually using atm, so its a dead option, and we're iterating on features way to fast to let people use a static version.

[1]. Its still needed for (c) a source install case.

[3] allow a distro spec from the environment yaml file sounds good

[Gustavo Niemeyer (niemeyer)]

This looks good, thanks for sorting this out so quickly. +1, assuming
both deployment scenarios were tested.

[0]

Ben's [0] is a very good point, but I agree we can sort it out in a
follow up. We have to deal with picking the proper package until the
sprint in Austin. We really can't let it go through without it, or
we'll be in trouble soon.

[1]

def get_machine_options(config, authorized_keys, packages=None,
                        repositories=None, scripts=None, **kw):
(...)
                params[p_name] = kw[a_name]
                del kw[a_name]
        params["region"] = region
(...)
        data=kw)

The way we have a blob of data spreading through to various locations
which poke at it and pick what they need isn't entirely comforting.
I guess it's fine since it's fairly well contained ATM, but we should
look at opportunities to refactor that into a more understandable
flow if we continue growing into this direction.

[2]

+ # XXX ideally should come from latest available or client release name.

We really have to sort out the situation of the way Ubuntu releases
are selected, and take into account the formula preference. We'll
necessarily have to solve that up to the sprint in Austin, otherwise
we risk going out with an unmanaged situation which will bite us
heavily later.

review: Approve

[Gustavo Niemeyer (niemeyer)]

Moved back to WIP until Ben's points are sorted.

[Kapil Thangavelu (hazmat)]

Excerpts from Benjamin Saller's message of Fri Jul 01 15:12:53 UTC 2011:

0 and 1 in the works.
>
> [1] This _should_ be in the deb.
>
> 123 class EC2UtilsTest(TestCase):
> 124 @@ -28,7 +28,7 @@
> 125 a LookupError is raised.
> 126 """
> 127 page = self.mocker.replace("twisted.web.client.getPage")
> 128 - page(SAMPLE_URI)
> 129 + page(IMAGE_URI_TEMPLATE % "lucid")
> 130 self.mocker.result(succeed(""))
> 131 self.mocker.replay()
> 132 d = get_current_ami(ubuntu_release_name="lucid")
>
> [2] Our consumers are already plotting about 11.10. Even in our test cases we might benefit from removing release names to avoid feeling of bit rot as things move forward. Every 6 months these references look older and older. release-1, release-2 etc might make as much sense. I think we should also remove actual release names from the codebase like get_current_ami.

I think this is just testing against a known data set, the notion of bitrot here seems limited, its a test verifying a parsing process with a known input and output.

> [3] related to the last note but I think ensemble.providers.ec2.utils.get_current_ami should be renamed and retargeted based on a default from the environment file and remove the default disto assumption from here. This is something that will have to generally be in sync with what happens on orchestra and lxc providers in the future (even if they don't actually use an AMI). When someone is ready to transition their dev or production deployment environment to a new Ubuntu rev series it should default to a single decision they have to take.
>

This is a little unclear to me, an ensemble environment might have multiple distributions within it. Its an easy option to add, i'm just unclear for both this and instance-type if we really want to be spec'ing this at the environment level. For a default distro if not spec'd in the environment, i'd assume the distro release name of the machine that the admin tools are running from.

Its unclear how this should react with nightlies, but perhaps that notion is more ec2 image centric.

[Benjamin Saller (bcsaller)]

We've agreed that the changes mentioned in [0] can come in a later branch. There is pressing need for a solution now, so +1

review: Approve

=== modified file 'ensemble/providers/ec2/launch.py'
--- ensemble/providers/ec2/launch.py	2011-06-02 17:50:24 +0000
+++ ensemble/providers/ec2/launch.py	2011-06-28 20:16:40 +0000
@@ -128,22 +128,39 @@
             return group_created.addCallback(lambda x: groups)
         return groups
 
+    def _get_branch_install_scripts(self, branch):
+        """Returns the commands to install ensemble from a bzr branch.
+
+        The branch identifier must be a valid publicly accessible bzr
+        url that can be used by bzr to checkout the branch. The install
+        commands will setup local directories and installation of the branch.
+        """
+        ensemble_vcs = ("cd /usr/lib/ensemble "
+                       "&& sudo /usr/bin/bzr co %s ensemble" % branch)
+        ensemble_install = [
+            "sudo apt-get install -y python-txzookeeper",
+            "sudo mkdir -p /usr/lib/ensemble",
+            ensemble_vcs,
+            "cd /usr/lib/ensemble/ensemble && sudo python setup.py develop"]
+
+        return ensemble_install
+
     def get_launch_scripts(self, machine_data):
         """Get the scripts that will execute after the machine has launched."""
 
+        launch_scripts = ["sudo apt-get -y install ensemble"]
+
+        # Support installation of ensemble directly from an LP hosted branch.
         if machine_data.get("ensemble-branch"):
-            ensemble_vcs = ENSEMBLE_UPDATE_TEMPLATE % (
-                "switch %s" % machine_data["ensemble-branch"])
-        else:
-            ensemble_vcs = ENSEMBLE_UPDATE
-
-        launch_scripts = [
-            "sudo mkdir -p /usr/lib/ensemble",
-            ensemble_vcs,
-            "cd /usr/lib/ensemble/ensemble && sudo python setup.py develop",
-            "cd /usr/lib/ensemble/txzookeeper && sudo /usr/bin/bzr up",
-            "cd /usr/lib/ensemble/txzookeeper && sudo python setup.py develop"]
-
+            launch_scripts = self._get_branch_install_scripts(
+                machine_data["ensemble-branch"])
+
+        # These should be in the deb
+        launch_scripts.extend([
+            "sudo mkdir -p /var/lib/ensemble",
+            "sudo mkdir -p /var/log/ensemble"])
+
+        # Every machine has its own agent.
         machine_agent_script = (
             "ENSEMBLE_MACHINE_ID=%(machine-id)s "
             "ENSEMBLE_ZOOKEEPER=%(ensemble-zookeeper-hosts)s "
@@ -151,7 +168,6 @@
             "--logfile=/var/log/ensemble/machine-agent.log "
             "--pidfile=/var/run/ensemble/machine-agent.pid")
         launch_scripts.append(machine_agent_script % machine_data)
-
         return launch_scripts
 
     def get_machine_variables(self, machine_data):

=== renamed file 'ensemble/providers/ec2/tests/data/lucid-release.txt' => 'ensemble/providers/ec2/tests/data/lucid.txt'
=== added file 'ensemble/providers/ec2/tests/data/natty.txt'
--- ensemble/providers/ec2/tests/data/natty.txt	1970-01-01 00:00:00 +0000
+++ ensemble/providers/ec2/tests/data/natty.txt	2011-06-28 20:16:40 +0000
@@ -0,0 +1,21 @@
+natty	server	release	20110426	ebs	amd64	ap-northeast-1	ami-dab812db	aki-d409a2d5		paravirtual
+natty	server	release	20110426	ebs	i386	ap-northeast-1	ami-d8b812d9	aki-d209a2d3		paravirtual
+natty	server	release	20110426	instance-store	amd64	ap-northeast-1	ami-a4b812a5	aki-d409a2d5		paravirtual
+natty	server	release	20110426	instance-store	i386	ap-northeast-1	ami-46b81247	aki-d209a2d3		paravirtual
+natty	server	release	20110426	ebs	amd64	ap-southeast-1	ami-60582132	aki-11d5aa43		paravirtual
+natty	server	release	20110426	ebs	i386	ap-southeast-1	ami-62582130	aki-13d5aa41		paravirtual
+natty	server	release	20110426	instance-store	amd64	ap-southeast-1	ami-aa5920f8	aki-11d5aa43		paravirtual
+natty	server	release	20110426	instance-store	i386	ap-southeast-1	ami-c0592092	aki-13d5aa41		paravirtual
+natty	server	release	20110426	ebs	amd64	eu-west-1	ami-379ea943	aki-4feec43b		paravirtual
+natty	server	release	20110426	ebs	i386	eu-west-1	ami-359ea941	aki-4deec439		paravirtual
+natty	server	release	20110426	instance-store	amd64	eu-west-1	ami-619ea915	aki-4feec43b		paravirtual
+natty	server	release	20110426	instance-store	i386	eu-west-1	ami-1b9fa86f	aki-4deec439		paravirtual
+natty	server	release	20110426	ebs	amd64	us-east-1	ami-1cad5275			hvm
+natty	server	release	20110426	ebs	amd64	us-east-1	ami-1aad5273	aki-427d952b		paravirtual
+natty	server	release	20110426	ebs	i386	us-east-1	ami-06ad526f	aki-407d9529		paravirtual
+natty	server	release	20110426	instance-store	amd64	us-east-1	ami-68ad5201	aki-427d952b		paravirtual
+natty	server	release	20110426	instance-store	i386	us-east-1	ami-e2af508b	aki-407d9529		paravirtual
+natty	server	release	20110426	ebs	amd64	us-west-1	ami-136f3c56	aki-9ba0f1de		paravirtual
+natty	server	release	20110426	ebs	i386	us-west-1	ami-116f3c54	aki-99a0f1dc		paravirtual
+natty	server	release	20110426	instance-store	amd64	us-west-1	ami-0b6f3c4e	aki-9ba0f1de		paravirtual
+natty	server	release	20110426	instance-store	i386	us-west-1	ami-596f3c1c	aki-99a0f1dc		paravirtual

=== modified file 'ensemble/providers/ec2/tests/test_launch.py'
--- ensemble/providers/ec2/tests/test_launch.py	2011-06-03 18:11:39 +0000
+++ ensemble/providers/ec2/tests/test_launch.py	2011-06-28 20:16:40 +0000
@@ -200,7 +200,7 @@
             machine_data={
                 "machine-id": "machine-2",
                 "ensemble-zookeeper-hosts": get_test_zookeeper_address()})
-        cmd = "cd /usr/lib/ensemble/ensemble && sudo /usr/bin/bzr switch %s"%(
+        cmd = "cd /usr/lib/ensemble && sudo /usr/bin/bzr co %s" % (
             custom_branch)
         self.failUnlessIn(cmd, variables["scripts"])
         self.assertEqual(variables["ensemble-branch"], custom_branch)

=== modified file 'ensemble/providers/ec2/tests/test_utils.py'
--- ensemble/providers/ec2/tests/test_utils.py	2011-06-03 18:11:39 +0000
+++ ensemble/providers/ec2/tests/test_utils.py	2011-06-28 20:16:40 +0000
@@ -13,11 +13,11 @@
 
 from ensemble.lib.testing import TestCase
 
-SAMPLE_URI = "\
-http://uec-images.ubuntu.com/query/lucid/server/released.current.txt"
+IMAGE_URI_TEMPLATE = "\
+http://uec-images.ubuntu.com/query/%s/server/released.current.txt"
 
-SAMPLE_FILE = os.path.join(os.path.dirname(inspect.getabsfile(ec2)),
-                           "tests", "data", "lucid-release.txt")
+IMAGE_DATA_DIR = os.path.join(
+    os.path.dirname(inspect.getabsfile(ec2)), "tests", "data")
 
 
 class EC2UtilsTest(TestCase):
@@ -28,7 +28,7 @@
         a LookupError is raised.
         """
         page = self.mocker.replace("twisted.web.client.getPage")
-        page(SAMPLE_URI)
+        page(IMAGE_URI_TEMPLATE % "lucid")
         self.mocker.result(succeed(""))
         self.mocker.replay()
         d = get_current_ami(ubuntu_release_name="lucid")
@@ -38,8 +38,10 @@
     def test_current_ami(self):
         """The current server machine image can be retrieved."""
         page = self.mocker.replace("twisted.web.client.getPage")
-        page(SAMPLE_URI)
-        self.mocker.result(succeed(open(SAMPLE_FILE).read()))
+        page(IMAGE_URI_TEMPLATE % "lucid")
+        self.mocker.result(succeed(
+            open(os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
+
         self.mocker.replay()
         d = get_current_ami(ubuntu_release_name="lucid")
 
@@ -52,8 +54,10 @@
     def test_current_ami_by_region(self):
         """The current server machine image can be retrieved by region."""
         page = self.mocker.replace("twisted.web.client.getPage")
-        page(SAMPLE_URI)
-        self.mocker.result(succeed(open(SAMPLE_FILE).read()))
+        page(IMAGE_URI_TEMPLATE % "lucid")
+        self.mocker.result(
+            succeed(open(
+                os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
         self.mocker.replay()
         d = get_current_ami(ubuntu_release_name="lucid", region="us-west-1")
 
@@ -69,8 +73,9 @@
         to guide image selection.
         """
         page = self.mocker.replace("twisted.web.client.getPage")
-        page(SAMPLE_URI)
-        self.mocker.result(succeed(open(SAMPLE_FILE).read()))
+        page(IMAGE_URI_TEMPLATE % "lucid")
+        self.mocker.result(succeed(
+            open(os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
         self.mocker.replay()
         d = get_current_ami(ubuntu_release_name="lucid", ebs=False)
 
@@ -173,18 +178,54 @@
         the ec2 client's run instance method. A known set of default
         is utilized.
         """
+
+        page = self.mocker.replace("twisted.web.client.getPage")
+        page(IMAGE_URI_TEMPLATE % "natty")
+        self.mocker.result(
+            succeed(open(
+                os.path.join(IMAGE_DATA_DIR, "natty.txt")).read()))
+        self.mocker.replay()
+
         config = {}
-
         d = get_machine_options(config, "zebra")
 
         def verify_result(result):
             self.assertTrue(isinstance(result, dict))
-            self.assertEqual(result["image_id"], "ami-d2ba45bb")
+            self.assertEqual(result["image_id"], "ami-06ad526f")
             self.assertEqual(result["instance_type"], "m1.small")
 
         d.addCallback(verify_result)
         return d
 
+    def test_get_machine_options_with_params(self):
+        """
+        Image parameters can be specified to determine which ami is to
+        be launched. Image paramters specified are not passed to the
+        instance via user data.
+        """
+        page = self.mocker.replace("twisted.web.client.getPage")
+        page(IMAGE_URI_TEMPLATE % "natty")
+        self.mocker.result(
+            succeed(open(
+                os.path.join(IMAGE_DATA_DIR, "natty.txt")).read()))
+        self.mocker.replay()
+
+        d = get_machine_options(
+            {"default-instance-type": "m1.large"},
+            "zebra", image_arch="amd64", image_ebs=False)
+
+        def verify_result(result):
+            self.assertTrue(isinstance(result, dict))
+            self.assertEqual(result["image_id"], "ami-68ad5201")
+            self.assertEqual(result["instance_type"], "m1.large")
+            self.assertNotIn(
+                "arch", result["user_data"])
+            self.assertNotIn(
+                "ebs", result["user_data"])
+
+        d.addCallback(verify_result)
+        return d
+
     def test_configured_default_image_has_precedence_over_region(self):
         """If the server machine image is specified it takes precendence over
         region."""

=== modified file 'ensemble/providers/ec2/utils.py'
--- ensemble/providers/ec2/utils.py	2011-06-03 18:11:39 +0000
+++ ensemble/providers/ec2/utils.py	2011-06-28 20:16:40 +0000
@@ -27,7 +27,7 @@
     return "https://ec2.%s.amazonaws.com" % region
 
 
-def get_current_ami(ubuntu_release_name="lucid", arch="i386", ebs=True,
+def get_current_ami(ubuntu_release_name="natty", arch="i386", ebs=True,
                     region="us-east-1", daily=False, desktop=False,
                     url_fetch=None):
     """Get the latest ami for the last release of ubuntu."""
@@ -143,6 +143,31 @@
     Given an ensemble environment configuration, extract the proper machine
     information to be used for launching the machine via the ec2 api.
     """
+
+    instance_type = config.get("default-instance-type", "m1.small")
+
+    # XXX ideally should come from latest available or client release name.
+    region = config.get("region", "us-east-1")
+    image_id = config.get("default-image-id", None)
+
+    # Lookup a standard ami by region, if none is specified, use any
+    # specified image specification to locate an image, and remove the
+    # image specification so it doesn't propogate to user data.
+    if not image_id:
+        params = {}
+        for a_name, p_name  in (
+            ("image_release_name", "ubuntu_release_name"),
+            ("image_arch", "arch"),
+            ("image_ebs", "ebs"),
+            ("image_daily", "daily")):
+            if a_name in kw:
+                params[p_name] = kw[a_name]
+                del kw[a_name]
+        params["region"] = region
+        d = get_current_ami(**params)
+    else:
+        d = succeed(image_id)
+
     user_data = format_cloud_ini(
         authorized_keys=authorized_keys,
         packages=packages,
@@ -150,13 +175,6 @@
         scripts=scripts,
         data=kw)
 
-    instance_type = config.get("default-instance-type", "m1.small")
-    # XXX ideally should come from latest available or client release name,
-    # for now to speed bootstrap, we use a prepopulated image by region.
-    region = config.get("region", "us-east-1")
-    image_id = config.get("default-image-id", AMI_REGION_MAP[region])
-    d = succeed(image_id)
-
     def on_machine_image(current_image_id):
         return {"image_id": current_image_id,
                 "min_count": 1,