)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":26250,"name":"Johannes Kulik","email":"johannes.kulik@sap.com","username":"jkulik"},"change_message_id":"f3d779b6c200f4db5fa87477d7fd37bd24dfb33a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b3b877b0_9e06bd50","updated":"2024-02-20 07:52:01.000000000","message":"Looking at the commit that introduced this code: We basically open up that bug now again unless we have very careful handling outside of the `glance` module, don\u0027t we?\n\nIs there some way to wrap the Iterator so it closes itself when it\u0027s done?","commit_id":"ec87ed466db975fa42b3153a26a5feda689717bc"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"e2899f6ec3c7797afdea0acd5053e7c36f03dbd6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"dbbc1be0_c6beaf88","updated":"2024-02-19 17:43:32.000000000","message":"Took a while, but I finally found it.","commit_id":"ec87ed466db975fa42b3153a26a5feda689717bc"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"eeae820353ac81af31f0b3eac25677688e765b67","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"bc626699_b954840e","in_reply_to":"b3b877b0_9e06bd50","updated":"2024-02-20 08:54:13.000000000","message":"That is the fun part, the iterator closes itself already when its done:\nhttps://opendev.org/openstack/python-glanceclient/src/branch/master/glanceclient/common/utils.py#L569-L573\n\n\nThe bug is the scenario of a failing verification, but the closing was added also when no verification is done. I reformulated the patch that makes this more clear. \n\nI am still unclear, how the patch actually addresses the problem, and suspect it calls close \"too often\". Not much of a problem by itself, but it would indicate that the problem is not correctly handled.\n\nThe scenario was `rbd`, but that is a completely different caller (https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L358-L368).\n\nThere the caller opens the file with a with-statement, which will close `fh` the same way the finally-block in `_verify_and_write` does. (I\u0027ll change the test to verify the number of times close has been called).\n\n\nHere is the \"normal\" download \n(https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L375-L388).","commit_id":"ec87ed466db975fa42b3153a26a5feda689717bc"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"cfd9986bfa8b67521feeeae7780e9fc677520c07","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"2b02c4ba_8911d585","updated":"2024-03-21 12:35:07.000000000","message":"Hi, \n\nit would be good to get this (or a similar patch) in. Without it, no instance creations with images from glance with the vmwareapi driver are possible.\n\nIt would also reduce the load on the vmwarepi CI, as it is currently mostly waiting for instances to come up.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"612d9bb2e6d3af84f71afbbee6bd7f8c1c35c262","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"94272e78_d8419566","updated":"2024-03-20 08:21:36.000000000","message":"I hope, this should cover it. The Zool tests are green, the vmwareapi CI test shows the effect of the change that we can now at least create instances again, if not all tests are green.\nFor the other tests, I have change requests in the pipeline, but this change (or a similar) needs to be merged before those make even sense.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8069627c65b0d4ecf4a588fd85e347771323ec66","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"3404a0ba_3bc83d6c","updated":"2024-04-12 10:30:14.000000000","message":"ignoring this need two core to review for a sec im not comfortable +w this change so while this appears to be correct and appears to pass tempest i real want another core to be the one to +w. the image verification feature is one if have only looked at breifly and im not sure if there is any implict depency on the order although im not seeing any new failure mode intoduced by the reodering.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"46ad2c1fb12d049f52d468e114d547bba0a20df1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"d87edeab_3e84bc2c","updated":"2024-04-30 16:02:56.000000000","message":"I hope, we are now set. Aren\u0027t we?","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"be99c4106fff75d00254e14f16540b60219d95d5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"348d3bc4_d6547d3f","updated":"2024-04-25 15:03:06.000000000","message":"Okay, well, the change here is now fully covered and is better than it was. Still would like to see the follow-up to clean up the potential leak, but that can be separate as noted.","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7061f15d95fc323d4aa884419228c9d3e3811dd6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"c489642a_b07648e7","updated":"2024-05-06 13:54:32.000000000","message":"recheck - nova-multi-cell: seemingly random timeouts for rabbitmq messages, https; nova-ceph-multistore: nova-compute node not updating. Last update: https://zuul.opendev.org/t/openstack/build/026e9e21f86c461e818eae1824d7e364/log/controller/logs/screen-n-cond-cell2.txt#1074. Can\u0027t find anything obvious in the nova-compute logs.","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6ada906d4ab962f49fc0b1587a2d94715e3d1c9d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"dd94b283_4acfc916","updated":"2024-05-01 10:04:07.000000000","message":"this still looks gerenally ok to me and dans concenrs have been adressed.\nso i guess we can proceed with this to unblock the vmware driver.\n\nbased on http://openstack-ci-logs.global.cloud.sap/openstack-nova-909474-27sgn/tempest.html image download appears to be working in general and there are now only  2 volume test failign form the enabeld set","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"}],"nova/image/glance.py":[{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"2952b7edf1843eccba6ea0e05db813b70881aaa1","unresolved":false,"context_lines":[{"line_number":428,"context_line":"            if data is None:"},{"line_number":429,"context_line":"                return image_chunks"},{"line_number":430,"context_line":"            else:"},{"line_number":431,"context_line":"                return"},{"line_number":432,"context_line":""},{"line_number":433,"context_line":"        try:"},{"line_number":434,"context_line":"            for chunk in image_chunks:"}],"source_content_type":"text/x-python","patch_set":2,"id":"78a5c290_12074330","line":431,"updated":"2024-02-20 09:21:32.000000000","message":"I suspect the bug report is incorrect.\nThe only way I can see that the iterator is not closed is actually in this code path (which would indeed re-introduce the bug)\n\nIf the caller passes the image_chunks to verify, but there is nothing to verify then we return `None` and no one consumes the iterator, and it won\u0027t get closed.\n\nThe bug report https://bugs.launchpad.net/nova/+bug/1948706 though describes a failed verification after a successful \"download\" over rbd.\n\nIn that scenario, the file should be closed even twice before the patch, and now thrice:\n1. By the with-statement here:\n   https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L363-L368\n2. The loop consumes all chunks here:\n   https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L434-L438\n   that triggers the closing of the iterator here:\n   https://github.com/openstack/python-glanceclient/blob/master/glanceclient/common/utils.py#L573\n3. The third one breaks our neck:\n   https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L468\n   \nMy understanding is, the only way the iterator cannot be closed is, by not consuming the chunks,\nwhich only happens here:\n  https://opendev.org/openstack/nova/src/branch/master/nova/image/glance.py#L429-L432\n \nEither\n- The image_chunks are returned, and the caller is not consuming the iterator, but then it would be the callers responsibility anyway to close the file (that\u0027s \"us\")\n- The function bails out early as an optimization, because there is nothing to be verified, or written. Then the source should be closed.","commit_id":"b0b857a668962e59c3e1d39ad1125541f06de3ee"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e99f26493461ae62b8476a78db4933620a6f3d10","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                data.flush()"},{"line_number":465,"context_line":"                self._safe_fsync(data)"},{"line_number":466,"context_line":"                data.close()"},{"line_number":467,"context_line":"            if isinstance(image_chunks, glance_utils.IterableWithLength):"},{"line_number":468,"context_line":"                image_chunks.iterable.close()"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"df4a0d28_15b101ce","side":"PARENT","line":467,"updated":"2024-02-20 11:56:21.000000000","message":"instead of removing this form the finally\n\ni would proably do this instead.\n\n``` \nif data is not None\n  if isinstance(image_chunks, glance_utils.IterableWithLength):\n      # No one consumes the chunks, so we better close the source\n      image_chunks.iterable.close()\n```\n\ni would like to ensure this is closed if we exit with an exception\n\nlooking at the code we only return the iterator if data is none so by checking\nif it is not none and only calling close in that case i belive it has the same effect as your current patch.","commit_id":"6e510eb62e00c34e98a5245a6de2dd2955ffb57a"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"22171e8e0db88f97abc464b42291207b4a54f323","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                data.flush()"},{"line_number":465,"context_line":"                self._safe_fsync(data)"},{"line_number":466,"context_line":"                data.close()"},{"line_number":467,"context_line":"            if isinstance(image_chunks, glance_utils.IterableWithLength):"},{"line_number":468,"context_line":"                image_chunks.iterable.close()"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"d25bf432_9e6fe476","side":"PARENT","line":467,"in_reply_to":"177cf496_194c0acc","updated":"2024-03-19 15:21:52.000000000","message":"I think, I finally understood how the bug occurred in the first place.\nWhen `_get_verifier` call raises an exception, we never iterate over the data, so we never enter the loop of the `glance_utils.IterableWithLength`, which then never close the file.\n\nDoing so at the end solved that issue, but raises #2053027.\n\nMy proposal is to move the `_get_verifier` call before any IO is being done, as it is a purely metadata call, removing the outfall of a \"random\" exception unrelated of the IO.","commit_id":"6e510eb62e00c34e98a5245a6de2dd2955ffb57a"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"293259fd5d326eb71795b1ab5eec1d230fc755dc","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                data.flush()"},{"line_number":465,"context_line":"                self._safe_fsync(data)"},{"line_number":466,"context_line":"                data.close()"},{"line_number":467,"context_line":"            if isinstance(image_chunks, glance_utils.IterableWithLength):"},{"line_number":468,"context_line":"                image_chunks.iterable.close()"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"177cf496_194c0acc","side":"PARENT","line":467,"in_reply_to":"367763a3_646ae646","updated":"2024-02-20 12:57:00.000000000","message":"Yes, you are right. I missed that the `_get_verifier` may raise an exception. Going back to the initial proposal.","commit_id":"6e510eb62e00c34e98a5245a6de2dd2955ffb57a"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"612d9bb2e6d3af84f71afbbee6bd7f8c1c35c262","unresolved":false,"context_lines":[{"line_number":464,"context_line":"                data.flush()"},{"line_number":465,"context_line":"                self._safe_fsync(data)"},{"line_number":466,"context_line":"                data.close()"},{"line_number":467,"context_line":"            if isinstance(image_chunks, glance_utils.IterableWithLength):"},{"line_number":468,"context_line":"                image_chunks.iterable.close()"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"c5dd4149_269277aa","side":"PARENT","line":467,"in_reply_to":"d25bf432_9e6fe476","updated":"2024-03-20 08:21:36.000000000","message":"Acknowledged","commit_id":"6e510eb62e00c34e98a5245a6de2dd2955ffb57a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"61c6dbe7fd995a7fa4017ac8e8e88de270c6c729","unresolved":true,"context_lines":[{"line_number":464,"context_line":"                data.flush()"},{"line_number":465,"context_line":"                self._safe_fsync(data)"},{"line_number":466,"context_line":"                data.close()"},{"line_number":467,"context_line":"            if isinstance(image_chunks, glance_utils.IterableWithLength):"},{"line_number":468,"context_line":"                image_chunks.iterable.close()"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"367763a3_646ae646","side":"PARENT","line":467,"in_reply_to":"df4a0d28_15b101ce","updated":"2024-02-20 11:59:56.000000000","message":"by the way this logic was added in https://github.com/openstack/nova/commit/43bca185fe2d00bb70d7486fa6c6a0b9eda1fc17\n\nas part of fixign https://bugs.launchpad.net/nova/+bug/1948706\n\nso we have to be careful not to regress that.","commit_id":"6e510eb62e00c34e98a5245a6de2dd2955ffb57a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f8b66304f79cec2729087a35b5c88c05ae62a9a3","unresolved":true,"context_lines":[{"line_number":356,"context_line":"                LOG.info(\"Successfully transferred using %s\", o.scheme)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"                if not verifier:"},{"line_number":359,"context_line":"                    return True"},{"line_number":360,"context_line":""},{"line_number":361,"context_line":"                # Load chunks from the downloaded image file"},{"line_number":362,"context_line":"                # for verification"}],"source_content_type":"text/x-python","patch_set":7,"id":"38095105_05d454b1","line":359,"updated":"2024-04-24 14:34:55.000000000","message":"This new condition/code you\u0027re adding is never hit in the unit tests, that I can see, so please add some coverage.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"21e44e5b05c1d55a87795cc262e435914df30809","unresolved":false,"context_lines":[{"line_number":356,"context_line":"                LOG.info(\"Successfully transferred using %s\", o.scheme)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"                if not verifier:"},{"line_number":359,"context_line":"                    return True"},{"line_number":360,"context_line":""},{"line_number":361,"context_line":"                # Load chunks from the downloaded image file"},{"line_number":362,"context_line":"                # for verification"}],"source_content_type":"text/x-python","patch_set":7,"id":"ff46ee2e_0ccc680e","line":359,"in_reply_to":"38095105_05d454b1","updated":"2024-04-25 12:39:49.000000000","message":"Done","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f89c5c675a6dac3c94fb7dc4720a2007b1e03def","unresolved":true,"context_lines":[{"line_number":377,"context_line":"        \"\"\"Calls out to Glance for data and writes data.\"\"\""},{"line_number":378,"context_line":"        # First, try to get the verifier, so we do not even start to download"},{"line_number":379,"context_line":"        # the image and then fail on the metadata"},{"line_number":380,"context_line":"        verifier \u003d self._get_verifier(context, image_id, trusted_certs)"},{"line_number":381,"context_line":""},{"line_number":382,"context_line":"        # Second, try to delegate image download to a special handler"},{"line_number":383,"context_line":"        if (self._download_handlers and dst_path is not None):"}],"source_content_type":"text/x-python","patch_set":7,"id":"889b59d3_90089d45","line":380,"updated":"2024-04-12 10:21:26.000000000","message":"image verification is optional so this may or may not be desirable.\nwe only verify the image when we have asigniture to verify against or certs that the image is signed with.\n\nthe old behaivor would have downloaded the image and skipped verification fi the metadta was not avaiable.\nwe shoudl be maintinaing that behavior.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"21e44e5b05c1d55a87795cc262e435914df30809","unresolved":false,"context_lines":[{"line_number":377,"context_line":"        \"\"\"Calls out to Glance for data and writes data.\"\"\""},{"line_number":378,"context_line":"        # First, try to get the verifier, so we do not even start to download"},{"line_number":379,"context_line":"        # the image and then fail on the metadata"},{"line_number":380,"context_line":"        verifier \u003d self._get_verifier(context, image_id, trusted_certs)"},{"line_number":381,"context_line":""},{"line_number":382,"context_line":"        # Second, try to delegate image download to a special handler"},{"line_number":383,"context_line":"        if (self._download_handlers and dst_path is not None):"}],"source_content_type":"text/x-python","patch_set":7,"id":"09d3dd6a_6be76d99","line":380,"in_reply_to":"889b59d3_90089d45","updated":"2024-04-25 12:39:49.000000000","message":"Acknowledged","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f8b66304f79cec2729087a35b5c88c05ae62a9a3","unresolved":true,"context_lines":[{"line_number":442,"context_line":"                    # If we return the iterator, then we should not close it"},{"line_number":443,"context_line":"                    may_close_iterator \u003d False"},{"line_number":444,"context_line":"                    return image_chunks"},{"line_number":445,"context_line":"                else:"},{"line_number":446,"context_line":"                    return"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"            for chunk in image_chunks:"}],"source_content_type":"text/x-python","patch_set":7,"id":"c759f576_6ddb4432","line":445,"updated":"2024-04-24 14:34:55.000000000","message":"I don\u0027t see anywhere in the tests that we follow this branch. I assume that means that we\u0027re not ever calling without a verifier *and* expecting to write the file, but that seems odd to me. I know it\u0027s not code you\u0027re adding here, so you don\u0027t need to add a test, I just noticed it while trying to reason about this change.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"21e44e5b05c1d55a87795cc262e435914df30809","unresolved":true,"context_lines":[{"line_number":442,"context_line":"                    # If we return the iterator, then we should not close it"},{"line_number":443,"context_line":"                    may_close_iterator \u003d False"},{"line_number":444,"context_line":"                    return image_chunks"},{"line_number":445,"context_line":"                else:"},{"line_number":446,"context_line":"                    return"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"            for chunk in image_chunks:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ffb94a6f_688f20b4","line":445,"in_reply_to":"c759f576_6ddb4432","updated":"2024-04-25 12:39:49.000000000","message":"The code is called with a verifier and we are writing to a file. That is probably the main use-case. That is handled via:\nhttps://review.opendev.org/c/openstack/nova/+/909474/7/nova/image/glance.py#b421\nand\nhttps://review.opendev.org/c/openstack/nova/+/909474/7/nova/image/glance.py#b437\n\nThe confusion comes probably from the superfluous if-else you are looking at:\n\n`not write_image` is only true when `data is None`\n\nWe can condense it to (after removing the close and may_close_iterator in finally)\n```\nif verifier is None and not write_image:\n  return image_chunks\n```","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"46ad2c1fb12d049f52d468e114d547bba0a20df1","unresolved":false,"context_lines":[{"line_number":442,"context_line":"                    # If we return the iterator, then we should not close it"},{"line_number":443,"context_line":"                    may_close_iterator \u003d False"},{"line_number":444,"context_line":"                    return image_chunks"},{"line_number":445,"context_line":"                else:"},{"line_number":446,"context_line":"                    return"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"            for chunk in image_chunks:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3e5497e9_7603f809","line":445,"in_reply_to":"ffb94a6f_688f20b4","updated":"2024-04-30 16:02:56.000000000","message":"Done","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3551115c31738dc75966696a3c4d5412d602a3dd","unresolved":true,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f73648d_9ad94cfe","line":482,"updated":"2024-04-23 17:01:10.000000000","message":"This implies that anyone that plans to use this as a generator must call the `close()` on their own right? That feels like a leak waiting to happen. Why don\u0027t we just iterate and yield the chunks ourselves so we can be sure it\u0027s closed when we\u0027re done? Basically change L444 above to:\n```\nfor chunk in image_chunks:\n    yield chunk\n```\nThen we can leave this `finally` block the same, always close, and avoid this `may_close_iterator` hack. Right?","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"21e44e5b05c1d55a87795cc262e435914df30809","unresolved":true,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"8a4ad52e_2f7c272c","line":482,"in_reply_to":"4a71efcd_e353783d","updated":"2024-04-25 12:39:49.000000000","message":"\u003e However, I think that if we explicitly do the chunk proxying we could always close it by the time we finally exit here, right? \n\nNot *always*. It would not be closed in the same case the `glance_utils.IterableWithLength`, because you propose do exactly the same. If no one actually iterates over the returned generator.\nThe way we leak the IterableWithLength file descriptor is like this:\n```\ndef glance():\n   try:\n     for i in range(1):\n       yield i\n   finally:\n      print(\"glance closed\")\n\n\nglance()\n```\nWe never reach the finally.\n\nThis works:\n```\nfor i in glance():\n   raise Exception()\n```\n\nMy understanding from your description is that we do the following:\n```\ndef nova_wrapper(chunk_iterator):\n  try:\n    for chunk in chunk_iterator:\n      yield chunk\n  finally:\n    print(\"nova closed\")\n\n\nnova_wrapper(glance())\n``` \n\nAnd in both cases, we either reach the finally-clause, because we started to iterate, or we don\u0027t.\n\n\n\n\u003e  If you want to disavow responsibility for cleaning that up in this fix and refactor later, then we can.  However it seems quite relevant to the general task of making this method less fragile on error and with fewer bits of undesirable residue left over from an error scenario.\n\nI certainly agree, it needs fixing, and I don\u0027t shy away to make a proposal for that.\nI just feel, that will be whole another discussion.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"46ad2c1fb12d049f52d468e114d547bba0a20df1","unresolved":false,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"a7dd5806_d81050c6","line":482,"in_reply_to":"4b466cc3_54628209","updated":"2024-04-30 16:02:56.000000000","message":"Ack","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"e3d0f6e7eacffd275dd4e984ba333cbb3bfb7626","unresolved":true,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"d294f1ca_c888f4be","line":482,"in_reply_to":"5f73648d_9ad94cfe","updated":"2024-04-24 08:28:15.000000000","message":"\u003e This implies that anyone that plans to use this as a generator must call the close() on their own right? \n\nNot entirely true. They need to call `close()`, if they never iterate over returned generator object.\nThat is the API of the underlying glanceclient, and that will be the case either way, if we return a new generator object here, or return it directly.\n\n\u003e That feels like a leak waiting to happen.\n\nYes, certainly. The implicit `close()` of the iterator has shown itself to be a fragile solution.\nAnd returning it means, any caller would face the same risk.\n\n\n\u003e Why don\u0027t we just iterate and yield the chunks ourselves so we can be sure it\u0027s closed when we\u0027re done? \n\nBecause you are doing then exactly the same that the underlying `glance_utils.IterableWithLength` does, and that didn\u0027t solve the problem.\n\nThe problem of the bug was creating a generator object to iterate, and then calling `_get_verifier` throwing an exception before even anyone started to iterate over the data.\n\nAnd yes, the risk of running into the same problem still exists for the caller. But I\u0027d argue, that is out of scope for a bug fix. It would require some refactoring.\n\nRegarding the `may_close_iterator`: I\u0027d say it already could be removed, as the exception causing that bug has already been moved out of the function. My motivation was to keep the change as limited as possible, though. I am happy to drop it though, if that makes a difference.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"781129fa2d56ae779deb3e4ae15e8bbdd0a52f6f","unresolved":true,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"4b466cc3_54628209","line":482,"in_reply_to":"8a4ad52e_2f7c272c","updated":"2024-04-25 13:44:56.000000000","message":"Hmm, okay that\u0027s not how I thought it worked, but maybe you\u0027re right. I\u0027ll play around with it a bit to convince myself.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f8b66304f79cec2729087a35b5c88c05ae62a9a3","unresolved":true,"context_lines":[{"line_number":479,"context_line":"                self._safe_fsync(data)"},{"line_number":480,"context_line":"                data.close()"},{"line_number":481,"context_line":"            if (isinstance(image_chunks, glance_utils.IterableWithLength) and"},{"line_number":482,"context_line":"                    may_close_iterator):"},{"line_number":483,"context_line":"                image_chunks.iterable.close()"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        if data is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"4a71efcd_e353783d","line":482,"in_reply_to":"d294f1ca_c888f4be","updated":"2024-04-24 14:34:55.000000000","message":"\u003e \u003e This implies that anyone that plans to use this as a generator must call the close() on their own right? \n\u003e \n\u003e Not entirely true. They need to call `close()`, if they never iterate over returned generator object.\n\u003e That is the API of the underlying glanceclient, and that will be the case either way, if we return a new generator object here, or return it directly.\n\nOkay but if we always *start* the iteration here and just proxy each chunk out then we would close that loophole as well, yeah?\n\n\u003e Yes, certainly. The implicit `close()` of the iterator has shown itself to be a fragile solution.\n\u003e And returning it means, any caller would face the same risk.\n\n...Unless we help here by doing the above I think.\n\n\u003e Because you are doing then exactly the same that the underlying `glance_utils.IterableWithLength` does, and that didn\u0027t solve the problem.\n\u003e \n\u003e The problem of the bug was creating a generator object to iterate, and then calling `_get_verifier` throwing an exception before even anyone started to iterate over the data.\n\nYeah, I understand the ordering part of the fix. I\u0027m addressing the additional close-or-not-close part you\u0027re adding here, which increases the complexity.\n\n\u003e And yes, the risk of running into the same problem still exists for the caller. But I\u0027d argue, that is out of scope for a bug fix. It would require some refactoring.\n\u003e \n\u003e Regarding the `may_close_iterator`: I\u0027d say it already could be removed, as the exception causing that bug has already been moved out of the function. My motivation was to keep the change as limited as possible, though. I am happy to drop it though, if that makes a difference.\n\nThe `may_close` flag adds some complexity to an already ridiculously-complex method, so if we can do without it, I\u0027d say that\u0027d be good. However, I think that if we explicitly do the chunk proxying we could *always* close it by the time we `finally` exit here, right? That would both reduce the complexity of this method *and* reduce the potential for a leak by the caller if it fails to call close when it didn\u0027t ever start iterating.\n\nI like the implicit close behavior, I just want it to not be leaky and this method right here seems like the place to close that gap for everything above. If you want to disavow responsibility for cleaning that up in this fix and refactor later, then we can. However it seems quite relevant to the general task of making this method less fragile on error and with fewer bits of undesirable residue left over from an error scenario.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8069627c65b0d4ecf4a588fd85e347771323ec66","unresolved":true,"context_lines":[{"line_number":519,"context_line":"                    img_signature\u003dimg_signature,"},{"line_number":520,"context_line":"                    img_signature_key_type\u003dimg_sig_key_type,"},{"line_number":521,"context_line":"                )"},{"line_number":522,"context_line":"            except cursive_exception.SignatureVerificationError:"},{"line_number":523,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":524,"context_line":"                    LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":525,"context_line":"                              \u0027for image: %s\u0027, image_id)"},{"line_number":526,"context_line":"            # Validate image signature certificate if trusted certificates"},{"line_number":527,"context_line":"            # were provided"},{"line_number":528,"context_line":"            # NOTE(jackie-truong): Certificate validation will occur if"},{"line_number":529,"context_line":"            # trusted_certs are provided, even if the certificate validation"},{"line_number":530,"context_line":"            # feature is disabled. This is to provide safety for the user."},{"line_number":531,"context_line":"            # We may want to consider making this a \"soft\" check in the future."},{"line_number":532,"context_line":"            if trusted_certs:"},{"line_number":533,"context_line":"                _verify_certs(context, img_sig_cert_uuid, trusted_certs)"},{"line_number":534,"context_line":"            elif CONF.glance.enable_certificate_validation:"},{"line_number":535,"context_line":"                msg \u003d (\u0027Image signature certificate validation enabled, \u0027"},{"line_number":536,"context_line":"                       \u0027but no trusted certificate IDs were provided. \u0027"},{"line_number":537,"context_line":"                       \u0027Unable to validate the certificate used to \u0027"},{"line_number":538,"context_line":"                       \u0027verify the image signature.\u0027)"},{"line_number":539,"context_line":"                LOG.warning(msg)"},{"line_number":540,"context_line":"                raise exception.CertificateValidationFailed(msg)"},{"line_number":541,"context_line":"            else:"},{"line_number":542,"context_line":"                LOG.debug(\u0027Certificate validation was not performed. A list \u0027"},{"line_number":543,"context_line":"                          \u0027of trusted image certificate IDs must be provided \u0027"},{"line_number":544,"context_line":"                          \u0027in order to validate an image certificate.\u0027)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        return verifier"},{"line_number":547,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dfcc6e31_5f7290a4","line":544,"range":{"start_line":522,"start_character":0,"end_line":544,"end_character":71},"updated":"2024-04-12 10:30:14.000000000","message":"i guess if any of these excptiones would have fired before they would still fire in the new version so perhapsp moving this up is ok after all.\n\nthe resaons im hesitating to say that this move is fine is we appear to have no existing test coverage for the orderign of when we initalise the verifyer\n\nso im not sure if we depend on the current behavior anywhere.\n\ni think you are correct and it is safe to make this change/optimization.","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"21e44e5b05c1d55a87795cc262e435914df30809","unresolved":false,"context_lines":[{"line_number":519,"context_line":"                    img_signature\u003dimg_signature,"},{"line_number":520,"context_line":"                    img_signature_key_type\u003dimg_sig_key_type,"},{"line_number":521,"context_line":"                )"},{"line_number":522,"context_line":"            except cursive_exception.SignatureVerificationError:"},{"line_number":523,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":524,"context_line":"                    LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":525,"context_line":"                              \u0027for image: %s\u0027, image_id)"},{"line_number":526,"context_line":"            # Validate image signature certificate if trusted certificates"},{"line_number":527,"context_line":"            # were provided"},{"line_number":528,"context_line":"            # NOTE(jackie-truong): Certificate validation will occur if"},{"line_number":529,"context_line":"            # trusted_certs are provided, even if the certificate validation"},{"line_number":530,"context_line":"            # feature is disabled. This is to provide safety for the user."},{"line_number":531,"context_line":"            # We may want to consider making this a \"soft\" check in the future."},{"line_number":532,"context_line":"            if trusted_certs:"},{"line_number":533,"context_line":"                _verify_certs(context, img_sig_cert_uuid, trusted_certs)"},{"line_number":534,"context_line":"            elif CONF.glance.enable_certificate_validation:"},{"line_number":535,"context_line":"                msg \u003d (\u0027Image signature certificate validation enabled, \u0027"},{"line_number":536,"context_line":"                       \u0027but no trusted certificate IDs were provided. \u0027"},{"line_number":537,"context_line":"                       \u0027Unable to validate the certificate used to \u0027"},{"line_number":538,"context_line":"                       \u0027verify the image signature.\u0027)"},{"line_number":539,"context_line":"                LOG.warning(msg)"},{"line_number":540,"context_line":"                raise exception.CertificateValidationFailed(msg)"},{"line_number":541,"context_line":"            else:"},{"line_number":542,"context_line":"                LOG.debug(\u0027Certificate validation was not performed. A list \u0027"},{"line_number":543,"context_line":"                          \u0027of trusted image certificate IDs must be provided \u0027"},{"line_number":544,"context_line":"                          \u0027in order to validate an image certificate.\u0027)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        return verifier"},{"line_number":547,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ab4d6938_17625cd6","line":544,"range":{"start_line":522,"start_character":0,"end_line":544,"end_character":71},"in_reply_to":"dfcc6e31_5f7290a4","updated":"2024-04-25 12:39:49.000000000","message":"Acknowledged","commit_id":"94cbae021299276a9e4bb51cf42ca1cdfaad394b"}],"nova/tests/unit/image/test_glance.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6ada906d4ab962f49fc0b1587a2d94715e3d1c9d","unresolved":true,"context_lines":[{"line_number":775,"context_line":"            open_mock.assert_called_once_with(mock.sentinel.dst_path, \u0027rb\u0027)"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":"            # verifier called with the value we got from rbd download"},{"line_number":778,"context_line":"            verifier.update.assert_has_calls("},{"line_number":779,"context_line":"                    ["},{"line_number":780,"context_line":"                        mock.call(\"rbd1\"),"},{"line_number":781,"context_line":"                        mock.call(\"rbd2\")"}],"source_content_type":"text/x-python","patch_set":8,"id":"db85d71f_5476d48e","line":778,"updated":"2024-05-01 10:04:07.000000000","message":"this is testing https://review.opendev.org/c/openstack/nova/+/909474/8/nova/image/glance.py#363","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6ada906d4ab962f49fc0b1587a2d94715e3d1c9d","unresolved":true,"context_lines":[{"line_number":800,"context_line":"        self._test_download_direct_rbd_uri_v2()"},{"line_number":801,"context_line":""},{"line_number":802,"context_line":"    def test_download_direct_rbd_uri_without_verifier_v2(self):"},{"line_number":803,"context_line":"        self._test_download_direct_rbd_uri_v2(with_verifier\u003dFalse)"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":"    @mock.patch(\u0027nova.image.glance.GlanceImageServiceV2._get_transfer_method\u0027)"},{"line_number":806,"context_line":"    @mock.patch(\u0027nova.image.glance.GlanceImageServiceV2.show\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"6b403b74_ceb74b78","line":803,"updated":"2024-05-01 10:04:07.000000000","message":"this covers this if branch https://review.opendev.org/c/openstack/nova/+/909474/8/nova/image/glance.py#358","commit_id":"198805c7c516e510f44890c4dec26a5fb8210cff"}]}