)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"402787467b14804f7c9470dee37aeac80f13d83f","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Boxiang Zhu \u003czhu.boxiang@99cloud.net\u003e"},{"line_number":5,"context_line":"CommitDate: 2020-03-04 09:54:01 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fix bug of cloning volume"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Remove the \u0027src_volume.close()\u0027. If closing the src_volume, the src_volume"},{"line_number":10,"context_line":"will fail to call the unprotect_snap function when the depth is larger than"},{"line_number":11,"context_line":"rbd_max_clone_depth."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: Ib713aa91b775d8ec07ffdb24dfe1db1b6ecf2921"},{"line_number":14,"context_line":"Closes-Bug: #1794956"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1fa4df85_45b18a46","line":11,"range":{"start_line":7,"start_character":0,"end_line":11,"end_character":20},"updated":"2020-03-09 16:00:32.000000000","message":"Please be more precise in describing what\u0027s going on here to help out people looking at the git history later.  I suggest something like\n\nRBD: workaround for segfault during volume clone\n\nfor the title.  It doesn\u0027t seem like the rest of the commit message is describing what\u0027s going on in the patch, though, because if I\u0027m counting correctly, your patch adds 1 more close() than we had previously.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"7a24cff9956eda929354eec40be896d75c71b954","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch fixes a bug in volume cloning where an exception during"},{"line_number":10,"context_line":"flattening would attempt to unprotect and remove the source volume"},{"line_number":11,"context_line":"snapshot after the source volume had already been closed.  This logic"},{"line_number":12,"context_line":"results in a segmentation fault which causes cinder to restart."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Co-authored-by: Jon Bernard \u003cjobernar@redhat.com\u003e"},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"1fa4df85_72b68f8d","line":12,"updated":"2020-03-12 14:03:58.000000000","message":"Maybe:\nThis patch fixes a bug in volume cloning where the source volume was being prematurely closed.  If the destination volume required flattening, and a exception occurred during flattening, the code would attempt to perform cleanup operations on an already closed volume.  This resulted\nin a segmentation fault which causes cinder to restart.","commit_id":"6f37fce64428002bd8ec4de96258fe391c460cef"}],"cinder/tests/unit/volume/drivers/test_rbd.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"402787467b14804f7c9470dee37aeac80f13d83f","unresolved":false,"context_lines":[{"line_number":1065,"context_line":"                self.assertEqual("},{"line_number":1066,"context_line":"                    1, self.mock_rbd.RBD.return_value.clone.call_count)"},{"line_number":1067,"context_line":"                self.assertEqual("},{"line_number":1068,"context_line":"                    1, self.mock_rbd.Image.return_value.close.call_count)"},{"line_number":1069,"context_line":"                self.assertTrue(mock_get_clone_depth.called)"},{"line_number":1070,"context_line":"                mock_resize.assert_not_called()"},{"line_number":1071,"context_line":"                mock_enable_repl.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_8239b8f2","line":1068,"updated":"2020-03-09 16:00:32.000000000","message":"I think you need to add a reference to the LP bug here saying why this *must* be 1","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"3373afc00b58e285e66bfdea40ef2e492861f755","unresolved":false,"context_lines":[{"line_number":1065,"context_line":"                self.assertEqual("},{"line_number":1066,"context_line":"                    1, self.mock_rbd.RBD.return_value.clone.call_count)"},{"line_number":1067,"context_line":"                self.assertEqual("},{"line_number":1068,"context_line":"                    1, self.mock_rbd.Image.return_value.close.call_count)"},{"line_number":1069,"context_line":"                self.assertTrue(mock_get_clone_depth.called)"},{"line_number":1070,"context_line":"                mock_resize.assert_not_called()"},{"line_number":1071,"context_line":"                mock_enable_repl.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_8961ab14","line":1068,"in_reply_to":"1fa4df85_8239b8f2","updated":"2020-03-12 01:43:52.000000000","message":"Update coming shortly, thanks for mentioning this.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"402787467b14804f7c9470dee37aeac80f13d83f","unresolved":false,"context_lines":[{"line_number":1188,"context_line":""},{"line_number":1189,"context_line":"                # We expect the driver to close both volumes, so 2 is expected"},{"line_number":1190,"context_line":"                self.assertEqual("},{"line_number":1191,"context_line":"                    2, self.mock_rbd.Image.return_value.close.call_count)"},{"line_number":1192,"context_line":"                self.assertTrue(mock_get_clone_depth.called)"},{"line_number":1193,"context_line":"                mock_enable_repl.assert_not_called()"},{"line_number":1194,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_e21f4c69","line":1191,"updated":"2020-03-09 16:00:32.000000000","message":"If the problem is that we\u0027re calling close() too many times, we should probably check to make sure that the 2x here are on different volumes.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"3373afc00b58e285e66bfdea40ef2e492861f755","unresolved":false,"context_lines":[{"line_number":1188,"context_line":""},{"line_number":1189,"context_line":"                # We expect the driver to close both volumes, so 2 is expected"},{"line_number":1190,"context_line":"                self.assertEqual("},{"line_number":1191,"context_line":"                    2, self.mock_rbd.Image.return_value.close.call_count)"},{"line_number":1192,"context_line":"                self.assertTrue(mock_get_clone_depth.called)"},{"line_number":1193,"context_line":"                mock_enable_repl.assert_not_called()"},{"line_number":1194,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_e97b7fbb","line":1191,"in_reply_to":"1fa4df85_e21f4c69","updated":"2020-03-12 01:43:52.000000000","message":"No, we calling close twice, once for the source volume (for clone()) and one for the destination volume - which must be opened in order to call flatten() - this is where the second close() is coming from.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"}],"cinder/volume/drivers/rbd.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"402787467b14804f7c9470dee37aeac80f13d83f","unresolved":false,"context_lines":[{"line_number":706,"context_line":"                        \u0027dest\u0027: dest_name,"},{"line_number":707,"context_line":"                        \u0027error\u0027: e})"},{"line_number":708,"context_line":"                LOG.exception(msg)"},{"line_number":709,"context_line":"                src_volume.close()"},{"line_number":710,"context_line":"                raise exception.VolumeBackendAPIException(data\u003dmsg)"},{"line_number":711,"context_line":""},{"line_number":712,"context_line":"            depth \u003d self._get_clone_depth(client, src_name)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_e268acf5","line":709,"updated":"2020-03-09 16:00:32.000000000","message":"I think you need a comment explaining why it makes sense to do this here rather than in the \u0027finally\u0027 ... otherwise someone is going to eventually refactor this and move the close() back to a \u0027finally\u0027 clause as a \"safer\" way to do this.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"3373afc00b58e285e66bfdea40ef2e492861f755","unresolved":false,"context_lines":[{"line_number":706,"context_line":"                        \u0027dest\u0027: dest_name,"},{"line_number":707,"context_line":"                        \u0027error\u0027: e})"},{"line_number":708,"context_line":"                LOG.exception(msg)"},{"line_number":709,"context_line":"                src_volume.close()"},{"line_number":710,"context_line":"                raise exception.VolumeBackendAPIException(data\u003dmsg)"},{"line_number":711,"context_line":""},{"line_number":712,"context_line":"            depth \u003d self._get_clone_depth(client, src_name)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_49e8937b","line":709,"in_reply_to":"1fa4df85_e268acf5","updated":"2020-03-12 01:43:52.000000000","message":"It makes sense here because the source volume is closed further down for the non-exception case.  We only want to close here before the raise because we’re about to return to the caller (and opened volumes cannot be left in that state, the must be closed).","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"5acc879d8cd5f4d848cd716b8d70709f7da28cb1","unresolved":false,"context_lines":[{"line_number":754,"context_line":"                src_volume.unprotect_snap(clone_snap)"},{"line_number":755,"context_line":"                src_volume.remove_snap(clone_snap)"},{"line_number":756,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"},{"line_number":757,"context_line":"                src_volume.close()"},{"line_number":758,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":759,"context_line":"                                                 volume_id\u003dvolume.id)"},{"line_number":760,"context_line":"            finally:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_5b016c05","line":757,"updated":"2020-03-09 14:02:13.000000000","message":"This is already done at line 761 in \"finally\", why duplicate it here?","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"3373afc00b58e285e66bfdea40ef2e492861f755","unresolved":false,"context_lines":[{"line_number":754,"context_line":"                src_volume.unprotect_snap(clone_snap)"},{"line_number":755,"context_line":"                src_volume.remove_snap(clone_snap)"},{"line_number":756,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"},{"line_number":757,"context_line":"                src_volume.close()"},{"line_number":758,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":759,"context_line":"                                                 volume_id\u003dvolume.id)"},{"line_number":760,"context_line":"            finally:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_69ed4f8a","line":757,"in_reply_to":"1fa4df85_5b016c05","updated":"2020-03-12 01:43:52.000000000","message":"I’m working on an update that removed the finally, it should look better there.","commit_id":"fd9da25f008cdea9926815bd98aad984dffe9e3a"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"1e54d1bd9ebfee786afd4bbee22a26637f81fd56","unresolved":false,"context_lines":[{"line_number":761,"context_line":"            # If we make it here, no exceptions were raised and the"},{"line_number":762,"context_line":"            # source volume remains open.  Close it now. Failure to do"},{"line_number":763,"context_line":"            # so may result in a segmentation fault in librbd."},{"line_number":764,"context_line":"            src_volume.close()"},{"line_number":765,"context_line":""},{"line_number":766,"context_line":"            self._extend_if_required(volume, src_vref)"},{"line_number":767,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1fa4df85_67932e5e","line":764,"updated":"2020-03-12 16:36:05.000000000","message":"If an error occurs on src_volume.remove_snap() or so above, this won\u0027t happen -- I think we need the \"finally\" block.","commit_id":"66750ce0edf583c6807aa968a9731eb4e90063d5"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"35d1ae2be2220d89d2a77e0b3f8a3514a48bf8bf","unresolved":false,"context_lines":[{"line_number":697,"context_line":"                                      client.ioctx, dest_name,"},{"line_number":698,"context_line":"                                      features\u003dclient.features)"},{"line_number":699,"context_line":"            except Exception as e:"},{"line_number":700,"context_line":"                src_volume.unprotect_snap(clone_snap)"},{"line_number":701,"context_line":"                src_volume.remove_snap(clone_snap)"},{"line_number":702,"context_line":"                msg \u003d (_(\"Failed to clone \u0027%(src_vol)s@%(src_snap)s\u0027 to \""},{"line_number":703,"context_line":"                         \"\u0027%(dest)s\u0027, error: %(error)s\") %"},{"line_number":704,"context_line":"                       {\u0027src_vol\u0027: src_name,"}],"source_content_type":"text/x-python","patch_set":5,"id":"1fa4df85_f6826a52","line":701,"range":{"start_line":700,"start_character":0,"end_line":701,"end_character":50},"updated":"2020-03-12 20:40:44.000000000","message":"If either of these raises an exception, we\u0027ll never close the src_volume.  So I think we need a try around these 2 lines, an except that\u0027s just a pass, and a finally that closes src_volume (that may be why there was a finally in the original code, it was just at the wrong level).  Then you just continue the current except block with line 702, remove line 709, and raise at line 110.  (So you\u0027re only going to close src_volume if you are going to raise an exception to exit the function, but you\u0027ll definitely close it.)","commit_id":"48a3daf4d9d013c99ef6cfe7e57ca821c51b7872"}]}