Merge lp://staging/~johannes.erdfelt/glance/image-undelete into lp://staging/~hudson-openstack/glance/trunk

*golf clap*

This will definitely be useful once delayed_delete is in, however, I did notice a few things:

1) There isn't very much testing around this feature. Can you more thoroughly test this at each level it is exposed?

2) The undelete action in db/api.py sets the status to 'active'. I don't think this is the best practice, as it could be undeleting an image with status 'killed' or 'saving'.

3) Can we call it 'restore' instead of 'undelete'? I wouldn't want 'delete' to be 'unadd'...

4) I'm not crazy about member actions in the API (i.e. POST /images/<id>/undelete). Can you rework it to use a PUT? Not sure what the best option would be...

1) Can you be more detailed what extra testing you think is necessary?

2) I'll work on saving the previous status and restoring that value.

3) I agree that 'restore' is a better term. I'll make those changes shortly as well.

4) I can't say I'm 100% pleased with the /undelete member (as well as /restore). However, I think PUT is worse. PUT stores the request body at the resource specified. Restoring a deleted image doesn't update the body of the resource, just makes metadata changes.

Maybe /images/<id>/actions (ala nova) or /images/restore (taking image id as a POST variable) would be better choices?

There's also the alternative of making this more a client side operation, by allowing clients to retrieve the original status and then making the client use existing API entry points to restore the status. It would be trickier from an implementation standpoint since the existing code returns 404 for deleted (but not purged) images.

It could be that images that are pending delete still let metadata updates, but they aren't returned in list operations.

I think this one of those REST impedance mismatch issues so we'll need to be creative.

> 1) Can you be more detailed what extra testing you think is necessary?

I think it is very necessary to test the glance cli tool, the api client, the api itself, the registry client, and the registy api. Each layer needs to be tested.

> 2) I'll work on saving the previous status and restoring that value.

Sounds good.

> 3) I agree that 'restore' is a better term. I'll make those changes shortly as
> well.

Excellent. If you have a better suggestion, I would love to hear it :)

> 4) I can't say I'm 100% pleased with the /undelete member (as well as
> /restore). However, I think PUT is worse. PUT stores the request body at the
> resource specified. Restoring a deleted image doesn't update the body of the
> resource, just makes metadata changes.
>
> Maybe /images/<id>/actions (ala nova) or /images/restore (taking image id as a
> POST variable) would be better choices?
>
> There's also the alternative of making this more a client side operation, by
> allowing clients to retrieve the original status and then making the client
> use existing API entry points to restore the status. It would be trickier from
> an implementation standpoint since the existing code returns 404 for deleted
> (but not purged) images.
>
> It could be that images that are pending delete still let metadata updates,
> but they aren't returned in list operations.
>
> I think this one of those REST impedance mismatch issues so we'll need to be
> creative.

I agree there isn't an obvious solution, so we will have to get creative. /images/<id>/action sounds best to me. Also, I'm thinking we should 404 on 'pending_delete', disallowing metadata updates. It should essentially be deleted.

Ok, this should now have all of the changes bcwaldon requested.

Testing looks great. Two minor nits:
1) Can you align restore_image and image_restore?
2) can you move /images/<id>/actions to /images/<id>/action? Nova uses 'action' for this kind of thing, while it uses 'actions' for admin-only reporting functionality

I've pushed up a fix that changes 'actions' to 'action'.

As far as restore_image vs image_restore, glance as a whole is very inconsistent. For instance, the client APIs generally follow <action>_<object>, whereas the DB API and some other code follow <object>_<action>.

I followed the surrounding code to determine which naming scheme to follow. My branch should be consistent within each module.

I'm indifferent which one is better, but I don't want this branch to explode in scope to fixing naming to be consistent within all of Glance. That's probably better done in another branch.

I'm cool with the naming, and thanks for updating the action collection. Looks good.

The attempt to merge lp:~johannes.erdfelt/glance/image-undelete into lp:glance failed. Below is the output from the failed tests.

running test
running egg_info
creating glance.egg-info
writing glance.egg-info/PKG-INFO
writing top-level names to glance.egg-info/top_level.txt
writing dependency_links to glance.egg-info/dependency_links.txt
writing manifest file 'glance.egg-info/SOURCES.txt'
reading manifest file 'glance.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'ChangeLog'
writing manifest file 'glance.egg-info/SOURCES.txt'
running build_ext

We test the following: ... ok
We test the following: ... ok
Test for LP Bugs #736295, #767203 ... ok
We test the following: ... ok
We test conditions that produced LP Bug #768969, where an image ... ok
Set up three test images and ensure each query param filter works ... ok
We test the following sequential series of actions: ... ok
Ensure marker and limit query params work ... ok
Set up three test images and ensure each query param filter works ... ok
We test the process flow where a user registers an image ... ok
A test against the actual datastore backend for the registry ... ok
A test that errors coming from the POST API do not ... ok
We test that various calls to the images and root endpoints are ... ok
Upload image, delete it, then restore it ... ok
Upload initial image, then attempt to upload duplicate image ... ok
Set up four test images and ensure each query param filter works ... ok
We test the following sequential series of actions: ... ok
Ensure marker and limit query params work ... ok
Set up three test images and ensure each query param filter works ... ok
We test the process flow where a user registers an image ... ok
A test against the actual datastore backend for the registry ... ok
A test that errors coming from the POST API do not ... ok
We test that various calls to the images and root endpoints are ... ok
Test logging output proper when verbose and debug ... ok
Test logging output proper when verbose and debug ... ok
A test for LP bug #704854 -- Exception thrown by registry ... ok
We test the following: ... ok
We test the following: ... ok
test that images don't get deleted immediatly and that the scrubber ... ok
test that images get deleted immediately by default ... ok
test that images can be restored after delayed delete ... ok
Tests raises BadRequest for invalid store header ... ok
Tests to add a basic image in the file store ... ok
Tests creates a queued image for no body and no loc header ... ok
Tests creates a queued image for no body and no loc header ... ok
Test that the image contents are checksummed properly ... ok
test_bad_container_format (tests.unit.test_api.TestGlanceAPI) ... ok
test_bad_disk_format (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Here, we try to delete an image that is in the queued state. ... ok
Test that the ETag header matches the x-image-meta-checksum ... ok
Tests that the /images/detail registry API returns a 400 ... ok
Tests that the /images registry API returns list of ...

Yeah, so the unit tests work fine on my development system. I even spent a while trying to figure out a way to cause the tests to fail like they did on the jenkins server.

Should we just assume it was a transient failure and try to merge again?