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

Proposed by Guillermo Gonzalez
Status: Merged
Merged at revision: 41
Proposed branch: lp:~verterok/charms/precise/storage/wait-a-bit-longer-if-needed
Merge into: lp:charms/storage
Diff against target: 205 lines (+32/-20)
4 files modified
config.yaml (+5/-0)
hooks/common_util.py (+7/-5)
hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed (+2/-1)
hooks/test_common_util.py (+18/-14)
To merge this branch: bzr merge lp:~verterok/charms/precise/storage/wait-a-bit-longer-if-needed
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Cory Johns Approve
Review via email: mp+263016@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Cory Johns (johnsca) wrote :

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

+1

review: Approve
Revision history for this message
Charles Butler (lazypower) wrote :

+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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
26 description: |26 description: |
27 The volume size in GB to request from the block-storage-broker.27 The volume size in GB to request from the block-storage-broker.
28 default: 528 default: 5
29 device_timeout:
30 type: int
31 description: |
32 Max time to wait for a device, in seconds.
33 default: 50
2934
=== 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 @@
11EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data"11EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data"
12OS_METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"12OS_METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
13PERSIST_DATA_FILE = "charm-data"13PERSIST_DATA_FILE = "charm-data"
14DEFAULT_DEVICE_TIMEOUT = 50
15DEVICE_RETRIES = 10.0
1416
1517
16def log(message, level=None):18def log(message, level=None):
@@ -123,10 +125,10 @@
123 sys.exit(1)125 sys.exit(1)
124126
125127
126def _assert_block_device(device_path):128def _assert_block_device(device_path, device_timeout=DEFAULT_DEVICE_TIMEOUT):
127 """C{device_path} is a valid block device or exit in error"""129 """C{device_path} is a valid block device or exit in error"""
128 isBlockDevice = False130 isBlockDevice = False
129 for x in range(0, 10):131 for x in range(0, int(DEVICE_RETRIES)):
130 if not os.path.exists(device_path):132 if not os.path.exists(device_path):
131 log("WARNING: %s does not exist" % device_path)133 log("WARNING: %s does not exist" % device_path)
132 else:134 else:
@@ -140,7 +142,7 @@
140 break142 break
141 except OSError:143 except OSError:
142 pass144 pass
143 sleep(5)145 sleep(device_timeout/DEVICE_RETRIES)
144 if not isBlockDevice:146 if not isBlockDevice:
145 log("ERROR: %s is not a block device." % device_path)147 log("ERROR: %s is not a block device." % device_path)
146 sys.exit(1)148 sys.exit(1)
@@ -211,7 +213,7 @@
211 return None213 return None
212214
213215
214def mount_volume(device_path=None):216def mount_volume(device_path=None, device_timeout=DEFAULT_DEVICE_TIMEOUT):
215 """217 """
216 Mount the attached volume, nfs or block-storage-broker type, to the218 Mount the attached volume, nfs or block-storage-broker type, to the
217 principal requested C{mountpoint}.219 principal requested C{mountpoint}.
@@ -262,7 +264,7 @@
262 "block-storage-broker.")264 "block-storage-broker.")
263 sys.exit(0)265 sys.exit(0)
264266
265 _assert_block_device(device_path)267 _assert_block_device(device_path, device_timeout)
266 if os.path.exists("%s1" % device_path):268 if os.path.exists("%s1" % device_path):
267 # Then we are supporting upgrade-charm path for deprecated269 # Then we are supporting upgrade-charm path for deprecated
268 # postgresql volume-map config parameter. Postgresql used to270 # postgresql volume-map config parameter. Postgresql used to
269271
=== 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 @@
48# data relation48# data relation
49device_path = common_util._get_from_relation(49device_path = common_util._get_from_relation(
50 "block-storage", "block-device-path")50 "block-storage", "block-device-path")
51common_util.mount_volume(device_path) # Will wait for mountpoint51common_util.mount_volume(device_path, # Will wait for mountpoint
52 device_timeout=config_data.get('device_timeout'))
5253
=== 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 @@
7from time import sleep7from time import sleep
88
99
10DEFAULT_DEVICE_TIMEOUT = 50
11DEVICE_RETRIES = 10.0
12
13
10class TestCommonUtil(mocker.MockerTestCase):14class TestCommonUtil(mocker.MockerTestCase):
1115
12 def setUp(self):16 def setUp(self):
@@ -400,7 +404,7 @@
400 self.mocker.result(False)404 self.mocker.result(False)
401 self.mocker.count(10)405 self.mocker.count(10)
402 sleep_mock = self.mocker.replace(sleep)406 sleep_mock = self.mocker.replace(sleep)
403 sleep_mock(5)407 sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
404 self.mocker.count(10)408 self.mocker.count(10)
405 self.mocker.replay()409 self.mocker.replay()
406410
@@ -426,7 +430,7 @@
426 self.mocker.count(10)430 self.mocker.count(10)
427 self.mocker.throw(OSError) # When device file doesn't exist431 self.mocker.throw(OSError) # When device file doesn't exist
428 sleep_mock = self.mocker.replace(sleep)432 sleep_mock = self.mocker.replace(sleep)
429 sleep_mock(5)433 sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
430 self.mocker.count(10)434 self.mocker.count(10)
431 self.mocker.replay()435 self.mocker.replay()
432436
@@ -608,7 +612,7 @@
608 has not yet defined by the relation principal yet.612 has not yet defined by the relation principal yet.
609 """613 """
610 self.assertIsNone(util.hookenv.relation_get("mountpoint"))614 self.assertIsNone(util.hookenv.relation_get("mountpoint"))
611 util.mount_volume()615 util.mount_volume(device_timeout=0)
612 message = "No mountpoint defined by relation. Cannot mount volume yet."616 message = "No mountpoint defined by relation. Cannot mount volume yet."
613 self.assertTrue(617 self.assertTrue(
614 util.hookenv.is_logged(message), "Not logged- %s" % message)618 util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -628,7 +632,7 @@
628 self.mocker.result(True)632 self.mocker.result(True)
629 self.mocker.replay()633 self.mocker.replay()
630634
631 util.mount_volume()635 util.mount_volume(device_timeout=1)
632 self.assertIn(("mountpoint", "/mnt/this"),636 self.assertIn(("mountpoint", "/mnt/this"),
633 util.hookenv._outgoing_relation_data)637 util.hookenv._outgoing_relation_data)
634 message = "Volume (/mnt/this) already mounted"638 message = "Volume (/mnt/this) already mounted"
@@ -655,7 +659,7 @@
655 self.mocker.result(True)659 self.mocker.result(True)
656 self.mocker.replay()660 self.mocker.replay()
657661
658 util.mount_volume()662 util.mount_volume(device_timeout=1)
659 message = "Volume (/mnt/this) already mounted"663 message = "Volume (/mnt/this) already mounted"
660 self.assertTrue(664 self.assertTrue(
661 util.hookenv.is_logged(message), "Not logged- %s" % message)665 util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -684,7 +688,7 @@
684 self.assertEqual(688 self.assertEqual(
685 util.hookenv.relation_get("mountpoint", rid="data:1"),689 util.hookenv.relation_get("mountpoint", rid="data:1"),
686 "/mnt/this")690 "/mnt/this")
687 util.mount_volume()691 util.mount_volume(device_timeout=1)
688 message = (692 message = (
689 "Missing required relation values. "693 "Missing required relation values. "
690 "nfs-relation-changed hook needs to run first.")694 "nfs-relation-changed hook needs to run first.")
@@ -727,7 +731,7 @@
727 self.mocker.replay()731 self.mocker.replay()
728732
729 self.assertEqual(util.get_provider(), "nfs")733 self.assertEqual(util.get_provider(), "nfs")
730 util.mount_volume()734 util.mount_volume(device_timeout=1)
731 message = "Mount (/mnt/this) successful"735 message = "Mount (/mnt/this) successful"
732 self.assertTrue(736 self.assertTrue(
733 util.hookenv.is_logged(message), "Not logged- %s" % message)737 util.hookenv.is_logged(message), "Not logged- %s" % message)
@@ -735,7 +739,7 @@
735 # Wrote proper fstab mount info to mount on reboot739 # Wrote proper fstab mount info to mount on reboot
736 fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"740 fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
737 with open(util.FSTAB_FILE) as input_file:741 with open(util.FSTAB_FILE) as input_file:
738 self.assertEqual(input_file.read(), fstab_content)742 self.assertEqual(input_file.read().strip(), fstab_content)
739743
740 # Reported success to principal744 # Reported success to principal
741 self.assertEqual(745 self.assertEqual(
@@ -785,7 +789,7 @@
785 self.mocker.result(False) # Mock not mounted789 self.mocker.result(False) # Mock not mounted
786790
787 assert_device = self.mocker.replace(util._assert_block_device)791 assert_device = self.mocker.replace(util._assert_block_device)
788 assert_device("/dev/vdx")792 assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
789 exists = self.mocker.replace(os.path.exists)793 exists = self.mocker.replace(os.path.exists)
790 exists("/dev/vdx1")794 exists("/dev/vdx1")
791 self.mocker.result(False)795 self.mocker.result(False)
@@ -815,9 +819,9 @@
815 util.hookenv.is_logged(message), "Not logged- %s" % message)819 util.hookenv.is_logged(message), "Not logged- %s" % message)
816820
817 # Wrote proper fstab mount info to mount on reboot821 # Wrote proper fstab mount info to mount on reboot
818 fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"822 fstab_content = "/dev/vdx /mnt/this ext4 defaults 0 0"
819 with open(util.FSTAB_FILE) as input_file:823 with open(util.FSTAB_FILE) as input_file:
820 self.assertEqual(input_file.read(), fstab_content)824 self.assertEqual(input_file.read().strip(), fstab_content)
821825
822 # Reported success to principal826 # Reported success to principal
823 self.assertEqual(827 self.assertEqual(
@@ -845,7 +849,7 @@
845 self.mocker.result(False) # Mock not mounted849 self.mocker.result(False) # Mock not mounted
846850
847 assert_device = self.mocker.replace(util._assert_block_device)851 assert_device = self.mocker.replace(util._assert_block_device)
848 assert_device("/dev/vdx")852 assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
849 exists = self.mocker.replace(os.path.exists)853 exists = self.mocker.replace(os.path.exists)
850 exists("/dev/vdx1")854 exists("/dev/vdx1")
851 self.mocker.result(True)855 self.mocker.result(True)
@@ -878,9 +882,9 @@
878 util.hookenv.is_logged(message), "Not logged- %s" % message)882 util.hookenv.is_logged(message), "Not logged- %s" % message)
879883
880 # Wrote proper fstab mount info to mount on reboot884 # Wrote proper fstab mount info to mount on reboot
881 fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"885 fstab_content = "/dev/vdx1 /mnt/this ext4 defaults 0 0"
882 with open(util.FSTAB_FILE) as input_file:886 with open(util.FSTAB_FILE) as input_file:
883 self.assertEqual(input_file.read(), fstab_content)887 self.assertEqual(input_file.read().strip(), fstab_content)
884888
885 # Reported success to principal889 # Reported success to principal
886 self.assertEqual(890 self.assertEqual(

Subscribers

People subscribed via source and target branches

to all changes: