)]}'
{"nova/compute/api.py":[{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"574147f498fbfe01c0dd02891b90a536a3aef895","unresolved":false,"context_lines":[{"line_number":1729,"context_line":"        else:"},{"line_number":1730,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1731,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1732,"context_line":"        if instance.info_cache is not None:"},{"line_number":1733,"context_line":"            instance.info_cache.delete()"},{"line_number":1734,"context_line":"        else:"},{"line_number":1735,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_18bdddb6","side":"PARENT","line":1732,"updated":"2015-08-06 05:24:34.000000000","message":"Is this needed to avoid the race? If not, i suggest to do this in a separate patch. Because Nova\u0027s cores don\u0027t accept unrelated changes. Especially in this patch, which had been reverted once.","commit_id":"19f2bba4fe3534a74e48f787bdade0263b96ac4c"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"b9223666717b2ba646837ee3833a37c54f78cd4c","unresolved":false,"context_lines":[{"line_number":1729,"context_line":"        else:"},{"line_number":1730,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1731,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1732,"context_line":"        if instance.info_cache is not None:"},{"line_number":1733,"context_line":"            instance.info_cache.delete()"},{"line_number":1734,"context_line":"        else:"},{"line_number":1735,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_ad28ed9a","side":"PARENT","line":1732,"in_reply_to":"1a4dcd0f_07cc62b9","updated":"2015-08-07 07:55:38.000000000","message":"I had understood it already. My point was that this relation is not clearly mentioned in the commit message. It says:\n\nRemoved unnecessary call ... because info_cache is deleted by calling instance.destroy...\n\nThis sounds for me as an optimization or code cleaning. But not as a required part of the solution. Core reviewers dislike unrelated changes, and, as i think, will ask you to revert this change.\n\nBut again - it is only my opinion. I don\u0027t insist on change the commit message.","commit_id":"19f2bba4fe3534a74e48f787bdade0263b96ac4c"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"2013a29097697df2eb39c6ac5c3b9a0a52391ce7","unresolved":false,"context_lines":[{"line_number":1729,"context_line":"        else:"},{"line_number":1730,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1731,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1732,"context_line":"        if instance.info_cache is not None:"},{"line_number":1733,"context_line":"            instance.info_cache.delete()"},{"line_number":1734,"context_line":"        else:"},{"line_number":1735,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_3be20e30","side":"PARENT","line":1732,"in_reply_to":"1a4dcd0f_18bdddb6","updated":"2015-08-06 07:50:24.000000000","message":"Yes, This is needed to avoid the race, because if we delete info_cache from here then in case of race condition when ObjectActionError is raised we are calling instance.refresh().\n\ninstance.refresh() calls info_cache.refresh(), which will fail with NotFound exception if we delete it before destroying the instance.","commit_id":"19f2bba4fe3534a74e48f787bdade0263b96ac4c"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"39ed9f291f527d852eb972dae75c0d64ea002cc9","unresolved":false,"context_lines":[{"line_number":1729,"context_line":"        else:"},{"line_number":1730,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1731,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1732,"context_line":"        if instance.info_cache is not None:"},{"line_number":1733,"context_line":"            instance.info_cache.delete()"},{"line_number":1734,"context_line":"        else:"},{"line_number":1735,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_91a8e38d","side":"PARENT","line":1732,"in_reply_to":"1a4dcd0f_3be20e30","updated":"2015-08-06 09:21:00.000000000","message":"I suppose this (prevention of the crash during refresh) should be mentioned in the commit message. But it is just an opinion.","commit_id":"19f2bba4fe3534a74e48f787bdade0263b96ac4c"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"e7915949c12325725211450e55e799dbe23f3ad6","unresolved":false,"context_lines":[{"line_number":1729,"context_line":"        else:"},{"line_number":1730,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1731,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1732,"context_line":"        if instance.info_cache is not None:"},{"line_number":1733,"context_line":"            instance.info_cache.delete()"},{"line_number":1734,"context_line":"        else:"},{"line_number":1735,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_07cc62b9","side":"PARENT","line":1732,"in_reply_to":"1a4dcd0f_91a8e38d","updated":"2015-08-07 06:43:05.000000000","message":"When we are calling instance.destroy it deletes the info_cache associated with this instance, so there is no need to delete that info_cache explicitly. Previously it was not causing the issues in instance.refresh because instance.refresh was called before calling instance.info_cache.delete from _delete method.","commit_id":"19f2bba4fe3534a74e48f787bdade0263b96ac4c"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"574147f498fbfe01c0dd02891b90a536a3aef895","unresolved":false,"context_lines":[{"line_number":1764,"context_line":"        except exception.ObjectActionError:"},{"line_number":1765,"context_line":"            # If an instance is deleted when it is in building state,"},{"line_number":1766,"context_line":"            # then there is a possibility the host is set None in"},{"line_number":1767,"context_line":"            # which case it will raise ObjectActionError at the time"},{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_c3c78a96","line":1767,"updated":"2015-08-06 05:24:34.000000000","message":"Could you explain for me, which part of instance object model raises the exception? How delete operations can be affected by updates?\n\nAs i understand, in bug 1456771 the stack trace and Hans\u0027es comment are related to crash of asynchronous boot instance process. But what is going on with delete operation?","commit_id":"0d1c2c2cfbe5b68a2ef257c2c0117274282ca71d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"e7915949c12325725211450e55e799dbe23f3ad6","unresolved":false,"context_lines":[{"line_number":1764,"context_line":"        except exception.ObjectActionError:"},{"line_number":1765,"context_line":"            # If an instance is deleted when it is in building state,"},{"line_number":1766,"context_line":"            # then there is a possibility the host is set None in"},{"line_number":1767,"context_line":"            # which case it will raise ObjectActionError at the time"},{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_8db99a17","line":1767,"in_reply_to":"1a4dcd0f_47537d39","updated":"2015-08-07 06:43:05.000000000","message":"Your main concern is about redirection of destroy to a compute manager which you think may cause the original bug 998117 with this patch.\n\nIf you analyse the master code carefully, ObjectActionError exception does not redirects the call directly to the compute manager. Instead it calls the callback function \"_do_delete\" based on \"is_local_delete\" value. When is_local_delete is true master code will call \"_do_delete\" with local\u003dTrue and will not redirect to compute manager.\n\nAs you suggested, if I revert the changes to in _delete method, The original issues associated with this patch will not be fixed. The code which you are asking to revert is the main culprit for the issue, because in current code if instance.destroy() is passed from line #1575 it simply terminates the instance and call returns at line #1581 and does not detach the volume and bdm entries remains there.","commit_id":"0d1c2c2cfbe5b68a2ef257c2c0117274282ca71d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"b9223666717b2ba646837ee3833a37c54f78cd4c","unresolved":false,"context_lines":[{"line_number":1764,"context_line":"        except exception.ObjectActionError:"},{"line_number":1765,"context_line":"            # If an instance is deleted when it is in building state,"},{"line_number":1766,"context_line":"            # then there is a possibility the host is set None in"},{"line_number":1767,"context_line":"            # which case it will raise ObjectActionError at the time"},{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_6adacb18","line":1767,"in_reply_to":"1a4dcd0f_8db99a17","updated":"2015-08-07 07:55:38.000000000","message":"But master code evaluates is_local_delete after instance.refresh. So the flow which is missed in you patch is (line numbers of master code):\n\n- L1575: _delete destroys the instance with empty host locally \n- L1583: instance is refreshed and gets non empty host\n- L1593: is_local_delete is evaluated as False\n- L1614: delete operation is redirected to compute node\n\nReverting something is not what i suggested. I thought it\u0027s possible to catch ObjectActionError in L1588 (your patch numbering) and redirect program flow to L1591. But now i\u0027m not sure on it, because it skips checks in L1577-1580. Is it possible that compute service is down while the instance has just now got host value?\n\nAnyway, the reason of -1 is the missed flow. The suggestion was just a proposition of how to fix it. You fell free to resolve it by other way.","commit_id":"0d1c2c2cfbe5b68a2ef257c2c0117274282ca71d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"39ed9f291f527d852eb972dae75c0d64ea002cc9","unresolved":false,"context_lines":[{"line_number":1764,"context_line":"        except exception.ObjectActionError:"},{"line_number":1765,"context_line":"            # If an instance is deleted when it is in building state,"},{"line_number":1766,"context_line":"            # then there is a possibility the host is set None in"},{"line_number":1767,"context_line":"            # which case it will raise ObjectActionError at the time"},{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_47537d39","line":1767,"in_reply_to":"1a4dcd0f_989c880f","updated":"2015-08-06 09:21:00.000000000","message":"Thank you.\n\nI\u0027ve digged the history and found https://review.openstack.org/#/c/7345/ where the reason of the exception was introduced. And it was introduced to avoid a race for the same api\u0027s method.\n\nPreviously _delete metod just called db.instance_destroy which didn\u0027t check renewal of host field and just delete db records. Apparently it leaded to a problem (probably some resources had stayed in compute node? frankly, i don\u0027t know it), which was fixed by catch the renewal of host field and redirect destroy to a compute manager.\n\nNow you catch the renewal but do not redirect destroy. This leads to potential ability of returnig of original bug 998117. Moreover, the same could be done by removing host condition from db.instance_destroy call in Instance.destroy.\n\nSo i think you have two options:\n\n- restore redirection (by handle the exception in _delete)\n- dig ahead what is wrong with local destroy with host set, and if it is nothing significant, then remove db host condition at all\n\nI\u0027d prefer the first way.","commit_id":"0d1c2c2cfbe5b68a2ef257c2c0117274282ca71d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"2013a29097697df2eb39c6ac5c3b9a0a52391ce7","unresolved":false,"context_lines":[{"line_number":1764,"context_line":"        except exception.ObjectActionError:"},{"line_number":1765,"context_line":"            # If an instance is deleted when it is in building state,"},{"line_number":1766,"context_line":"            # then there is a possibility the host is set None in"},{"line_number":1767,"context_line":"            # which case it will raise ObjectActionError at the time"},{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_989c880f","line":1767,"in_reply_to":"1a4dcd0f_c3c78a96","updated":"2015-08-06 07:50:24.000000000","message":"Feodor,\n\nhttps://github.com/openstack/nova/blob/master/nova/objects/instance.py#L532 raises this exception in a race condition when calling db.instance_destroy().\n\nIn bug 1456771, Hans has only mentioned about the compute logs but when I debug into the nova api logs, I found the main issue was in delete api with ObjectActionError.\n\nIn my original patch also I had only made changes in delete api, so boot instance process could not cause the race here.\n\nThe same exception handling is there in current master code at line #1582. I have just reformatted the code to fix the associated issues.","commit_id":"0d1c2c2cfbe5b68a2ef257c2c0117274282ca71d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"497cfa708342e4fae83545666fb919a976d7472a","unresolved":false,"context_lines":[{"line_number":1581,"context_line":"                except exception.ComputeHostNotFound:"},{"line_number":1582,"context_line":"                    is_local_delete \u003d True"},{"line_number":1583,"context_line":""},{"line_number":1584,"context_line":"            if is_local_delete:"},{"line_number":1585,"context_line":"                # If instance is in shelved_offloaded state or compute node"},{"line_number":1586,"context_line":"                # isn\u0027t up or is unknown i.e. instance.host is None, delete"},{"line_number":1587,"context_line":"                # instance from db and clean bdms info and network info."}],"source_content_type":"text/x-python","patch_set":3,"id":"fa1b9901_1ee5edda","line":1584,"updated":"2015-08-19 17:36:14.000000000","message":"i expected something like that:\n\n while True:\n     if not \u003cexpression from L1572-3):\n         try:\n             service \u003d ...\n             if self.servicegroup_api.service_is_up(service):\n                 break\n         except exception.ComputeHostNotFound:\n             pass\n     try:\n         self._local_delete(...)\n     except exception.ObjectActionError:\n         # this block handles a case when initially instance host\n         # is None but during deletion gives a value by an another thread\n         # in that case deletion should be redirected to a compute node\n\n         # verify that host was None, otherwise the reason of\n         # the exception differs\n         if not instance.host:\n             raise\n         instance.refresh()\n         # verify that host is now not None, otherwise the reason of\n         # the exception differs\n         if not instance.host:\n             raise\n     else:\n         quotas.commit()\n         return\n \u003cbelow block with redirection\u003e","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"c68948e858a92c152c0ca87c7cb91044283e78dd","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_f07a4a76","line":1771,"updated":"2015-08-07 08:02:30.000000000","message":"Lets continue here. So, your patch misses the flow of master code:\n\n- L1575: _delete destroys the instance with empty host locally\n- L1583: instance is refreshed and gets non empty host\n- L1593: is_local_delete is evaluated as False\n- L1614: delete operation is redirected to compute node","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"830b03127f9a0112f39f3758b5c7d802a7842b75","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa1b9901_8f3e25b7","line":1771,"in_reply_to":"1a4dcd0f_2989d6ca","updated":"2015-08-17 12:48:25.000000000","message":"I have addressed the above scenario and uploaded a new patch set for review. Thanks!","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"c78a2d0dfea8df4d7aede97cc4c591d7a59b2bdc","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_cbdcef1a","line":1771,"in_reply_to":"1a4dcd0f_2d8250c9","updated":"2015-08-10 10:38:36.000000000","message":"Hi Feodor,\n\nI came up with with a scenario which validates the changes against ObjectActionError exception and proves that ObjectActionError doesn\u0027t meant to be redirected to the compute manager on master code as well. The changes works exactly similar to the current master while ObjectActionError is raised.\n\n* Create an instance.\n* Shelve instance.\n* Delete snapshot taken during shelve instance.\n* Unshelve instance. (Instance goes in to error state and host is set to None)\n* Raise ConstraintNotMet exception manually from db.instance_destroy method to simulate the race condition.\n* Delete instance and you\u0027ll find delete api is not redirected to compute manager after ObjectActionError is caught and deletes the instance locally from _local_delete.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"28ef7bd70bd8fbca55cf9982be0f2f5146ea13eb","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_7ae6c2e9","line":1771,"in_reply_to":"1a4dcd0f_2d8250c9","updated":"2015-08-10 10:54:30.000000000","message":"So i see two consistent solutions:\n\n- be brave: remove and do not handle host validation;\n- be ware: continue to handle host validation and to redirect the deletion to a compute node.\n\nI cannot get a piont of a trade-off: continue to validate host changing, but do not handle it respectively.\n\nto be continued...","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"307344525ab51ee3da209e121ffe43e866b51624","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_f3c3dd36","line":1771,"in_reply_to":"1a4dcd0f_30d13bfc","updated":"2015-08-10 11:49:20.000000000","message":"PS. Perhaps i missed a some new case in which we can not just locally delete the instance regardless that its host has been changed. But must reload it and delete locally again. Though i don\u0027t see such case.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06272435d46deec22eff50217b9d3c1f99c2c2c8","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_30d13bfc","line":1771,"in_reply_to":"1a4dcd0f_7ae6c2e9","updated":"2015-08-10 11:32:29.000000000","message":"Though i\u0027d not except that some core reviewrs can approve you solution. I saw enough odd decisions to think so. Therefore i suggest to ask a core reviewr to make a decision. Apparently it\u0027s better than to change now the review to any side (to make me happy), but then revert changes if the core revier whant exactly your (not consistent for me) solution.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06272435d46deec22eff50217b9d3c1f99c2c2c8","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_9d7148e3","line":1771,"in_reply_to":"1a4dcd0f_9aecee90","updated":"2015-08-10 11:32:29.000000000","message":"And about your scenario again.\n\nWhat is the purpose of ConstraintNotMet exception it that case? Why not to eliminate this exception and just destroy the instance at the first attempt?","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"cfce87412e958b0a7e42f133d6247bb1c2eff664","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_9aecee90","line":1771,"in_reply_to":"1a4dcd0f_cbdcef1a","updated":"2015-08-10 11:01:07.000000000","message":"About your scenario.\n\nIt\u0027s correct that it\u0027s not redirected. SHELVED_OFFLOADED state is specially processed when is_local_delete has being evaluated.\n\nBut the difference is master code redirects the call under some conditions, but your patch doesn\u0027t redirect it in any case.\n\nWhen i tell about redirect i mean the redirection under appropriate conditions.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"1014f298c5fad35e889c19d7faf128adbd9ba7e4","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_2e023c3f","line":1771,"in_reply_to":"1a4dcd0f_ce787892","updated":"2015-08-10 12:17:09.000000000","message":"As we can see from line #1570 on master ObjectActionError exception may only occur when instance.host is set to None and is not SHELVED_OFFLOADED. So when host is not set, why we\u0027ll need to redirect to the compute node in that case. In that case it will always delete the instance locally whether ObjectActionError exception is raised or not.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"c5ce404ed3b23ab105ff876b10aa579b1ed0bdb6","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_2989d6ca","line":1771,"in_reply_to":"1a4dcd0f_ce787892","updated":"2015-08-10 12:40:58.000000000","message":"I mean a case when we must catch host field changing to re-run deletion. If ConstantNotMet is eliminated, will you case have a reason to return it back (to reload the instance and to repeat its destroing)?\n\nThe scenario:\n\n- Create an instance\n- Pause manager in the beginning of manager\u0027s _build_and_run_instance (before resource tracker\u0027s instance_claim call)\n- Delete the instance\n- Pause api in master L1572\n- Unpause manager and run it until return out of _build_and_run_instance, then pause it again\n- Unpause api. It\u0027ll fail at instance.destroy, evaluate is_local_delete to False, and call manager.\n\nI don\u0027t propose to remove ObjectActionError at all. See https://review.openstack.org/#/c/210972/\n\nI agree that cleanups should be uploaded as separate patches. But if we decide to remove the host validation, your original (Ic630ae7d026a9697afec46ac9ea40aea0f5b5ffb) will be suitable, but it must be commited after removing of host validation.\n\nAnd again - i think you shold ask a core reviewer.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"05878bcddda80d7a1c5ec8f4d5110d2e3a9b590e","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_2d8250c9","line":1771,"in_reply_to":"1a4dcd0f_d7457bb3","updated":"2015-08-10 07:53:57.000000000","message":"If it\u0027s possible to do not validate empty host, you\u0027ll not need to change _local_destroy at all.\n\nBut i suppose some core reviewers will not approve elimination of the validatation, because:\n\n- what a race condition was actual for bug 998117 is not exactly known now;\n- races are ones of most hard bugs to be fixed;\n- so it\u0027s probably safer (has a lower price) to leave this check.\n\nBut the same safety reason leads us to keep the redirection if the host validation fails. Because the redirection is a part of a solution which eliminate the race.\n\nto be continued...","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"d62781c011393b412ea76e90aeccf4428c525869","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_d7457bb3","line":1771,"in_reply_to":"1a4dcd0f_f07a4a76","updated":"2015-08-10 06:37:27.000000000","message":"About bug 998117.\n\nI guess the race there was that instance parts (like network resources) stayed on compute node after api node had deleted the instance. I\u0027ve observed that version and found that manager\u0027s run_instance (now named build_and_run_instance) didn\u0027t remove anything on crash. But currently it cleans up all.\n\nSo at the first glance currently the reason of that race is obsolete. I\u0027m cheking by I2a54ea929d2506872ba47d081ad4e4750f04daf5 if it\u0027s possible not to validate empty host.\n\nto be continued...","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"b8bde0a46093748e69eb4434e2d25abfce9e29ef","unresolved":false,"context_lines":[{"line_number":1768,"context_line":"            # of destroying the instance. Give one more chance to"},{"line_number":1769,"context_line":"            # destroy instance."},{"line_number":1770,"context_line":"            instance.refresh()"},{"line_number":1771,"context_line":"            instance.destroy()"},{"line_number":1772,"context_line":"        compute_utils.notify_about_instance_usage("},{"line_number":1773,"context_line":"            self.notifier, context, instance, \"%s.end\" % delete_type,"},{"line_number":1774,"context_line":"            system_metadata\u003dsys_meta)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_ce787892","line":1771,"in_reply_to":"1a4dcd0f_f3c3dd36","updated":"2015-08-10 12:02:02.000000000","message":"IIUYC, the case which I have mentioned above is where instance is reloaded from line#1583 and deleted locally from line#1623 onmaster. In case of ConstraintNotMet exception is raised from db.\n\nI could not found any scenario where it is redirected to the compute manager. Could you please help me to point one.\n\nIn case if we need not handle host validation and to remove this ObjectActionError exception at all, IMO we can fix that in a separate patch to keep this patch clean and to keep the changes required to fix the associated bugs only. We should not block this patch if some further cleanup is required which is not related to this issue as the change solve its purpose and does not raise any race conditions now.","commit_id":"6a4bd6792273faba68a4db0567467addff515d3d"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"38f6b3b0b40f1e6f55f189a3a097d02724a3bd9c","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_5b4c6ac7","side":"PARENT","line":1795,"updated":"2015-09-07 17:30:25.000000000","message":"why is it OK to drop this here? I guess it might be if its done in the DB layer anyways.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"9d05ca8283d256e4388b7c20cfd2eb75cf75727a","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba15a1d1_82b462f2","side":"PARENT","line":1795,"in_reply_to":"ba15a1d1_e5ca3d4f","updated":"2015-09-22 13:31:48.000000000","message":"I think there may be a misunderstanding here.  What I\u0027m suggesting is to create a patch with this change, and then have the current patch depend on it.  This should ensure that the race condition is covered because the fix for it will be before the change that can cause it.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"56f251a399c86b8acdb396bdf0d653dbd9da732d","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba15a1d1_e5ca3d4f","side":"PARENT","line":1795,"in_reply_to":"ba15a1d1_ef777ed2","updated":"2015-09-14 17:46:53.000000000","message":"As mentioned in the commit message if we do not make this change instance.refresh raises InstanceInfoCacheNotFound exception in a race condition on this patch.\n\nIf you want to submit a separate patch, we\u0027ll need to submit it as a dependent patch else Jenkins will fail on this patch if race condition occurs.\n\nPlease let me know your suggestions.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"2538981d7d421bcab86f56c11093421c04e6fef8","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_074ce774","side":"PARENT","line":1795,"in_reply_to":"da20952f_0410a558","updated":"2015-09-10 05:24:07.000000000","message":"Feodor, Thanks for the explaination and your help in reviewing this patch.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"88da3dbf1efad9ae0339ce387d5c2f4ac87b6d85","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba15a1d1_ef777ed2","side":"PARENT","line":1795,"in_reply_to":"da20952f_074ce774","updated":"2015-09-14 17:06:27.000000000","message":"I see that it\u0027s related.  But as far as I can tell the change can be done independently of this one without breaking.  If that is incorrect let me know.\n\nI know that both of these changes are needed to fully address the issue, but it can still be done in two patches as far as I can tell.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"e2cf874d381983360c764070982969f4d321ff1a","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_0410a558","side":"PARENT","line":1795,"in_reply_to":"da20952f_49ceb6b8","updated":"2015-09-08 19:37:27.000000000","message":"@Andrew, @John, it\u0027s a related change, see an explanation in comments on PS2. This is also mentioned in the commit message.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"212e0d86b7042283538ca0d5c0d43e6d47bce0cb","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_bbd826f0","side":"PARENT","line":1795,"in_reply_to":"da20952f_5b4c6ac7","updated":"2015-09-07 17:31:15.000000000","message":"ah, yes, it is, probably should be a separate change, but lets ignore that.","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"58fa7b94ec84f7bf85ce8923e09e9d64461baecc","unresolved":false,"context_lines":[{"line_number":1792,"context_line":"        else:"},{"line_number":1793,"context_line":"            LOG.warning(_LW(\"instance\u0027s host %s is down, deleting from \""},{"line_number":1794,"context_line":"                            \"database\"), instance.host, instance\u003dinstance)"},{"line_number":1795,"context_line":"        if instance.info_cache is not None:"},{"line_number":1796,"context_line":"            instance.info_cache.delete()"},{"line_number":1797,"context_line":"        else:"},{"line_number":1798,"context_line":"            # NOTE(yoshimatsu): Avoid AttributeError if instance.info_cache"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_49ceb6b8","side":"PARENT","line":1795,"in_reply_to":"da20952f_bbd826f0","updated":"2015-09-08 19:19:48.000000000","message":"I think this should be in a separate change.  It has a different purpose than the rest of the patch and has no dependence on anything else here.  Is there a reason this can\u0027t be done as a separate patch and have this patch depend on it?","commit_id":"003a2f6fcb3295d15d61825bfe6fa7e07033693a"}]}