)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"7bf1534211a3ec28694665dae54a526ec63813ff","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Max Lobur \u003cmax_lobur@outlook.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-02-16 17:32:38 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Refactor vendor_passthru to use conductor async workers."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"To acheive sync validation + async vendor action we used two rpc calls."},{"line_number":10,"context_line":"Currently we have async workers mechanism in conductor so this patch"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"AAAAU3%2F%2F7FA%3D","line":7,"updated":"2014-02-24 02:45:08.000000000","message":"Max, I checked the build log, found this H803  issue:\n\nH803  git commit title (\u0027Refactor vendor_passthru to use conductor async workers.\u0027) should not end with period","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"0a2f7f6efe8f1edd085e70edf3e0610d4314b9d2","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Max Lobur \u003cmax_lobur@outlook.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2014-02-16 17:32:38 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Refactor vendor_passthru to use conductor async workers."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"To acheive sync validation + async vendor action we used two rpc calls."},{"line_number":10,"context_line":"Currently we have async workers mechanism in conductor so this patch"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"AAAAU3%2F%2F0Qc%3D","line":7,"in_reply_to":"AAAAU3%2F%2F7FA%3D","updated":"2014-02-24 16:54:19.000000000","message":"thank you, I knew that, was going to fix in subsequent rev","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"}],"ironic/conductor/manager.py":[{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"c5826935b6c98cf032abdf8abba19e9d6d20bcbe","unresolved":false,"context_lines":[{"line_number":240,"context_line":"        LOG.debug(_(\"RPC vendor_passthru called for node %s.\")"},{"line_number":241,"context_line":"                    % node_id)"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        task \u003d task_manager.TaskManager(context, node_id, shared\u003dFalse)"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        try:"},{"line_number":246,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUX%2F%2FBzc%3D","line":243,"updated":"2014-02-17 15:07:44.000000000","message":"Are you sure that all vendor_passthru calls should use exclusive lock?","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"643f37436d972ef59ad86375e888640c6155ed7d","unresolved":false,"context_lines":[{"line_number":240,"context_line":"        LOG.debug(_(\"RPC vendor_passthru called for node %s.\")"},{"line_number":241,"context_line":"                    % node_id)"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        task \u003d task_manager.TaskManager(context, node_id, shared\u003dFalse)"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        try:"},{"line_number":246,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Ffn8%3D","line":243,"in_reply_to":"AAAAU3%2F%2FlGc%3D","updated":"2014-02-26 10:10:53.000000000","message":"Indeed, thanks Ruby, I will add the comment\n\nInteresting proposal regarding lock, I\u0027ll take a look what we can. Probably will propose this in a separate patch","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"35f1cf9c7338cd91d0fab2699f5a046b461d7105","unresolved":false,"context_lines":[{"line_number":240,"context_line":"        LOG.debug(_(\"RPC vendor_passthru called for node %s.\")"},{"line_number":241,"context_line":"                    % node_id)"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        task \u003d task_manager.TaskManager(context, node_id, shared\u003dFalse)"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        try:"},{"line_number":246,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2FlGc%3D","line":243,"in_reply_to":"AAAAU3%2F%2FmFg%3D","updated":"2014-02-25 21:42:14.000000000","message":"I think a comment would be useful here. Eg, \u0027Even though all vendor_passthru calls may not require an exclusive lock, we need to do so to guarantee that the state doesn\u0027t unexpectedly change between doing a vendor.validate and vendor.vendor_passthru.\u0027 Or something like that ;)\n\nIf vendor.vendor_passthru is smart and doesn\u0027t need an exclusive lock and is nice enough to care :-), maybe it can change the type of lock when it starts?","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":5805,"name":"Chris Krelle","email":"nobodycam@gmail.com","username":"nobodycam"},"change_message_id":"412a2a4ec871890e9df18c8cbc7566f533b03075","unresolved":false,"context_lines":[{"line_number":240,"context_line":"        LOG.debug(_(\"RPC vendor_passthru called for node %s.\")"},{"line_number":241,"context_line":"                    % node_id)"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        task \u003d task_manager.TaskManager(context, node_id, shared\u003dFalse)"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        try:"},{"line_number":246,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2FmFg%3D","line":243,"in_reply_to":"AAAAUX%2F%2FBs8%3D","updated":"2014-02-25 18:50:05.000000000","message":"I agree with max on this. the lock should not be released between vendor.validate and vendor.vendor_passthru  calls.","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"20c84a5fbe000004b2d1bcc6e7a240179738db6e","unresolved":false,"context_lines":[{"line_number":240,"context_line":"        LOG.debug(_(\"RPC vendor_passthru called for node %s.\")"},{"line_number":241,"context_line":"                    % node_id)"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        task \u003d task_manager.TaskManager(context, node_id, shared\u003dFalse)"},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"        try:"},{"line_number":246,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUX%2F%2FBs8%3D","line":243,"in_reply_to":"AAAAUX%2F%2FBzc%3D","updated":"2014-02-17 15:16:48.000000000","message":"No, some of them may not require exclusive lock.\n\nBut, if we want to guarantee that there will be no intrusion between vendor.validate and vendor.vendor_passthru we need to keep exclusive lock all the time.\n\nThis probably require further discussion with the team","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"0ec9f3ce7eb526d9963690abddc6a6e7bbd99984","unresolved":false,"context_lines":[{"line_number":269,"context_line":"        except Exception as e:"},{"line_number":270,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":271,"context_line":"                task.node.last_error \u003d \\"},{"line_number":272,"context_line":"                    _(\"Failed to validate vendor info. Error: %s\") % e"},{"line_number":273,"context_line":"                task.node.save(context)"},{"line_number":274,"context_line":"                # Release node lock if error occurred."},{"line_number":275,"context_line":"                task.release_resources()"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAVX%2F%2F%2FKo%3D","line":272,"updated":"2014-02-27 00:33:41.000000000","message":"I would wrap the validate_vendor_action in a try block, and move this except block to there. I would also LOG the error, but not save it to the last_error field as risks exposing driver details to the user.\n\nEg:\n\n  if not getattr(...):\n    raise exception.Unsupported...\n  try:\n    driver_data \u003d task.driver.vendor.validate(...)\n  except IronicException as e:\n    with excutils.save_and_reraise_exception():\n      LOG.error(...)  # full message\n      task.node.last_error \u003d ... # summary without \"e\"\n\n  try:\n    thread \u003d ...\n  except NoFreeConductorWorker:\n    with excutils.save_and_reraise_exception():\n      task.release_resources\n\n  thread.link ...\n  return driver_data","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"c41b7a4c478be79ec9b60aef7dee67795ead72f5","unresolved":false,"context_lines":[{"line_number":269,"context_line":"        except Exception as e:"},{"line_number":270,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":271,"context_line":"                task.node.last_error \u003d \\"},{"line_number":272,"context_line":"                    _(\"Failed to validate vendor info. Error: %s\") % e"},{"line_number":273,"context_line":"                task.node.save(context)"},{"line_number":274,"context_line":"                # Release node lock if error occurred."},{"line_number":275,"context_line":"                task.release_resources()"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAVX%2F%2F3BE%3D","line":272,"in_reply_to":"AAAAVX%2F%2F%2FKo%3D","updated":"2014-02-27 19:06:56.000000000","message":"Thanks, I will apply this","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"11df0141df67ae25fd25c1c9afdc08f21223313b","unresolved":false,"context_lines":[{"line_number":310,"context_line":"            if not getattr(task.driver, \u0027vendor\u0027, None):"},{"line_number":311,"context_line":"                raise exception.UnsupportedDriverExtension("},{"line_number":312,"context_line":"                    driver\u003dtask.node.driver,"},{"line_number":313,"context_line":"                    extension\u003d\u0027vendor passthru\u0027)"},{"line_number":314,"context_line":""},{"line_number":315,"context_line":"            driver_data \u003d task.driver.vendor.validate(task, task.node,"},{"line_number":316,"context_line":"                                                      method\u003ddriver_method,"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAVn%2F%2F%2Fjo%3D","line":313,"updated":"2014-03-02 21:24:17.000000000","message":"In this case we also need to release resources, therefore I putted this inside try (comparing to Deva\u0027s comment in previous rev)","commit_id":"0e4db906cad39bba98f88afe1bc0a434316fbd63"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e6974a400fe0a058bfbed99df1664334998c4da7","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                    driver\u003dtask.node.driver,"},{"line_number":313,"context_line":"                    extension\u003d\u0027vendor passthru\u0027)"},{"line_number":314,"context_line":""},{"line_number":315,"context_line":"            driver_data \u003d task.driver.vendor.validate(task, task.node,"},{"line_number":316,"context_line":"                                                      method\u003ddriver_method,"},{"line_number":317,"context_line":"                                                      **info)"},{"line_number":318,"context_line":"        except exception.IronicException as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAVn%2F%2Fgb4%3D","line":315,"updated":"2014-03-05 17:55:48.000000000","message":"I was wondering about this. Some of the *.validate()\u0027s return True and some don\u0027t. I just spoke to Deva and Lucas about it, and Deva said it was a bug; they shouldn\u0027t return anything. See https://bugs.launchpad.net/ironic/+bug/1288363.","commit_id":"0e4db906cad39bba98f88afe1bc0a434316fbd63"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"7e109020736ab101b82486018c1cf9c90e16385d","unresolved":false,"context_lines":[{"line_number":318,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":319,"context_line":"                LOG.error(_(\"Vendor info validation failed. Error: %s\") % e)"},{"line_number":320,"context_line":"                task.node.last_error \u003d _(\"Vendor info validation failed.\")"},{"line_number":321,"context_line":"                task.node.save(context)"},{"line_number":322,"context_line":"                # Release node lock if error occurred."},{"line_number":323,"context_line":"                task.release_resources()"},{"line_number":324,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAVn%2F%2FWgY%3D","line":321,"updated":"2014-03-06 17:23:37.000000000","message":"As commented at https://review.openstack.org/#/c/78047/3/ironic/conductor/manager.py\n\nI don\u0027t think we should be using the last_error field in a sync operation, the reason why last_error was created was to hold errors that happens in the background and therefore adding it here will break that consistence.","commit_id":"8820fcd0426d1510f7d3ec46aa4a9658fd91b0da"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"df435a3bc1236f658f7fd89cd8a94fb482ee9d98","unresolved":false,"context_lines":[{"line_number":318,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":319,"context_line":"                LOG.error(_(\"Vendor info validation failed. Error: %s\") % e)"},{"line_number":320,"context_line":"                task.node.last_error \u003d _(\"Vendor info validation failed.\")"},{"line_number":321,"context_line":"                task.node.save(context)"},{"line_number":322,"context_line":"                # Release node lock if error occurred."},{"line_number":323,"context_line":"                task.release_resources()"},{"line_number":324,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAVn%2F%2FTco%3D","line":321,"in_reply_to":"AAAAVn%2F%2FWgY%3D","updated":"2014-03-06 23:08:19.000000000","message":"Yep, since it will be reflected in the logs, this makes sense to me, I\u0027ll remove","commit_id":"8820fcd0426d1510f7d3ec46aa4a9658fd91b0da"}],"ironic/conductor/rpcapi.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"4e512591402c6aa96572c8c0437ee43f88e98ac8","unresolved":false,"context_lines":[{"line_number":56,"context_line":"        1.8 - Added change_node_maintenance_mode."},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    \"\"\""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    RPC_API_VERSION \u003d \u00271.8\u0027"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    def __init__(self, topic\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUX%2F%2FB7g%3D","line":59,"updated":"2014-02-17 14:59:30.000000000","message":"Does this change require update of rpc api ver?","commit_id":"acdfe05258b2f984c4e04bd4b62a694abc8cc436"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"74cd0fa4356a38720ee70f7f14e15a0080bc3bfe","unresolved":false,"context_lines":[{"line_number":148,"context_line":"        :param driver_method: name of method for driver."},{"line_number":149,"context_line":"        :param info: info for node driver."},{"line_number":150,"context_line":"        :param topic: RPC topic. Defaults to self.topic."},{"line_number":151,"context_line":"        :returns: driver data returned from driver.validate, if any."},{"line_number":152,"context_line":"        :raises: InvalidParameterValue if supplied info is not valid."},{"line_number":153,"context_line":"        :raises: UnsupportedDriverExtension if current driver does not have"},{"line_number":154,"context_line":"                 vendor interface."}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAVn%2F%2Fd5U%3D","line":151,"updated":"2014-03-05 22:30:01.000000000","message":"needs to be deleted, thanks Ruby!","commit_id":"b4d35f1569fdfa59d0e96dff04b15162135ae23f"}],"ironic/drivers/modules/pxe.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"11df0141df67ae25fd25c1c9afdc08f21223313b","unresolved":false,"context_lines":[{"line_number":695,"context_line":""},{"line_number":696,"context_line":"        return True"},{"line_number":697,"context_line":""},{"line_number":698,"context_line":"    @task_manager.require_exclusive_lock"},{"line_number":699,"context_line":"    def _continue_deploy(self, task, node, **kwargs):"},{"line_number":700,"context_line":"        \"\"\"Resume a deployment upon getting POST data from deploy ramdisk."},{"line_number":701,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAVn%2F%2F%2FkI%3D","line":698,"updated":"2014-03-02 21:24:17.000000000","message":"manager_utils.node_power_action path requires exclusive lock, therefore I added it on this level too","commit_id":"0e4db906cad39bba98f88afe1bc0a434316fbd63"}],"ironic/tests/conductor/test_manager.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"35f1cf9c7338cd91d0fab2699f5a046b461d7105","unresolved":false,"context_lines":[{"line_number":451,"context_line":"        self.assertRaises(exception.InvalidParameterValue,"},{"line_number":452,"context_line":"                          self.service.vendor_passthru,"},{"line_number":453,"context_line":"                          self.context, n[\u0027uuid\u0027], \u0027unsupported_method\u0027, info)"},{"line_number":454,"context_line":"        # Waiting to make sure the below perform_mock assertions are valid."},{"line_number":455,"context_line":"        self.service._worker_pool.waitall()"},{"line_number":456,"context_line":""},{"line_number":457,"context_line":"        node.refresh(self.context)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Fkh8%3D","line":454,"updated":"2014-02-25 21:42:14.000000000","message":"Ok, I\u0027ll bite. What perform_mock assertions? Maybe I missed it, but I don\u0027t see any mocks.","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"643f37436d972ef59ad86375e888640c6155ed7d","unresolved":false,"context_lines":[{"line_number":451,"context_line":"        self.assertRaises(exception.InvalidParameterValue,"},{"line_number":452,"context_line":"                          self.service.vendor_passthru,"},{"line_number":453,"context_line":"                          self.context, n[\u0027uuid\u0027], \u0027unsupported_method\u0027, info)"},{"line_number":454,"context_line":"        # Waiting to make sure the below perform_mock assertions are valid."},{"line_number":455,"context_line":"        self.service._worker_pool.waitall()"},{"line_number":456,"context_line":""},{"line_number":457,"context_line":"        node.refresh(self.context)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Ffmc%3D","line":454,"in_reply_to":"AAAAU3%2F%2Fkh8%3D","updated":"2014-02-26 10:10:53.000000000","message":"You\u0027re right, the comment should be updated. I guess I copied it from somewhere.\n\nIt should say:\n\n# Waiting to make sure the below assertions are valid.","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"35f1cf9c7338cd91d0fab2699f5a046b461d7105","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                          self.service.vendor_passthru,"},{"line_number":488,"context_line":"                          self.context, n[\u0027uuid\u0027], \u0027whatever_method\u0027, info)"},{"line_number":489,"context_line":"        # Waiting to make sure the below perform_mock assertions are valid."},{"line_number":490,"context_line":"        self.service._worker_pool.waitall()"},{"line_number":491,"context_line":"        node.refresh(self.context)"},{"line_number":492,"context_line":"        self.assertIsNotNone(node.last_error)"},{"line_number":493,"context_line":"        # Verify reservation has been cleared."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Fkes%3D","line":490,"updated":"2014-02-25 21:42:14.000000000","message":"This is a really small nit. Why do you sometimes have a blank space between this call and the node.refresh() (eg line 473), and sometimes not.","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"3f2afe1dc4bd82c62c1131a06f56995b9a7bc91f","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                          self.service.vendor_passthru,"},{"line_number":488,"context_line":"                          self.context, n[\u0027uuid\u0027], \u0027whatever_method\u0027, info)"},{"line_number":489,"context_line":"        # Waiting to make sure the below perform_mock assertions are valid."},{"line_number":490,"context_line":"        self.service._worker_pool.waitall()"},{"line_number":491,"context_line":"        node.refresh(self.context)"},{"line_number":492,"context_line":"        self.assertIsNotNone(node.last_error)"},{"line_number":493,"context_line":"        # Verify reservation has been cleared."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Fc%2B4%3D","line":490,"in_reply_to":"AAAAU3%2F%2Ffhs%3D","updated":"2014-02-26 15:03:25.000000000","message":"Thanks for the explanation. Yes, AAA does make it easier to read/understand.\n\nI did find the comment above the _worker_pool.waitall() a bit confusing when I first saw it though. Probably because I interpreted it as \u0027I\u0027m waiting to make sure those asserts are valid\u0027, vs something like \u0027wait until something is finished, so that the asserts will be valid\u0027. (But that\u0027s just me.) ;)","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"643f37436d972ef59ad86375e888640c6155ed7d","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                          self.service.vendor_passthru,"},{"line_number":488,"context_line":"                          self.context, n[\u0027uuid\u0027], \u0027whatever_method\u0027, info)"},{"line_number":489,"context_line":"        # Waiting to make sure the below perform_mock assertions are valid."},{"line_number":490,"context_line":"        self.service._worker_pool.waitall()"},{"line_number":491,"context_line":"        node.refresh(self.context)"},{"line_number":492,"context_line":"        self.assertIsNotNone(node.last_error)"},{"line_number":493,"context_line":"        # Verify reservation has been cleared."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Ffhs%3D","line":490,"in_reply_to":"AAAAU3%2F%2Fkes%3D","updated":"2014-02-26 10:10:53.000000000","message":"Ah good catch :) I\u0027m trying to follow AAA (arrange act assert) pattern in tests. The gist is that each step should be clearly separated from others. \n\nFor example test_vendor_passthru_success test has all three steps, where act is self.service.vendor_passthru, therefore a line before refresh makes sense there. \n\nIn this tests there is no clear \"act\" step, it build into the first assert, therefore it\u0027s a matter of dispute whether to separate by blank line or not. I think it should be separated...\n\nThis may seem complex, but I\u0027ve seen cases when this pattern really made tests clearer and forced developer to perform only one act per tests - prevent messy tests. Thanks for this catch, I\u0027ll update the code","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"}],"ironic/tests/drivers/test_pxe.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"35f1cf9c7338cd91d0fab2699f5a046b461d7105","unresolved":false,"context_lines":[{"line_number":673,"context_line":"                fake_deploy))"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"        with task_manager.acquire(self.context, self.node.uuid,"},{"line_number":676,"context_line":"                                  shared\u003dTrue) as task:"},{"line_number":677,"context_line":"            task.resources[0].driver.vendor.vendor_passthru(task, self.node,"},{"line_number":678,"context_line":"                    method\u003d\u0027pass_deploy_info\u0027, address\u003d\u0027123456\u0027, iqn\u003d\u0027aaa-bbb\u0027,"},{"line_number":679,"context_line":"                    key\u003d\u0027fake-56789\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2FkYo%3D","line":676,"updated":"2014-02-25 21:42:14.000000000","message":"shared \u003d True here and it works?","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"e34bbcd17dc8b5c7eb89949942313dde650c5d15","unresolved":false,"context_lines":[{"line_number":673,"context_line":"                fake_deploy))"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"        with task_manager.acquire(self.context, self.node.uuid,"},{"line_number":676,"context_line":"                                  shared\u003dTrue) as task:"},{"line_number":677,"context_line":"            task.resources[0].driver.vendor.vendor_passthru(task, self.node,"},{"line_number":678,"context_line":"                    method\u003d\u0027pass_deploy_info\u0027, address\u003d\u0027123456\u0027, iqn\u003d\u0027aaa-bbb\u0027,"},{"line_number":679,"context_line":"                    key\u003d\u0027fake-56789\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAVX%2F%2Fn5o%3D","line":676,"in_reply_to":"AAAAU3%2F%2Fffo%3D","updated":"2014-03-02 17:30:48.000000000","message":"Ruby, this turns to be a very useful finding\n\nIt did not fail similar to test_continue_deploy_fail and test_continue_deploy_ramdisk_fails because it this test there were no attempt to change node power state\n\nPls take a look https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/pxe.py#L710-L716\n\nnode_power_change always requires exclusive lock, and those decorator @task_manager.require_exclusive_lock above the node_power_change actually fired exception, but it did not bubble up because we don\u0027t reraise the exception in _continue_deploy (see the link above). I didn\u0027t notice this before. In the next rev I added @require_lock decorator above the _continue_deploy as well, this makes the error behavior clearer, and this test needs to be changed too. Thanks!","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"643f37436d972ef59ad86375e888640c6155ed7d","unresolved":false,"context_lines":[{"line_number":673,"context_line":"                fake_deploy))"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"        with task_manager.acquire(self.context, self.node.uuid,"},{"line_number":676,"context_line":"                                  shared\u003dTrue) as task:"},{"line_number":677,"context_line":"            task.resources[0].driver.vendor.vendor_passthru(task, self.node,"},{"line_number":678,"context_line":"                    method\u003d\u0027pass_deploy_info\u0027, address\u003d\u0027123456\u0027, iqn\u003d\u0027aaa-bbb\u0027,"},{"line_number":679,"context_line":"                    key\u003d\u0027fake-56789\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAU3%2F%2Fffo%3D","line":676,"in_reply_to":"AAAAU3%2F%2FkYo%3D","updated":"2014-02-26 10:10:53.000000000","message":"that\u0027s probably because of MonkeyPatch(\u0027ironic.drivers.modules.deploy_utils.deploy\u0027,\tabove. I\u0027ll investigate this","commit_id":"b881f180a4614f44695594196e755a012e0fc68b"}]}