Merge lp:~chad.smith/charm-helpers/retry-add-apt-repository into lp:charm-helpers
- retry-add-apt-repository
- Merge into devel
Proposed by
Chad Smith
Status: | Merged |
---|---|
Merged at revision: | 705 |
Proposed branch: | lp:~chad.smith/charm-helpers/retry-add-apt-repository |
Merge into: | lp:charm-helpers |
Diff against target: |
230 lines (+91/-39) 2 files modified
charmhelpers/fetch/ubuntu.py (+51/-31) tests/fetch/test_fetch.py (+40/-8) |
To merge this branch: | bzr merge lp:~chad.smith/charm-helpers/retry-add-apt-repository |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Eric Snow (community) | Approve | ||
David Britton (community) | Approve | ||
Review via email: mp+318951@code.launchpad.net |
Commit message
Description of the change
Add 3 retries and logging messages to add-apt-repository attempts.
This branch involves some slight refactoring of _run_apt_command so that the common code _retry_command() can be reused by both _run_apt_command and add_source.
Note this branch also includes the local environment now in add-apt-repository calls which should also help in environments with proxy settings present.
To post a comment you must log in.
- 708. By Chad Smith
-
comment fix
- 709. By Chad Smith
-
- rename -APT_NO_
LOCK_RETRY_ COUNT -> CMD_RETRY_COUNT
- Retry add-apt-repository CMD_RETRY_COUNT (30) times instead of 3
- update unit tests - 710. By Chad Smith
-
lints
Revision history for this message
Eric Snow (ericsnowcurrently) : | # |
review:
Needs Fixing
Revision history for this message
Chad Smith (chad.smith) : | # |
- 711. By Chad Smith
-
address review comments, sensible defaults, drop fatal from from _run_with_retries to simplify logic
Revision history for this message
Chad Smith (chad.smith) : | # |
Revision history for this message
Eric Snow (ericsnowcurrently) wrote : | # |
Thanks! That's exactly how I was thinking it should look. :)
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmhelpers/fetch/ubuntu.py' | |||
2 | --- charmhelpers/fetch/ubuntu.py 2016-11-14 16:28:24 +0000 | |||
3 | +++ charmhelpers/fetch/ubuntu.py 2017-03-03 20:38:43 +0000 | |||
4 | @@ -116,8 +116,8 @@ | |||
5 | 116 | } | 116 | } |
6 | 117 | 117 | ||
7 | 118 | APT_NO_LOCK = 100 # The return code for "couldn't acquire lock" in APT. | 118 | APT_NO_LOCK = 100 # The return code for "couldn't acquire lock" in APT. |
10 | 119 | APT_NO_LOCK_RETRY_DELAY = 10 # Wait 10 seconds between apt lock checks. | 119 | CMD_RETRY_DELAY = 10 # Wait 10 seconds between command retries. |
11 | 120 | APT_NO_LOCK_RETRY_COUNT = 30 # Retry to acquire the lock X times. | 120 | CMD_RETRY_COUNT = 30 # Retry a failing fatal command X times. |
12 | 121 | 121 | ||
13 | 122 | 122 | ||
14 | 123 | def filter_installed_packages(packages): | 123 | def filter_installed_packages(packages): |
15 | @@ -249,7 +249,8 @@ | |||
16 | 249 | source.startswith('http') or | 249 | source.startswith('http') or |
17 | 250 | source.startswith('deb ') or | 250 | source.startswith('deb ') or |
18 | 251 | source.startswith('cloud-archive:')): | 251 | source.startswith('cloud-archive:')): |
20 | 252 | subprocess.check_call(['add-apt-repository', '--yes', source]) | 252 | cmd = ['add-apt-repository', '--yes', source] |
21 | 253 | _run_with_retries(cmd) | ||
22 | 253 | elif source.startswith('cloud:'): | 254 | elif source.startswith('cloud:'): |
23 | 254 | install(filter_installed_packages(['ubuntu-cloud-keyring']), | 255 | install(filter_installed_packages(['ubuntu-cloud-keyring']), |
24 | 255 | fatal=True) | 256 | fatal=True) |
25 | @@ -286,41 +287,60 @@ | |||
26 | 286 | key]) | 287 | key]) |
27 | 287 | 288 | ||
28 | 288 | 289 | ||
29 | 290 | def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,), | ||
30 | 291 | retry_message="", cmd_env=None): | ||
31 | 292 | """Run a command and retry until success or max_retries is reached. | ||
32 | 293 | |||
33 | 294 | :param: cmd: str: The apt command to run. | ||
34 | 295 | :param: max_retries: int: The number of retries to attempt on a fatal | ||
35 | 296 | command. Defaults to CMD_RETRY_COUNT. | ||
36 | 297 | :param: retry_exitcodes: tuple: Optional additional exit codes to retry. | ||
37 | 298 | Defaults to retry on exit code 1. | ||
38 | 299 | :param: retry_message: str: Optional log prefix emitted during retries. | ||
39 | 300 | :param: cmd_env: dict: Environment variables to add to the command run. | ||
40 | 301 | """ | ||
41 | 302 | |||
42 | 303 | env = os.environ.copy() | ||
43 | 304 | if cmd_env: | ||
44 | 305 | env.update(cmd_env) | ||
45 | 306 | |||
46 | 307 | if not retry_message: | ||
47 | 308 | retry_message = "Failed executing '{}'".format(" ".join(cmd)) | ||
48 | 309 | retry_message += ". Will retry in {} seconds".format(CMD_RETRY_DELAY) | ||
49 | 310 | |||
50 | 311 | retry_count = 0 | ||
51 | 312 | result = None | ||
52 | 313 | |||
53 | 314 | retry_results = (None,) + retry_exitcodes | ||
54 | 315 | while result in retry_results: | ||
55 | 316 | try: | ||
56 | 317 | result = subprocess.check_call(cmd, env=env) | ||
57 | 318 | except subprocess.CalledProcessError as e: | ||
58 | 319 | retry_count = retry_count + 1 | ||
59 | 320 | if retry_count > max_retries: | ||
60 | 321 | raise | ||
61 | 322 | result = e.returncode | ||
62 | 323 | log(retry_message) | ||
63 | 324 | time.sleep(CMD_RETRY_DELAY) | ||
64 | 325 | |||
65 | 326 | |||
66 | 289 | def _run_apt_command(cmd, fatal=False): | 327 | def _run_apt_command(cmd, fatal=False): |
73 | 290 | """Run an APT command. | 328 | """Run an apt command with optional retries. |
74 | 291 | 329 | ||
69 | 292 | Checks the output and retries if the fatal flag is set | ||
70 | 293 | to True. | ||
71 | 294 | |||
72 | 295 | :param: cmd: str: The apt command to run. | ||
75 | 296 | :param: fatal: bool: Whether the command's output should be checked and | 330 | :param: fatal: bool: Whether the command's output should be checked and |
76 | 297 | retried. | 331 | retried. |
77 | 298 | """ | 332 | """ |
82 | 299 | env = os.environ.copy() | 333 | # Provide DEBIAN_FRONTEND=noninteractive if not present in the environment. |
83 | 300 | 334 | cmd_env = { | |
84 | 301 | if 'DEBIAN_FRONTEND' not in env: | 335 | 'DEBIAN_FRONTEND': os.environ.get('DEBIAN_FRONTEND', 'noninteractive')} |
81 | 302 | env['DEBIAN_FRONTEND'] = 'noninteractive' | ||
85 | 303 | 336 | ||
86 | 304 | if fatal: | 337 | if fatal: |
105 | 305 | retry_count = 0 | 338 | _run_with_retries( |
106 | 306 | result = None | 339 | cmd, cmd_env=cmd_env, retry_exitcodes=(1, APT_NO_LOCK,), |
107 | 307 | 340 | retry_message="Couldn't acquire DPKG lock") | |
90 | 308 | # If the command is considered "fatal", we need to retry if the apt | ||
91 | 309 | # lock was not acquired. | ||
92 | 310 | |||
93 | 311 | while result is None or result == APT_NO_LOCK: | ||
94 | 312 | try: | ||
95 | 313 | result = subprocess.check_call(cmd, env=env) | ||
96 | 314 | except subprocess.CalledProcessError as e: | ||
97 | 315 | retry_count = retry_count + 1 | ||
98 | 316 | if retry_count > APT_NO_LOCK_RETRY_COUNT: | ||
99 | 317 | raise | ||
100 | 318 | result = e.returncode | ||
101 | 319 | log("Couldn't acquire DPKG lock. Will retry in {} seconds." | ||
102 | 320 | "".format(APT_NO_LOCK_RETRY_DELAY)) | ||
103 | 321 | time.sleep(APT_NO_LOCK_RETRY_DELAY) | ||
104 | 322 | |||
108 | 323 | else: | 341 | else: |
109 | 342 | env = os.environ.copy() | ||
110 | 343 | env.update(cmd_env) | ||
111 | 324 | subprocess.call(cmd, env=env) | 344 | subprocess.call(cmd, env=env) |
112 | 325 | 345 | ||
113 | 326 | 346 | ||
114 | 327 | 347 | ||
115 | === modified file 'tests/fetch/test_fetch.py' | |||
116 | --- tests/fetch/test_fetch.py 2016-09-20 17:07:51 +0000 | |||
117 | +++ tests/fetch/test_fetch.py 2017-03-03 20:38:43 +0000 | |||
118 | @@ -192,7 +192,35 @@ | |||
119 | 192 | source = "ppa:test-ppa" | 192 | source = "ppa:test-ppa" |
120 | 193 | fetch.add_source(source=source) | 193 | fetch.add_source(source=source) |
121 | 194 | check_call.assert_called_with( | 194 | check_call.assert_called_with( |
123 | 195 | ['add-apt-repository', '--yes', source]) | 195 | ['add-apt-repository', '--yes', source], env=getenv()) |
124 | 196 | |||
125 | 197 | @patch("charmhelpers.fetch.ubuntu.log") | ||
126 | 198 | @patch.object(osplatform, 'get_platform') | ||
127 | 199 | @patch('subprocess.check_call') | ||
128 | 200 | @patch('time.sleep') | ||
129 | 201 | def test_add_source_ppa_retries_30_times(self, sleep, check_call, | ||
130 | 202 | platform, log): | ||
131 | 203 | platform.return_value = 'ubuntu' | ||
132 | 204 | imp.reload(fetch) | ||
133 | 205 | |||
134 | 206 | self.call_count = 0 | ||
135 | 207 | |||
136 | 208 | def side_effect(*args, **kwargs): | ||
137 | 209 | """Raise an 3 times, then return 0 """ | ||
138 | 210 | self.call_count += 1 | ||
139 | 211 | if self.call_count <= fetch.ubuntu.CMD_RETRY_COUNT: | ||
140 | 212 | raise subprocess.CalledProcessError( | ||
141 | 213 | returncode=1, cmd="some add-apt-repository command") | ||
142 | 214 | else: | ||
143 | 215 | return 0 | ||
144 | 216 | check_call.side_effect = side_effect | ||
145 | 217 | |||
146 | 218 | source = "ppa:test-ppa" | ||
147 | 219 | fetch.add_source(source=source) | ||
148 | 220 | check_call.assert_called_with( | ||
149 | 221 | ['add-apt-repository', '--yes', source], env=getenv()) | ||
150 | 222 | sleep.assert_called_with(10) | ||
151 | 223 | self.assertTrue(fetch.ubuntu.CMD_RETRY_COUNT, sleep.call_count) | ||
152 | 196 | 224 | ||
153 | 197 | @patch('charmhelpers.fetch.ubuntu.log') | 225 | @patch('charmhelpers.fetch.ubuntu.log') |
154 | 198 | @patch.object(osplatform, 'get_platform') | 226 | @patch.object(osplatform, 'get_platform') |
155 | @@ -204,7 +232,7 @@ | |||
156 | 204 | source = "http://archive.ubuntu.com/ubuntu raring-backports main" | 232 | source = "http://archive.ubuntu.com/ubuntu raring-backports main" |
157 | 205 | fetch.add_source(source=source) | 233 | fetch.add_source(source=source) |
158 | 206 | check_call.assert_called_with( | 234 | check_call.assert_called_with( |
160 | 207 | ['add-apt-repository', '--yes', source]) | 235 | ['add-apt-repository', '--yes', source], env=getenv()) |
161 | 208 | 236 | ||
162 | 209 | @patch('charmhelpers.fetch.centos.log') | 237 | @patch('charmhelpers.fetch.centos.log') |
163 | 210 | @patch.object(osplatform, 'get_platform') | 238 | @patch.object(osplatform, 'get_platform') |
164 | @@ -233,7 +261,7 @@ | |||
165 | 233 | source = "https://example.com" | 261 | source = "https://example.com" |
166 | 234 | fetch.add_source(source=source) | 262 | fetch.add_source(source=source) |
167 | 235 | check_call.assert_called_with( | 263 | check_call.assert_called_with( |
169 | 236 | ['add-apt-repository', '--yes', source]) | 264 | ['add-apt-repository', '--yes', source], env=getenv()) |
170 | 237 | 265 | ||
171 | 238 | @patch('charmhelpers.fetch.ubuntu.log') | 266 | @patch('charmhelpers.fetch.ubuntu.log') |
172 | 239 | @patch.object(osplatform, 'get_platform') | 267 | @patch.object(osplatform, 'get_platform') |
173 | @@ -261,7 +289,7 @@ | |||
174 | 261 | source = "deb http://archive.ubuntu.com/ubuntu raring-backports main" | 289 | source = "deb http://archive.ubuntu.com/ubuntu raring-backports main" |
175 | 262 | fetch.add_source(source=source) | 290 | fetch.add_source(source=source) |
176 | 263 | check_call.assert_called_with( | 291 | check_call.assert_called_with( |
178 | 264 | ['add-apt-repository', '--yes', source]) | 292 | ['add-apt-repository', '--yes', source], env=getenv()) |
179 | 265 | 293 | ||
180 | 266 | @patch.object(osplatform, 'get_platform') | 294 | @patch.object(osplatform, 'get_platform') |
181 | 267 | @patch.object(fetch.ubuntu, 'filter_installed_packages') | 295 | @patch.object(fetch.ubuntu, 'filter_installed_packages') |
182 | @@ -357,9 +385,10 @@ | |||
183 | 357 | 385 | ||
184 | 358 | source = "http://archive.ubuntu.com/ubuntu raring-backports main" | 386 | source = "http://archive.ubuntu.com/ubuntu raring-backports main" |
185 | 359 | key_id = "akey" | 387 | key_id = "akey" |
186 | 388 | check_call.return_value = 0 # Successful exit code | ||
187 | 360 | fetch.add_source(source=source, key=key_id) | 389 | fetch.add_source(source=source, key=key_id) |
188 | 361 | check_call.assert_has_calls([ | 390 | check_call.assert_has_calls([ |
190 | 362 | call(['add-apt-repository', '--yes', source]), | 391 | call(['add-apt-repository', '--yes', source], env=getenv()), |
191 | 363 | call(['apt-key', 'adv', '--keyserver', | 392 | call(['apt-key', 'adv', '--keyserver', |
192 | 364 | 'hkp://keyserver.ubuntu.com:80', '--recv', key_id]) | 393 | 'hkp://keyserver.ubuntu.com:80', '--recv', key_id]) |
193 | 365 | ]) | 394 | ]) |
194 | @@ -390,6 +419,7 @@ | |||
195 | 390 | @patch('subprocess.check_call') | 419 | @patch('subprocess.check_call') |
196 | 391 | def test_add_source_https_and_key_id_ubuntu(self, check_call, | 420 | def test_add_source_https_and_key_id_ubuntu(self, check_call, |
197 | 392 | platform, log): | 421 | platform, log): |
198 | 422 | check_call.return_value = 0 # Success from both calls | ||
199 | 393 | platform.return_value = 'ubuntu' | 423 | platform.return_value = 'ubuntu' |
200 | 394 | imp.reload(fetch) | 424 | imp.reload(fetch) |
201 | 395 | 425 | ||
202 | @@ -397,7 +427,7 @@ | |||
203 | 397 | key_id = "GPGPGP" | 427 | key_id = "GPGPGP" |
204 | 398 | fetch.add_source(source=source, key=key_id) | 428 | fetch.add_source(source=source, key=key_id) |
205 | 399 | check_call.assert_has_calls([ | 429 | check_call.assert_has_calls([ |
207 | 400 | call(['add-apt-repository', '--yes', source]), | 430 | call(['add-apt-repository', '--yes', source], env=getenv()), |
208 | 401 | call(['apt-key', 'adv', '--keyserver', | 431 | call(['apt-key', 'adv', '--keyserver', |
209 | 402 | 'hkp://keyserver.ubuntu.com:80', '--recv', key_id]) | 432 | 'hkp://keyserver.ubuntu.com:80', '--recv', key_id]) |
210 | 403 | ]) | 433 | ]) |
211 | @@ -442,16 +472,18 @@ | |||
212 | 442 | received_args = [] | 472 | received_args = [] |
213 | 443 | received_key = StringIO() | 473 | received_key = StringIO() |
214 | 444 | 474 | ||
216 | 445 | def _check_call(arg, stdin=None): | 475 | def _check_call(arg, stdin=None, env=None): |
217 | 446 | '''side_effect to store the stdin passed to check_call process.''' | 476 | '''side_effect to store the stdin passed to check_call process.''' |
218 | 447 | if stdin is not None: | 477 | if stdin is not None: |
219 | 448 | received_args.extend(arg) | 478 | received_args.extend(arg) |
220 | 449 | received_key.write(stdin.read()) | 479 | received_key.write(stdin.read()) |
221 | 480 | return 0 # Successful return code checked by add-apt-repository | ||
222 | 450 | 481 | ||
223 | 451 | with patch('subprocess.check_call', | 482 | with patch('subprocess.check_call', |
224 | 452 | side_effect=_check_call) as check_call: | 483 | side_effect=_check_call) as check_call: |
225 | 453 | fetch.add_source(source=source, key=key) | 484 | fetch.add_source(source=source, key=key) |
227 | 454 | check_call.assert_any_call(['add-apt-repository', '--yes', source]) | 485 | check_call.assert_any_call( |
228 | 486 | ['add-apt-repository', '--yes', source], env=getenv()) | ||
229 | 455 | self.assertEqual(['apt-key', 'add', '-'], received_args) | 487 | self.assertEqual(['apt-key', 'add', '-'], received_args) |
230 | 456 | self.assertEqual(key, received_key.getvalue()) | 488 | self.assertEqual(key, received_key.getvalue()) |
231 | 457 | 489 |
Small nits inline. Code tests OK, lints OK. Thanks for the contribution. +1 with the small fixes addressed!