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

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

One in-line question, otherwise +1.

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

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?

> > > - 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.