)]}'
{"ironic/api/controllers/v1/node.py":[{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b620b407f2ae0ac6ebc30efb762774fbf661a439","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            raise wsme.exc.ClientSideError(msg, status_code\u003d409)  # Conflict."},{"line_number":141,"context_line":"        except exception.IronicException as e:"},{"line_number":142,"context_line":"            # Make sure Remote Exception is printed correctly."},{"line_number":143,"context_line":"            raise wsme.exc.ClientSideError(e.format_message())"},{"line_number":144,"context_line":"        except Exception as e:"},{"line_number":145,"context_line":"            raise wsme.exc.ClientSideError(str(e))"},{"line_number":146,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9rrM%3D","line":143,"updated":"2014-02-05 03:40:00.000000000","message":"I don\u0027t think we should allow IronicExceptions to propagate up and be formatted by Pecan. They may contain valuable information such as different error codes, which will be lost by explicitly formatting them. Furthermore, they may in fact not be client-side errors.","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b620b407f2ae0ac6ebc30efb762774fbf661a439","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            # Make sure Remote Exception is printed correctly."},{"line_number":143,"context_line":"            raise wsme.exc.ClientSideError(e.format_message())"},{"line_number":144,"context_line":"        except Exception as e:"},{"line_number":145,"context_line":"            raise wsme.exc.ClientSideError(str(e))"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"        # FIXME(lucasagomes): Currently WSME doesn\u0027t support returning"},{"line_number":148,"context_line":"        # the Location header. Once it\u0027s implemented we should use the"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9rrE%3D","line":145,"updated":"2014-02-05 03:40:00.000000000","message":"ditto - no way to know this is a client side error, and if we get here, it\u0027s probably not.","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"ef94f9fe83682ba0a7bbc846be1c5e3f4c7629d7","unresolved":false,"context_lines":[{"line_number":142,"context_line":"            # Make sure Remote Exception is printed correctly."},{"line_number":143,"context_line":"            raise wsme.exc.ClientSideError(e.format_message())"},{"line_number":144,"context_line":"        except Exception as e:"},{"line_number":145,"context_line":"            raise wsme.exc.ClientSideError(str(e))"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"        # FIXME(lucasagomes): Currently WSME doesn\u0027t support returning"},{"line_number":148,"context_line":"        # the Location header. Once it\u0027s implemented we should use the"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9dsc%3D","line":145,"in_reply_to":"AAAAUH%2F9rrE%3D","updated":"2014-02-06 19:21:06.000000000","message":"I removed these handlers so exceptions are not converted to ClientSideError, they sent to the client as is.\n\nOr how did you mean we should notify the client?","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"}],"ironic/common/thread_utils.py":[{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b620b407f2ae0ac6ebc30efb762774fbf661a439","unresolved":false,"context_lines":[{"line_number":24,"context_line":"    :returns: FutureWithWait object"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    \"\"\""},{"line_number":27,"context_line":"    with futures.ThreadPoolExecutor(max_workers\u003d1) as executor:"},{"line_number":28,"context_line":"        future \u003d executor.submit(f, *f_args, **f_kwargs)"},{"line_number":29,"context_line":"        return FutureWithWait(future)"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9rR4%3D","line":27,"updated":"2014-02-05 03:40:00.000000000","message":"We should not create a new ThreadPoolExecutor for every request -- we should create one and only one ThreadPoolExecutor as part of the ConductorService instance, with a configurable max_workers, and then submit jobs to it, and terminate the ThreadPoolExecutor when the service shuts down.","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"ef94f9fe83682ba0a7bbc846be1c5e3f4c7629d7","unresolved":false,"context_lines":[{"line_number":24,"context_line":"    :returns: FutureWithWait object"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    \"\"\""},{"line_number":27,"context_line":"    with futures.ThreadPoolExecutor(max_workers\u003d1) as executor:"},{"line_number":28,"context_line":"        future \u003d executor.submit(f, *f_args, **f_kwargs)"},{"line_number":29,"context_line":"        return FutureWithWait(future)"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9drQ%3D","line":27,"in_reply_to":"AAAAUH%2F9rR4%3D","updated":"2014-02-06 19:21:06.000000000","message":"done as per your mock up","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"}],"ironic/conductor/manager.py":[{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b620b407f2ae0ac6ebc30efb762774fbf661a439","unresolved":false,"context_lines":[{"line_number":178,"context_line":""},{"line_number":179,"context_line":"            return node_obj"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def background_change_node_power_state(self, context, node_id,"},{"line_number":182,"context_line":"                                           new_state):"},{"line_number":183,"context_line":"        \"\"\"RPC method to encapsulate changes to a node\u0027s state."},{"line_number":184,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9roo%3D","line":181,"updated":"2014-02-05 03:40:00.000000000","message":"I see no benefit to changing the method name. Soon all methods will be done in the background, so it will be redundant to see them all prefixed with \"background_*\"","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"ef94f9fe83682ba0a7bbc846be1c5e3f4c7629d7","unresolved":false,"context_lines":[{"line_number":178,"context_line":""},{"line_number":179,"context_line":"            return node_obj"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def background_change_node_power_state(self, context, node_id,"},{"line_number":182,"context_line":"                                           new_state):"},{"line_number":183,"context_line":"        \"\"\"RPC method to encapsulate changes to a node\u0027s state."},{"line_number":184,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F9dq0%3D","line":181,"in_reply_to":"AAAAUH%2F9roo%3D","updated":"2014-02-06 19:21:06.000000000","message":"I agree, reverted this","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"7ca2f3c95b65474ba2ffcc693cc95e54c51f8aba","unresolved":false,"context_lines":[{"line_number":221,"context_line":"            future.add_done_callback(lambda f: task.release_resources())"},{"line_number":222,"context_line":"        else:"},{"line_number":223,"context_line":"            # Otherwise release immediately."},{"line_number":224,"context_line":"            task.release_resources()"},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"        return future"},{"line_number":227,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BdY8%3D","line":224,"updated":"2014-01-25 22:27:24.000000000","message":"The complexity of this may be reduced if we will submit dummy task (lambda: None) when the node power action is not required. background_node_power_action will always return future (instead of future/None), add_done_callback will be fired despite the future is completed at that moment. So the code will be:\n\n                try:\n\t            future \u003d utils.background_node_power_action(task, task.node,\n\t                                                        new_state)\n                    future.add_done_callback(lambda f: task.release_resources())\n\t        except Exception:\n\t            # Release resource if validation has failed.\n\t            task.release_resources()\n\t            raise\n\nAny thoughts?","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"c683a68f21bfad9b13cf29ba356a0778896056df","unresolved":false,"context_lines":[{"line_number":42,"context_line":"building or tearing down the TFTP environment for a node, notifying Neutron of"},{"line_number":43,"context_line":"a change, etc."},{"line_number":44,"context_line":"\"\"\""},{"line_number":45,"context_line":"from eventlet import greenpool"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"from oslo.config import cfg"},{"line_number":48,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9YVI%3D","line":45,"updated":"2014-02-07 10:24:00.000000000","message":"IMO dependence from eventlet in non-common code is not good.","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":42,"context_line":"building or tearing down the TFTP environment for a node, notifying Neutron of"},{"line_number":43,"context_line":"a change, etc."},{"line_number":44,"context_line":"\"\"\""},{"line_number":45,"context_line":"from eventlet import greenpool"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"from oslo.config import cfg"},{"line_number":48,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cio%3D","line":45,"updated":"2014-02-06 21:23:59.000000000","message":"missing blank line before import","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"aa18c88de42ac229e2d2cd2eb574c630f1fc3969","unresolved":false,"context_lines":[{"line_number":42,"context_line":"building or tearing down the TFTP environment for a node, notifying Neutron of"},{"line_number":43,"context_line":"a change, etc."},{"line_number":44,"context_line":"\"\"\""},{"line_number":45,"context_line":"from eventlet import greenpool"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"from oslo.config import cfg"},{"line_number":48,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9YPY%3D","line":45,"in_reply_to":"AAAAUH%2F9YVI%3D","updated":"2014-02-07 10:33:38.000000000","message":"yep, thanks for the suggestion, already moving this to a separate util","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        \"\"\"Consistent hash ring which maps drivers to conductors.\"\"\""},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        self.worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"},{"line_number":120,"context_line":"        \"\"\"GreehPool of background workers for performing tasks async.\"\"\""},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    # TODO(deva): add stop() to call unregister_conductor"},{"line_number":123,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cik%3D","line":120,"updated":"2014-02-06 21:23:59.000000000","message":"typo: GreenPool","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        \"\"\"Consistent hash ring which maps drivers to conductors.\"\"\""},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        self.worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"},{"line_number":120,"context_line":"        \"\"\"GreehPool of background workers for performing tasks async.\"\"\""},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"    # TODO(deva): add stop() to call unregister_conductor"},{"line_number":123,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cb8%3D","line":120,"in_reply_to":"AAAAUH%2F9cik%3D","updated":"2014-02-06 21:57:06.000000000","message":"heh, copied from your mock up :-P","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":211,"context_line":"        task \u003d task_manager.TaskManager(context)"},{"line_number":212,"context_line":"        task.acquire_resources(node_id, shared\u003dFalse)"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"        try:"},{"line_number":215,"context_line":"            # Synchronously validate requested action."},{"line_number":216,"context_line":"            action_required \u003d utils.validate_node_power_action(task, task.node,"},{"line_number":217,"context_line":"                                                               new_state)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9chY%3D","line":214,"updated":"2014-02-06 21:23:59.000000000","message":"I think it would be better to encapsulate all actions here in a single try: block, so that regardless of which call raises an exception (whether validate_node_Power_action or worker_pool.spawn), we will still call release_resource()","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":211,"context_line":"        task \u003d task_manager.TaskManager(context)"},{"line_number":212,"context_line":"        task.acquire_resources(node_id, shared\u003dFalse)"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"        try:"},{"line_number":215,"context_line":"            # Synchronously validate requested action."},{"line_number":216,"context_line":"            action_required \u003d utils.validate_node_power_action(task, task.node,"},{"line_number":217,"context_line":"                                                               new_state)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cac%3D","line":214,"in_reply_to":"AAAAUH%2F9chY%3D","updated":"2014-02-06 21:57:06.000000000","message":"Spawn won\u0027t raise exception, AFAIR if exception will occur inside spawned thread, it won\u0027t bubble here. To let it bubble to this thread we should explicitly call thread.wait()\n\nMakes sense to you?","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":224,"context_line":"        if action_required:"},{"line_number":225,"context_line":"            # Start requested action in the background."},{"line_number":226,"context_line":"            thread \u003d self.worker_pool.spawn(utils.perform_node_power_action,"},{"line_number":227,"context_line":"                                            task, task.node, new_state)"},{"line_number":228,"context_line":"            # Release node lock at the end."},{"line_number":229,"context_line":"            thread.link(lambda t: task.release_resources())"},{"line_number":230,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cUc%3D","line":227,"updated":"2014-02-06 21:57:06.000000000","message":"Also I wanted to note here: pool.spawn will block if there are no free workers until someone is freed. So, if the client would issue 61th request, it will get here, acquire lock and then just wait... And then two bad things happen:\n\n1. We\u0027re holding the lock without doing something.\n\n2. Meanwhile either RPC timer in the API or HTTP request timer on the client side will terminate request due to timeout. The user probably would want re-issue request, but the first one will actually complete, once it get free worker. This will be confusing.\n\nWe probably would need to check worker pool is full and raise exception if so. But there will be another race, since this is multi-threaded environment we could pass such check, but right before our spawn some other thread will pick the last worker. I thought about wrapping the check in python \"lock\" statement at this place, but not sure how it works with green threads, need to check\n\nI wish spawn could throw an exception if there is no workers, but looking to the source - http://searchcode.com/codesearch/view/15543497 seems there is no such option\n\nIf you have some ideas - please share","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"8ef38b68edf7e9a087edabf5b778ac7408f51af1","unresolved":false,"context_lines":[{"line_number":224,"context_line":"        if action_required:"},{"line_number":225,"context_line":"            # Start requested action in the background."},{"line_number":226,"context_line":"            thread \u003d self.worker_pool.spawn(utils.perform_node_power_action,"},{"line_number":227,"context_line":"                                            task, task.node, new_state)"},{"line_number":228,"context_line":"            # Release node lock at the end."},{"line_number":229,"context_line":"            thread.link(lambda t: task.release_resources())"},{"line_number":230,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9Yio%3D","line":227,"in_reply_to":"AAAAUH%2F9cUc%3D","updated":"2014-02-07 09:57:45.000000000","message":"@lockutils.synchronized is OpenStack-kind way to solve this.","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            # Release node lock immediately."},{"line_number":232,"context_line":"            task.release_resources()"},{"line_number":233,"context_line":"        return thread"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    # NOTE(deva): There is a race condition in the RPC API for vendor_passthru."},{"line_number":236,"context_line":"    # Between the validate_vendor_action and do_vendor_action calls, it\u0027s"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9chw%3D","line":233,"updated":"2014-02-06 21:23:59.000000000","message":"why \"return thread\"?\n\nI don\u0027t see a reason for this to all be in a separate, private method. What am I missing?","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"8ef38b68edf7e9a087edabf5b778ac7408f51af1","unresolved":false,"context_lines":[{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            # Release node lock immediately."},{"line_number":232,"context_line":"            task.release_resources()"},{"line_number":233,"context_line":"        return thread"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    # NOTE(deva): There is a race condition in the RPC API for vendor_passthru."},{"line_number":236,"context_line":"    # Between the validate_vendor_action and do_vendor_action calls, it\u0027s"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9YiI%3D","line":233,"in_reply_to":"AAAAUH%2F9cZA%3D","updated":"2014-02-07 09:57:45.000000000","message":"I realized that I can use worker_pool.waitall() to achieve this","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            # Release node lock immediately."},{"line_number":232,"context_line":"            task.release_resources()"},{"line_number":233,"context_line":"        return thread"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    # NOTE(deva): There is a race condition in the RPC API for vendor_passthru."},{"line_number":236,"context_line":"    # Between the validate_vendor_action and do_vendor_action calls, it\u0027s"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cZA%3D","line":233,"in_reply_to":"AAAAUH%2F9chw%3D","updated":"2014-02-06 21:57:06.000000000","message":"This is only because I\u0027m testing this method, and to make sure my test is not time dependent (to avoid sleeps) I have a private method that return thread, so in the test I can call thread.wait() and then check all the mocks etc.","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"}],"ironic/conductor/task_manager.py":[{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":58,"context_line":"using link method of the thread object."},{"line_number":59,"context_line":"For example (somewhere inside manager)::"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    task \u003d task_manager.TaskManager(context)"},{"line_number":62,"context_line":"    # Acquire resources manually."},{"line_number":63,"context_line":"    task.acquire_resources(node_id, shared\u003dFalse)"},{"line_number":64,"context_line":"    # Initiate background task."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9chI%3D","line":61,"updated":"2014-02-06 21:23:59.000000000","message":"Thanks for adding example to the doc! It should also include proper try clause to ensure lock is released. Eg,\n\n  task \u003d task_manager....\n  try:\n    task.acquire_resources\n    thread \u003d self.woker_pool.spawn\n  except:\n    with excutils.save_and_reraise:\n      task.release_resource\n  thread.link(lambda ...)","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":58,"context_line":"using link method of the thread object."},{"line_number":59,"context_line":"For example (somewhere inside manager)::"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    task \u003d task_manager.TaskManager(context)"},{"line_number":62,"context_line":"    # Acquire resources manually."},{"line_number":63,"context_line":"    task.acquire_resources(node_id, shared\u003dFalse)"},{"line_number":64,"context_line":"    # Initiate background task."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cS0%3D","line":61,"in_reply_to":"AAAAUH%2F9chI%3D","updated":"2014-02-06 21:57:06.000000000","message":"Right, I will fix","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":69,"context_line":"link callback will be called whenever:"},{"line_number":70,"context_line":"    - background task finished with no errors."},{"line_number":71,"context_line":"    - background task has crashed with exception."},{"line_number":72,"context_line":"    - callback was added after the background task has finished or crashed. "},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"Eventually, driver functionality may be wrapped by tasks to facilitate"},{"line_number":75,"context_line":"multi-node tasks more easily. Once implemented, it might look like this::"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9chQ%3D","line":72,"updated":"2014-02-06 21:23:59.000000000","message":"white space on EOL","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":69,"context_line":"link callback will be called whenever:"},{"line_number":70,"context_line":"    - background task finished with no errors."},{"line_number":71,"context_line":"    - background task has crashed with exception."},{"line_number":72,"context_line":"    - callback was added after the background task has finished or crashed. "},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"Eventually, driver functionality may be wrapped by tasks to facilitate"},{"line_number":75,"context_line":"multi-node tasks more easily. Once implemented, it might look like this::"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cSs%3D","line":72,"in_reply_to":"AAAAUH%2F9chQ%3D","updated":"2014-02-06 21:57:06.000000000","message":"will fix","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        \"\"\""},{"line_number":153,"context_line":"        # Do not allow multiple acquire calls."},{"line_number":154,"context_line":"        if self.resources:"},{"line_number":155,"context_line":"            raise Exception(_(\"Task manager already has resources.\"))"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        self.shared \u003d shared"},{"line_number":158,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cfw%3D","line":155,"updated":"2014-02-06 21:23:59.000000000","message":"should be an IronicException","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a76371194437c5a94a31e40e3130c69eec481821","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        \"\"\""},{"line_number":153,"context_line":"        # Do not allow multiple acquire calls."},{"line_number":154,"context_line":"        if self.resources:"},{"line_number":155,"context_line":"            raise Exception(_(\"Task manager already has resources.\"))"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        self.shared \u003d shared"},{"line_number":158,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cSM%3D","line":155,"in_reply_to":"AAAAUH%2F9cfw%3D","updated":"2014-02-06 21:57:06.000000000","message":"Well, I thought this will be used for debugging purposes only, e.g. we should not have any pathway leading to this exception in production code. But yea, I know how it is :) I will fix","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"ad7d5495ce4f2baeffb817ffa5e8de19379d4748","unresolved":false,"context_lines":[{"line_number":171,"context_line":"        \"\"\"Release all the resources acquired for this TaskManager.\"\"\""},{"line_number":172,"context_line":"        # Do not allow multiple release calls."},{"line_number":173,"context_line":"        if not self.resources:"},{"line_number":174,"context_line":"            raise Exception(_(\"Task manager doesn\u0027t have resources to \""},{"line_number":175,"context_line":"                              \"release.\"))"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"        node_ids \u003d [r.id for r in self.resources]"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F9cfg%3D","line":174,"updated":"2014-02-06 21:23:59.000000000","message":"ditto","commit_id":"a40161cff122eac514a67aedc4b81315e95411c7"}],"ironic/conductor/utils.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"7ca2f3c95b65474ba2ffcc693cc95e54c51f8aba","unresolved":false,"context_lines":[{"line_number":128,"context_line":"    if _validate_node_power_action(task, node, state):"},{"line_number":129,"context_line":"        future \u003d thread_utils.do_in_background("},{"line_number":130,"context_line":"            _perform_node_power_action, task, node, state)"},{"line_number":131,"context_line":"        return future"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BdYs%3D","line":131,"updated":"2014-01-25 22:27:24.000000000","message":"To decrease complexity in manager code we can keep interface consistent like:\n\n   if _validate_node_power_action(task, node, state):\n           return thread_utils.do_in_background(\n                          _perform_node_power_action, task, node, state)\n   # Submit dummy task and return future to keep interface consistent\n   return thread_utils.do_in_background(lambda: None)","commit_id":"4a0a707ee5d66215007985a9ced8794aeb68ee2f"}],"ironic/conductor/worker_pool.py":[{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b3a14f89dae44f7831dcdb26e8deafe0606df74b","unresolved":false,"context_lines":[{"line_number":27,"context_line":"@lockutils.synchronized(WORKER_POOL_LOCK, \u0027ironic-\u0027)"},{"line_number":28,"context_line":"def get_worker_pool():"},{"line_number":29,"context_line":"    \"\"\"GreenPool of background workers for performing tasks async.\"\"\""},{"line_number":30,"context_line":"    global _worker_pool"},{"line_number":31,"context_line":"    # Singleton worker pool."},{"line_number":32,"context_line":"    if _worker_pool is None:"},{"line_number":33,"context_line":"        _worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2Frck%3D","line":30,"updated":"2014-02-11 15:19:05.000000000","message":"eeep, please don\u0027t use globals","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"c388b4b6a189c8a7dbbe44d570f53b5808b5695b","unresolved":false,"context_lines":[{"line_number":27,"context_line":"@lockutils.synchronized(WORKER_POOL_LOCK, \u0027ironic-\u0027)"},{"line_number":28,"context_line":"def get_worker_pool():"},{"line_number":29,"context_line":"    \"\"\"GreenPool of background workers for performing tasks async.\"\"\""},{"line_number":30,"context_line":"    global _worker_pool"},{"line_number":31,"context_line":"    # Singleton worker pool."},{"line_number":32,"context_line":"    if _worker_pool is None:"},{"line_number":33,"context_line":"        _worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2FrB4%3D","line":30,"in_reply_to":"AAAAUX%2F%2Frck%3D","updated":"2014-02-11 15:55:50.000000000","message":"will do","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"8812a5f1e5ab4f48978262bb17725fedf2673796","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    # Singleton worker pool."},{"line_number":32,"context_line":"    if _worker_pool is None:"},{"line_number":33,"context_line":"        _worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"},{"line_number":34,"context_line":"    return _worker_pool"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"@lockutils.synchronized(SPAWN_lOCK, \u0027ironic-\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2F63Q%3D","line":34,"updated":"2014-02-09 20:19:40.000000000","message":"This method probably could be replaced with simple:\n\nworker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b3a14f89dae44f7831dcdb26e8deafe0606df74b","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    return _worker_pool"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"@lockutils.synchronized(SPAWN_lOCK, \u0027ironic-\u0027)"},{"line_number":38,"context_line":"def spawn(func, *args, **kwargs):"},{"line_number":39,"context_line":"    \"\"\"Create a greenthread to run func(*args, **kwargs)."},{"line_number":40,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2FrZo%3D","line":37,"updated":"2014-02-11 15:19:05.000000000","message":"why do you have lockutils around spawn? isn\u0027t GreenPool.spawn() already thread safe?","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"c388b4b6a189c8a7dbbe44d570f53b5808b5695b","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    return _worker_pool"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"@lockutils.synchronized(SPAWN_lOCK, \u0027ironic-\u0027)"},{"line_number":38,"context_line":"def spawn(func, *args, **kwargs):"},{"line_number":39,"context_line":"    \"\"\"Create a greenthread to run func(*args, **kwargs)."},{"line_number":40,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2FrB8%3D","line":37,"in_reply_to":"AAAAUX%2F%2FrZo%3D","updated":"2014-02-11 15:55:50.000000000","message":"This is because of the code inside. There are two steps:\n\n1. if pool.free():\n\n2. pool.spawn(func, *args, **kwargs)\n\nIf we don\u0027t lock this method, there might be another thread spawning the last worker from the pool in-between 1 and 2. And in this case we\u0027ll get the AMQP thread waiting for blocked spawn","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":6623,"name":"Roman Prykhodchenko","email":"me@romcheg.me","username":"prykhodchenko"},"change_message_id":"096ad244718957151c36055437d92a6a58e6d49f","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    if pool.free():"},{"line_number":50,"context_line":"        return pool.spawn(func, *args, **kwargs)"},{"line_number":51,"context_line":"    else:"},{"line_number":52,"context_line":"        raise exception.NoFreeConductorWorker()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9WBs%3D","line":52,"updated":"2014-02-11 13:00:04.000000000","message":"I think this means that performing certain long-running operations extensively, e.g., powering on a lot of nodes, might lead to a DOS.","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"9e3843e14001bc34acb2e66b14a4b981fc522ea5","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    if pool.free():"},{"line_number":50,"context_line":"        return pool.spawn(func, *args, **kwargs)"},{"line_number":51,"context_line":"    else:"},{"line_number":52,"context_line":"        raise exception.NoFreeConductorWorker()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2Frt8%3D","line":52,"in_reply_to":"AAAAUH%2F9WBs%3D","updated":"2014-02-11 14:42:54.000000000","message":"the desired behavior is to restrict the number of simultaneously running background tasks on the conductor. If we don\u0027t raise the exception here, pool.spawn method will block while pool have no free workers. This block will live inside the greenthread generated by AMQP lib. And we\u0027ll get troubles - refer to manager.py file in previous revision - I described it there. So desired behavior is to let user know that there are no free workers and he should try later.","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"c388b4b6a189c8a7dbbe44d570f53b5808b5695b","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    if pool.free():"},{"line_number":50,"context_line":"        return pool.spawn(func, *args, **kwargs)"},{"line_number":51,"context_line":"    else:"},{"line_number":52,"context_line":"        raise exception.NoFreeConductorWorker()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2FrJ4%3D","line":52,"in_reply_to":"AAAAUX%2F%2FrXQ%3D","updated":"2014-02-11 15:55:50.000000000","message":"Yup, I forgot to consider CONF.rpc_thread_pool_size for AMQP greenthreads. I agree with the comment\n\nAnother point is that with this patch we actually doubling possible IO load. F.e. some IO may be issued from inside of AMQP thread, and other from our workers. Number of possible threads requiring IO now is CONF.rpc_thread_pool_size * 2, because each AMQP thread could spawn IO worker, return, and then get another request with sync IO. To solve this we need either:\n\n1. To make CONF.rpc_thread_pool_size default 30 instead of 60\n\n2. To make sure all the IO is performed inside background workers (which makes sense, IO is often long running - perfect candidate to move to background)\n\n3. Ignore this issue :-P\n\nWhat do you think guys?","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"b3a14f89dae44f7831dcdb26e8deafe0606df74b","unresolved":false,"context_lines":[{"line_number":49,"context_line":"    if pool.free():"},{"line_number":50,"context_line":"        return pool.spawn(func, *args, **kwargs)"},{"line_number":51,"context_line":"    else:"},{"line_number":52,"context_line":"        raise exception.NoFreeConductorWorker()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUX%2F%2FrXQ%3D","line":52,"in_reply_to":"AAAAUX%2F%2Frt8%3D","updated":"2014-02-11 15:19:05.000000000","message":"So, there are already a limited # of greenthreads to which AMQP can hand messages -- CONF.rpc_thread_pool_size of them, to be specific.\n\nIf we don\u0027t raise here, if instead it just blocks, then we have the potential for all greenthreads in the RPC pool to block. But we have this already -- all greenthreads might become busy doing IO, or waiting on some operation, etc.\n\nI don\u0027t see how this is introducing any new DOS vector. It is actually allowing the user to know that things are busy, rather than just blocking and timing out.","commit_id":"3076bbe1ea8fafc91b448015029cb86f417f9320"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"77a4f282b1c871aa4f00db179920a5a183b2decd","unresolved":false,"context_lines":[{"line_number":21,"context_line":"SPAWN_lOCK \u003d \"conductor_worker_spawn\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"_worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"},{"line_number":25,"context_line":"\"\"\"Singleton GreenPool of background workers for performing tasks async.\"\"\""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUX%2F%2FcQU%3D","line":24,"updated":"2014-02-13 07:46:51.000000000","message":"This is getting instantiated at parse time, which is before oslo.config has loaded config files, so it will ignore settings of CONF.rpc_thread_pool_size.\n\nI think the worker_pool should be created within ConductorManager.start(), and spawn_worker should be a private method of ConductorManager.","commit_id":"1b8fbe292d1ae71df3091620809277b12a80b84f"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"761cb5554186a8dee91770357ae310ccad5b2ffc","unresolved":false,"context_lines":[{"line_number":21,"context_line":"SPAWN_lOCK \u003d \"conductor_worker_spawn\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"_worker_pool \u003d greenpool.GreenPool(size\u003dCONF.rpc_thread_pool_size)"},{"line_number":25,"context_line":"\"\"\"Singleton GreenPool of background workers for performing tasks async.\"\"\""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUX%2F%2Fav0%3D","line":24,"in_reply_to":"AAAAUX%2F%2FcQU%3D","updated":"2014-02-13 12:36:32.000000000","message":"Nice catch! During my testing I replaced CONF.rpc_thread_pool_size with hardcoded 3 and therefore missed it. Did not know this about oslo.config, thanks!\n\nHmm, then these places probably broken too..\n\nhttps://github.com/openstack/ironic/blob/master/ironic/api/config.py#L40\n\nhttps://github.com/openstack/ironic/blob/master/ironic/common/hash_ring.py#L49\n\nhttps://github.com/openstack/ironic/blob/master/ironic/conductor/manager.py#L350\n\nhttps://github.com/openstack/ironic/blob/master/ironic/conductor/manager.py#L355\n\nI filled a bug for this https://bugs.launchpad.net/ironic/+bug/1279774","commit_id":"1b8fbe292d1ae71df3091620809277b12a80b84f"}],"ironic/tests/conductor/test_conductor_utils.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"603f6c74f736fa7790d745fd112fddf9c0cb7939","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            self.assertEqual(node[\u0027target_power_state\u0027], None)"},{"line_number":121,"context_line":"            self.assertEqual(node[\u0027last_error\u0027], None)"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"    def test_change_node_power_state_already_locked(self):"},{"line_number":124,"context_line":"        \"\"\"Test if an exception is thrown when applying an exclusive"},{"line_number":125,"context_line":"        lock to the node failed."},{"line_number":126,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2Bd9Q%3D","side":"PARENT","line":123,"updated":"2014-01-25 16:37:10.000000000","message":"This test is actually testing lock mechanism which is manager\u0027s responsibility. Therefore it has been moved to the test_manager.py","commit_id":"4d3abebada84b2616e30c5a4619b424d2b829c17"}]}