Ubuntu CI Engine

Merge lp:~vila/uci-engine/cleanup-data-store into lp:uci-engine

  1. cleanup-data-store
  2. Merge into trunk
Proposed by Vincent Ladeuil
Status: Needs review
Proposed branch: lp:~vila/uci-engine/cleanup-data-store
Merge into: lp:uci-engine
Prerequisite: lp:~vila/uci-engine/stores-with-path
Diff against target: 213 lines (+47/-61)
2 files modified
ci-utils/ci_utils/data_store.py (+37/-43)
tests/test_data_store.py (+10/-18)
To merge this branch: bzr merge lp:~vila/uci-engine/cleanup-data-store
Reviewer Review Type Date Requested Status
Joe Talbott (community) Needs Information
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+242218@code.launchpad.net

This proposal supersedes a proposal from 2014-11-19.

Commit message

Cleanup data_store.py and the related tests

Description of the change

This cleans up data store and its tests as a pre-requisite for implementing reliable container deletion.

To post a comment you must log in.
lp:~vila/uci-engine/cleanup-data-store updated
905. By Vincent Ladeuil

Tested with ./run-tests ^tests.test_style ^tests.test_data_store befor submission.

(and some cosmetic changes)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:905
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1728/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1728/rebuild

review: Approve (continuous-integration)
Reply
Revision history for this message
Joe Talbott (joetalbott) wrote :

One in-line question, otherwise +1.

review: Needs Information
Reply
Revision history for this message
Vincent Ladeuil (vila) wrote :

Replied inline, good question ! The answer is tricky and at the core of the eventually consistent swift data model.

Reply
Revision history for this message
Joe Talbott (joetalbott) wrote :

On Thu, Nov 20, 2014 at 10:12:31PM -0000, Vincent Ladeuil wrote:
> Replied inline, good question ! The answer is tricky and at the core of the eventually consistent swift data model.
>
 [snip]
> > === modified file 'tests/test_data_store.py'
> > --- tests/test_data_store.py 2014-10-08 09:54:56 +0000
> > +++ tests/test_data_store.py 2014-11-19 14:42:09 +0000
> > @@ -59,7 +59,6 @@
> > """ Test adding a file. """
> >
> > self.ds.put_file(self.filename, self.contents)
> > - self.addCleanup(self.ds.delete_file, self.filename)
>
> Yes, the container is deleted recursively (via a cleanup in setUp).
>
> Unless the test started failing I didn't realize this cleanup was harmful:
> * this cleanup delete the file,
> * the container cleanup:
> * list the container content,
> * find the file above that hasn't been deleted yet,
> * try to delete it but fails because the file is now deleted.
>
> It's hard to understand until you start realizing that each of the requests may end up on a different swift node during the inconsistency window: some nodes think the file is gone, some others think it's still there.
>
> I've tried to explain in the revno 900 commit message:
> Tests should not addCleanup(self.ds.delete_file, self.filename).
>
> It can lead to self.addCleanup(self.ds.delete, recursive=True) failing with 404 because the file is deleted after delete() calls list_files() but before it calls delete_file() for the same file.
>

That makes perfect sense.

> > files = self.ds.list_files()
> > self.assertEqual([self.filename], files)
> >
> > @@ -67,18 +66,20 @@
> > """ Test adding a file has correct contents. """
> >
> > self.ds.put_file(self.filename, self.contents)
> > - self.addCleanup(self.ds.delete_file, self.filename)
> > contents = self.ds.get_file(self.filename)
> >
> > self.assertEqual(contents, self.contents)
> >
> > - def test_public_container(self):
> > - """ Test accesssing a public container. """
> > + def test_not_public_fails(self):
> > + link = self.ds.put_file(self.filename, self.contents)
> > + self.addCleanup(self.ds.delete_file, self.filename)

So this should be removed as well?

Reply
Revision history for this message
Vincent Ladeuil (vila) wrote :

> > > - def test_public_container(self):
> > > - """ Test accesssing a public container. """
> > > + def test_not_public_fails(self):
> > > + link = self.ds.put_file(self.filename, self.contents)
> > > + self.addCleanup(self.ds.delete_file, self.filename)
>
> So this should be removed as well?

Mille sabords !

That one didn't fail....

As an experiment, I'd like to keep it and see if/when it fails.

Looking at these tests again, some use list_files() to check the container content which is *wrong*. Yet, even when the other ones were failing almost always, those ones never fail...

Yeah, I think the best way to keep learning how swift works and how wide/narrow is that inconsistency window is to keep them this way, at least we know how to fix them now.

Reply

Unmerged revisions

905. By Vincent Ladeuil

Tested with ./run-tests ^tests.test_style ^tests.test_data_store befor submission.

(and some cosmetic changes)

904. By Vincent Ladeuil

This test pollutes the run output.

903. By Vincent Ladeuil

Log auth info (except for the password).

Rewrite exception in list_files.

902. By Vincent Ladeuil

Simplify clear() and delete() by removing duplication.

901. By Vincent Ladeuil

Remove useless import.

900. By Vincent Ladeuil

Tests should not addCleanup(self.ds.delete_file, self.filename).

It can lead to self.addCleanup(self.ds.delete, recursive=True) failing with 404 because the file is deleted after delete() calls list_files() but before it calls delete_file() for the same file.

899. By Vincent Ladeuil

Remove duplication.

898. By Vincent Ladeuil

Fix pyflakes issue.

897. By Vincent Ladeuil

Now that the 'cli' is gone, nobody requires artifact paths to be reduced to their basename. The corresponding tests are not required anymore either.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ci-utils/ci_utils/data_store.py'
2--- ci-utils/ci_utils/data_store.py 2014-11-19 14:42:09 +0000
3+++ ci-utils/ci_utils/data_store.py 2014-11-19 14:42:09 +0000
4@@ -15,8 +15,8 @@
5
6 import logging
7
8+
9 from swiftclient import client
10-from swiftclient.client import ClientException
11
12
13 class DataStoreException(Exception):
14@@ -49,33 +49,45 @@
15
16 if len(missing_fields) > 0:
17 raise DataStoreException(
18- "missing fields: {}".format(', '.join(missing_fields))
19- )
20+ "missing fields: {}".format(', '.join(missing_fields)))
21
22 def ensure_swift_client(self):
23+ """Ensures we have a valid swift client and a container.
24+
25+ This allows callers of the DataStore to call any part of the API
26+ without worrying about creating the container.
27+ """
28 if self.client is not None:
29 return self.client
30
31 self.validate_auth_config(self.auth_config)
32 config = self.auth_config
33- self.client = client.Connection(
34- authurl=config.get('auth_url'),
35- user=config.get('auth_user'),
36- key=config.get('auth_password'),
37- os_options={'tenant_name': config.get('auth_tenant_name'),
38- 'region_name': config.get('auth_region')},
39- auth_version=('/v1.' in config.get('auth_url') and '1.0' or '2.0'))
40+ # All authentication data bar the password so we can log them.
41+ auth = dict(authurl=config.get('auth_url'),
42+ user=config.get('auth_user'),
43+ os_options={'tenant_name': config.get('auth_tenant_name'),
44+ 'region_name': config.get('auth_region')},
45+ auth_version=('/v1.' in config.get('auth_url') and '1.0'
46+ or '2.0'))
47+ self.client = client.Connection(key=config.get('auth_password'),
48+ **auth)
49 try:
50+ # Creating a container is idempotent and can be repeated
51 self._create_container(self.container_id)
52 except client.ClientException:
53+ # This is the first connection attempt in the object life
54 logging.exception('Unable to connect to swift')
55- raise DataStoreException("Missing or invalid authentication info.")
56+ raise DataStoreException(
57+ "Invalid authentication info: {}".format(auth))
58 self.change_visibility(public=self.public)
59 self.container_url = "{}/{}".format(self.client.url, self.container_id)
60
61 def list_files(self):
62 self.ensure_swift_client()
63- _, objects = self.client.get_container(self.container_id)
64+ try:
65+ _, objects = self.client.get_container(self.container_id)
66+ except client.ClientException as e:
67+ raise DataStoreException("Failed to get container: {}".format(e))
68 return [x['name'] for x in objects]
69
70 def put_file(self, filename, contents, content_type=None):
71@@ -85,10 +97,9 @@
72 self.client.put_object(self.container_id, obj=name,
73 contents=contents,
74 content_type=content_type)
75- except ClientException as e:
76+ except client.ClientException as e:
77 raise DataStoreException(
78- "Failed to upload file: {}, Error: {}".format(filename, e)
79- )
80+ "Failed to upload file: {}, Error: {}".format(filename, e))
81
82 return self.file_path(filename)
83
84@@ -99,10 +110,9 @@
85 contents = None
86 try:
87 hdr, contents = self.client.get_object(self.container_id, obj=name)
88- except ClientException as e:
89+ except client.ClientException as e:
90 raise DataStoreException(
91- "Failed to get file: {}, Error: {}".format(filename, e)
92- )
93+ "Failed to get file: {}, Error: {}".format(filename, e))
94 return contents
95
96 def change_visibility(self, public=False):
97@@ -120,40 +130,24 @@
98 name = self.file_name(filename)
99 try:
100 self.client.delete_object(self.container_id, obj=name)
101- except ClientException as e:
102+ except client.ClientException as e:
103 raise DataStoreException(
104- "Failed to delete file: {}, Error: {}".format(filename, e)
105- )
106+ "Failed to delete file: {}, Error: {}".format(filename, e))
107
108 def clear(self):
109- self.ensure_swift_client()
110- try:
111- files = self.client.get_container(self.container_id)[1]
112-
113- for f in files:
114- try:
115- self.client.delete_object(self.container_id,
116- obj=f['name'])
117- except ClientException as e:
118- raise DataStoreException(
119- "Failed to delete file: {}".format(e)
120- )
121- except ClientException as e:
122- raise DataStoreException(
123- "Failed to get container: {}".format(e)
124- )
125+ files = self.list_files()
126+ for fname in files:
127+ self.delete_file(fname)
128
129 def delete(self, recursive=False):
130 self.ensure_swift_client()
131+ if recursive:
132+ self.clear()
133 try:
134- if recursive:
135- self.clear()
136-
137 self.client.delete_container(self.container_id)
138- except ClientException as e:
139+ except client.ClientException as e:
140 raise DataStoreException(
141- "Failed to delete container: {}".format(e)
142- )
143+ "Failed to delete container: {}".format(e))
144
145 def file_name(self, filename):
146 """Returns the file name that is used inside the data store.
147
148=== modified file 'tests/test_data_store.py'
149--- tests/test_data_store.py 2014-10-08 09:54:56 +0000
150+++ tests/test_data_store.py 2014-11-19 14:42:09 +0000
151@@ -59,7 +59,6 @@
152 """ Test adding a file. """
153
154 self.ds.put_file(self.filename, self.contents)
155- self.addCleanup(self.ds.delete_file, self.filename)
156 files = self.ds.list_files()
157 self.assertEqual([self.filename], files)
158
159@@ -67,18 +66,20 @@
160 """ Test adding a file has correct contents. """
161
162 self.ds.put_file(self.filename, self.contents)
163- self.addCleanup(self.ds.delete_file, self.filename)
164 contents = self.ds.get_file(self.filename)
165
166 self.assertEqual(contents, self.contents)
167
168- def test_public_container(self):
169- """ Test accesssing a public container. """
170+ def test_not_public_fails(self):
171+ link = self.ds.put_file(self.filename, self.contents)
172+ self.addCleanup(self.ds.delete_file, self.filename)
173+ with self.assertRaises(urllib2.HTTPError) as cm:
174+ urllib2.urlopen(link)
175+ self.assertEqual(401, cm.exception.code)
176
177+ def test_public_succeeds(self):
178 self.ds.put_file(self.filename, self.contents)
179- self.addCleanup(self.ds.delete_file, self.filename)
180 self.ds.change_visibility(public=True)
181-
182 response = urllib2.urlopen(self.ds.file_path(self.filename))
183 self.assertEqual(response.getcode(), 200, response.read())
184
185@@ -93,18 +94,6 @@
186 files = self.ds.list_files()
187 self.assertEqual([], files)
188
189- def test_put_file_link(self):
190- """ Test put file returns a valid link. """
191-
192- link = self.ds.put_file(self.filename, self.contents)
193- self.addCleanup(self.ds.delete_file, self.filename)
194- with self.assertRaises(urllib2.HTTPError) as cm:
195- resp = urllib2.urlopen(link)
196- self.assertEqual(401, cm.exception.code)
197- self.ds.change_visibility(public=True)
198- resp = urllib2.urlopen(link)
199- self.assertEqual(200, resp.getcode(), resp.read())
200-
201
202 class TestDataStoreBadConfigs(unittest.TestCase):
203
204@@ -132,6 +121,9 @@
205 ds = data_store.DataStore("test", auth_config=conf)
206 ds.ensure_swift_client()
207
208+ # FIXME: This needs log_on_failure from
209+ # test_runner/tstrun/tests/__init__.py. The needed refactoring would make
210+ # the current MP too big -- vila 2014-11-19
211 def test_bad_region(self):
212 conf = self.get_config()
213 conf['auth_region'] = "I don't exist"

Subscribers

People subscribed via source and target branches