Merge lp:~chad.smith/charms/precise/storage/storage-ec2-support into lp:~dpb/charms/precise/storage/trunk

[Chad Smith]

Allow get_instance_id to work on both Openstack and EC2 instances.

To do this we first attempt a curl of the OS_METADATA_URL to pull the instance uuid which is used by nova commands for volume attachment.

If curl of OS_METADATA_URL fails to return valid JSON, we then try to curl EC2_METADATA_URL to grab the EC2 instance id.

This branch adds missing unit tests for get_instance_id as well.

[lp:~chad.smith/charms/precise/storage/storage-ec2-support]

35. By Chad Smith on 2014-02-18

docstring fix

36. By Chad Smith on 2014-02-25

support attached devices that are already partitioned, such as old postgresql charm upgrade_charm path

[Fernando Correa Neto (fcorrea)]

Code looks good but we have an issue because in order to mount an existing volume, the BSB service must be running on the same id.
That means that we must know upfront in which EC2 AZ the volume is, and if possible, deploy the BSB service in the same AZ so it can be successfully be attached.

review: Approve

[David Britton (dpb)]

On Tue, Mar 04, 2014 at 09:43:16AM -0000, Fernando Correa Neto wrote:
> Review: Approve
>
> Code looks good but we have an issue because in order to mount an
> existing volume, the BSB service must be running on the same id. That
> means that we must know upfront in which EC2 AZ the volume is, and if
> possible, deploy the BSB service in the same AZ so it can be
> successfully be attached.

Should the AZ be specified in the BSB config?

--
David Britton <email address hidden>

[David Britton (dpb)]

[0] Can we get rid of the 'else' clauses (two of them) in the get_instance_id() method? Just return directly from the try block?

Otherwise, +1.

review: Approve

=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-02-13 00:41:13 +0000
+++ hooks/common_util.py	2014-02-25 23:09:49 +0000
@@ -9,7 +9,8 @@
 
 CHARM_DIR = os.environ.get("CHARM_DIR")
 FSTAB_FILE = "/etc/fstab"
-METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
+EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data/instance-id"
+OS_METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
 PERSIST_DATA_FILE = "charm-data"
 
 
@@ -17,6 +18,7 @@
     """Quaint little log wrapper for juju logging"""
     hookenv.log(message, level)
 
+
 # XXX We can remove _persist_data when juju bug:1279018 is fixed to provide
 # relation-data from *-relation-departed hook runs
 def _persist_data(key, value):
@@ -262,6 +264,14 @@
             sys.exit(0)
 
         _assert_block_device(device_path)
+        if os.path.exists("%s1" % device_path):
+            # Then we are supporting upgrade-charm path for deprecated
+            # postgresql volume-map config parameter. Postgresql used to
+            # partition the volume into a single partition at 100% of the
+            # volume size, but the storage charm doesn't do that partitioning
+            device_path = "%s1" % device_path
+            log("Mounting %s as the device path as the root device is already "
+                "partitioned." % device_path)
         _format_device(device_path)
         _set_label(device_path, mountpoint)
         fstype = subprocess.check_output(
@@ -288,6 +298,24 @@
 
 
 def get_instance_id():
-    metadata = json.loads(
-        subprocess.check_output("curl %s" % METADATA_URL, shell=True))
-    return metadata["uuid"]
+    """
+    Curl metadata for openstack from C{OS_METADATA_URL} to obtain the C{uuid}.
+    If that fails, we are likely on EC2 and curl the C{EC2_METADATA_URL} to get
+    the EC2 C{instance-id}.
+    """
+    try:
+        metadata = json.loads(
+            subprocess.check_output("curl %s" % OS_METADATA_URL, shell=True))
+    except ValueError:
+        pass  # We are probably on EC2, try below
+    else:
+        return metadata["uuid"]
+    try:
+        metadata = subprocess.check_output(
+            "curl %s" % EC2_METADATA_URL, shell=True)
+    except subprocess.CalledProcessError, e:
+        log("Error: couldn't discover ec2 instance id. %s" % str(e),
+            hookenv.ERROR)
+        sys.exit(1)
+    else:
+        return metadata.strip()

=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-02-13 00:41:13 +0000
+++ hooks/test_common_util.py	2014-02-25 23:09:49 +0000
@@ -140,6 +140,58 @@
         message = "Error: no provider defined in storage charm config"
         self.assertTrue(util.hookenv.is_logged(message), "Error not logged")
 
+    def test_get_instance_id_when_openstack(self):
+        """
+        L{get_instance_id} will attempt a curl for openstack metadata to
+        extract the C{uuid} as the instance_id from openstack JSON metadata.
+        """
+        os_metadata = {
+            "uuid": "i-123123"
+        }
+        curl = self.mocker.replace(subprocess.check_output)
+        curl("curl %s" % util.OS_METADATA_URL, shell=True)
+        self.mocker.result(json.dumps(os_metadata))
+        self.mocker.replay()
+        self.assertEqual(util.get_instance_id(), "i-123123")
+
+    def test_get_instance_id_when_ec2(self):
+        """
+        L{get_instance_id} will first attempt a curl for openstack metadata.
+        When that fails to be valid json, because we are on an EC2 instance,
+        it will curl the C{EC2_METADATA_URL} to grab the C{instance-id}.
+        """
+        instance_id = "i-123123"
+        curl = self.mocker.replace(subprocess.check_output)
+        curl("curl %s" % util.OS_METADATA_URL, shell=True)
+        self.mocker.throw(ValueError("invalid JSON"))
+        curl("curl %s" % util.EC2_METADATA_URL, shell=True)
+        self.mocker.result(instance_id)
+        self.mocker.replay()
+        self.assertEqual(util.get_instance_id(), instance_id)
+
+    def test_get_instance_id_fails_on_ec2(self):
+        """
+        When L{get_instance_id} fails to curl C{EC2_METADATA_URL} it will log
+        the error and exit in error.
+        """
+        ec2_command = "curl %s" % util.EC2_METADATA_URL
+
+        curl = self.mocker.replace(subprocess.check_output)
+        curl("curl %s" % util.OS_METADATA_URL, shell=True)
+        self.mocker.throw(ValueError("invalid JSON"))
+        curl(ec2_command, shell=True)
+        self.mocker.throw(
+            subprocess.CalledProcessError(
+                1, "curl %s" % util.EC2_METADATA_URL))
+        self.mocker.replay()
+        result = self.assertRaises(SystemExit, util.get_instance_id)
+        self.assertEqual(result.code, 1)
+        message = (
+            "Error: couldn't discover ec2 instance id. Command '%s' returned "
+            "non-zero exit status 1" % ec2_command)
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
     def test_is_mounted_when_mountpoint_is_mounted(self):
         """
         L{is_mounted} returns C{True} when the mountpath is mounted.
@@ -499,9 +551,8 @@
         with open(persist_path) as infile:
             data = json.load(infile)
             self.assertEqual(data["mountpoint"], "/mnt/this")
-            
 
-    def test_mount_volume_mountpoint_from_relation_data(self):
+    def test_mount_volume_mountpoint_from_relation_data_already_mounted(self):
         """
         L{mount_volume} will log a message and return when the C{mountpoint} is
         already defined in relation data and the requested C{mountpoint} is
@@ -647,6 +698,9 @@
 
         assert_device = self.mocker.replace(util._assert_block_device)
         assert_device("/dev/vdx")
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx1")
+        self.mocker.result(False)
         create_partition = self.mocker.replace(
             util._format_device)
         create_partition("/dev/vdx")
@@ -659,7 +713,6 @@
         get_fstype("blkid -o value -s TYPE /dev/vdx", shell=True)
         self.mocker.result("ext4\n")
 
-        exists = self.mocker.replace(os.path.exists)
         exists("/mnt/this")
         self.mocker.result(True)
 
@@ -682,3 +735,66 @@
         self.assertEqual(
             util.hookenv._outgoing_relation_data,
             (("mountpoint", "/mnt/this"),))
+
+    def test_mount_volume_bsb_provider_success_postgresql_charm_upgrade(self):
+        """
+        Old versions of the postgresql charm would have created a single
+        partition with max size from an external attached volume if the user
+        had used postgresql charm's deprecated volume-map parameter. When the
+        storage charm sees a single partition on the attached device, it will
+        attempt to mount that partition instead of the root device.
+        The success of the mount will log a message and the storage charm will
+        call relation-set C{mountpoint} to report the successful mount to the
+        principal.
+        """
+        util.hookenv._incoming_relation_data = (
+            ("data:1", "mountpoint", "/mnt/this"),)
+
+        util.hookenv._config = (("provider", "block-storage-broker"),)
+
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(False)  # Mock not mounted
+
+        assert_device = self.mocker.replace(util._assert_block_device)
+        assert_device("/dev/vdx")
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx1")
+        self.mocker.result(True)
+        create_partition = self.mocker.replace(
+            util._format_device)
+        create_partition("/dev/vdx1")
+
+        _set_label = self.mocker.replace(util._set_label)
+        _set_label("/dev/vdx1", "/mnt/this")
+
+        # Ensure blkid is called to discover fstype of nova device
+        get_fstype = self.mocker.replace(subprocess.check_output)
+        get_fstype("blkid -o value -s TYPE /dev/vdx1", shell=True)
+        self.mocker.result("ext4\n")
+
+        exists("/mnt/this")
+        self.mocker.result(True)
+
+        mount = self.mocker.replace(subprocess.check_call)
+        mount("mount -t ext4 /dev/vdx1 /mnt/this", shell=True)
+        self.mocker.replay()
+
+        self.assertEqual(util.get_provider(), "block-storage-broker")
+        util.mount_volume("/dev/vdx")
+        messages = ["Mount (/mnt/this) successful",
+                    "Mounting /dev/vdx1 as the device path as the root device "
+                    "is already partitioned."]
+        for message in messages:
+            self.assertTrue(
+                util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+        # Wrote proper fstab mount info to mount on reboot
+        fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"
+        with open(util.FSTAB_FILE) as input_file:
+            self.assertEqual(input_file.read(), fstab_content)
+
+        # Reported success to principal
+        self.assertEqual(
+            util.hookenv._outgoing_relation_data,
+            (("mountpoint", "/mnt/this"),))