Skip to content
This repository has been archived by the owner on Jul 8, 2021. It is now read-only.

Fix initial list_directory function. #18

Merged
merged 2 commits into from
Oct 21, 2014

Conversation

sathlan
Copy link
Contributor

@sathlan sathlan commented Sep 17, 2014

docker-registry has a local database which can be queried using a simple
REST API. If you trash this database, it is supposed to be recreated
when you restart docker-registry. This fix the recreation of the db which
happens thanks to results of the the list_directory function. Previously
this was empty.

To test the problem and review the solution:

  1. assuming a working swift and docker registry with swift driver installed;
  2. push some images;
  3. stop docker-registry;
  4. check sqlalchemy_index_database in docker registry conf and "rm" the database (by default a sqlite file);
  5. restart docker-registry;

With the old code the database will be empty. With the new one the db
will be filled with the already pushed images. The main problem come
from the facts that:

$ curl -i -X GET -H "X-Auth-Token: AUTH_token" 'http://localhost:8080/v1/AUTH_test/docker?prefix=lvl_1/&delimiter=/&format=json'

[{"hash": "a83a7dcf68a3dd1d83584abd8e230829", "last_modified": "2014-09-17T15:26:25.585390", "bytes": 29, "name": "lvl_1/file_2", "content_type": "application/octet-stream"}, {"subdir": "lvl_1/lvl_2/"}]

and

$ curl -i -X GET -H "X-Auth-Token: AUTH_token" 'http://localhost:8080/v1/AUTH_test/docker?path=lvl_1/&format=json'

[{"hash": "a83a7dcf68a3dd1d83584abd8e230829", "last_modified": "2014-09-17T15:26:25.585390", "bytes": 29, "name": "lvl_1/file_2", "content_type": "application/octet-stream"}]

do not return the same thing. The main difference being that the
subdirectories are not return when using the "path" syntax.

I tested with swift 1.8.0 and check that the Pseudo-hierarchical folders
and directories behaviour was the same on swift 2.2.0.

This would close #14 and #15. The "fill it if not there" mechanism is
described in this issue docker-archive/docker-registry#516

docker-registry has a local database which can be queried using a simple
REST API.  If you trash this database, it is supposed to be recreated
when you restart docker-registry.  This fix the recreation of the db which
happens thanks to results of the the list_directory function.  Previously
this was empty.

To test the problem and review the solution:

 1. assuming a working swift and docker registry with swift driver installed;
 2. push some images;
 3. stop docker-registry;
 4. check sqlalchemy_index_database in docker registry conf and "rm" the database (by default a sqlite file);
 5. restart docker-registry;

With the old code the database will be empty.  With the new one the db
will be filled with the already pushed images.  The main problem come
from the facts that:

    $ curl -i -X GET -H "X-Auth-Token: AUTH_token" 'http://localhost:8080/v1/AUTH_test/docker?prefix=lvl_1/&delimiter=/&format=json'

    [{"hash": "a83a7dcf68a3dd1d83584abd8e230829", "last_modified": "2014-09-17T15:26:25.585390", "bytes": 29, "name": "lvl_1/file_2", "content_type": "application/octet-stream"}, {"subdir": "lvl_1/lvl_2/"}]

and

    $ curl -i -X GET -H "X-Auth-Token: AUTH_token" 'http://localhost:8080/v1/AUTH_test/docker?path=lvl_1/&format=json'

    [{"hash": "a83a7dcf68a3dd1d83584abd8e230829", "last_modified": "2014-09-17T15:26:25.585390", "bytes": 29, "name": "lvl_1/file_2", "content_type": "application/octet-stream"}]

do not return the same thing.  The main difference being that the
subdirectories are not return when using the "path" syntax.

I tested with swift 1.8.0 and check that the Pseudo-hierarchical folders
and directories behaviour was the same on swift 2.2.0.

This would close bacongobbler#14 and bacongobbler#15.  The "fill it if not there" mechanism is
described in this issue  docker-archive/docker-registry#516
@bacongobbler
Copy link
Owner

Hi @sathlan, could you please supply some regression tests that demonstrate this behaviour? It would be greatly appreciated :)

@sathlan
Copy link
Contributor Author

sathlan commented Sep 18, 2014

Hi @bacongobbler, I'll look into it.

@sathlan
Copy link
Contributor Author

sathlan commented Sep 18, 2014

Well, It may require me some time ... and/or I may use some help.

The problem is that to test it I have to test the _walk_storage function . I do not know python well enough to do it quickly, though.

In the meantime, I have "improved" the mocked get_containter function. This now behaves more like the real thing. It only works for 3 level deep files, but this would be enough to raise the error, would I be able to test _walk_storage.

So if you have any suggestion to where to start to test _walk_storage, I'll take it, if not, I'll try by myself, given some time :)

@sathlan
Copy link
Contributor Author

sathlan commented Sep 22, 2014

Hi @bacongobbler,

It seems there is no easy way to properly test this, as seen in this thread docker-archive/docker-registry#585.
Would it be ok for you if I include a driver specific non-regression test, that copy the _walk_storage function function behavior well enough to trigger the error with the old version and get fixed with the new version ?

@bacongobbler
Copy link
Owner

Yep! as long as there's some way we can somehow show this behaviour and ensure that we don't see this error again in future releases.

@sathlan
Copy link
Contributor Author

sathlan commented Sep 23, 2014

Oki, I added the tests, the get_container is a bit more complicated that I wanted, but it mimics the behavior of the swift API better. Hopes it's ok :)

@sathlan sathlan force-pushed the fix/list_directory branch from 22066f8 to 6d75537 Compare October 16, 2014 02:54
@sathlan sathlan force-pushed the fix/list_directory branch from 6d75537 to 44148d8 Compare October 16, 2014 02:59
@sathlan
Copy link
Contributor Author

sathlan commented Oct 16, 2014

Hi,

sorry for the lag. So after the integration of the test in docker-archive/docker-registry#596, here is the complete patch with the non regression test and a "better" mocking function.

@bacongobbler
Copy link
Owner

lgtm! thanks :)

bacongobbler pushed a commit that referenced this pull request Oct 21, 2014
Fix initial list_directory function.
@bacongobbler bacongobbler merged commit c52606f into bacongobbler:master Oct 21, 2014
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list_directory always return empty list
2 participants