)]}'
{"cinder/image/image_utils.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"2824f9177fff7d8ec8981967935fe24926dcaa8e","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            message \u003d _(\"Insufficient free space on %(location)s for image \""},{"line_number":299,"context_line":"                        \"conversion.\") % {\u0027location\u0027: conversion_dir}"},{"line_number":300,"context_line":"            LOG.error(message)"},{"line_number":301,"context_line":"            # Cleanup the residue destination image as it is unusable and"},{"line_number":302,"context_line":"            # taking unnecessary space"},{"line_number":303,"context_line":"            fileutils.delete_if_exists(dest)"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        raise"},{"line_number":306,"context_line":"    duration \u003d timeutils.delta_seconds(start_time, timeutils.utcnow())"}],"source_content_type":"text/x-python","patch_set":6,"id":"ff570b3c_026ad2b1","line":303,"range":{"start_line":301,"start_character":0,"end_line":303,"end_character":44},"updated":"2020-05-12 21:32:59.000000000","message":"I wonder if we should move this out of the \"if\".  execute() can raise ProcessExecutionError or OSError.  If it\u0027s OSError, we\u0027re probably hosed, so maybe just let that go, but since we\u0027re not setting check_exit_code in the call to execute(), any non-zero result from the qemu-convert will raise a ProcessExecutionError, in which case there\u0027s no reason to keep around the dest, so might as well delete it if it exists before we raise.","commit_id":"b1456d455e039d0fd8490bad63d217e1309ebb4a"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"b0b3a2786e41aa8a76407908c5ddf6a5857c8841","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            message \u003d _(\"Insufficient free space on %(location)s for image \""},{"line_number":299,"context_line":"                        \"conversion.\") % {\u0027location\u0027: conversion_dir}"},{"line_number":300,"context_line":"            LOG.error(message)"},{"line_number":301,"context_line":"            # Cleanup the residue destination image as it is unusable and"},{"line_number":302,"context_line":"            # taking unnecessary space"},{"line_number":303,"context_line":"            fileutils.delete_if_exists(dest)"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        raise"},{"line_number":306,"context_line":"    duration \u003d timeutils.delta_seconds(start_time, timeutils.utcnow())"}],"source_content_type":"text/x-python","patch_set":6,"id":"ff570b3c_ccd488c0","line":303,"range":{"start_line":301,"start_character":0,"end_line":303,"end_character":44},"in_reply_to":"ff570b3c_026ad2b1","updated":"2020-05-13 08:07:14.000000000","message":"Since the original issue was reported when there was no space left on the disk, i added it in the if block.\nbut as we want to clean that image anyway, it makes sense outside the if block as well.","commit_id":"b1456d455e039d0fd8490bad63d217e1309ebb4a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"095e163baef2cd1253917b51c800a1f503dfc70b","unresolved":false,"context_lines":[{"line_number":242,"context_line":"                   run_as_root\u003dTrue, cipher_spec\u003dNone,"},{"line_number":243,"context_line":"                   passphrase_file\u003dNone, compress\u003dFalse):"},{"line_number":244,"context_line":"    \"\"\"Convert image to other format."},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    :param prefix: command prefix, i.e. cgexec for throttling"},{"line_number":247,"context_line":"    :param source: source filename"},{"line_number":248,"context_line":"    :param dest: destination filename"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_b548855c","line":245,"updated":"2020-06-10 18:07:19.000000000","message":"I suggest adding a note in this docstring saying something like:\n\nNOTE: if the qemu-img convert command fails and this function raises an exception, a non-empty dest file may be left in the filesystem.  It is the responsibility of the caller to decide what to do with this file.","commit_id":"df9ea2be3e8ab5952350378ad60fed4c5631a3d3"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"d8f531a9d4be4fe2b0bf37b4928a8ddf6d06462f","unresolved":false,"context_lines":[{"line_number":242,"context_line":"                   run_as_root\u003dTrue, cipher_spec\u003dNone,"},{"line_number":243,"context_line":"                   passphrase_file\u003dNone, compress\u003dFalse):"},{"line_number":244,"context_line":"    \"\"\"Convert image to other format."},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    :param prefix: command prefix, i.e. cgexec for throttling"},{"line_number":247,"context_line":"    :param source: source filename"},{"line_number":248,"context_line":"    :param dest: destination filename"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e4c30e56","line":245,"in_reply_to":"ff570b3c_b548855c","updated":"2020-06-23 09:52:17.000000000","message":"Done","commit_id":"df9ea2be3e8ab5952350378ad60fed4c5631a3d3"}],"cinder/volume/drivers/rbd.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f39c39a6fbb0733b4d4b808c7d5f85089d60cd56","unresolved":false,"context_lines":[{"line_number":934,"context_line":"                       tmp_image.name, volume.name]"},{"line_number":935,"context_line":"                cmd.extend(self._ceph_args())"},{"line_number":936,"context_line":"                self._execute(*cmd)"},{"line_number":937,"context_line":"        finally:"},{"line_number":938,"context_line":"            fileutils.delete_if_exists(tmp_key.name)"},{"line_number":939,"context_line":"            fileutils.delete_if_exists(tmp_image.name)"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_e09b0e15","line":937,"updated":"2020-04-27 15:19:45.000000000","message":"What\u0027s your theory about what\u0027s happening here?  As Fernando points out, the context manager should delete the file if an exception occurs within the context.  I suspect because we\u0027re passing the file out to the operating system to be executed, there\u0027s another file handle being held (ours in python plus the one \"out there\") that\u0027s preventing deletion.  If that\u0027s the case, I wonder whether the below statements are really cleaning stuff up.  oslo fileutils uses os.unlink() whose docs say \"On Windows, attempting to remove a file that is in use causes an exception to be raised; on Unix, the directory entry is removed but the storage allocated to the file is not made available until the original file is no longer in use.\"  So I wonder whether the temporary storage is being deleted, or just the directory entry, in which case we\u0027re just treating a symptom.","commit_id":"d4c8152a6cba3a382cf2df8c3b094ce7af800025"},{"author":{"_account_id":30555,"name":"Fernando Ferraz","display_name":"Fernando Ferraz","email":"fesilva@redhat.com","username":"fernandoperches"},"change_message_id":"0f73ec51c49ff2062db39cf590a5642e093b9cd2","unresolved":false,"context_lines":[{"line_number":934,"context_line":"                       tmp_image.name, volume.name]"},{"line_number":935,"context_line":"                cmd.extend(self._ceph_args())"},{"line_number":936,"context_line":"                self._execute(*cmd)"},{"line_number":937,"context_line":"        finally:"},{"line_number":938,"context_line":"            fileutils.delete_if_exists(tmp_key.name)"},{"line_number":939,"context_line":"            fileutils.delete_if_exists(tmp_image.name)"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_69440275","line":937,"in_reply_to":"1f493fa4_060c4deb","updated":"2020-04-27 20:13:19.000000000","message":"Hi Rajat, I may be wrong but _TemporaryFileWrapper [1] should guarantee the temp file deletion in a with statement.\n\n[1] https://github.com/python/cpython/blob/5d1f32d33ba24d0aa87235ae40207bb57778388b/Lib/tempfile.py#L490","commit_id":"d4c8152a6cba3a382cf2df8c3b094ce7af800025"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4fea910941e7962ad781ecaf36c3be3704bb3a08","unresolved":false,"context_lines":[{"line_number":934,"context_line":"                       tmp_image.name, volume.name]"},{"line_number":935,"context_line":"                cmd.extend(self._ceph_args())"},{"line_number":936,"context_line":"                self._execute(*cmd)"},{"line_number":937,"context_line":"        finally:"},{"line_number":938,"context_line":"            fileutils.delete_if_exists(tmp_key.name)"},{"line_number":939,"context_line":"            fileutils.delete_if_exists(tmp_image.name)"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_ebcda231","line":937,"in_reply_to":"1f493fa4_060c4deb","updated":"2020-04-28 03:05:46.000000000","message":"Looks like the NamedTemporaryFile is handed to you in a wrapper that defines __enter__() and __exit__() methods, so it does implement the context manager interface, just not using the decorators.\n\nMy point about os.unlink is that if the reason why the cleanup isn\u0027t happening in the normal (context manager) case is that some other process is holding an open file handle, then calling os.unlink via fileutils may be removing the directory entry but not actually freeing the storage (at least the docs say that\u0027s what can happen).  In other words, the file no longer showing as existing in a directory may not actually mean that the resource we\u0027re worried about has been freed up.\n\nHow often does this happen?  If it happens a lot, then we do want to make sure we\u0027re fixing the right thing.","commit_id":"d4c8152a6cba3a382cf2df8c3b094ce7af800025"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"31409f5e5b70b876f5d5379b20a531c735a3e7b3","unresolved":false,"context_lines":[{"line_number":934,"context_line":"                       tmp_image.name, volume.name]"},{"line_number":935,"context_line":"                cmd.extend(self._ceph_args())"},{"line_number":936,"context_line":"                self._execute(*cmd)"},{"line_number":937,"context_line":"        finally:"},{"line_number":938,"context_line":"            fileutils.delete_if_exists(tmp_key.name)"},{"line_number":939,"context_line":"            fileutils.delete_if_exists(tmp_image.name)"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_3c035675","line":937,"in_reply_to":"1f493fa4_69440275","updated":"2020-04-27 20:53:48.000000000","message":"Good point. As much as it\u0027s interesting to find out why that doesn\u0027t happen. This patch is still a safety mechanism to ensure we delete it if it\u0027s leftout which is currently the issue.","commit_id":"d4c8152a6cba3a382cf2df8c3b094ce7af800025"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"de446bea3a0ecf93b7d25f737982df85ae45d50c","unresolved":false,"context_lines":[{"line_number":934,"context_line":"                       tmp_image.name, volume.name]"},{"line_number":935,"context_line":"                cmd.extend(self._ceph_args())"},{"line_number":936,"context_line":"                self._execute(*cmd)"},{"line_number":937,"context_line":"        finally:"},{"line_number":938,"context_line":"            fileutils.delete_if_exists(tmp_key.name)"},{"line_number":939,"context_line":"            fileutils.delete_if_exists(tmp_image.name)"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_060c4deb","line":937,"in_reply_to":"1f493fa4_e09b0e15","updated":"2020-04-27 19:06:34.000000000","message":"It should\u0027ve if the NamedTemporaryFile method was defined properly to be used as a context[1].\nIt doesn\u0027t use the contextmanager decorator, and should ideally have this try, finally block to handle the cleanup but it doesn\u0027t which makes me wonder why we use that method like this here anyway.\nRegarding the os.unlink point, the exception occurred so we can safely assume the file isn\u0027t in use and will be deleted.\nthe same fileutils module is used in rest of cinder to perform the cleanup.\nAlso Sofia tested this out with the exception and the results are expected.\n\n[1] https://github.com/python/cpython/blob/master/Lib/tempfile.py#L514","commit_id":"d4c8152a6cba3a382cf2df8c3b094ce7af800025"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"1bef817d1f2f59507cb56db3f9e1bb8b69b21d15","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"                # Replace the original image with the now encrypted image"},{"line_number":1590,"context_line":"                os.rename(dest_image_path, src_image_path)"},{"line_number":1591,"context_line":"            finally:"},{"line_number":1592,"context_line":"                fileutils.delete_if_exists(dest_image_path)"},{"line_number":1593,"context_line":""},{"line_number":1594,"context_line":"    def _copy_image_to_volume(self, context, volume, image_service, image_id,"},{"line_number":1595,"context_line":"                              encrypted\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff570b3c_99e4bbae","line":1592,"updated":"2020-05-11 21:14:51.000000000","message":"Nice work tracking this down.  I wonder whether it might be better to handle this within the convert_image function so that the caller doesn\u0027t have to worry about it, though.  Either way, I\u0027d suggest adding a docstring to image_utils.convert_image() saying something about it cleaning up after itself.","commit_id":"7da011a527b841f1237a0b351de8c04915ca9e6f"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"72245e161cbf1a84907a47ee4797d9c6279cb4c8","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"                # Replace the original image with the now encrypted image"},{"line_number":1590,"context_line":"                os.rename(dest_image_path, src_image_path)"},{"line_number":1591,"context_line":"            finally:"},{"line_number":1592,"context_line":"                fileutils.delete_if_exists(dest_image_path)"},{"line_number":1593,"context_line":""},{"line_number":1594,"context_line":"    def _copy_image_to_volume(self, context, volume, image_service, image_id,"},{"line_number":1595,"context_line":"                              encrypted\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff570b3c_6cbc7805","line":1592,"in_reply_to":"ff570b3c_99e4bbae","updated":"2020-05-12 10:21:15.000000000","message":"Yeah, i checked all the calls and no caller is taking care of the residue file so i also think it\u0027s better to be handled in convert_image.","commit_id":"7da011a527b841f1237a0b351de8c04915ca9e6f"}],"releasenotes/notes/cleanup-dest-image-during-exception-3851dbba0004f136.yaml":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ea57315a4ab7c2bfba6abe2a784049a3a1663856","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Added cleanup for residue images if the image"},{"line_number":5,"context_line":"    convert operation fails."}],"source_content_type":"text/x-yaml","patch_set":7,"id":"ff570b3c_ce104353","line":4,"range":{"start_line":4,"start_character":4,"end_line":4,"end_character":9},"updated":"2020-05-22 01:55:01.000000000","message":"Let\u0027s give this some context by mentioning the bug, e.g.,\n\n  `Bug #1873738 \u003chttps://bugs.launchpad.net/cinder/+bug/1873738\u003e`_: Added","commit_id":"bfb1c44b2bcd7cd14effa78c34ae159db652f57d"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2aec59150cdf717bbc0bf236ce567da6a7034382","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Added cleanup for residue images if the image"},{"line_number":5,"context_line":"    convert operation fails."}],"source_content_type":"text/x-yaml","patch_set":7,"id":"ff570b3c_6949318a","line":4,"range":{"start_line":4,"start_character":4,"end_line":4,"end_character":9},"in_reply_to":"ff570b3c_ce104353","updated":"2020-05-22 06:27:14.000000000","message":"Done","commit_id":"bfb1c44b2bcd7cd14effa78c34ae159db652f57d"}]}