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
1=== modified file 'config.yaml'
2--- config.yaml 2014-12-10 16:32:12 +0000
3+++ config.yaml 2015-06-25 18:28:38 +0000
4@@ -26,3 +26,8 @@
5 description: |
6 The volume size in GB to request from the block-storage-broker.
7 default: 5
8+ device_timeout:
9+ type: int
10+ description: |
11+ Max time to wait for a device, in seconds.
12+ default: 50
13
14=== modified file 'hooks/common_util.py'
15--- hooks/common_util.py 2014-12-10 16:32:12 +0000
16+++ hooks/common_util.py 2015-06-25 18:28:38 +0000
17@@ -11,6 +11,8 @@
18 EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data"
19 OS_METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
20 PERSIST_DATA_FILE = "charm-data"
21+DEFAULT_DEVICE_TIMEOUT = 50
22+DEVICE_RETRIES = 10.0
23
24
25 def log(message, level=None):
26@@ -123,10 +125,10 @@
27 sys.exit(1)
28
29
30-def _assert_block_device(device_path):
31+def _assert_block_device(device_path, device_timeout=DEFAULT_DEVICE_TIMEOUT):
32 """C{device_path} is a valid block device or exit in error"""
33 isBlockDevice = False
34- for x in range(0, 10):
35+ for x in range(0, int(DEVICE_RETRIES)):
36 if not os.path.exists(device_path):
37 log("WARNING: %s does not exist" % device_path)
38 else:
39@@ -140,7 +142,7 @@
40 break
41 except OSError:
42 pass
43- sleep(5)
44+ sleep(device_timeout/DEVICE_RETRIES)
45 if not isBlockDevice:
46 log("ERROR: %s is not a block device." % device_path)
47 sys.exit(1)
48@@ -211,7 +213,7 @@
49 return None
50
51
52-def mount_volume(device_path=None):
53+def mount_volume(device_path=None, device_timeout=DEFAULT_DEVICE_TIMEOUT):
54 """
55 Mount the attached volume, nfs or block-storage-broker type, to the
56 principal requested C{mountpoint}.
57@@ -262,7 +264,7 @@
58 "block-storage-broker.")
59 sys.exit(0)
60
61- _assert_block_device(device_path)
62+ _assert_block_device(device_path, device_timeout)
63 if os.path.exists("%s1" % device_path):
64 # Then we are supporting upgrade-charm path for deprecated
65 # postgresql volume-map config parameter. Postgresql used to
66
67=== modified file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
68--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2014-03-07 08:01:15 +0000
69+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2015-06-25 18:28:38 +0000
70@@ -48,4 +48,5 @@
71 # data relation
72 device_path = common_util._get_from_relation(
73 "block-storage", "block-device-path")
74-common_util.mount_volume(device_path) # Will wait for mountpoint
75+common_util.mount_volume(device_path, # Will wait for mountpoint
76+ device_timeout=config_data.get('device_timeout'))
77
78=== modified file 'hooks/test_common_util.py'
79--- hooks/test_common_util.py 2014-12-10 16:32:12 +0000
80+++ hooks/test_common_util.py 2015-06-25 18:28:38 +0000
81@@ -7,6 +7,10 @@
82 from time import sleep
83
84
85+DEFAULT_DEVICE_TIMEOUT = 50
86+DEVICE_RETRIES = 10.0
87+
88+
89 class TestCommonUtil(mocker.MockerTestCase):
90
91 def setUp(self):
92@@ -400,7 +404,7 @@
93 self.mocker.result(False)
94 self.mocker.count(10)
95 sleep_mock = self.mocker.replace(sleep)
96- sleep_mock(5)
97+ sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
98 self.mocker.count(10)
99 self.mocker.replay()
100
101@@ -426,7 +430,7 @@
102 self.mocker.count(10)
103 self.mocker.throw(OSError) # When device file doesn't exist
104 sleep_mock = self.mocker.replace(sleep)
105- sleep_mock(5)
106+ sleep_mock(DEFAULT_DEVICE_TIMEOUT/DEVICE_RETRIES)
107 self.mocker.count(10)
108 self.mocker.replay()
109
110@@ -608,7 +612,7 @@
111 has not yet defined by the relation principal yet.
112 """
113 self.assertIsNone(util.hookenv.relation_get("mountpoint"))
114- util.mount_volume()
115+ util.mount_volume(device_timeout=0)
116 message = "No mountpoint defined by relation. Cannot mount volume yet."
117 self.assertTrue(
118 util.hookenv.is_logged(message), "Not logged- %s" % message)
119@@ -628,7 +632,7 @@
120 self.mocker.result(True)
121 self.mocker.replay()
122
123- util.mount_volume()
124+ util.mount_volume(device_timeout=1)
125 self.assertIn(("mountpoint", "/mnt/this"),
126 util.hookenv._outgoing_relation_data)
127 message = "Volume (/mnt/this) already mounted"
128@@ -655,7 +659,7 @@
129 self.mocker.result(True)
130 self.mocker.replay()
131
132- util.mount_volume()
133+ util.mount_volume(device_timeout=1)
134 message = "Volume (/mnt/this) already mounted"
135 self.assertTrue(
136 util.hookenv.is_logged(message), "Not logged- %s" % message)
137@@ -684,7 +688,7 @@
138 self.assertEqual(
139 util.hookenv.relation_get("mountpoint", rid="data:1"),
140 "/mnt/this")
141- util.mount_volume()
142+ util.mount_volume(device_timeout=1)
143 message = (
144 "Missing required relation values. "
145 "nfs-relation-changed hook needs to run first.")
146@@ -727,7 +731,7 @@
147 self.mocker.replay()
148
149 self.assertEqual(util.get_provider(), "nfs")
150- util.mount_volume()
151+ util.mount_volume(device_timeout=1)
152 message = "Mount (/mnt/this) successful"
153 self.assertTrue(
154 util.hookenv.is_logged(message), "Not logged- %s" % message)
155@@ -735,7 +739,7 @@
156 # Wrote proper fstab mount info to mount on reboot
157 fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
158 with open(util.FSTAB_FILE) as input_file:
159- self.assertEqual(input_file.read(), fstab_content)
160+ self.assertEqual(input_file.read().strip(), fstab_content)
161
162 # Reported success to principal
163 self.assertEqual(
164@@ -785,7 +789,7 @@
165 self.mocker.result(False) # Mock not mounted
166
167 assert_device = self.mocker.replace(util._assert_block_device)
168- assert_device("/dev/vdx")
169+ assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
170 exists = self.mocker.replace(os.path.exists)
171 exists("/dev/vdx1")
172 self.mocker.result(False)
173@@ -815,9 +819,9 @@
174 util.hookenv.is_logged(message), "Not logged- %s" % message)
175
176 # Wrote proper fstab mount info to mount on reboot
177- fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"
178+ fstab_content = "/dev/vdx /mnt/this ext4 defaults 0 0"
179 with open(util.FSTAB_FILE) as input_file:
180- self.assertEqual(input_file.read(), fstab_content)
181+ self.assertEqual(input_file.read().strip(), fstab_content)
182
183 # Reported success to principal
184 self.assertEqual(
185@@ -845,7 +849,7 @@
186 self.mocker.result(False) # Mock not mounted
187
188 assert_device = self.mocker.replace(util._assert_block_device)
189- assert_device("/dev/vdx")
190+ assert_device("/dev/vdx", DEFAULT_DEVICE_TIMEOUT)
191 exists = self.mocker.replace(os.path.exists)
192 exists("/dev/vdx1")
193 self.mocker.result(True)
194@@ -878,9 +882,9 @@
195 util.hookenv.is_logged(message), "Not logged- %s" % message)
196
197 # Wrote proper fstab mount info to mount on reboot
198- fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"
199+ fstab_content = "/dev/vdx1 /mnt/this ext4 defaults 0 0"
200 with open(util.FSTAB_FILE) as input_file:
201- self.assertEqual(input_file.read(), fstab_content)
202+ self.assertEqual(input_file.read().strip(), fstab_content)
203
204 # Reported success to principal
205 self.assertEqual(

Subscribers

People subscribed via source and target branches

to all changes: