)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14810,"name":"Kan","email":"sofaopenstack@gmail.com","username":"Kan"},"change_message_id":"bc04d6a98a6f3edcdd6d5efff31ab764a70b64e8","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Set node last_error in TaskManager"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If exception raised as result of async worker thread execution but"},{"line_number":10,"context_line":"thread does not set last_error field TaskManager write error message"},{"line_number":11,"context_line":"to last_error."},{"line_number":12,"context_line":"Typical use cases: quick debug, help with out of tree drivers which"},{"line_number":13,"context_line":"do not handle async errors properly."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7a5de9d1_0e07e118","line":11,"range":{"start_line":9,"start_character":0,"end_line":11,"end_character":14},"updated":"2016-01-31 07:37:59.000000000","message":"A little hard to understand at the first glance, how about:\n\u0027Exception might be raised as result of async worker thread execution but thread does not set last_error field. Add a functionality to write error message to last_error in TaskManager for that case.\u0027","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e3d7e92be01828eecb3e2e8ec2a760aacca73166","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Set node last_error in TaskManager"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If exception raised as result of async worker thread execution but"},{"line_number":10,"context_line":"thread does not set last_error field TaskManager write error message"},{"line_number":11,"context_line":"to last_error."},{"line_number":12,"context_line":"Typical use cases: quick debug, help with out of tree drivers which"},{"line_number":13,"context_line":"do not handle async errors properly."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7a5de9d1_b13ec927","line":11,"range":{"start_line":9,"start_character":0,"end_line":11,"end_character":14},"in_reply_to":"7a5de9d1_0e07e118","updated":"2016-02-02 16:07:15.000000000","message":"Done","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"}],"ironic/conductor/task_manager.py":[{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"9d3488a0646f8052fc7b9588afe30e6bc2b51c61","unresolved":false,"context_lines":[{"line_number":334,"context_line":"                except exception.NodeNotFound:"},{"line_number":335,"context_line":"                    pass"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def _thread_release_resources(self, t):"},{"line_number":338,"context_line":"        \"\"\"Thread.link() callback to release resources.\"\"\""},{"line_number":339,"context_line":"        self._write_exception(t)"},{"line_number":340,"context_line":"        self.release_resources()"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a5de9d1_5460ffce","line":337,"updated":"2016-02-01 06:54:07.000000000","message":"nit:I see your tests are testing the function of _write_exception, better to add a test to see the _write_exception was called in _thread_release_resource too.","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e3d7e92be01828eecb3e2e8ec2a760aacca73166","unresolved":false,"context_lines":[{"line_number":334,"context_line":"                except exception.NodeNotFound:"},{"line_number":335,"context_line":"                    pass"},{"line_number":336,"context_line":""},{"line_number":337,"context_line":"    def _thread_release_resources(self, t):"},{"line_number":338,"context_line":"        \"\"\"Thread.link() callback to release resources.\"\"\""},{"line_number":339,"context_line":"        self._write_exception(t)"},{"line_number":340,"context_line":"        self.release_resources()"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a5de9d1_de1cb415","line":337,"in_reply_to":"7a5de9d1_5460ffce","updated":"2016-02-02 16:07:15.000000000","message":"There are already two tests: link is called with _thread_release_resources https://github.com/openstack/ironic/blob/master/ironic/tests/unit/conductor/test_task_manager.py#L342 and linked function is called after thread execution https://github.com/openstack/ironic/blob/master/ironic/tests/unit/conductor/test_task_manager.py#L624 Not sure that test for _thread_release_resources() that contains 2 lines itself is needed.","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"9d3488a0646f8052fc7b9588afe30e6bc2b51c61","unresolved":false,"context_lines":[{"line_number":415,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":416,"context_line":"                    try:"},{"line_number":417,"context_line":"                        # Execute the on_error hook if set"},{"line_number":418,"context_line":"                        if self._on_error_method:"},{"line_number":419,"context_line":"                            self._on_error_method(e, *self._on_error_args,"},{"line_number":420,"context_line":"                                                  **self._on_error_kwargs)"},{"line_number":421,"context_line":"                    except Exception:"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a5de9d1_f4432b11","line":418,"updated":"2016-02-01 06:54:07.000000000","message":"I am not quite understand how does the link works :(, but have a concern that if you write the last_error in _thread_release_resources(), the last_error will be overwrite by the _on_error_method here like provisioning_error_handler. So is it better to handle the exception here too?\nI assume the Thread.link() callback exception was handled here before, but I could be totally wrong.","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"a940be5832b243e6f7ea82b2ff432b5a5e9c7bc8","unresolved":false,"context_lines":[{"line_number":415,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":416,"context_line":"                    try:"},{"line_number":417,"context_line":"                        # Execute the on_error hook if set"},{"line_number":418,"context_line":"                        if self._on_error_method:"},{"line_number":419,"context_line":"                            self._on_error_method(e, *self._on_error_args,"},{"line_number":420,"context_line":"                                                  **self._on_error_kwargs)"},{"line_number":421,"context_line":"                    except Exception:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5a5ae5dd_e22476b0","line":418,"in_reply_to":"7a5de9d1_b139e9da","updated":"2016-02-03 09:52:50.000000000","message":"Thanks to explain :)","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e3d7e92be01828eecb3e2e8ec2a760aacca73166","unresolved":false,"context_lines":[{"line_number":415,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":416,"context_line":"                    try:"},{"line_number":417,"context_line":"                        # Execute the on_error hook if set"},{"line_number":418,"context_line":"                        if self._on_error_method:"},{"line_number":419,"context_line":"                            self._on_error_method(e, *self._on_error_args,"},{"line_number":420,"context_line":"                                                  **self._on_error_kwargs)"},{"line_number":421,"context_line":"                    except Exception:"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a5de9d1_b139e9da","line":418,"in_reply_to":"7a5de9d1_f4432b11","updated":"2016-02-02 16:07:15.000000000","message":"_on_error_method handles thread spawning error (raised from l404 in the thread that does spawn), but _thread_release_resources() executing after new thread is finished (with or w/o exception).","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"},{"author":{"_account_id":11076,"name":"Shivanand Tendulker","email":"stendulker@gmail.com","username":"stendulker"},"change_message_id":"14a3995c176bd73e596d8601abd51740a54c5a56","unresolved":false,"context_lines":[{"line_number":325,"context_line":"                thread.wait()"},{"line_number":326,"context_line":"            except Exception as e:"},{"line_number":327,"context_line":"                method \u003d self._spawn_args[0].__name__"},{"line_number":328,"context_line":"                msg \u003d _(\"Async executing of %(method)s failed with error: \""},{"line_number":329,"context_line":"                        \"%(error)s\") % {\u0027method\u0027: method,"},{"line_number":330,"context_line":"                                        \u0027error\u0027: six.text_type(e)}"},{"line_number":331,"context_line":"                node.last_error \u003d msg"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a5ae5dd_64754df8","line":328,"range":{"start_line":328,"start_character":31,"end_line":328,"end_character":40},"updated":"2016-02-06 12:43:18.000000000","message":"nit: s/ executing/ execution/","commit_id":"22461ccce842ab001639922a9387062e56554b40"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"c7c9cc3adf6b2c27dc3c053a72b4c48980c19478","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if node and node.last_error is None:"},{"line_number":329,"context_line":"            try:"},{"line_number":330,"context_line":"                exc \u003d future.exception()"},{"line_number":331,"context_line":"            except futurist.CancelledError:"},{"line_number":332,"context_line":"                pass"},{"line_number":333,"context_line":"            else:"},{"line_number":334,"context_line":"                if exc is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_6f4a3d85","line":331,"updated":"2016-02-26 06:57:52.000000000","message":"The exception is because future is cancelled before completing, so that means the task is still failed, why don\u0027t we log the reason?","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"00310b2c8fdfee93d0f696b80b8e0cbf59709e02","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if node and node.last_error is None:"},{"line_number":329,"context_line":"            try:"},{"line_number":330,"context_line":"                exc \u003d future.exception()"},{"line_number":331,"context_line":"            except futurist.CancelledError:"},{"line_number":332,"context_line":"                pass"},{"line_number":333,"context_line":"            else:"},{"line_number":334,"context_line":"                if exc is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_39f80849","line":331,"in_reply_to":"9aed3d3a_6262eb3f","updated":"2016-02-29 14:21:42.000000000","message":"Done","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"8e7286c507e7654e7bf575792fd03c9083b365b8","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if node and node.last_error is None:"},{"line_number":329,"context_line":"            try:"},{"line_number":330,"context_line":"                exc \u003d future.exception()"},{"line_number":331,"context_line":"            except futurist.CancelledError:"},{"line_number":332,"context_line":"                pass"},{"line_number":333,"context_line":"            else:"},{"line_number":334,"context_line":"                if exc is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_6262eb3f","line":331,"in_reply_to":"9aed3d3a_6714bd9f","updated":"2016-02-29 10:10:33.000000000","message":"Would logging an exception in this case enough to bring visibility to the error ?","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"f36af3f92fa1109a37c41f2e28ecd9da3f7ac547","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if node and node.last_error is None:"},{"line_number":329,"context_line":"            try:"},{"line_number":330,"context_line":"                exc \u003d future.exception()"},{"line_number":331,"context_line":"            except futurist.CancelledError:"},{"line_number":332,"context_line":"                pass"},{"line_number":333,"context_line":"            else:"},{"line_number":334,"context_line":"                if exc is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_b8a00492","line":331,"in_reply_to":"9aed3d3a_6f4a3d85","updated":"2016-02-26 09:35:41.000000000","message":"It\u0027s not exception from task itself, it\u0027s possible Ironic\u0027s/futurist internal issue. A future cannot be cancelled if it is running or has already completed: http://docs.openstack.org/developer/futurist/api.html#futures","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"550b4bb1dd8f5f902dc296b796746458129c4581","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if node and node.last_error is None:"},{"line_number":329,"context_line":"            try:"},{"line_number":330,"context_line":"                exc \u003d future.exception()"},{"line_number":331,"context_line":"            except futurist.CancelledError:"},{"line_number":332,"context_line":"                pass"},{"line_number":333,"context_line":"            else:"},{"line_number":334,"context_line":"                if exc is not None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_6714bd9f","line":331,"in_reply_to":"9aed3d3a_b8a00492","updated":"2016-02-29 09:45:33.000000000","message":"hmmm, I am not familiar with futurist but still have a concern about this. Anyway, I will remove my -1 and see other expertise\u0027comments :)","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"8e7286c507e7654e7bf575792fd03c9083b365b8","unresolved":false,"context_lines":[{"line_number":344,"context_line":""},{"line_number":345,"context_line":"    def _thread_release_resources(self, fut):"},{"line_number":346,"context_line":"        \"\"\"Thread callback to release resources.\"\"\""},{"line_number":347,"context_line":"        self._write_exception(fut)"},{"line_number":348,"context_line":"        self.release_resources()"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"    def process_event(self, event, callback\u003dNone, call_args\u003dNone,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_e2a55ba2","line":347,"updated":"2016-02-29 10:10:33.000000000","message":"I wonder if we need to be extra careful when invoking this method here, e.g putting it in a try...finally block to make sure the realease_resources() will get execute even if _write_exception() raise an exception for whatever reason, otherwise we may end up with a dead-lock.\n\nFor example, if the exception() method from futurist start raising something else than CancelledError we will have problems here.\n\ne.g\n\n try:\n     self._write_exception(fut)\n finally:\n     self.release_resources()","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"00310b2c8fdfee93d0f696b80b8e0cbf59709e02","unresolved":false,"context_lines":[{"line_number":344,"context_line":""},{"line_number":345,"context_line":"    def _thread_release_resources(self, fut):"},{"line_number":346,"context_line":"        \"\"\"Thread callback to release resources.\"\"\""},{"line_number":347,"context_line":"        self._write_exception(fut)"},{"line_number":348,"context_line":"        self.release_resources()"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"    def process_event(self, event, callback\u003dNone, call_args\u003dNone,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9aed3d3a_198c24d9","line":347,"in_reply_to":"9aed3d3a_e2a55ba2","updated":"2016-02-29 14:21:42.000000000","message":"Good catch, done.","commit_id":"4318c1070b737ad076af960cdb2752a27e4c5280"}],"ironic/tests/unit/conductor/test_task_manager.py":[{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"9d3488a0646f8052fc7b9588afe30e6bc2b51c61","unresolved":false,"context_lines":[{"line_number":341,"context_line":"        spawn_mock.assert_called_once_with(1, 2, foo\u003d\u0027bar\u0027, cat\u003d\u0027meow\u0027)"},{"line_number":342,"context_line":"        thread_mock.link.assert_called_once_with("},{"line_number":343,"context_line":"            task._thread_release_resources)"},{"line_number":344,"context_line":"        self.assertFalse(thread_mock.cancel.called)"},{"line_number":345,"context_line":"        # Since we mocked link(), we\u0027re testing that __exit__ didn\u0027t"},{"line_number":346,"context_line":"        # release resources pending the finishing of the background"},{"line_number":347,"context_line":"        # thread"}],"source_content_type":"text/x-python","patch_set":1,"id":"7a5de9d1_34a223fe","line":344,"updated":"2016-02-01 06:54:07.000000000","message":"like here, self.assertTrue(task._write_exception.called)","commit_id":"ff4118d446031ef3bb1baeda0e00a0249a37c13c"}]}