curtin

Merge lp:~raharper/curtin/trunk.add-gpg-retry into lp:~curtin-dev/curtin/trunk

  1. trunk.add-gpg-retry
  2. Merge into trunk
Proposed by Ryan Harper
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
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

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

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:445
https://jenkins.ubuntu.com/server/job/curtin-ci/309/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/309

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/309/rebuild

review: Approve (continuous-integration)
lp:~raharper/curtin/trunk.add-gpg-retry updated
446. By Ryan Harper

Don't hardcode retries value in gpg module.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:446
https://jenkins.ubuntu.com/server/job/curtin-ci/310/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/310

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/310/rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

Ryan, this looks good.
Some comments in line, possibly me just misunderstanding.

Revision history for this message
Ryan Harper (raharper) wrote :
Download full text (8.2 KiB)

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/unittests/test_gpg.py'
> > --- tests/unittests/test_gpg.py 1970-01-01 00:00:00 +0000
> > +++ tests/unittests/test_gpg.py 2017-02-02 22:52:32 +0000
> > @@ -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(TestCase):
> > +
> > + @patch('curtin.util.subp')
> > + def test_export_armour(self, mock_subp):
> > + key = 'DEADBEEF'
> > + expected_armour = textwrap.dedent("""
> > + -----BEGIN PGP PUBLIC KEY BLOCK-----
> > + Version: GnuPG v1
> > +
> > + deadbeef
> > + -----END PGP PUBLIC KEY BLOCK----
> > + """)
> > + mock_subp.side_effect = iter([(expected_armour, "")])
>
> 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_armour(key)
> > + mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > + capture=True)
> > + self.assertEqual(expected_armour, armour)
> > +
> > + @patch('curtin.util.subp')
> > + def test_export_armour_missingkey(self, mock_subp):
> > + key = 'DEADBEEF'
> > + mock_subp.side_effect = iter([util.ProcessExecutionError()])
> > +
> > + expected_armour = gpg.export_armour(key)
> > + mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > + capture=True)
> > + self.assertEqual(None, expected_armour)
> > +
> > + @patch('curtin.util.subp')
> > + def test_recv_key(self, mock_subp):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.ubuntu.com'
> > + mock_subp.side_effect = iter([("", "")])
> > +
> > + 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.assert_called_with(["gpg", "--keyserver", keyserver,
> > + "--recv", key], capture=True,
> > + retries=None)
> > +
> > + @patch('time.sleep')
> > + @patch('curtin.util._subp')
> > + def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.ubuntu.com'
> > + retries = (1, 2, 5, 10)
> > + mock_under_subp.side_effect = iter([
> > + util.ProcessExecutionError(), # 1
> > + util.ProcessExecutionError(), # 2
> > + util.ProcessExecutionError(), # 3
> > + ...

Read more...

Revision history for this message
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.

Revision history for this message
Scott Moser (smoser) wrote :

go ahead and cleanup the repetitive arrays with the * operator, and then i'm good with this.
Thanks.

review: Approve
lp:~raharper/curtin/trunk.add-gpg-retry updated
447. By Ryan Harper

Unittest cleanup after review

- Use list expansion for repeated calls
- Remove extra comma when calling gpg.recv_key

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:447
https://jenkins.ubuntu.com/server/job/curtin-ci/311/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/311

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/311/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 if 'keyserver' in ent:
6 keyserver = ent['keyserver']
7
8- ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver)
9+ ent['key'] = gpg.getkeybyid(ent['keyid'], keyserver,
10+ retries=(1, 2, 5, 10))
11
12 if 'key' in ent:
13 add_apt_key_raw(ent['key'], target)
14
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 return armour
20
21
22-def recv_key(key, keyserver):
23+def recv_key(key, keyserver, retries=None):
24 """Receive gpg key from the specified keyserver"""
25 LOG.debug('Receive gpg key "%s"', key)
26 try:
27 util.subp(["gpg", "--keyserver", keyserver, "--recv", key],
28- capture=True)
29+ capture=True, retries=retries)
30 except util.ProcessExecutionError as error:
31 raise ValueError(('Failed to import key "%s" '
32 'from server "%s" - error %s') %
33@@ -57,12 +57,12 @@
34 LOG.warn('Failed delete key "%s": %s', key, error)
35
36
37-def getkeybyid(keyid, keyserver='keyserver.ubuntu.com'):
38+def getkeybyid(keyid, keyserver='keyserver.ubuntu.com', retries=None):
39 """get gpg keyid from keyserver"""
40 armour = export_armour(keyid)
41 if not armour:
42 try:
43- recv_key(keyid, keyserver=keyserver)
44+ recv_key(keyid, keyserver=keyserver, retries=retries)
45 armour = export_armour(keyid)
46 except ValueError:
47 LOG.exception('Failed to obtain gpg key %s', keyid)
48
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 retries = []
54 if "retries" in kwargs:
55 retries = kwargs.pop("retries")
56+ if not retries:
57+ # allow retries=None
58+ retries = []
59
60 if args:
61 cmd = args[0]
62
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 keycfg = cfg[self.aptlistfile]
68 mockgetkey.assert_called_with(keycfg['keyid'],
69 keycfg.get('keyserver',
70- 'keyserver.ubuntu.com'))
71+ 'keyserver.ubuntu.com'),
72+ retries=(1, 2, 5, 10))
73 mockkey.assert_called_with(expectedkey, TARGET)
74
75 # filename should be ignored on key only
76@@ -407,7 +408,8 @@
77 self._add_apt_sources(cfg, TARGET, template_params=params,
78 aa_repo_match=self.matcher)
79
80- mockgetkey.assert_called_with('03683F77', 'test.random.com')
81+ mockgetkey.assert_called_with('03683F77', 'test.random.com',
82+ retries=(1, 2, 5, 10))
83 mockadd.assert_called_with('fakekey', TARGET)
84
85 # filename should be ignored on key only
86
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+from unittest import TestCase
92+from mock import call, patch
93+import textwrap
94+
95+from curtin import gpg
96+from curtin import util
97+
98+
99+class TestCurtinGpg(TestCase):
100+
101+ @patch('curtin.util.subp')
102+ def test_export_armour(self, mock_subp):
103+ key = 'DEADBEEF'
104+ expected_armour = textwrap.dedent("""
105+ -----BEGIN PGP PUBLIC KEY BLOCK-----
106+ Version: GnuPG v1
107+
108+ deadbeef
109+ -----END PGP PUBLIC KEY BLOCK----
110+ """)
111+ mock_subp.side_effect = iter([(expected_armour, "")])
112+
113+ armour = gpg.export_armour(key)
114+ mock_subp.assert_called_with(["gpg", "--export", "--armour", key],
115+ capture=True)
116+ self.assertEqual(expected_armour, armour)
117+
118+ @patch('curtin.util.subp')
119+ def test_export_armour_missingkey(self, mock_subp):
120+ key = 'DEADBEEF'
121+ mock_subp.side_effect = iter([util.ProcessExecutionError()])
122+
123+ expected_armour = gpg.export_armour(key)
124+ mock_subp.assert_called_with(["gpg", "--export", "--armour", key],
125+ capture=True)
126+ self.assertEqual(None, expected_armour)
127+
128+ @patch('curtin.util.subp')
129+ def test_recv_key(self, mock_subp):
130+ key = 'DEADBEEF'
131+ keyserver = 'keyserver.ubuntu.com'
132+ mock_subp.side_effect = iter([("", "")])
133+
134+ gpg.recv_key(key, keyserver)
135+ mock_subp.assert_called_with(["gpg", "--keyserver", keyserver,
136+ "--recv", key], capture=True,
137+ retries=None)
138+
139+ @patch('time.sleep')
140+ @patch('curtin.util._subp')
141+ def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
142+ key = 'DEADBEEF'
143+ keyserver = 'keyserver.ubuntu.com'
144+ retries = (1, 2, 5, 10)
145+ nr_calls = 5
146+ mock_under_subp.side_effect = iter([
147+ util.ProcessExecutionError()] * nr_calls)
148+
149+ with self.assertRaises(ValueError):
150+ gpg.recv_key(key, keyserver, retries=retries)
151+
152+ print("_subp calls: %s" % mock_under_subp.call_args_list)
153+ print("sleep calls: %s" % mock_sleep.call_args_list)
154+ expected_calls = nr_calls * [
155+ call(["gpg", "--keyserver", keyserver, "--recv", key],
156+ capture=True)]
157+ mock_under_subp.assert_has_calls(expected_calls)
158+
159+ expected_calls = [call(1), call(2), call(5), call(10)]
160+ mock_sleep.assert_has_calls(expected_calls)
161+
162+ @patch('time.sleep')
163+ @patch('curtin.util._subp')
164+ def test_recv_key_retry_works(self, mock_under_subp, mock_sleep):
165+ key = 'DEADBEEF'
166+ keyserver = 'keyserver.ubuntu.com'
167+ nr_calls = 2
168+ mock_under_subp.side_effect = iter([
169+ util.ProcessExecutionError(), # 1
170+ ("", ""),
171+ ])
172+
173+ gpg.recv_key(key, keyserver, retries=[1])
174+
175+ print("_subp calls: %s" % mock_under_subp.call_args_list)
176+ print("sleep calls: %s" % mock_sleep.call_args_list)
177+ expected_calls = nr_calls * [
178+ call(["gpg", "--keyserver", keyserver, "--recv", key],
179+ capture=True)]
180+ mock_under_subp.assert_has_calls(expected_calls)
181+ mock_sleep.assert_has_calls([call(1)])
182+
183+ @patch('curtin.util.subp')
184+ def test_delete_key(self, mock_subp):
185+ key = 'DEADBEEF'
186+ mock_subp.side_effect = iter([("", "")])
187+
188+ gpg.delete_key(key)
189+ mock_subp.assert_called_with(["gpg", "--batch", "--yes",
190+ "--delete-keys", key], capture=True)
191+
192+ @patch('curtin.gpg.delete_key')
193+ @patch('curtin.gpg.recv_key')
194+ @patch('curtin.gpg.export_armour')
195+ def test_getkeybyid(self, mock_export, mock_recv, mock_del):
196+ key = 'DEADBEEF'
197+ keyserver = 'my.keyserver.xyz.co.uk'
198+
199+ mock_export.side_effect = iter([
200+ None,
201+ "-----BEGIN PGP PUBLIC KEY BLOCK-----",
202+ ])
203+
204+ gpg.getkeybyid(key, keyserver=keyserver)
205+
206+ mock_export.assert_has_calls([call(key), call(key)])
207+ mock_recv.assert_has_calls([
208+ call(key, keyserver=keyserver, retries=None)])
209+ mock_del.assert_has_calls([call(key)])
210+
211+ @patch('curtin.gpg.delete_key')
212+ @patch('curtin.gpg.recv_key')
213+ @patch('curtin.gpg.export_armour')
214+ def test_getkeybyid_exists(self, mock_export, mock_recv, mock_del):
215+ key = 'DEADBEEF'
216+
217+ mock_export.side_effect = iter([
218+ "-----BEGIN PGP PUBLIC KEY BLOCK-----",
219+ ])
220+
221+ gpg.getkeybyid(key)
222+
223+ mock_export.assert_has_calls([call(key)])
224+ self.assertEqual([], mock_recv.call_args_list)
225+ self.assertEqual([], mock_del.call_args_list)
226+
227+ @patch('curtin.gpg.delete_key')
228+ @patch('curtin.gpg.recv_key')
229+ @patch('curtin.gpg.export_armour')
230+ def test_getkeybyid_raises(self, mock_export, mock_recv, mock_del):
231+ key = 'DEADBEEF'
232+ keyserver = 'my.keyserver.xyz.co.uk'
233+
234+ mock_export.side_effect = iter([
235+ None,
236+ "-----BEGIN PGP PUBLIC KEY BLOCK-----",
237+ ])
238+ mock_recv.side_effect = iter([
239+ ValueError("Failed to import key %s from server %s" %
240+ (key, keyserver)),
241+ ])
242+
243+ with self.assertRaises(ValueError):
244+ gpg.getkeybyid(key, keyserver=keyserver)
245+
246+ mock_export.assert_has_calls([call(key)])
247+ mock_recv.assert_has_calls([
248+ call(key, keyserver=keyserver, retries=None)])
249+ mock_del.assert_has_calls([call(key)])

Subscribers

People subscribed via source and target branches