Merge lp:~chad.smith/charm-helpers/retry-add-apt-repository into lp:charm-helpers

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
Reviewer Review Type Date Requested Status
Eric Snow (community) Approve
David Britton Approve
Review via email: mp+318951@code.launchpad.net

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 on 2017-03-03

comment fix

Revision history for this message
David Britton (dpb) wrote :

Small nits inline. Code tests OK, lints OK. Thanks for the contribution. +1 with the small fixes addressed!

review: Approve
709. By Chad Smith on 2017-03-03

- 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 on 2017-03-03

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 on 2017-03-03

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 }
6
7 APT_NO_LOCK = 100 # The return code for "couldn't acquire lock" in APT.
8-APT_NO_LOCK_RETRY_DELAY = 10 # Wait 10 seconds between apt lock checks.
9-APT_NO_LOCK_RETRY_COUNT = 30 # Retry to acquire the lock X times.
10+CMD_RETRY_DELAY = 10 # Wait 10 seconds between command retries.
11+CMD_RETRY_COUNT = 30 # Retry a failing fatal command X times.
12
13
14 def filter_installed_packages(packages):
15@@ -249,7 +249,8 @@
16 source.startswith('http') or
17 source.startswith('deb ') or
18 source.startswith('cloud-archive:')):
19- subprocess.check_call(['add-apt-repository', '--yes', source])
20+ cmd = ['add-apt-repository', '--yes', source]
21+ _run_with_retries(cmd)
22 elif source.startswith('cloud:'):
23 install(filter_installed_packages(['ubuntu-cloud-keyring']),
24 fatal=True)
25@@ -286,41 +287,60 @@
26 key])
27
28
29+def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,),
30+ retry_message="", cmd_env=None):
31+ """Run a command and retry until success or max_retries is reached.
32+
33+ :param: cmd: str: The apt command to run.
34+ :param: max_retries: int: The number of retries to attempt on a fatal
35+ command. Defaults to CMD_RETRY_COUNT.
36+ :param: retry_exitcodes: tuple: Optional additional exit codes to retry.
37+ Defaults to retry on exit code 1.
38+ :param: retry_message: str: Optional log prefix emitted during retries.
39+ :param: cmd_env: dict: Environment variables to add to the command run.
40+ """
41+
42+ env = os.environ.copy()
43+ if cmd_env:
44+ env.update(cmd_env)
45+
46+ if not retry_message:
47+ retry_message = "Failed executing '{}'".format(" ".join(cmd))
48+ retry_message += ". Will retry in {} seconds".format(CMD_RETRY_DELAY)
49+
50+ retry_count = 0
51+ result = None
52+
53+ retry_results = (None,) + retry_exitcodes
54+ while result in retry_results:
55+ try:
56+ result = subprocess.check_call(cmd, env=env)
57+ except subprocess.CalledProcessError as e:
58+ retry_count = retry_count + 1
59+ if retry_count > max_retries:
60+ raise
61+ result = e.returncode
62+ log(retry_message)
63+ time.sleep(CMD_RETRY_DELAY)
64+
65+
66 def _run_apt_command(cmd, fatal=False):
67- """Run an APT command.
68-
69- Checks the output and retries if the fatal flag is set
70- to True.
71-
72- :param: cmd: str: The apt command to run.
73+ """Run an apt command with optional retries.
74+
75 :param: fatal: bool: Whether the command's output should be checked and
76 retried.
77 """
78- env = os.environ.copy()
79-
80- if 'DEBIAN_FRONTEND' not in env:
81- env['DEBIAN_FRONTEND'] = 'noninteractive'
82+ # Provide DEBIAN_FRONTEND=noninteractive if not present in the environment.
83+ cmd_env = {
84+ 'DEBIAN_FRONTEND': os.environ.get('DEBIAN_FRONTEND', 'noninteractive')}
85
86 if fatal:
87- retry_count = 0
88- result = None
89-
90- # If the command is considered "fatal", we need to retry if the apt
91- # lock was not acquired.
92-
93- while result is None or result == APT_NO_LOCK:
94- try:
95- result = subprocess.check_call(cmd, env=env)
96- except subprocess.CalledProcessError as e:
97- retry_count = retry_count + 1
98- if retry_count > APT_NO_LOCK_RETRY_COUNT:
99- raise
100- result = e.returncode
101- log("Couldn't acquire DPKG lock. Will retry in {} seconds."
102- "".format(APT_NO_LOCK_RETRY_DELAY))
103- time.sleep(APT_NO_LOCK_RETRY_DELAY)
104-
105+ _run_with_retries(
106+ cmd, cmd_env=cmd_env, retry_exitcodes=(1, APT_NO_LOCK,),
107+ retry_message="Couldn't acquire DPKG lock")
108 else:
109+ env = os.environ.copy()
110+ env.update(cmd_env)
111 subprocess.call(cmd, env=env)
112
113
114
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 source = "ppa:test-ppa"
120 fetch.add_source(source=source)
121 check_call.assert_called_with(
122- ['add-apt-repository', '--yes', source])
123+ ['add-apt-repository', '--yes', source], env=getenv())
124+
125+ @patch("charmhelpers.fetch.ubuntu.log")
126+ @patch.object(osplatform, 'get_platform')
127+ @patch('subprocess.check_call')
128+ @patch('time.sleep')
129+ def test_add_source_ppa_retries_30_times(self, sleep, check_call,
130+ platform, log):
131+ platform.return_value = 'ubuntu'
132+ imp.reload(fetch)
133+
134+ self.call_count = 0
135+
136+ def side_effect(*args, **kwargs):
137+ """Raise an 3 times, then return 0 """
138+ self.call_count += 1
139+ if self.call_count <= fetch.ubuntu.CMD_RETRY_COUNT:
140+ raise subprocess.CalledProcessError(
141+ returncode=1, cmd="some add-apt-repository command")
142+ else:
143+ return 0
144+ check_call.side_effect = side_effect
145+
146+ source = "ppa:test-ppa"
147+ fetch.add_source(source=source)
148+ check_call.assert_called_with(
149+ ['add-apt-repository', '--yes', source], env=getenv())
150+ sleep.assert_called_with(10)
151+ self.assertTrue(fetch.ubuntu.CMD_RETRY_COUNT, sleep.call_count)
152
153 @patch('charmhelpers.fetch.ubuntu.log')
154 @patch.object(osplatform, 'get_platform')
155@@ -204,7 +232,7 @@
156 source = "http://archive.ubuntu.com/ubuntu raring-backports main"
157 fetch.add_source(source=source)
158 check_call.assert_called_with(
159- ['add-apt-repository', '--yes', source])
160+ ['add-apt-repository', '--yes', source], env=getenv())
161
162 @patch('charmhelpers.fetch.centos.log')
163 @patch.object(osplatform, 'get_platform')
164@@ -233,7 +261,7 @@
165 source = "https://example.com"
166 fetch.add_source(source=source)
167 check_call.assert_called_with(
168- ['add-apt-repository', '--yes', source])
169+ ['add-apt-repository', '--yes', source], env=getenv())
170
171 @patch('charmhelpers.fetch.ubuntu.log')
172 @patch.object(osplatform, 'get_platform')
173@@ -261,7 +289,7 @@
174 source = "deb http://archive.ubuntu.com/ubuntu raring-backports main"
175 fetch.add_source(source=source)
176 check_call.assert_called_with(
177- ['add-apt-repository', '--yes', source])
178+ ['add-apt-repository', '--yes', source], env=getenv())
179
180 @patch.object(osplatform, 'get_platform')
181 @patch.object(fetch.ubuntu, 'filter_installed_packages')
182@@ -357,9 +385,10 @@
183
184 source = "http://archive.ubuntu.com/ubuntu raring-backports main"
185 key_id = "akey"
186+ check_call.return_value = 0 # Successful exit code
187 fetch.add_source(source=source, key=key_id)
188 check_call.assert_has_calls([
189- call(['add-apt-repository', '--yes', source]),
190+ call(['add-apt-repository', '--yes', source], env=getenv()),
191 call(['apt-key', 'adv', '--keyserver',
192 'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
193 ])
194@@ -390,6 +419,7 @@
195 @patch('subprocess.check_call')
196 def test_add_source_https_and_key_id_ubuntu(self, check_call,
197 platform, log):
198+ check_call.return_value = 0 # Success from both calls
199 platform.return_value = 'ubuntu'
200 imp.reload(fetch)
201
202@@ -397,7 +427,7 @@
203 key_id = "GPGPGP"
204 fetch.add_source(source=source, key=key_id)
205 check_call.assert_has_calls([
206- call(['add-apt-repository', '--yes', source]),
207+ call(['add-apt-repository', '--yes', source], env=getenv()),
208 call(['apt-key', 'adv', '--keyserver',
209 'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
210 ])
211@@ -442,16 +472,18 @@
212 received_args = []
213 received_key = StringIO()
214
215- def _check_call(arg, stdin=None):
216+ def _check_call(arg, stdin=None, env=None):
217 '''side_effect to store the stdin passed to check_call process.'''
218 if stdin is not None:
219 received_args.extend(arg)
220 received_key.write(stdin.read())
221+ return 0 # Successful return code checked by add-apt-repository
222
223 with patch('subprocess.check_call',
224 side_effect=_check_call) as check_call:
225 fetch.add_source(source=source, key=key)
226- check_call.assert_any_call(['add-apt-repository', '--yes', source])
227+ check_call.assert_any_call(
228+ ['add-apt-repository', '--yes', source], env=getenv())
229 self.assertEqual(['apt-key', 'add', '-'], received_args)
230 self.assertEqual(key, received_key.getvalue())
231

Subscribers

People subscribed via source and target branches