)]}'
{"glance/async/flows/base_import.py":[{"author":{"_account_id":7012,"name":"Brianna Poulos","email":"Brianna.Poulos@jhuapl.edu","username":"brianna-poulos"},"change_message_id":"d046568d9d1d7351e732b57013d05c88552a68e3","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        try:"},{"line_number":294,"context_line":"            image \u003d self.image_repo.get(image_id)"},{"line_number":295,"context_line":"            image.delete()"},{"line_number":296,"context_line":"            #NOTE(sabari): Deleting the image is probably the option for now"},{"line_number":297,"context_line":"            # until flow failures are handled better by retries."},{"line_number":298,"context_line":"            self.image_repo.remove(image)"},{"line_number":299,"context_line":"        except exception.NotFound:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba3cc151_9b94838b","line":296,"updated":"2015-07-08 15:18:13.000000000","message":"There should be a space between \"#\" and \"NOTE\"","commit_id":"81466a9a820fe94230864ae09e7dc16bc240ed8f"},{"author":{"_account_id":7575,"name":"Sabari","email":"smurugesan@vmware.com","username":"sabari"},"change_message_id":"fcea81300ffba3c3e92cc1ccd895d026803cf3fc","unresolved":false,"context_lines":[{"line_number":70,"context_line":"        return image.image_id"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    def revert(self, *args, **kwargs):"},{"line_number":73,"context_line":"        # TODO(flaper87): Define the revert rules for images on failures."},{"line_number":74,"context_line":"        # Deleting the image may not be what we want since users could upload"},{"line_number":75,"context_line":"        # the image data in a separate step. However, it really depends on"},{"line_number":76,"context_line":"        # when the failure happened. I guess we should check if data has been"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a50d1a3_78bf28aa","side":"PARENT","line":73,"updated":"2015-07-22 20:42:05.000000000","message":"@flaper87(past): The revert for CreateImage task should delete the image. However, whether this task should be reverted is something to be decided by the flow\u0027s retry strategy.  For now, I modified the strategy to revert all tasks. But in the future, the retry flow will make an informed decision. \n\n@flaper87(present): Does this address your TODO ? :)","commit_id":"d385fe5cefef7bb0a05f3a6a70b64fc4ad8d59ff"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"28eecc0af2e60983b118251af71fe8844c40ba06","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                  {\u0027task_id\u0027: self.task_id, \u0027image_id\u0027: result})"},{"line_number":82,"context_line":"        try:"},{"line_number":83,"context_line":"            image \u003d self.image_repo.get(result)"},{"line_number":84,"context_line":"            image.delete()"},{"line_number":85,"context_line":"            self.image_repo.remove(image)"},{"line_number":86,"context_line":"        except exception.NotFound:"},{"line_number":87,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_966386b1","line":84,"updated":"2015-09-25 07:14:57.000000000","message":"why is this calling delete *and* remove?","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"5f325df7faf5c0368cd36d69520e576d8a88b6c1","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                  {\u0027task_id\u0027: self.task_id, \u0027image_id\u0027: result})"},{"line_number":82,"context_line":"        try:"},{"line_number":83,"context_line":"            image \u003d self.image_repo.get(result)"},{"line_number":84,"context_line":"            image.delete()"},{"line_number":85,"context_line":"            self.image_repo.remove(image)"},{"line_number":86,"context_line":"        except exception.NotFound:"},{"line_number":87,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_d272617c","line":84,"in_reply_to":"9a1a9d01_799583e2","updated":"2015-09-25 12:01:59.000000000","message":"mmmhh... Following the domain model chain down the path of the `delete` method, it should delete the image and the data.\n\nHere[0] the image repo is created. It\u0027s chained with the locations proxy, which should take care of deleting the image data. I wonder why that\u0027s not happening.\n\nProbably, what\u0027s really missing is calling `save` after the `delete` call since the status change doesn\u0027t do that.[1]\n\n[0] https://github.com/openstack/glance/blob/0f26b30a6bfa3a3ee0483a6f65ada289f2d476a3/glance/gateway.py#L74\n\n[1] https://github.com/openstack/glance/blob/0f26b30a6bfa3a3ee0483a6f65ada289f2d476a3/glance/domain/__init__.py#L242","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":7575,"name":"Sabari","email":"smurugesan@vmware.com","username":"sabari"},"change_message_id":"f5c05e9169e3b4bb4a1720c9e9af680c5b0446f0","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                  {\u0027task_id\u0027: self.task_id, \u0027image_id\u0027: result})"},{"line_number":82,"context_line":"        try:"},{"line_number":83,"context_line":"            image \u003d self.image_repo.get(result)"},{"line_number":84,"context_line":"            image.delete()"},{"line_number":85,"context_line":"            self.image_repo.remove(image)"},{"line_number":86,"context_line":"        except exception.NotFound:"},{"line_number":87,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_799583e2","line":84,"in_reply_to":"9a1a9d01_966386b1","updated":"2015-09-25 08:15:01.000000000","message":"AFAIK, the delete marks the status as \u0027deleted\u0027 and removes the data from store, but the db record is deleted only when image_repo.remove() is called. \n\nCalling remove without \u0027delete\u0027 doesn\u0027t move the status of image to \u0027deleted\u0027 or remove the store data.\n\nThat\u0027s my cursory understanding of our domain model :). \n\nWe also do the same in v2 delete :- https://github.com/openstack/glance/blob/0f26b30a6bfa3a3ee0483a6f65ada289f2d476a3/glance/api/v2/images.py#L236","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":7575,"name":"Sabari","email":"smurugesan@vmware.com","username":"sabari"},"change_message_id":"a727d85caea6be769b321e4b56207766ed45ea9b","unresolved":false,"context_lines":[{"line_number":81,"context_line":"                  {\u0027task_id\u0027: self.task_id, \u0027image_id\u0027: result})"},{"line_number":82,"context_line":"        try:"},{"line_number":83,"context_line":"            image \u003d self.image_repo.get(result)"},{"line_number":84,"context_line":"            image.delete()"},{"line_number":85,"context_line":"            self.image_repo.remove(image)"},{"line_number":86,"context_line":"        except exception.NotFound:"},{"line_number":87,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_93605937","line":84,"in_reply_to":"9a1a9d01_d272617c","updated":"2015-09-25 20:35:56.000000000","message":"Even though ImageRepoProxy is chained with locations, it doesn\u0027t act upon upon \u0027remove\u0027 and thus doesn\u0027t delete the image data [0].\n\nCalling image_repo.\u0027save\u0027 after marking status as delete won\u0027t remove tags, locations, member\u0027s etc associated with the image. Only \u0027remove\u0027 does all that [1].\n\nSo, looks like we need both image.delete() and image_repo.remove(image) for a clean deletion.\n\n[0] - https://github.com/openstack/glance/blob/0f26b30a6bfa3a3ee0483a6f65ada289f2d476a3/glance/location.py#L38 (No remove method)\n\n[1] - https://github.com/openstack/glance/blob/878c6a6599470a8faa5a480aaa9419881893a808/glance/db/__init__.py#L293","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"28eecc0af2e60983b118251af71fe8844c40ba06","unresolved":false,"context_lines":[{"line_number":325,"context_line":"                                   \u0027image_id\u0027: image_id})"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            image \u003d self.image_repo.get(image_id)"},{"line_number":328,"context_line":"            image.delete()"},{"line_number":329,"context_line":"        except exception.NotFound:"},{"line_number":330,"context_line":"            pass"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_16e53608","line":328,"updated":"2015-09-25 07:14:57.000000000","message":"This shouldn\u0027t delete the image object but the data. The image record should be deleted when reverting the `_CreateImage` task.\n\nIt should be (probably) enough to implement `revert` for the `_CreateImage` task only since these two should always be together and deleting the image should get rid of the data too.","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":7575,"name":"Sabari","email":"smurugesan@vmware.com","username":"sabari"},"change_message_id":"f5c05e9169e3b4bb4a1720c9e9af680c5b0446f0","unresolved":false,"context_lines":[{"line_number":325,"context_line":"                                   \u0027image_id\u0027: image_id})"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            image \u003d self.image_repo.get(image_id)"},{"line_number":328,"context_line":"            image.delete()"},{"line_number":329,"context_line":"        except exception.NotFound:"},{"line_number":330,"context_line":"            pass"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_59bbdf72","line":328,"in_reply_to":"9a1a9d01_16e53608","updated":"2015-09-25 08:15:01.000000000","message":"This doesn\u0027t delete the image from the db. Marks it as deleted and removes the data from store. The status is never persisted to db, so the image retains its last state. May be I should just call the store API\u0027s remove_from_backend method instead of using this shortcut :).\n\nI wanted the ImportToStore\u0027s revert to just remove the data from the store. Thus only reverting the work of this task.","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":7575,"name":"Sabari","email":"smurugesan@vmware.com","username":"sabari"},"change_message_id":"a727d85caea6be769b321e4b56207766ed45ea9b","unresolved":false,"context_lines":[{"line_number":325,"context_line":"                                   \u0027image_id\u0027: image_id})"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            image \u003d self.image_repo.get(image_id)"},{"line_number":328,"context_line":"            image.delete()"},{"line_number":329,"context_line":"        except exception.NotFound:"},{"line_number":330,"context_line":"            pass"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_f390b5ee","line":328,"in_reply_to":"9a1a9d01_32b5ddcc","updated":"2015-09-25 20:35:56.000000000","message":"You brought a very good point. If any other task fails after this, it\u0027s the responsibility of that task to revert the state to \u0027saving\u0027. e.g  the _SaveImage below is responsible for moving the image to \u0027active\u0027 and hence should revert it to \u0027saving\u0027 upon failure.\n\nFor this task, I should remove the image data \u0026 move the status back to \u0027queued\u0027 (may be for sanity also check if previous state was \u0027saving\u0027).\n\nThis is getting a little bigger than I thought. Thanks for the perspective :)","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"5f325df7faf5c0368cd36d69520e576d8a88b6c1","unresolved":false,"context_lines":[{"line_number":325,"context_line":"                                   \u0027image_id\u0027: image_id})"},{"line_number":326,"context_line":"        try:"},{"line_number":327,"context_line":"            image \u003d self.image_repo.get(image_id)"},{"line_number":328,"context_line":"            image.delete()"},{"line_number":329,"context_line":"        except exception.NotFound:"},{"line_number":330,"context_line":"            pass"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"9a1a9d01_32b5ddcc","line":328,"in_reply_to":"9a1a9d01_59bbdf72","updated":"2015-09-25 12:01:59.000000000","message":"I\u0027d agree that, if we want to do anything here, it should be limited to deleting the data from the store calling the store utils and not the `delete` method.\n\nThat said, there\u0027s another issue here. We don\u0027t know exactly what step failed.\n\nIf the image is in status \u0027saving\u0027 then it probably failed saving the data or updating the image status. If it\u0027s in status active, it might have failed somewhere after this task. The problem here is that we don\u0027t know what status the image will have to be moved to once this method is called.\n\nIf it is saving, it should probably stay as-is and the data could be removed. If not, then we can\u0027t just remove the data and not change the status to something else: (killed?)","commit_id":"5b4426163839c7ef0e809232a29d1e54ea23dab5"}]}