)]}'
{"cinder/volume/drivers/rbd.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"012ef831d0b5f0b492aa9c150201d95d8060d0ea","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            LOG.error(_LE(\"Error connecting to ceph cluster: %s\"), exc)"},{"line_number":319,"context_line":"            # shutdown cannot raise an exception"},{"line_number":320,"context_line":"            client.shutdown()"},{"line_number":321,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dstr(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a80dd14_1f78778e","line":318,"updated":"2015-03-12 08:59:13.000000000","message":"According to oslo\u0027s i18n documentation this should go like this:\n  msg \u003d _(\u0027Error connecting to ceph cluster\u0027)\n  LOG.exception(msg)\n  client.shutdown()\n  raise exception.VolumeBackendAPIException(msg)\nYou don\u0027t need to pass str(exc) when raising the exception, as the exception that raises already has that information.","commit_id":"cb065fda768000052053166434130d0ae69b3190"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"344e424b05f8c09933ff3311d90f03de92a7a145","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            LOG.error(_LE(\"Error connecting to ceph cluster: %s\"), exc)"},{"line_number":319,"context_line":"            # shutdown cannot raise an exception"},{"line_number":320,"context_line":"            client.shutdown()"},{"line_number":321,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dstr(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a80dd14_0457a727","line":318,"updated":"2015-03-10 17:41:56.000000000","message":"Wouldn\u0027t it be better to use LOG.exception method here?","commit_id":"cb065fda768000052053166434130d0ae69b3190"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"3ef1640ebb3ba0c635e0556e489a6ccf1d5e44eb","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            LOG.error(_LE(\"Error connecting to ceph cluster: %s\"), exc)"},{"line_number":319,"context_line":"            # shutdown cannot raise an exception"},{"line_number":320,"context_line":"            client.shutdown()"},{"line_number":321,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dstr(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a80dd14_bd586afb","line":318,"in_reply_to":"9a80dd14_020cb741","updated":"2015-03-11 21:26:33.000000000","message":"Does seem to be a little redundant then. But I suppose extra logging is better than not enough logging.","commit_id":"cb065fda768000052053166434130d0ae69b3190"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"d5e03d8b24957fd3b794af1242e8e444571a3c23","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            LOG.error(_LE(\"Error connecting to ceph cluster: %s\"), exc)"},{"line_number":319,"context_line":"            # shutdown cannot raise an exception"},{"line_number":320,"context_line":"            client.shutdown()"},{"line_number":321,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dstr(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a80dd14_020cb741","line":318,"in_reply_to":"9a80dd14_0457a727","updated":"2015-03-11 21:19:55.000000000","message":"They do, on the line 321.","commit_id":"cb065fda768000052053166434130d0ae69b3190"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"3ef1640ebb3ba0c635e0556e489a6ccf1d5e44eb","unresolved":false,"context_lines":[{"line_number":318,"context_line":"            LOG.error(_LE(\"Error connecting to ceph cluster: %s\"), exc)"},{"line_number":319,"context_line":"            # shutdown cannot raise an exception"},{"line_number":320,"context_line":"            client.shutdown()"},{"line_number":321,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dstr(exc))"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":"    def _disconnect_from_rados(self, client, ioctx):"},{"line_number":324,"context_line":"        # closing an ioctx cannot raise an exception"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a80dd14_7dba72e2","line":321,"updated":"2015-03-11 21:26:33.000000000","message":"Not part of this patchset, but calling str() on an exception is generally not good.\n\nNot something I would normally call out, but if you feel inclined and you end up doing another update to this patchset it might be good to change this to be:\n\ndata\u003dsix.text_type(exc)","commit_id":"cb065fda768000052053166434130d0ae69b3190"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"030a0a4315b38bb81e366e9ff5ad1ec06777aa9e","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            msg \u003d (_(\"Error connecting to ceph cluster: %s\") % exc)"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a80dd14_601b8a1c","line":318,"updated":"2015-03-12 14:11:16.000000000","message":"There should be no need for exc in the string since LOG.exception should print traceback with that info and raised exception already has the traceback.","commit_id":"bc16c6e8e409c42e66f68ac04a7a4ef372905709"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"66a9d75fcde385f65a91438069d4f7185a6415b5","unresolved":false,"context_lines":[{"line_number":315,"context_line":"            ioctx \u003d client.open_ioctx(pool)"},{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error as exc:"},{"line_number":318,"context_line":"            msg \u003d (_(\"Error connecting to ceph cluster: %s\") % exc)"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a80dd14_03581831","line":318,"in_reply_to":"9a80dd14_601b8a1c","updated":"2015-03-12 14:24:11.000000000","message":"I agree with Gorka completely. This ends up being redundant.\n\nThere is also an issue with formatting the exception this way. If the exception contains Unicode characters the str conversion on it will cause an error.\n\nI would recommend the following:\n\nclient.shutdown()\nLOG.exception(_LE(\u0027Error connecting to ceph cluster.\u0027))\nraise exception.VolumeBackendAPIException(data\u003dsix.text_type(exc))","commit_id":"bc16c6e8e409c42e66f68ac04a7a4ef372905709"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"65d62e2e094103340e7f3e7aacef775772851eae","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error:"},{"line_number":318,"context_line":"            msg \u003d _(\"Error connecting to ceph cluster.\")"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"},{"line_number":322,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a80dd14_fa239aa4","line":319,"updated":"2015-03-12 17:03:24.000000000","message":"I think we lost in between logging guidelines[1] and what should we do in scope of this bug.\n\nIf I didn\u0027t miss anything in oslo.log and python log module sources, this patch  won\u0027t fix the issue. Rados exception message won\u0027t be logged.\n\nTo fix the issue we need something like this:\nexcept self.rados.Error as exc:\n    msg \u003d _(\"Error connecting to ceph cluster.\")\n    LOG.error(msg)\n    LOG.exception(_LE(\"Exception occurs: %s.\"), exc)\n    # shutdown cannot raise an exception\n    client.shutdown()    \n    raise exception.VolumeBackendAPIException(data\u003dmsg)\n\n    \nUnfortunately, I have not got environment to test it now, I\u0027ll post update after I\u0027ll be available to test it.\n\n[1] https://review.openstack.org/#/c/147312/5/doc/source/usage.rst","commit_id":"5e580c7af4ddede8be3e24be10754b70728e18c8"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"677e09c0708115a0812c78cd8236377a5e0e5e78","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error:"},{"line_number":318,"context_line":"            msg \u003d _(\"Error connecting to ceph cluster.\")"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"},{"line_number":322,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a80dd14_ebb3cee6","line":319,"updated":"2015-03-12 18:18:35.000000000","message":"Ivan, I don\u0027t understand what you mean about troubleshooting and patch breaking something.\nUnless there\u0027s something weird going on in Cinder\u0027s logging (I\u0027m new to Cinder so I can\u0027t be sure) when you do LOG.exception as is being done here you get everything, the message passed to exception, the traceback from the exception and the message that was used to created the exception.\nSo I believe the patch is Ok as it is and no more information is added with your proposed change.","commit_id":"5e580c7af4ddede8be3e24be10754b70728e18c8"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"0093e75bb6834e5687d2d74d4cbe50d6c7e92b89","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error:"},{"line_number":318,"context_line":"            msg \u003d _(\"Error connecting to ceph cluster.\")"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"},{"line_number":322,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a80dd14_ab0631cc","line":319,"in_reply_to":"9a80dd14_1a730981","updated":"2015-03-12 20:00:54.000000000","message":"Gorka, Sean, \nI\u0027m sorry, you\u0027re both right.","commit_id":"5e580c7af4ddede8be3e24be10754b70728e18c8"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"e2596b17b338f6a2992ffbad741ad4abf75cb4fd","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error:"},{"line_number":318,"context_line":"            msg \u003d _(\"Error connecting to ceph cluster.\")"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"},{"line_number":322,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a80dd14_1a730981","line":319,"in_reply_to":"9a80dd14_ebb3cee6","updated":"2015-03-12 18:52:54.000000000","message":"You are correct Gorka. With the code as you have it right now the logging will be correct. LOG.exception will take care of logging the exception, no need to explicitly pass it in to the log call. And with the exception message just being msg instead of the original str(exc) it is safe.\n\nNo need to change this in my opinion.","commit_id":"5e580c7af4ddede8be3e24be10754b70728e18c8"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"c7384ca1f12f0e71fd587aed725e96cf8559d113","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            return client, ioctx"},{"line_number":317,"context_line":"        except self.rados.Error:"},{"line_number":318,"context_line":"            msg \u003d _(\"Error connecting to ceph cluster.\")"},{"line_number":319,"context_line":"            LOG.exception(msg)"},{"line_number":320,"context_line":"            # shutdown cannot raise an exception"},{"line_number":321,"context_line":"            client.shutdown()"},{"line_number":322,"context_line":"            raise exception.VolumeBackendAPIException(data\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a80dd14_15c4c190","line":319,"in_reply_to":"9a80dd14_fa239aa4","updated":"2015-03-12 17:30:51.000000000","message":"Just tested with a fresh devstack install.\n\nIn the current code we raise exception.VolumeBackendAPIException(data\u003dstr(exc))\n\nThis exception is logger in the cinder-volume and error message from Ceph occurs in log. So the bug is not reproducible as described. Current patch breaks it.\n\nMy proposal above it to add full traceback to logs to make troubleshooting easier. It follows logging guidelines [1]\n\n[1]http://docs.openstack.org/developer/oslo.i18n/guidelines.html#examples","commit_id":"5e580c7af4ddede8be3e24be10754b70728e18c8"}]}