)]}'
{"ironic/api/controllers/v1/node.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"97264b6e8f28d54db1204c97e01d4e0eec82db7d","unresolved":false,"context_lines":[{"line_number":104,"context_line":"        if rpc_node.target_power_state is not None:"},{"line_number":105,"context_line":"            msg \u003d (_(\u0027Power operation for node %s is already in progress.\u0027) %"},{"line_number":106,"context_line":"                     rpc_node[\u0027uuid\u0027])"},{"line_number":107,"context_line":"            LOG.exception(msg)"},{"line_number":108,"context_line":"            raise wsme.exc.ClientSideError(msg, status_code\u003d409)"},{"line_number":109,"context_line":"        # Note that there is a race condition. The node state(s) could change"},{"line_number":110,"context_line":"        # by the time the RPC call is made and the TaskManager manager gets a"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9cjI%3D","line":107,"updated":"2013-12-11 13:27:47.000000000","message":"I\u0027m sorry, is this a related change? I see that there are more places to add logging below, would it be better to fix all these stuff in a separate patch?","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"cb2be0e62f64128876a1aad6e0cb0b099a19f820","unresolved":false,"context_lines":[{"line_number":104,"context_line":"        if rpc_node.target_power_state is not None:"},{"line_number":105,"context_line":"            msg \u003d (_(\u0027Power operation for node %s is already in progress.\u0027) %"},{"line_number":106,"context_line":"                     rpc_node[\u0027uuid\u0027])"},{"line_number":107,"context_line":"            LOG.exception(msg)"},{"line_number":108,"context_line":"            raise wsme.exc.ClientSideError(msg, status_code\u003d409)"},{"line_number":109,"context_line":"        # Note that there is a race condition. The node state(s) could change"},{"line_number":110,"context_line":"        # by the time the RPC call is made and the TaskManager manager gets a"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9BRA%3D","line":107,"in_reply_to":"AAAATn%2F9cjI%3D","updated":"2013-12-17 16:44:32.000000000","message":"Agreed, as I was implementing the /provisioning and added the LOG there I thought about adding it to the /power as well, but I agree it\u0027s not related to this change. Will be removed on the next patch set\n\nThanks :)","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"fd3b81eac9237ec00d7a8490123b6c9b2652f6eb","unresolved":false,"context_lines":[{"line_number":143,"context_line":"            LOG.exception(msg)"},{"line_number":144,"context_line":"            raise wsme.exc.ClientSideError(msg, status_code\u003d409)"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9hYA%3D","line":146,"updated":"2013-12-10 16:36:05.000000000","message":"And if rpc_node.provision_state is already ACTIVE? What do you think about undeploy by the same way from ACTIVE to None?","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"cb2be0e62f64128876a1aad6e0cb0b099a19f820","unresolved":false,"context_lines":[{"line_number":143,"context_line":"            LOG.exception(msg)"},{"line_number":144,"context_line":"            raise wsme.exc.ClientSideError(msg, status_code\u003d409)"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9BPY%3D","line":146,"in_reply_to":"AAAATn%2F9hYA%3D","updated":"2013-12-17 16:44:32.000000000","message":"\u003e And if rpc_node.provision_state is already ACTIVE? \n\n+1 to add a sanity check to verify if the current state \u003d\u003d target state :)\n\n\u003e What do you think about undeploy by the same way from ACTIVE to None?\n\nYes, thanks for the suggestion, I think we are going to use NOSTATE (None) to do it yes. \n\nThe idea right now is just to make it easier to extend this method for other things (like  undeploy) as in the moment we don\u0027t support undeploying a node (needs plumbing before coming to the API, e.g do_node_undeploy()).","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"71924c119160ade4c3dbdf3a2fa80ff2efa70e26","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9dxo%3D","line":148,"updated":"2013-12-11 08:26:17.000000000","message":"Lucas, looks like rpcapi.do_node_deploy is not a synchronized method, so how do you think if there is race here when the api is call for same node in parallel?","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"11dccd06a0886a3295b54eb43acae913ebdde7ba","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F89zw%3D","line":148,"in_reply_to":"AAAATn%2F8%2FvE%3D","updated":"2013-12-18 10:19:13.000000000","message":"My bad, you\u0027re totally right, I missed the part that it\u0027s going to return 202 for both requests but only one actually went thru.\n\nI think we could just use the same bug, since the problem seems to be the same.\n\nWill take a look at ur comments, thank you for pointing that out","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"10346d8f7df8baeb644f44d1d42d2020e4349498","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F89t8%3D","line":148,"in_reply_to":"AAAATn%2F89v8%3D","updated":"2013-12-18 10:37:49.000000000","message":"Thanks for the suggestions on that bug, I proposed yet another way to solve it as well.\n\nhttps://bugs.launchpad.net/ironic/+bug/1259910","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"8bc4cd87d18a64d317e6df514daf22dc4e513af5","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F89v8%3D","line":148,"in_reply_to":"AAAATn%2F89zw%3D","updated":"2013-12-18 10:27:40.000000000","message":"+1 for using the same bug. TY","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"8a56b5323b87f12fba53a9d73b30aedad246300c","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F8%2FvE%3D","line":148,"in_reply_to":"AAAATn%2F9BJ0%3D","updated":"2013-12-17 22:28:36.000000000","message":"Lucas, I think it\u0027s still possible to have race here. Please take a look on the similar bug https://bugs.launchpad.net/ironic/+bug/1259910. Since do_node_deploy is async it can silently fail because it couldn\u0027t acquire the node, as you said. As far as I understood it will be silent fail - no exception will be sent from Conductor back to the API. So the client will receive 202 but in truth the request will fail. What do you think? \n\nIf you agree I propose to fill a bug for this as well, or expand those bug, because this problem is wider than the patch. \n\nBTW I wrote my thoughts how to resolve it in comments section of bug 1259910","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"cb2be0e62f64128876a1aad6e0cb0b099a19f820","unresolved":false,"context_lines":[{"line_number":145,"context_line":""},{"line_number":146,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":147,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":148,"context_line":"                                                rpc_node)"},{"line_number":149,"context_line":"        else:"},{"line_number":150,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":151,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9BJ0%3D","line":148,"in_reply_to":"AAAATn%2F9dxo%3D","updated":"2013-12-17 16:44:32.000000000","message":"I don\u0027t think we are going to hit this problem. Even If two requests comes in at the exactly same time and pass that validation (l141) that checks if the target_provision_state is not None the do_node_deploy() function is acquiring the node object with shared\u003dFalse[1], so one of the requests will fail because it couldn\u0027t acquire the node. That method also sets the target_provision_state, making subsequent requests to fail on the API side.\n\n[1] https://github.com/openstack/ironic/blob/master/ironic/conductor/manager.py#L234","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"e561f890cfd8dc04253d071c2ff9a2977a4c68d6","unresolved":false,"context_lines":[{"line_number":159,"context_line":""},{"line_number":160,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":161,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":162,"context_line":"                                                rpc_node)"},{"line_number":163,"context_line":"        else:"},{"line_number":164,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":165,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAT3%2F%2Fwjc%3D","line":162,"updated":"2014-01-02 08:48:53.000000000","message":"Lucas, looks like the do_node_deploy last argument is node_id, not the node object[1]: \n\n[1] https://github.com/openstack/ironic/blob/master/ironic/conductor/manager.py#L222\n\nSo the line should be:\n\npecan.request.rpcapi.do_node_deploy(pecan.request.context,\n                                    rpc_node[\u0027uuid\u0027])","commit_id":"33d48f3426ee95be6b655b2dca6ca1e278a175cf"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"291ce1106880b0ead988bffaf639605d3c25b39b","unresolved":false,"context_lines":[{"line_number":159,"context_line":""},{"line_number":160,"context_line":"        if target \u003d\u003d ir_states.ACTIVE:"},{"line_number":161,"context_line":"            pecan.request.rpcapi.do_node_deploy(pecan.request.context,"},{"line_number":162,"context_line":"                                                rpc_node)"},{"line_number":163,"context_line":"        else:"},{"line_number":164,"context_line":"            msg \u003d _(\"Target provision state \u0027%s\u0027 not supported.\") % target"},{"line_number":165,"context_line":"            LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAT3%2F%2FrYg%3D","line":162,"in_reply_to":"AAAAT3%2F%2Fwjc%3D","updated":"2014-01-03 16:42:11.000000000","message":"Ah great, will fix! Thanks Haomeng","commit_id":"33d48f3426ee95be6b655b2dca6ca1e278a175cf"}],"ironic/tests/api/test_nodes.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"97264b6e8f28d54db1204c97e01d4e0eec82db7d","unresolved":false,"context_lines":[{"line_number":686,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node[\u0027uuid\u0027],"},{"line_number":687,"context_line":"                                 {\u0027target\u0027: states.ACTIVE},"},{"line_number":688,"context_line":"                                 expect_errors\u003dTrue)"},{"line_number":689,"context_line":"        self.assertEqual(ret.status_code, 409)"},{"line_number":690,"context_line":""},{"line_number":691,"context_line":"    def test_provision_state_not_supported(self):"},{"line_number":692,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node[\u0027uuid\u0027],"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9ceU%3D","line":689,"updated":"2013-12-11 13:27:47.000000000","message":"Could you please split this test to three, or maybe add comments for each thing tested here?","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"cb2be0e62f64128876a1aad6e0cb0b099a19f820","unresolved":false,"context_lines":[{"line_number":686,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node[\u0027uuid\u0027],"},{"line_number":687,"context_line":"                                 {\u0027target\u0027: states.ACTIVE},"},{"line_number":688,"context_line":"                                 expect_errors\u003dTrue)"},{"line_number":689,"context_line":"        self.assertEqual(ret.status_code, 409)"},{"line_number":690,"context_line":""},{"line_number":691,"context_line":"    def test_provision_state_not_supported(self):"},{"line_number":692,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node[\u0027uuid\u0027],"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9BI8%3D","line":689,"in_reply_to":"AAAATn%2F9ceU%3D","updated":"2013-12-17 16:44:32.000000000","message":"+1 will make it better :) it was a copy-cat from the test_power_state_in_progress()\n\nThanks!","commit_id":"274eae6efc3e46591b7d07318aa467162d1d3d4c"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"e561f890cfd8dc04253d071c2ff9a2977a4c68d6","unresolved":false,"context_lines":[{"line_number":657,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node.uuid,"},{"line_number":658,"context_line":"                            {\u0027target\u0027: states.ACTIVE})"},{"line_number":659,"context_line":"        self.assertEqual(ret.status_code, 202)"},{"line_number":660,"context_line":"        self.mock_dnd.assert_called_once_with(mock.ANY, mock.ANY)"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def test_provision_state_in_progress(self):"},{"line_number":663,"context_line":"        ndict \u003d dbutils.get_test_node(id\u003d1, uuid\u003dutils.generate_uuid(),"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAT3%2F%2Fwhw%3D","line":660,"updated":"2014-01-02 08:48:53.000000000","message":"Lucas, should we check the node\u0027s provision_state/target_provision_state/last_error to make sure the provision is successfully or not?","commit_id":"33d48f3426ee95be6b655b2dca6ca1e278a175cf"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"0254443114829fb6d2611e1518aa2c717184a93b","unresolved":false,"context_lines":[{"line_number":657,"context_line":"        ret \u003d self.put_json(\u0027/nodes/%s/states/provision\u0027 % self.node.uuid,"},{"line_number":658,"context_line":"                            {\u0027target\u0027: states.ACTIVE})"},{"line_number":659,"context_line":"        self.assertEqual(ret.status_code, 202)"},{"line_number":660,"context_line":"        self.mock_dnd.assert_called_once_with(mock.ANY, mock.ANY)"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def test_provision_state_in_progress(self):"},{"line_number":663,"context_line":"        ndict \u003d dbutils.get_test_node(id\u003d1, uuid\u003dutils.generate_uuid(),"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAT3%2F%2FrQQ%3D","line":660,"in_reply_to":"AAAAT3%2F%2Fwhw%3D","updated":"2014-01-03 17:06:22.000000000","message":"Hi Haomeng, I think that on the API it\u0027s ok to just mock the functions and assert that it\u0027s being called with the right parameters. The tests you mentioned are already being done by the function testing the do_node_deploy() method[1]\n\n[1] https://github.com/openstack/ironic/blob/master/ironic/tests/conductor/test_manager.py#L385-L399\n\nThanks for the suggestion","commit_id":"33d48f3426ee95be6b655b2dca6ca1e278a175cf"}]}