Merge lp:~chad.smith/charms/precise/storage/storage-volume-label-availability-zone into lp:~dpb/charms/precise/storage/trunk

[Chad Smith]

Volumes in EC2-land can only be attached to instances that live in the same availability zone. Block-storage-broker takes whatever volume_label is requested by the storage subordinate and attempts to attach it to the instance-id which is requesting that volume_label. If the instance and volume live in different AZs, the attach will fail and Block-storage-broker hooks will fail.

    To avoid trying to euca-attach-volume of a volume in another AZ, we need to request that our volume label (volume_label translates to a volume TAG in EC2) specify the availability zone in which that volume is created. For openstack and nova, the AZ is always nova, but for ec2, the AZ will be determined as the AZ in which the requesting instance-id lives (us-east-1b, us-west-1a etc.)

This way, the block-storage-broker will not attempt to mount any discovered volume from different availability zones.

There will need to be more significant work in the block-storage-broker charm to also attempt to assess volumes and instances in incompatible AZs, but this current branch prevents a storage-principal unit from requesting an invalid volume_label.

This branch adds a get_availability_zone function to allow extracting this value from EC2 or openstack metadata.

Also in this branch:
   dropping the lsof -b (non-blocking) option, as called currently it was not a valid option. Will look at non-blocking lsof in a separate branch.

[David Britton (dpb)]

[0] When relating to bsb, I recieved:

2014-03-20 22:59:28 INFO worker.uniter.jujuc server.go:108 running hook tool "juju-log" ["Validated charm configuration credentials have access to block storage service"]
2014-03-20 22:59:28 DEBUG worker.uniter.jujuc server.go:109 hook context id "block-storage-broker-ec2/0:block-storage-relation-changed:3470414816341938807"; dir "/var/lib/juju/agents/unit-block-storage-broker-ec2-0/charm"
2014-03-20 22:59:28 INFO juju juju-log.go:66 block-storage-broker-ec2/0 block-storage:2: Validated charm configuration credentials have access to block storage service
2014-03-20 22:59:29 INFO worker.uniter.jujuc server.go:108 running hook tool "juju-log" ["Creating a 9Gig volume named (postgresql/0 us-west-1c volume) for instance i-53df8d0c"]
2014-03-20 22:59:29 DEBUG worker.uniter.jujuc server.go:109 hook context id "block-storage-broker-ec2/0:block-storage-relation-changed:3470414816341938807"; dir "/var/lib/juju/agents/unit-block-storage-broker-ec2-0/charm"
2014-03-20 22:59:29 INFO juju juju-log.go:66 block-storage-broker-ec2/0 block-storage:2: Creating a 9Gig volume named (postgresql/0 us-west-1c volume) for instance i-53df8d0c
2014-03-20 22:59:29 INFO juju.worker.uniter context.go:255 HOOK 'availability_zone'
2014-03-20 22:59:29 ERROR juju.worker.uniter uniter.go:350 hook failed: exit status 1

[lp:~chad.smith/charms/precise/storage/storage-volume-label-availability-zone]

40. By Chad Smith on 2014-03-21

drop unused CHARM_DIR global

[David Britton (dpb)]

Note: Looks good during testing. Starting code review now.

[lp:~chad.smith/charms/precise/storage/storage-volume-label-availability-zone]

41. By Chad Smith on 2014-03-21

drop misused -b option to lsof

42. By Chad Smith on 2014-03-21

lint

[Fernando Correa Neto (fcorrea)]

Looks great! +1

[1]
71 + except subprocess.CalledProcessError, e:
72 + log("Error: couldn't discover ec2 availability zone. %s" % str(e),
73 + hookenv.ERROR)

Even though this block is for the ec2 try, it only indicates that it failed grabbing both nova and ec2, in which case I think we could log something more generic. Maybe "Error: couldn't discover volume availability zone..."

review: Approve

[David Britton (dpb)]

+1, let's merge this in as well.

review: Approve

=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-03-21 22:53:33 +0000
+++ hooks/common_util.py	2014-03-21 22:53:33 +0000
@@ -7,9 +7,8 @@
 from stat import S_ISBLK, ST_MODE
 import json
 
-CHARM_DIR = os.environ.get("CHARM_DIR")
 FSTAB_FILE = "/etc/fstab"
-EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data/instance-id"
+EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data"
 OS_METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
 PERSIST_DATA_FILE = "charm-data"
 
@@ -108,12 +107,12 @@
             else:  # Then device is in use let's report the offending process
                 message = "WARNING: umount %s failed. Retrying." % mountpoint
                 try:
-                    lsof = subprocess.check_output(["lsof", mountpoint, "-b"])
+                    lsof = subprocess.check_output(["lsof", mountpoint])
                 except subprocess.CalledProcessError, e:
                     log("WARNING: %s" % str(e), hookenv.WARNING)
                 else:
                     process_name = lsof.split("\n")[1].split()[0]
-                    message += " Device in use by (%s)" % process_name 
+                    message += " Device in use by (%s)" % process_name
                 log(message, hookenv.WARNING)
             sleep(5)
 
@@ -306,16 +305,39 @@
     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"]
+    except ValueError:
+        pass  # We are probably on EC2, try below
     try:
         metadata = subprocess.check_output(
-            "curl %s" % EC2_METADATA_URL, shell=True)
+            "curl %s/instance-id" % EC2_METADATA_URL, shell=True)
+        return metadata.strip()
     except subprocess.CalledProcessError, e:
         log("Error: couldn't discover ec2 instance id. %s" % str(e),
             hookenv.ERROR)
         sys.exit(1)
-    else:
+
+
+def get_availability_zone():
+    """
+    Curl metadata for openstack from C{OS_METADATA_URL} to obtain the
+    C{availability_zone}.
+    If that fails, we are likely on EC2, so curl the
+    C{EC2_METADATA_URL/placement/availability-zone} to get the EC2 availability
+    zone.
+    """
+    try:
+        metadata = json.loads(
+            subprocess.check_output("curl %s" % OS_METADATA_URL, shell=True))
+        return metadata["availability_zone"]
+    except ValueError:
+        pass  # We are probably on EC2, try below
+    try:
+        metadata = subprocess.check_output(
+            "curl %s/placement/availability-zone" % EC2_METADATA_URL,
+            shell=True)
         return metadata.strip()
+    except subprocess.CalledProcessError, e:
+        log("Error: couldn't discover ec2 availability zone. %s" % str(e),
+            hookenv.ERROR)
+        sys.exit(1)

=== modified file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2014-02-13 00:41:13 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2014-03-21 22:53:33 +0000
@@ -8,6 +8,7 @@
 
 
 instance_id = common_util.get_instance_id()
+availability_zone = common_util.get_availability_zone()
 config_data = hookenv.config()
 volume_size = config_data.get("volume_size", None)
 volume_map = config_data.get("volume_map", None)
@@ -16,7 +17,7 @@
 relids = hookenv.relation_ids("data")
 for relid in relids:
     for unit in hookenv.related_units(relid):
-        label = "%s nova volume" % unit
+        label = "%s %s volume" % (unit, availability_zone)
         if volume_map is not None:
             # Grab the volume_label for our instance if it exists so we can
             # request that volume-id from the block-storage-broker

=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-03-21 22:53:33 +0000
+++ hooks/test_common_util.py	2014-03-21 22:53:33 +0000
@@ -164,7 +164,7 @@
         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)
+        curl("curl %s/instance-id" % util.EC2_METADATA_URL, shell=True)
         self.mocker.result(instance_id)
         self.mocker.replay()
         self.assertEqual(util.get_instance_id(), instance_id)
@@ -174,15 +174,14 @@
         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
+        ec2_command = "curl %s/instance-id" % 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))
+            subprocess.CalledProcessError(1, ec2_command))
         self.mocker.replay()
         result = self.assertRaises(SystemExit, util.get_instance_id)
         self.assertEqual(result.code, 1)
@@ -192,6 +191,60 @@
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
+    def test_get_availability_zone_when_openstack(self):
+        """
+        L{get_availability_zone} will attempt a curl for openstack metadata to
+        extract the C{uuid} as the instance_id from openstack JSON metadata.
+        """
+        os_metadata = {
+            "availability_zone": "nova"
+        }
+        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_availability_zone(), "nova")
+
+    def test_get_availability_zone_when_ec2(self):
+        """
+        L{get_availability_zone} 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{availability-zone}.
+        """
+        availability_zone = "us-east-1b"
+        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/placement/availability-zone" %
+             util.EC2_METADATA_URL, shell=True)
+        self.mocker.result(availability_zone)
+        self.mocker.replay()
+        self.assertEqual(util.get_availability_zone(), availability_zone)
+
+    def test_get_availability_zone_fails_on_ec2(self):
+        """
+        When L{get_availability_zone} fails to curl C{EC2_METADATA_URL} it will
+        log the error and exit in error.
+        """
+        ec2_command = ("curl %s/placement/availability-zone" %
+                       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, ec2_command))
+        self.mocker.replay()
+        result = self.assertRaises(SystemExit, util.get_availability_zone)
+        self.assertEqual(result.code, 1)
+        message = (
+            "Error: couldn't discover ec2 availability zone. 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.
@@ -262,7 +315,8 @@
 
         lsof_output = (
             "COMMAND     PID   USER   FD   TYPE DEVICE SIZE/OFF  NODE NAME\n"
-            "postgresql 30544 csmith   10r   CHR    1,9    0t0  2056 /mnt/this")
+            "postgresql 30544 csmith   10r   CHR    1,9    0t0  2056 "
+            "/mnt/this")
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -272,7 +326,7 @@
         self.mocker.result(2)  # Failure from the shell cmd
         self.mocker.count(20)
         lsof = self.mocker.replace(subprocess.check_output)
-        lsof(["lsof", "/mnt/this", "-b"])
+        lsof(["lsof", "/mnt/this"])
         self.mocker.result(lsof_output)
         self.mocker.count(20)
         sleep_mock = self.mocker.replace(sleep)
@@ -297,10 +351,10 @@
         self.mocker.result(2)  # Failure from the umount shell cmd
         self.mocker.count(20)
         lsof = self.mocker.replace(subprocess.check_output)
-        lsof(["lsof", "/mnt/this", "-b"])
+        lsof(["lsof", "/mnt/this"])
         self.mocker.count(20)
         self.mocker.throw(
-            subprocess.CalledProcessError(1, "lsof /mnt/this")) # lsof fail
+            subprocess.CalledProcessError(1, "lsof /mnt/this"))  # lsof fail
         sleep_mock = self.mocker.replace(sleep)
         sleep_mock(5)
         self.mocker.count(20)
@@ -310,7 +364,8 @@
         messages = [
             "ERROR: Unmount failed, leaving relation in errored state",
             "WARNING: umount /mnt/this failed. Retrying.",
-            "WARNING: Command 'lsof /mnt/this' returned non-zero exit status 1"]
+            "WARNING: Command 'lsof /mnt/this' returned non-zero exit status "
+            "1"]
         for message in messages:
             self.assertTrue(
                 util.hookenv.is_logged(message), "Not logged- %s" % message)