)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b9ba5fecc7c8f9e4a643d4b30b1bdcb2f558c560","unresolved":false,"context_lines":[{"line_number":14,"context_line":"from \u0027shelved_offloaded\u0027 until some time after the VM has been"},{"line_number":15,"context_line":"\"unshelved\"."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"\u0027d969169\u0027 introduced a change that allowed for deallocation of resources"},{"line_number":18,"context_line":"when they were set to the \u0027shelved_offloaded\u0027 state. However, the"},{"line_number":19,"context_line":"resource (de)allocation code path assumes any VM with a state of"},{"line_number":20,"context_line":"\u0027shelved_offloaded\u0027 should have resources deallocated from it, rather"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9abb7d3a_a0014d08","line":17,"range":{"start_line":17,"start_character":1,"end_line":17,"end_character":8},"updated":"2016-06-01 09:39:11.000000000","message":"FYI (nit): If you write \u0027change \u003cchange id\u003e\u0027 here instead, gerrit will automatically turn it into an appropriate hyperlink, which is really handy as a reviewer. Unlike commit, it also remains relevant across stable backports.\n\nAppears to be the wrong reference, though? This looks like it just fixes a couple of nits in tests.","commit_id":"cb53935e6fbb80b84c95466cea54ede4fcb436e8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"28024e9ec420136c3b7bd8ea57d6047582b088bc","unresolved":false,"context_lines":[{"line_number":14,"context_line":"from \u0027shelved_offloaded\u0027 until some time after the VM has been"},{"line_number":15,"context_line":"\"unshelved\"."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"\u0027d969169\u0027 introduced a change that allowed for deallocation of resources"},{"line_number":18,"context_line":"when they were set to the \u0027shelved_offloaded\u0027 state. However, the"},{"line_number":19,"context_line":"resource (de)allocation code path assumes any VM with a state of"},{"line_number":20,"context_line":"\u0027shelved_offloaded\u0027 should have resources deallocated from it, rather"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9abb7d3a_c66d84cd","line":17,"range":{"start_line":17,"start_character":1,"end_line":17,"end_character":8},"in_reply_to":"9abb7d3a_a0014d08","updated":"2016-06-01 10:37:52.000000000","message":"Done","commit_id":"cb53935e6fbb80b84c95466cea54ede4fcb436e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"95755af4414f9f340ebc93422edb057297c44e07","unresolved":false,"context_lines":[{"line_number":14,"context_line":"from \u0027shelved_offloaded\u0027 until some time after the VM has been"},{"line_number":15,"context_line":"\"unshelved\"."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change 323269 introduced a change that allowed for deallocation of"},{"line_number":18,"context_line":"resources when they were set to the \u0027shelved_offloaded\u0027 state. However,"},{"line_number":19,"context_line":"the resource (de)allocation code path assumes any VM with a state of"},{"line_number":20,"context_line":"\u0027shelved_offloaded\u0027 should have resources deallocated from it, rather"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9abb7d3a_9192944a","line":17,"updated":"2016-06-01 11:34:18.000000000","message":"That\u0027s the review id of this change :)\n\nI mean the change id of the change you\u0027re referring to. i.e. This is change Ie2e7b91937fc3d61bb1197fffc3549bebc65e8aa . Notice how gerrit turned it into a hyperlink to a query for this change id, even in the comment. In this case it comes straight here, but if the change had been backported, it would show all versions of it on all branches.","commit_id":"d86ae29e1775fafc6c5b46acb67417ce102e485b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bd01e18a62aac8d9ebc360c28c387644b06cad64","unresolved":false,"context_lines":[{"line_number":14,"context_line":"from \u0027shelved_offloaded\u0027 until some time after the VM has been"},{"line_number":15,"context_line":"\"unshelved\"."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change 323269 introduced a change that allowed for deallocation of"},{"line_number":18,"context_line":"resources when they were set to the \u0027shelved_offloaded\u0027 state. However,"},{"line_number":19,"context_line":"the resource (de)allocation code path assumes any VM with a state of"},{"line_number":20,"context_line":"\u0027shelved_offloaded\u0027 should have resources deallocated from it, rather"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7aa08908_03bd6bd7","line":17,"in_reply_to":"9abb7d3a_9192944a","updated":"2016-06-21 19:23:01.000000000","message":"I must have been half asleep when I did that. What you suggest is what I\u0027d intended :)","commit_id":"d86ae29e1775fafc6c5b46acb67417ce102e485b"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"7d1c06d0929618710b256dad5288240199bc2036","unresolved":false,"context_lines":[{"line_number":22,"context_line":"path is executed, resources are incorrectly deallocated from the"},{"line_number":23,"context_line":"instance twice."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"Enhance the aformentioned check to account for task state in addition to"},{"line_number":26,"context_line":"VM state. This ensures a VM that\u0027s still in \u0027shelved_offloaded\u0027 state,"},{"line_number":27,"context_line":"but is in fact being unshelved, does not trigger deallocation."},{"line_number":28,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3aaa91ec_7576ba3f","line":25,"updated":"2016-06-21 17:03:33.000000000","message":"s/aformentioned/aforementioned/","commit_id":"7c5d9dc96c863fc09246bde13ad553be157acaf1"}],"nova/compute/resource_tracker.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b9ba5fecc7c8f9e4a643d4b30b1bdcb2f558c560","unresolved":false,"context_lines":[{"line_number":812,"context_line":"        is_new_instance \u003d uuid not in self.tracked_instances"},{"line_number":813,"context_line":"        is_removed_instance \u003d is_removed or ("},{"line_number":814,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL and"},{"line_number":815,"context_line":"            instance[\u0027task_state\u0027] !\u003d task_states.SPAWNING)"},{"line_number":816,"context_line":""},{"line_number":817,"context_line":"        if is_new_instance:"},{"line_number":818,"context_line":"            self.tracked_instances[uuid] \u003d obj_base.obj_to_primitive(instance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_1db99b15","line":815,"updated":"2016-06-01 09:39:11.000000000","message":"This represents a new tight coupling between this code and \u0027something else\u0027, presumably compute manager. This is a wart, but as discussed on IRC we\u0027ll live with it.\n\nHowever, could you please put a big comment here explaining exactly why SPAWNING, and where it comes from? Conditions under which we might remove the tight coupling would also be appreciated.\n\nIn fact (and I\u0027m entirely expecting to have read this wrong, btw, but it\u0027s not clear!) it looks to me like this will be called by _build_and_run_instance *before* task_state is set to SPAWNING. We do with rt.instance_claim(), which calls _update_update_from_instance(), and we only set task_state to SPAWNING after that. Wouldn\u0027t this still suffer the same issue?\n\nAs I say I\u0027ve probably misread it, but given that this is tricky a comment here would be greatly appreciated.","commit_id":"cb53935e6fbb80b84c95466cea54ede4fcb436e8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"28024e9ec420136c3b7bd8ea57d6047582b088bc","unresolved":false,"context_lines":[{"line_number":812,"context_line":"        is_new_instance \u003d uuid not in self.tracked_instances"},{"line_number":813,"context_line":"        is_removed_instance \u003d is_removed or ("},{"line_number":814,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL and"},{"line_number":815,"context_line":"            instance[\u0027task_state\u0027] !\u003d task_states.SPAWNING)"},{"line_number":816,"context_line":""},{"line_number":817,"context_line":"        if is_new_instance:"},{"line_number":818,"context_line":"            self.tracked_instances[uuid] \u003d obj_base.obj_to_primitive(instance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_abcc89bb","line":815,"in_reply_to":"9abb7d3a_1db99b15","updated":"2016-06-01 10:37:52.000000000","message":"\u003e This represents a new tight coupling between this code and \u0027something else\u0027, presumably compute manager. This is a wart, but as discussed on IRC we\u0027ll live with it.\n\nThe whole code needs to be cleaned up at some point, but, as you say, that\u0027s another discussion.\n\n \u003e In fact (and I\u0027m entirely expecting to have read this wrong, btw, but it\u0027s not clear!) it looks to me like this will be called by _build_and_run_instance *before* task_state is set to SPAWNING. We do with rt.instance_claim(), which calls _update_update_from_instance(), and we only set task_state to SPAWNING after that. Wouldn\u0027t this still suffer the same issue?\n\nI don\u0027t think so. You should be looking at \u0027_unshelve_instance\u0027, rather than \u0027_update_update_from_instance\u0027. In this function, setting the task state is the first thing done after we notify users about entering a new state. I will add a comment explaining this, however.","commit_id":"cb53935e6fbb80b84c95466cea54ede4fcb436e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a303588a89f4624bb189e5eb71178e6747552a97","unresolved":false,"context_lines":[{"line_number":812,"context_line":"        is_new_instance \u003d uuid not in self.tracked_instances"},{"line_number":813,"context_line":"        is_removed_instance \u003d is_removed or ("},{"line_number":814,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL and"},{"line_number":815,"context_line":"            instance[\u0027task_state\u0027] !\u003d task_states.SPAWNING)"},{"line_number":816,"context_line":""},{"line_number":817,"context_line":"        if is_new_instance:"},{"line_number":818,"context_line":"            self.tracked_instances[uuid] \u003d obj_base.obj_to_primitive(instance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_ccd003cc","line":815,"in_reply_to":"9abb7d3a_abcc89bb","updated":"2016-06-01 11:22:40.000000000","message":"Yes, of course. Knew I\u0027d have missed something. Thanks for the comment.","commit_id":"cb53935e6fbb80b84c95466cea54ede4fcb436e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"95755af4414f9f340ebc93422edb057297c44e07","unresolved":false,"context_lines":[{"line_number":820,"context_line":"        # unshelved (readded)."},{"line_number":821,"context_line":"        is_removed_instance \u003d is_removed or ("},{"line_number":822,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL and"},{"line_number":823,"context_line":"            instance[\u0027task_state\u0027] !\u003d task_states.SPAWNING)"},{"line_number":824,"context_line":""},{"line_number":825,"context_line":"        if is_new_instance:"},{"line_number":826,"context_line":"            self.tracked_instances[uuid] \u003d obj_base.obj_to_primitive(instance)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_c011e705","line":823,"updated":"2016-06-01 11:34:18.000000000","message":"I\u0027ve been looking at this in more detail, and I think there\u0027s a much cleaner way to do this. This code already uses the tracked_instances dict to defend against double removal. For example, in update_usage:\n\n  # don\u0027t update usage for this instance unless it submitted a resource\n  # claim first:\n  if uuid in self.tracked_instances:\n    ...\n\nThis could just be:\n\n  is_removed_instance \u003d not is_new_instance and (is_removed or\n    instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL)\n\ni.e. Don\u0027t deallocate resources unless they\u0027re already being tracked, same as update_usage. Assuming this is logically correct (and please, please, please check carefully), I vastly prefer this from a code maintenance pov because there\u0027s no additional tight coupling outside this module.","commit_id":"d86ae29e1775fafc6c5b46acb67417ce102e485b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bd01e18a62aac8d9ebc360c28c387644b06cad64","unresolved":false,"context_lines":[{"line_number":820,"context_line":"        # unshelved (readded)."},{"line_number":821,"context_line":"        is_removed_instance \u003d is_removed or ("},{"line_number":822,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL and"},{"line_number":823,"context_line":"            instance[\u0027task_state\u0027] !\u003d task_states.SPAWNING)"},{"line_number":824,"context_line":""},{"line_number":825,"context_line":"        if is_new_instance:"},{"line_number":826,"context_line":"            self.tracked_instances[uuid] \u003d obj_base.obj_to_primitive(instance)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3aaa91ec_72bed000","line":823,"in_reply_to":"9abb7d3a_c011e705","updated":"2016-06-21 19:23:01.000000000","message":"\u003e i.e. Don\u0027t deallocate resources unless they\u0027re already being tracked, same as update_usage. Assuming this is logically correct (and please, please, please check carefully), I vastly prefer this from a code maintenance pov because there\u0027s no additional tight coupling outside this module.\n\nI checked this out, and it seems to work as expected and makes sense to me. Fixed.","commit_id":"d86ae29e1775fafc6c5b46acb67417ce102e485b"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"7d1c06d0929618710b256dad5288240199bc2036","unresolved":false,"context_lines":[{"line_number":843,"context_line":"        \"\"\"Update usage for a single instance.\"\"\""},{"line_number":844,"context_line":""},{"line_number":845,"context_line":"        uuid \u003d instance[\u0027uuid\u0027]"},{"line_number":846,"context_line":"        is_new_instance \u003d uuid not in self.tracked_instances"},{"line_number":847,"context_line":"        is_removed_instance \u003d not is_new_instance and (is_removed or"},{"line_number":848,"context_line":"            instance[\u0027vm_state\u0027] in vm_states.ALLOW_RESOURCE_REMOVAL)"},{"line_number":849,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3aaa91ec_5573be4d","line":846,"updated":"2016-06-21 17:03:33.000000000","message":"At a minimum here, we\u0027d want a comment indicating the above will be True for instances that are in the process of unshelving.\n\nPerhaps something like this?\n\n # NOTE(sfinucan): Both brand new instances as well as instances that are being\n #                 unshelved will have is_new_instance \u003d\u003d True","commit_id":"7c5d9dc96c863fc09246bde13ad553be157acaf1"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"7d1c06d0929618710b256dad5288240199bc2036","unresolved":false,"context_lines":[{"line_number":855,"context_line":"            self.tracked_instances.pop(uuid)"},{"line_number":856,"context_line":"            sign \u003d -1"},{"line_number":857,"context_line":""},{"line_number":858,"context_line":"        self.stats.update_stats_for_instance(instance, is_removed_instance)"},{"line_number":859,"context_line":"        self.compute_node.stats \u003d copy.deepcopy(self.stats)"},{"line_number":860,"context_line":""},{"line_number":861,"context_line":"        # if it\u0027s a new or deleted instance:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3aaa91ec_b58e6225","line":858,"updated":"2016-06-21 17:03:33.000000000","message":"Please add a unit test that covers the new conditional path.","commit_id":"7c5d9dc96c863fc09246bde13ad553be157acaf1"}]}