Merge lp:~raharper/curtin/trunk.add-gpg-retry into lp:~curtin-dev/curtin/trunk
- trunk.add-gpg-retry
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 445 |
Proposed branch: | lp:~raharper/curtin/trunk.add-gpg-retry |
Merge into: | lp:~curtin-dev/curtin/trunk |
Diff against target: |
249 lines (+172/-7) 5 files modified
curtin/commands/apt_config.py (+2/-1) curtin/gpg.py (+4/-4) curtin/util.py (+3/-0) tests/unittests/test_apt_source.py (+4/-2) tests/unittests/test_gpg.py (+159/-0) |
To merge this branch: | bzr merge lp:~raharper/curtin/trunk.add-gpg-retry |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Server Team CI bot | continuous-integration | Approve | |
Scott Moser (community) | Approve | ||
Review via email: mp+316264@code.launchpad.net |
Commit message
Description of the change
gpg: retry when recv'ing gpg keys fail
Retry gpg recv operations in case error is transient.
Add unittests to validate curtin.gpg including retry behavior
Server Team CI bot (server-team-bot) wrote : | # |
- 446. By Ryan Harper
-
Don't hardcode retries value in gpg module.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:446
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Scott Moser (smoser) wrote : | # |
Ryan, this looks good.
Some comments in line, possibly me just misunderstanding.
Ryan Harper (raharper) wrote : | # |
On Thu, Feb 2, 2017 at 8:27 PM, Scott Moser <email address hidden> wrote:
> Ryan, this looks good.
> Some comments in line, possibly me just misunderstanding.
>
>
> Diff comments:
>
> >
> > === added file 'tests/
> > --- tests/unittests
> > +++ tests/unittests
> > @@ -0,0 +1,174 @@
> > +from unittest import TestCase
> > +from mock import call, patch
> > +import textwrap
> > +
> > +from curtin import gpg
> > +from curtin import util
> > +
> > +
> > +class TestCurtinGpg(
> > +
> > + @patch(
> > + def test_export_
> > + key = 'DEADBEEF'
> > + expected_armour = textwrap.dedent("""
> > + -----BEGIN PGP PUBLIC KEY BLOCK-----
> > + Version: GnuPG v1
> > +
> > + deadbeef
> > + -----END PGP PUBLIC KEY BLOCK----
> > + """)
> > + mock_subp.
>
> what is the second side affect for ? the ""?
> I dont think subp should be called twice for this, is it ?
>
subp usually returns an (out, err) tuple; It's a habit in case we need to
do something with the error in the calling code.
>
> > +
> > + armour = gpg.export_
> > + mock_subp.
> key],
> > + capture=True)
> > + self.assertEqua
> > +
> > + @patch(
> > + def test_export_
> > + key = 'DEADBEEF'
> > + mock_subp.
> > +
> > + expected_armour = gpg.export_
> > + mock_subp.
> key],
> > + capture=True)
> > + self.assertEqua
> > +
> > + @patch(
> > + def test_recv_key(self, mock_subp):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.
> > + mock_subp.
> > +
> > + gpg.recv_key(key, keyserver, )
>
> calling here is wierd with the following ,
>
> And again, why the 2 side_affects ? shouldn't subp just be called once?
>
It's a single side-effect with a tuple return.
the iter() is for python mock on trusty which doesn't convert the list of
side-effects into an
iterable directly.
>
> > + mock_subp.
> > + "--recv", key], capture=True,
> > + retries=None)
> > +
> > + @patch(
> > + @patch(
> > + def test_recv_
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.
> > + retries = (1, 2, 5, 10)
> > + mock_under_
> > + util.ProcessExe
> > + util.ProcessExe
> > + util.ProcessExe
> > + ...
Scott Moser (smoser) wrote : | # |
> > what is the second side affect for ? the ""?
> > I dont think subp should be called twice for this, is it ?
> >
>
> subp usually returns an (out, err) tuple; It's a habit in case we need to
> do something with the error in the calling code.
Yeah, i figured i was missing something. You are correct. Sorry for the noise.
> > > + gpg.recv_key(key, keyserver, )
> >
> > calling here is wierd with the following ,
> >
> > And again, why the 2 side_affects ? shouldn't subp just be called once?
> >
>
> It's a single side-effect with a tuple return.
>
> the iter() is for python mock on trusty which doesn't convert the list of
> side-effects into an
> iterable directly.
the subp comment was noise as mentioned above.
But why:
gpg.recv_key(key, keyserver, )
and not
gpg.recv_key(key, keyserver)
Just looks wierd.
> > you dont' use key in this test.
> >
>
> ? it's passed to gpg.recv_key) and checked in the expected_calls.
Not my best review. sorry :) Again, you are correct.
Scott Moser (smoser) wrote : | # |
go ahead and cleanup the repetitive arrays with the * operator, and then i'm good with this.
Thanks.
- 447. By Ryan Harper
-
Unittest cleanup after review
- Use list expansion for repeated calls
- Remove extra comma when calling gpg.recv_key
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:447
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
1 | === modified file 'curtin/commands/apt_config.py' | |||
2 | --- curtin/commands/apt_config.py 2016-12-01 07:12:04 +0000 | |||
3 | +++ curtin/commands/apt_config.py 2017-02-03 16:06:45 +0000 | |||
4 | @@ -362,7 +362,8 @@ | |||
5 | 362 | if 'keyserver' in ent: | 362 | if 'keyserver' in ent: |
6 | 363 | keyserver = ent['keyserver'] | 363 | keyserver = ent['keyserver'] |
7 | 364 | 364 | ||
9 | 365 | ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver) | 365 | ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver, |
10 | 366 | retries=(1, 2, 5, 10)) | ||
11 | 366 | 367 | ||
12 | 367 | if 'key' in ent: | 368 | if 'key' in ent: |
13 | 368 | add_apt_key_raw(ent['key'], target) | 369 | add_apt_key_raw(ent['key'], target) |
14 | 369 | 370 | ||
15 | === modified file 'curtin/gpg.py' | |||
16 | --- curtin/gpg.py 2016-06-23 15:04:21 +0000 | |||
17 | +++ curtin/gpg.py 2017-02-03 16:06:45 +0000 | |||
18 | @@ -36,12 +36,12 @@ | |||
19 | 36 | return armour | 36 | return armour |
20 | 37 | 37 | ||
21 | 38 | 38 | ||
23 | 39 | def recv_key(key, keyserver): | 39 | def recv_key(key, keyserver, retries=None): |
24 | 40 | """Receive gpg key from the specified keyserver""" | 40 | """Receive gpg key from the specified keyserver""" |
25 | 41 | LOG.debug('Receive gpg key "%s"', key) | 41 | LOG.debug('Receive gpg key "%s"', key) |
26 | 42 | try: | 42 | try: |
27 | 43 | util.subp(["gpg", "--keyserver", keyserver, "--recv", key], | 43 | util.subp(["gpg", "--keyserver", keyserver, "--recv", key], |
29 | 44 | capture=True) | 44 | capture=True, retries=retries) |
30 | 45 | except util.ProcessExecutionError as error: | 45 | except util.ProcessExecutionError as error: |
31 | 46 | raise ValueError(('Failed to import key "%s" ' | 46 | raise ValueError(('Failed to import key "%s" ' |
32 | 47 | 'from server "%s" - error %s') % | 47 | 'from server "%s" - error %s') % |
33 | @@ -57,12 +57,12 @@ | |||
34 | 57 | LOG.warn('Failed delete key "%s": %s', key, error) | 57 | LOG.warn('Failed delete key "%s": %s', key, error) |
35 | 58 | 58 | ||
36 | 59 | 59 | ||
38 | 60 | def getkeybyid(keyid, keyserver='keyserver.ubuntu.com'): | 60 | def getkeybyid(keyid, keyserver='keyserver.ubuntu.com', retries=None): |
39 | 61 | """get gpg keyid from keyserver""" | 61 | """get gpg keyid from keyserver""" |
40 | 62 | armour = export_armour(keyid) | 62 | armour = export_armour(keyid) |
41 | 63 | if not armour: | 63 | if not armour: |
42 | 64 | try: | 64 | try: |
44 | 65 | recv_key(keyid, keyserver=keyserver) | 65 | recv_key(keyid, keyserver=keyserver, retries=retries) |
45 | 66 | armour = export_armour(keyid) | 66 | armour = export_armour(keyid) |
46 | 67 | except ValueError: | 67 | except ValueError: |
47 | 68 | LOG.exception('Failed to obtain gpg key %s', keyid) | 68 | LOG.exception('Failed to obtain gpg key %s', keyid) |
48 | 69 | 69 | ||
49 | === modified file 'curtin/util.py' | |||
50 | --- curtin/util.py 2017-02-01 03:18:21 +0000 | |||
51 | +++ curtin/util.py 2017-02-03 16:06:45 +0000 | |||
52 | @@ -155,6 +155,9 @@ | |||
53 | 155 | retries = [] | 155 | retries = [] |
54 | 156 | if "retries" in kwargs: | 156 | if "retries" in kwargs: |
55 | 157 | retries = kwargs.pop("retries") | 157 | retries = kwargs.pop("retries") |
56 | 158 | if not retries: | ||
57 | 159 | # allow retries=None | ||
58 | 160 | retries = [] | ||
59 | 158 | 161 | ||
60 | 159 | if args: | 162 | if args: |
61 | 160 | cmd = args[0] | 163 | cmd = args[0] |
62 | 161 | 164 | ||
63 | === modified file 'tests/unittests/test_apt_source.py' | |||
64 | --- tests/unittests/test_apt_source.py 2016-11-14 22:55:12 +0000 | |||
65 | +++ tests/unittests/test_apt_source.py 2017-02-03 16:06:45 +0000 | |||
66 | @@ -364,7 +364,8 @@ | |||
67 | 364 | keycfg = cfg[self.aptlistfile] | 364 | keycfg = cfg[self.aptlistfile] |
68 | 365 | mockgetkey.assert_called_with(keycfg['keyid'], | 365 | mockgetkey.assert_called_with(keycfg['keyid'], |
69 | 366 | keycfg.get('keyserver', | 366 | keycfg.get('keyserver', |
71 | 367 | 'keyserver.ubuntu.com')) | 367 | 'keyserver.ubuntu.com'), |
72 | 368 | retries=(1, 2, 5, 10)) | ||
73 | 368 | mockkey.assert_called_with(expectedkey, TARGET) | 369 | mockkey.assert_called_with(expectedkey, TARGET) |
74 | 369 | 370 | ||
75 | 370 | # filename should be ignored on key only | 371 | # filename should be ignored on key only |
76 | @@ -407,7 +408,8 @@ | |||
77 | 407 | self._add_apt_sources(cfg, TARGET, template_params=params, | 408 | self._add_apt_sources(cfg, TARGET, template_params=params, |
78 | 408 | aa_repo_match=self.matcher) | 409 | aa_repo_match=self.matcher) |
79 | 409 | 410 | ||
81 | 410 | mockgetkey.assert_called_with('03683F77', 'test.random.com') | 411 | mockgetkey.assert_called_with('03683F77', 'test.random.com', |
82 | 412 | retries=(1, 2, 5, 10)) | ||
83 | 411 | mockadd.assert_called_with('fakekey', TARGET) | 413 | mockadd.assert_called_with('fakekey', TARGET) |
84 | 412 | 414 | ||
85 | 413 | # filename should be ignored on key only | 415 | # filename should be ignored on key only |
86 | 414 | 416 | ||
87 | === added file 'tests/unittests/test_gpg.py' | |||
88 | --- tests/unittests/test_gpg.py 1970-01-01 00:00:00 +0000 | |||
89 | +++ tests/unittests/test_gpg.py 2017-02-03 16:06:45 +0000 | |||
90 | @@ -0,0 +1,159 @@ | |||
91 | 1 | from unittest import TestCase | ||
92 | 2 | from mock import call, patch | ||
93 | 3 | import textwrap | ||
94 | 4 | |||
95 | 5 | from curtin import gpg | ||
96 | 6 | from curtin import util | ||
97 | 7 | |||
98 | 8 | |||
99 | 9 | class TestCurtinGpg(TestCase): | ||
100 | 10 | |||
101 | 11 | @patch('curtin.util.subp') | ||
102 | 12 | def test_export_armour(self, mock_subp): | ||
103 | 13 | key = 'DEADBEEF' | ||
104 | 14 | expected_armour = textwrap.dedent(""" | ||
105 | 15 | -----BEGIN PGP PUBLIC KEY BLOCK----- | ||
106 | 16 | Version: GnuPG v1 | ||
107 | 17 | |||
108 | 18 | deadbeef | ||
109 | 19 | -----END PGP PUBLIC KEY BLOCK---- | ||
110 | 20 | """) | ||
111 | 21 | mock_subp.side_effect = iter([(expected_armour, "")]) | ||
112 | 22 | |||
113 | 23 | armour = gpg.export_armour(key) | ||
114 | 24 | mock_subp.assert_called_with(["gpg", "--export", "--armour", key], | ||
115 | 25 | capture=True) | ||
116 | 26 | self.assertEqual(expected_armour, armour) | ||
117 | 27 | |||
118 | 28 | @patch('curtin.util.subp') | ||
119 | 29 | def test_export_armour_missingkey(self, mock_subp): | ||
120 | 30 | key = 'DEADBEEF' | ||
121 | 31 | mock_subp.side_effect = iter([util.ProcessExecutionError()]) | ||
122 | 32 | |||
123 | 33 | expected_armour = gpg.export_armour(key) | ||
124 | 34 | mock_subp.assert_called_with(["gpg", "--export", "--armour", key], | ||
125 | 35 | capture=True) | ||
126 | 36 | self.assertEqual(None, expected_armour) | ||
127 | 37 | |||
128 | 38 | @patch('curtin.util.subp') | ||
129 | 39 | def test_recv_key(self, mock_subp): | ||
130 | 40 | key = 'DEADBEEF' | ||
131 | 41 | keyserver = 'keyserver.ubuntu.com' | ||
132 | 42 | mock_subp.side_effect = iter([("", "")]) | ||
133 | 43 | |||
134 | 44 | gpg.recv_key(key, keyserver) | ||
135 | 45 | mock_subp.assert_called_with(["gpg", "--keyserver", keyserver, | ||
136 | 46 | "--recv", key], capture=True, | ||
137 | 47 | retries=None) | ||
138 | 48 | |||
139 | 49 | @patch('time.sleep') | ||
140 | 50 | @patch('curtin.util._subp') | ||
141 | 51 | def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep): | ||
142 | 52 | key = 'DEADBEEF' | ||
143 | 53 | keyserver = 'keyserver.ubuntu.com' | ||
144 | 54 | retries = (1, 2, 5, 10) | ||
145 | 55 | nr_calls = 5 | ||
146 | 56 | mock_under_subp.side_effect = iter([ | ||
147 | 57 | util.ProcessExecutionError()] * nr_calls) | ||
148 | 58 | |||
149 | 59 | with self.assertRaises(ValueError): | ||
150 | 60 | gpg.recv_key(key, keyserver, retries=retries) | ||
151 | 61 | |||
152 | 62 | print("_subp calls: %s" % mock_under_subp.call_args_list) | ||
153 | 63 | print("sleep calls: %s" % mock_sleep.call_args_list) | ||
154 | 64 | expected_calls = nr_calls * [ | ||
155 | 65 | call(["gpg", "--keyserver", keyserver, "--recv", key], | ||
156 | 66 | capture=True)] | ||
157 | 67 | mock_under_subp.assert_has_calls(expected_calls) | ||
158 | 68 | |||
159 | 69 | expected_calls = [call(1), call(2), call(5), call(10)] | ||
160 | 70 | mock_sleep.assert_has_calls(expected_calls) | ||
161 | 71 | |||
162 | 72 | @patch('time.sleep') | ||
163 | 73 | @patch('curtin.util._subp') | ||
164 | 74 | def test_recv_key_retry_works(self, mock_under_subp, mock_sleep): | ||
165 | 75 | key = 'DEADBEEF' | ||
166 | 76 | keyserver = 'keyserver.ubuntu.com' | ||
167 | 77 | nr_calls = 2 | ||
168 | 78 | mock_under_subp.side_effect = iter([ | ||
169 | 79 | util.ProcessExecutionError(), # 1 | ||
170 | 80 | ("", ""), | ||
171 | 81 | ]) | ||
172 | 82 | |||
173 | 83 | gpg.recv_key(key, keyserver, retries=[1]) | ||
174 | 84 | |||
175 | 85 | print("_subp calls: %s" % mock_under_subp.call_args_list) | ||
176 | 86 | print("sleep calls: %s" % mock_sleep.call_args_list) | ||
177 | 87 | expected_calls = nr_calls * [ | ||
178 | 88 | call(["gpg", "--keyserver", keyserver, "--recv", key], | ||
179 | 89 | capture=True)] | ||
180 | 90 | mock_under_subp.assert_has_calls(expected_calls) | ||
181 | 91 | mock_sleep.assert_has_calls([call(1)]) | ||
182 | 92 | |||
183 | 93 | @patch('curtin.util.subp') | ||
184 | 94 | def test_delete_key(self, mock_subp): | ||
185 | 95 | key = 'DEADBEEF' | ||
186 | 96 | mock_subp.side_effect = iter([("", "")]) | ||
187 | 97 | |||
188 | 98 | gpg.delete_key(key) | ||
189 | 99 | mock_subp.assert_called_with(["gpg", "--batch", "--yes", | ||
190 | 100 | "--delete-keys", key], capture=True) | ||
191 | 101 | |||
192 | 102 | @patch('curtin.gpg.delete_key') | ||
193 | 103 | @patch('curtin.gpg.recv_key') | ||
194 | 104 | @patch('curtin.gpg.export_armour') | ||
195 | 105 | def test_getkeybyid(self, mock_export, mock_recv, mock_del): | ||
196 | 106 | key = 'DEADBEEF' | ||
197 | 107 | keyserver = 'my.keyserver.xyz.co.uk' | ||
198 | 108 | |||
199 | 109 | mock_export.side_effect = iter([ | ||
200 | 110 | None, | ||
201 | 111 | "-----BEGIN PGP PUBLIC KEY BLOCK-----", | ||
202 | 112 | ]) | ||
203 | 113 | |||
204 | 114 | gpg.getkeybyid(key, keyserver=keyserver) | ||
205 | 115 | |||
206 | 116 | mock_export.assert_has_calls([call(key), call(key)]) | ||
207 | 117 | mock_recv.assert_has_calls([ | ||
208 | 118 | call(key, keyserver=keyserver, retries=None)]) | ||
209 | 119 | mock_del.assert_has_calls([call(key)]) | ||
210 | 120 | |||
211 | 121 | @patch('curtin.gpg.delete_key') | ||
212 | 122 | @patch('curtin.gpg.recv_key') | ||
213 | 123 | @patch('curtin.gpg.export_armour') | ||
214 | 124 | def test_getkeybyid_exists(self, mock_export, mock_recv, mock_del): | ||
215 | 125 | key = 'DEADBEEF' | ||
216 | 126 | |||
217 | 127 | mock_export.side_effect = iter([ | ||
218 | 128 | "-----BEGIN PGP PUBLIC KEY BLOCK-----", | ||
219 | 129 | ]) | ||
220 | 130 | |||
221 | 131 | gpg.getkeybyid(key) | ||
222 | 132 | |||
223 | 133 | mock_export.assert_has_calls([call(key)]) | ||
224 | 134 | self.assertEqual([], mock_recv.call_args_list) | ||
225 | 135 | self.assertEqual([], mock_del.call_args_list) | ||
226 | 136 | |||
227 | 137 | @patch('curtin.gpg.delete_key') | ||
228 | 138 | @patch('curtin.gpg.recv_key') | ||
229 | 139 | @patch('curtin.gpg.export_armour') | ||
230 | 140 | def test_getkeybyid_raises(self, mock_export, mock_recv, mock_del): | ||
231 | 141 | key = 'DEADBEEF' | ||
232 | 142 | keyserver = 'my.keyserver.xyz.co.uk' | ||
233 | 143 | |||
234 | 144 | mock_export.side_effect = iter([ | ||
235 | 145 | None, | ||
236 | 146 | "-----BEGIN PGP PUBLIC KEY BLOCK-----", | ||
237 | 147 | ]) | ||
238 | 148 | mock_recv.side_effect = iter([ | ||
239 | 149 | ValueError("Failed to import key %s from server %s" % | ||
240 | 150 | (key, keyserver)), | ||
241 | 151 | ]) | ||
242 | 152 | |||
243 | 153 | with self.assertRaises(ValueError): | ||
244 | 154 | gpg.getkeybyid(key, keyserver=keyserver) | ||
245 | 155 | |||
246 | 156 | mock_export.assert_has_calls([call(key)]) | ||
247 | 157 | mock_recv.assert_has_calls([ | ||
248 | 158 | call(key, keyserver=keyserver, retries=None)]) | ||
249 | 159 | mock_del.assert_has_calls([call(key)]) |
PASSED: Continuous integration, rev:445 /jenkins. ubuntu. com/server/ job/curtin- ci/309/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-arm64/ 309 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-ppc64el/ 309 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-s390x/ 309 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= vm-amd64/ 309 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= vm-i386/ 309
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/curtin- ci/309/ rebuild
https:/