Merge lp:~verterok/charms/precise/storage/wait-a-bit-longer-if-needed into lp:charms/storage

[Guillermo Gonzalez]

Add a device_timeout config, default to current 50sec. value to allow configuring how long it waits for a device. Also fix a couple of tests that failed when comparing strings with a \n at the end

Add a device_timeout config, default to current 50sec. value to allow configuring how long it waits for a device. Also fix a couple of tests that failed when comparing strings with a \n at the end

[Cory Johns (johnsca)]

This seems entirely reasonable to me, and both the existing and new test cases pass.

+1

review: Approve

[Charles Butler (lazypower)]

+1 LGTM. Thank you for taking the time to submit this fix for the charm store. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2014-12-10 16:32:12 +0000
+++ config.yaml	2015-06-25 18:28:38 +0000
@@ -26,3 +26,8 @@
         description: |
             The volume size in GB to request from the block-storage-broker.
         default: 5
+    device_timeout: 
+        type: int
+        description: |
+          Max time to wait for a device, in seconds.
+        default: 50

=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-12-10 16:32:12 +0000
+++ hooks/common_util.py	2015-06-25 18:28:38 +0000
@@ -11,6 +11,8 @@
 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"
+DEFAULT_DEVICE_TIMEOUT = 50
+DEVICE_RETRIES = 10.0
 
 
 def log(message, level=None):
@@ -123,10 +125,10 @@
         sys.exit(1)
 
 
-def _assert_block_device(device_path):
+def _assert_block_device(device_path, device_timeout=DEFAULT_DEVICE_TIMEOUT):
     """C{device_path} is a valid block device or exit in error"""
     isBlockDevice = False
-    for x in range(0, 10):
+    for x in range(0, int(DEVICE_RETRIES)):
         if not os.path.exists(device_path):
             log("WARNING: %s does not exist" % device_path)
         else:
@@ -140,7 +142,7 @@
                     break
             except OSError:
                 pass
-        sleep(5)
+        sleep(device_timeout/DEVICE_RETRIES)
     if not isBlockDevice:
         log("ERROR: %s is not a block device." % device_path)
         sys.exit(1)
@@ -211,7 +213,7 @@
     return None
 
 
-def mount_volume(device_path=None):
+def mount_volume(device_path=None, device_timeout=DEFAULT_DEVICE_TIMEOUT):
     """
     Mount the attached volume, nfs or block-storage-broker type, to the
     principal requested C{mountpoint}.
@@ -262,7 +264,7 @@
                 "block-storage-broker.")
             sys.exit(0)
 
-        _assert_block_device(device_path)
+        _assert_block_device(device_path, device_timeout)
         if os.path.exists("%s1" % device_path):
             # Then we are supporting upgrade-charm path for deprecated
             # postgresql volume-map config parameter. Postgresql used to

=== 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-03-07 08:01:15 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2015-06-25 18:28:38 +0000
@@ -48,4 +48,5 @@
 # data relation
 device_path = common_util._get_from_relation(
     "block-storage", "block-device-path")
-common_util.mount_volume(device_path)  # Will wait for mountpoint
+common_util.mount_volume(device_path,  # Will wait for mountpoint
+                         device_timeout=config_data.get('device_timeout'))

=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-12-10 16:32:12 +0000
+++ hooks/test_common_util.py	2015-06-25 18:28:38 +0000
@@ -7,6 +7,10 @@
 from time import sleep
 
 
+DEFAULT_DEVICE_TIMEOUT = 50
+DEVICE_RETRIES = 10.0
+
+
 class TestCommonUtil(mocker.MockerTestCase):
 
     def setUp(self):
@@ -400,7 +404,7 @@
         self.mocker.result(False)
         self.mocker.count(10)
         sleep_mock = self.mocker.replace(sleep)
-        sleep_mock(5)
+        sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
         self.mocker.count(10)
         self.mocker.replay()
 
@@ -426,7 +430,7 @@
         self.mocker.count(10)
         self.mocker.throw(OSError)  # When device file doesn't exist
         sleep_mock = self.mocker.replace(sleep)
-        sleep_mock(5)
+        sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
         self.mocker.count(10)
         self.mocker.replay()
 
@@ -608,7 +612,7 @@
         has not yet defined by the relation principal yet.
         """
         self.assertIsNone(util.hookenv.relation_get("mountpoint"))
-        util.mount_volume()
+        util.mount_volume(device_timeout=0)
         message = "No mountpoint defined by relation. Cannot mount volume yet."
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -628,7 +632,7 @@
         self.mocker.result(True)
         self.mocker.replay()
 
-        util.mount_volume()
+        util.mount_volume(device_timeout=1)
         self.assertIn(("mountpoint", "/mnt/this"),
                       util.hookenv._outgoing_relation_data)
         message = "Volume (/mnt/this) already mounted"
@@ -655,7 +659,7 @@
         self.mocker.result(True)
         self.mocker.replay()
 
-        util.mount_volume()
+        util.mount_volume(device_timeout=1)
         message = "Volume (/mnt/this) already mounted"
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -684,7 +688,7 @@
             self.assertEqual(
                 util.hookenv.relation_get("mountpoint", rid="data:1"),
                 "/mnt/this")
-            util.mount_volume()
+            util.mount_volume(device_timeout=1)
             message = (
                 "Missing required relation values. "
                 "nfs-relation-changed hook needs to run first.")
@@ -727,7 +731,7 @@
         self.mocker.replay()
 
         self.assertEqual(util.get_provider(), "nfs")
-        util.mount_volume()
+        util.mount_volume(device_timeout=1)
         message = "Mount (/mnt/this) successful"
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -735,7 +739,7 @@
         # Wrote proper fstab mount info to mount on reboot
         fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
         with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
+            self.assertEqual(input_file.read().strip(), fstab_content)
 
         # Reported success to principal
         self.assertEqual(
@@ -785,7 +789,7 @@
         self.mocker.result(False)  # Mock not mounted
 
         assert_device = self.mocker.replace(util._assert_block_device)
-        assert_device("/dev/vdx")
+        assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
         exists = self.mocker.replace(os.path.exists)
         exists("/dev/vdx1")
         self.mocker.result(False)
@@ -815,9 +819,9 @@
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
         # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"
+        fstab_content = "/dev/vdx /mnt/this ext4 defaults 0 0"
         with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
+            self.assertEqual(input_file.read().strip(), fstab_content)
 
         # Reported success to principal
         self.assertEqual(
@@ -845,7 +849,7 @@
         self.mocker.result(False)  # Mock not mounted
 
         assert_device = self.mocker.replace(util._assert_block_device)
-        assert_device("/dev/vdx")
+        assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
         exists = self.mocker.replace(os.path.exists)
         exists("/dev/vdx1")
         self.mocker.result(True)
@@ -878,9 +882,9 @@
                 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"
+        fstab_content = "/dev/vdx1 /mnt/this ext4 defaults 0 0"
         with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
+            self.assertEqual(input_file.read().strip(), fstab_content)
 
         # Reported success to principal
         self.assertEqual(