)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Detach volume after deleting instance with no host"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If an instance is booted from a volume, shelved, and goes into an error"},{"line_number":10,"context_line":"state due to some reason. Volume from which instance is booted, remains"},{"line_number":11,"context_line":"in-use state even the instance is deleted because instance has no host"},{"line_number":12,"context_line":"associated with it."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5aef4532_1b9aa063","line":10,"range":{"start_line":10,"start_character":24,"end_line":10,"end_character":25},"updated":"2016-03-08 17:24:55.000000000","message":"the full stop here seems wrong to me, the sentence is not completed yet.","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"44029f46a4fac81aac06abed6004165204e37bc6","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Detach volume after deleting instance with no host"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If an instance is booted from a volume, shelved, and goes into an error"},{"line_number":10,"context_line":"state due to some reason. Volume from which instance is booted, remains"},{"line_number":11,"context_line":"in-use state even the instance is deleted because instance has no host"},{"line_number":12,"context_line":"associated with it."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5aef4532_2e739816","line":10,"range":{"start_line":10,"start_character":24,"end_line":10,"end_character":25},"in_reply_to":"5aef4532_1b9aa063","updated":"2016-03-09 16:37:47.000000000","message":"Done.\n\nThis was the original commit message from the reverted commit.","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"}],"nova/compute/api.py":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":1639,"context_line":"                                     instance.vm_state \u003d\u003d vm_states.ERROR):"},{"line_number":1640,"context_line":"                try:"},{"line_number":1641,"context_line":"                    # If instance is in shelved_offloaded state or"},{"line_number":1642,"context_line":"                    # instance.host is None, then delete instance"},{"line_number":1643,"context_line":"                    # from db and clean bdms info and network info."},{"line_number":1644,"context_line":"                    self._local_delete(context, instance, bdms, delete_type,"},{"line_number":1645,"context_line":"                                       cb)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_db3798ae","line":1642,"range":{"start_line":1642,"start_character":22,"end_line":1642,"end_character":43},"updated":"2016-03-08 17:24:55.000000000","message":"the comment is not correct if instance.host is None AND the vm is in error state.","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":1666,"context_line":"                self._confirm_resize_on_deleting(context, instance)"},{"line_number":1667,"context_line":""},{"line_number":1668,"context_line":"            is_local_delete \u003d True"},{"line_number":1669,"context_line":"            if instance.host and not shelved_offloaded:"},{"line_number":1670,"context_line":"                try:"},{"line_number":1671,"context_line":"                    # Check if compute service is up."},{"line_number":1672,"context_line":"                    service \u003d objects.Service.get_by_compute_host("},{"line_number":1673,"context_line":"                        context.elevated(), instance.host)"},{"line_number":1674,"context_line":"                    is_local_delete \u003d not self.servicegroup_api.service_is_up("},{"line_number":1675,"context_line":"                        service)"},{"line_number":1676,"context_line":"                except exception.ComputeHostNotFound:"},{"line_number":1677,"context_line":"                    pass"},{"line_number":1678,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_9bc9907b","line":1675,"range":{"start_line":1669,"start_character":0,"end_line":1675,"end_character":32},"updated":"2016-03-08 17:24:55.000000000","message":"With this change we introduce a new case for a local_delete, having this code duplication here is a bit confusing. I\u0027d suggest to create a method to set the correctly the is_local_delete and refactor the code here checking the is_local_delete value just once.","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"44029f46a4fac81aac06abed6004165204e37bc6","unresolved":false,"context_lines":[{"line_number":1666,"context_line":"                self._confirm_resize_on_deleting(context, instance)"},{"line_number":1667,"context_line":""},{"line_number":1668,"context_line":"            is_local_delete \u003d True"},{"line_number":1669,"context_line":"            if instance.host and not shelved_offloaded:"},{"line_number":1670,"context_line":"                try:"},{"line_number":1671,"context_line":"                    # Check if compute service is up."},{"line_number":1672,"context_line":"                    service \u003d objects.Service.get_by_compute_host("},{"line_number":1673,"context_line":"                        context.elevated(), instance.host)"},{"line_number":1674,"context_line":"                    is_local_delete \u003d not self.servicegroup_api.service_is_up("},{"line_number":1675,"context_line":"                        service)"},{"line_number":1676,"context_line":"                except exception.ComputeHostNotFound:"},{"line_number":1677,"context_line":"                    pass"},{"line_number":1678,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_5ea223fe","line":1675,"range":{"start_line":1669,"start_character":0,"end_line":1675,"end_character":32},"in_reply_to":"5aef4532_9bc9907b","updated":"2016-03-09 16:37:47.000000000","message":"Done","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":1858,"context_line":""},{"line_number":1859,"context_line":"        # cleanup volumes"},{"line_number":1860,"context_line":"        self._local_cleanup_bdm_volumes(bdms, instance, context)"},{"line_number":1861,"context_line":""},{"line_number":1862,"context_line":"        cb(context, instance, bdms, local\u003dTrue)"},{"line_number":1863,"context_line":"        sys_meta \u003d instance.system_metadata"},{"line_number":1864,"context_line":"        instance.destroy()"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_9ba070a1","line":1861,"updated":"2016-03-08 17:24:55.000000000","message":"nit: remove this","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"44029f46a4fac81aac06abed6004165204e37bc6","unresolved":false,"context_lines":[{"line_number":1858,"context_line":""},{"line_number":1859,"context_line":"        # cleanup volumes"},{"line_number":1860,"context_line":"        self._local_cleanup_bdm_volumes(bdms, instance, context)"},{"line_number":1861,"context_line":""},{"line_number":1862,"context_line":"        cb(context, instance, bdms, local\u003dTrue)"},{"line_number":1863,"context_line":"        sys_meta \u003d instance.system_metadata"},{"line_number":1864,"context_line":"        instance.destroy()"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_1e8c1b7e","line":1861,"in_reply_to":"5aef4532_9ba070a1","updated":"2016-03-09 16:37:47.000000000","message":"Done","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"eadac7300101abcbb5c67b6277c81584e8039fbe","unresolved":false,"context_lines":[{"line_number":1635,"context_line":"                return"},{"line_number":1636,"context_line":"            shelved_offloaded \u003d (instance.vm_state"},{"line_number":1637,"context_line":"                                 \u003d\u003d vm_states.SHELVED_OFFLOADED)"},{"line_number":1638,"context_line":"            if shelved_offloaded or (not instance.host and"},{"line_number":1639,"context_line":"                                     instance.vm_state \u003d\u003d vm_states.ERROR):"},{"line_number":1640,"context_line":"                try:"},{"line_number":1641,"context_line":"                    # If instance is in shelved_offloaded state or"},{"line_number":1642,"context_line":"                    # instance.host is None, then delete instance"}],"source_content_type":"text/x-python","patch_set":8,"id":"3afc51ec_4312c663","line":1639,"range":{"start_line":1638,"start_character":2,"end_line":1639,"end_character":75},"updated":"2016-03-11 17:57:15.000000000","message":"I am confused now, but can we just check here if it is a local_delete (not sure if we need to check the the vm_state is in ERROR) and if so call the local_delete commit quotas and return. If it is not a local delete we can simplify the code and do what it is supposed to do.","commit_id":"e23977009673aa70dfeb841c3ec161d6f00aa60a"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"89fbe9b0535aa1d138a3151d92889125275e58ab","unresolved":false,"context_lines":[{"line_number":1635,"context_line":"                return"},{"line_number":1636,"context_line":"            shelved_offloaded \u003d (instance.vm_state"},{"line_number":1637,"context_line":"                                 \u003d\u003d vm_states.SHELVED_OFFLOADED)"},{"line_number":1638,"context_line":"            if shelved_offloaded or (not instance.host and"},{"line_number":1639,"context_line":"                                     instance.vm_state \u003d\u003d vm_states.ERROR):"},{"line_number":1640,"context_line":"                try:"},{"line_number":1641,"context_line":"                    # If instance is in shelved_offloaded state or"},{"line_number":1642,"context_line":"                    # instance.host is None, then delete instance"}],"source_content_type":"text/x-python","patch_set":8,"id":"3afc51ec_e41dd567","line":1639,"range":{"start_line":1638,"start_character":2,"end_line":1639,"end_character":75},"in_reply_to":"3afc51ec_4312c663","updated":"2016-03-11 19:53:48.000000000","message":"Yeah, the code is messy.  I inherited the commit this way and added the ERROR check to avoid the race condition in the gate that caused it to be reverted last time around.  The race condition is local_delete was being called on instances that were in the BUILDING state between Nova API and being scheduled to a host.\n\nFor this reason, the list of bugs this patch fixes was adjusted and we only attempt the local_delete call for host-less instances in the error state, maintaining the original behavior for all other host-less instances.\n\nI will try to refactor the code to make it more readable while maintaining this functionality.","commit_id":"e23977009673aa70dfeb841c3ec161d6f00aa60a"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"eadac7300101abcbb5c67b6277c81584e8039fbe","unresolved":false,"context_lines":[{"line_number":1861,"context_line":""},{"line_number":1862,"context_line":"        # cleanup volumes"},{"line_number":1863,"context_line":"        self._local_cleanup_bdm_volumes(bdms, instance, context)"},{"line_number":1864,"context_line":""},{"line_number":1865,"context_line":"        cb(context, instance, bdms, local\u003dTrue)"},{"line_number":1866,"context_line":"        sys_meta \u003d instance.system_metadata"},{"line_number":1867,"context_line":"        instance.destroy()"}],"source_content_type":"text/x-python","patch_set":8,"id":"3afc51ec_c306d61f","line":1864,"updated":"2016-03-11 17:57:15.000000000","message":"nit: please remove it","commit_id":"e23977009673aa70dfeb841c3ec161d6f00aa60a"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"4a4e3789f5f7c714cf84b1fd6f778081413f9cc7","unresolved":false,"context_lines":[{"line_number":1805,"context_line":"                except Exception as exc:"},{"line_number":1806,"context_line":"                    err_str \u003d _LW(\"Ignoring volume cleanup failure due to %s\")"},{"line_number":1807,"context_line":"                    LOG.warn(err_str % exc, instance\u003dinstance)"},{"line_number":1808,"context_line":"                # Instances that make it through volume allocation on a host"},{"line_number":1809,"context_line":"                # and later fail have terminate_connection and detach done"},{"line_number":1810,"context_line":"                # on their volumes before the reschedule.  The volumes are"},{"line_number":1811,"context_line":"                # left allocated so they do not need to be re-allocated on"},{"line_number":1812,"context_line":"                # the next host.  When this is the case"},{"line_number":1813,"context_line":"                # the terminate_connection and detach calls above will fail"}],"source_content_type":"text/x-python","patch_set":10,"id":"3afc51ec_e91b82c8","line":1810,"range":{"start_line":1808,"start_character":15,"end_line":1810,"end_character":58},"updated":"2016-03-15 10:53:31.000000000","message":"can you re-wording it, please? it is not very clear","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"4756e25be1d879ed23a1697dc0e804ba42eca57a","unresolved":false,"context_lines":[{"line_number":1805,"context_line":"                except Exception as exc:"},{"line_number":1806,"context_line":"                    err_str \u003d _LW(\"Ignoring volume cleanup failure due to %s\")"},{"line_number":1807,"context_line":"                    LOG.warn(err_str % exc, instance\u003dinstance)"},{"line_number":1808,"context_line":"                # Instances that make it through volume allocation on a host"},{"line_number":1809,"context_line":"                # and later fail have terminate_connection and detach done"},{"line_number":1810,"context_line":"                # on their volumes before the reschedule.  The volumes are"},{"line_number":1811,"context_line":"                # left allocated so they do not need to be re-allocated on"},{"line_number":1812,"context_line":"                # the next host.  When this is the case"},{"line_number":1813,"context_line":"                # the terminate_connection and detach calls above will fail"}],"source_content_type":"text/x-python","patch_set":10,"id":"1af94dfe_0a68ccf4","line":1810,"range":{"start_line":1808,"start_character":15,"end_line":1810,"end_character":58},"in_reply_to":"3afc51ec_e91b82c8","updated":"2016-03-15 18:29:40.000000000","message":"Done","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"4a4e3789f5f7c714cf84b1fd6f778081413f9cc7","unresolved":false,"context_lines":[{"line_number":1812,"context_line":"                # the next host.  When this is the case"},{"line_number":1813,"context_line":"                # the terminate_connection and detach calls above will fail"},{"line_number":1814,"context_line":"                # and the volume deletion will still be attempted."},{"line_number":1815,"context_line":"                try:"},{"line_number":1816,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":1817,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":1818,"context_line":"                except Exception as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"3afc51ec_a925fa09","line":1815,"updated":"2016-03-15 10:53:31.000000000","message":"how/why this change is related to this patch?\nis it addressing the same bug?\nReading the commit message I do not understand why this change is here, can you update the commit message or separate this change in a different patch?","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"d88c89372ec8dd32674732524bb3596abdaecb84","unresolved":false,"context_lines":[{"line_number":1812,"context_line":"                # the next host.  When this is the case"},{"line_number":1813,"context_line":"                # the terminate_connection and detach calls above will fail"},{"line_number":1814,"context_line":"                # and the volume deletion will still be attempted."},{"line_number":1815,"context_line":"                try:"},{"line_number":1816,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":1817,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":1818,"context_line":"                except Exception as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"1af94dfe_a97386b0","line":1815,"in_reply_to":"1af94dfe_2a1ddf0f","updated":"2016-03-16 10:24:02.000000000","message":"sorry my fault, I was confused by the gerrit UI which was hiding lines (as usual) and it looked like those lines were part of a completely different and unrelated method.\nMy bad, apologize.","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"e4c4a2788d4b42938046802bffdd75bcb6858a53","unresolved":false,"context_lines":[{"line_number":1812,"context_line":"                # the next host.  When this is the case"},{"line_number":1813,"context_line":"                # the terminate_connection and detach calls above will fail"},{"line_number":1814,"context_line":"                # and the volume deletion will still be attempted."},{"line_number":1815,"context_line":"                try:"},{"line_number":1816,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":1817,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":1818,"context_line":"                except Exception as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"1af94dfe_2a1ddf0f","line":1815,"in_reply_to":"3afc51ec_a925fa09","updated":"2016-03-15 12:50:41.000000000","message":"This change covers this case, which is at the start of the commit message:\nIf an instance is booted from a volume, shelved, and goes into an error state due to some reason, the volume from which instance is booted remains even the instance is deleted because instance has no host associated with it.\n\nThe change here is necessary as noted in the comment which I will try to re-word.\n\nThe error case is:\n1. Nova boot with volume in BDM (either boot from volume or boot from image with a volume specified on the create server API call).  The volume has delete_on_termination\u003dTrue in the BDM.\n2. Nova boot fails on a host, compute manager cleans up and by design does not delete the volume.\n3. The create server request gets rescheduled to other hosts and all fail in the same manner.\n4. After the final failure the VM is left in the error state and instance.host is not set.\n5. User attempts to delete the instance and it gets into this block.\n6. The volume is not attached to any VM on any hypervisor so the volume_api.terminate_connection and volume_api.detach calls fail.  We catch the exception and log the warning, and call volume_api.delete in a separate try-except so the non-attached volumes will still be deleted.\n\nThis is part of what is behind bug 1404867.","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":1643,"context_line":"                    quotas.commit()"},{"line_number":1644,"context_line":"                    return"},{"line_number":1645,"context_line":"                except exception.ObjectActionError:"},{"line_number":1646,"context_line":"                    instance.refresh()"},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"            if instance.vm_state \u003d\u003d vm_states.RESIZED:"},{"line_number":1649,"context_line":"                self._confirm_resize_on_deleting(context, instance)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_b3d5491d","line":1646,"updated":"2016-06-07 22:20:31.000000000","message":"The intent of this is to refresh the host field since ObjectActionError during a destroy means that the host has changed. Wish we had a code comment here that explains it.\n\nPoint being, I think the idea is to continue on to try the rest of the code with the refreshed host. The scenario here would be an unscheduled instance getting scheduled at same time destroy is attempted.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0b0173f3d9eb7d5f8595276242cbb41be22630cf","unresolved":false,"context_lines":[{"line_number":1643,"context_line":"                    quotas.commit()"},{"line_number":1644,"context_line":"                    return"},{"line_number":1645,"context_line":"                except exception.ObjectActionError:"},{"line_number":1646,"context_line":"                    instance.refresh()"},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"            if instance.vm_state \u003d\u003d vm_states.RESIZED:"},{"line_number":1649,"context_line":"                self._confirm_resize_on_deleting(context, instance)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_75eb3eb1","line":1646,"in_reply_to":"7aa08908_0ec920ad","updated":"2016-06-09 21:09:15.000000000","message":"No, I was just noting the reason for instance.refresh() for the benefit of other reviewers. I think it makes sense to \u0027fall through\u0027 to the rest of the code after refreshing the host","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"6d5a1fce0e99c7224ba38d7070bd246c3960d52f","unresolved":false,"context_lines":[{"line_number":1643,"context_line":"                    quotas.commit()"},{"line_number":1644,"context_line":"                    return"},{"line_number":1645,"context_line":"                except exception.ObjectActionError:"},{"line_number":1646,"context_line":"                    instance.refresh()"},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"            if instance.vm_state \u003d\u003d vm_states.RESIZED:"},{"line_number":1649,"context_line":"                self._confirm_resize_on_deleting(context, instance)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_0ec920ad","line":1646,"in_reply_to":"7aa08908_b3d5491d","updated":"2016-06-09 13:08:15.000000000","message":"Is your suggestion here to reset the local_delete variable?","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":1643,"context_line":"                    quotas.commit()"},{"line_number":1644,"context_line":"                    return"},{"line_number":1645,"context_line":"                except exception.ObjectActionError:"},{"line_number":1646,"context_line":"                    instance.refresh()"},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"            if instance.vm_state \u003d\u003d vm_states.RESIZED:"},{"line_number":1649,"context_line":"                self._confirm_resize_on_deleting(context, instance)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_469dc158","line":1646,"in_reply_to":"7aa08908_b3d5491d","updated":"2016-06-09 16:44:39.000000000","message":"You are correct.  I will add a comment.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":1655,"context_line":"                    quotas.commit()"},{"line_number":1656,"context_line":"                    return"},{"line_number":1657,"context_line":"                except exception.ObjectActionError:"},{"line_number":1658,"context_line":"                    instance.refresh()"},{"line_number":1659,"context_line":""},{"line_number":1660,"context_line":"            else:"},{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_b3a389b1","line":1658,"updated":"2016-06-07 22:20:31.000000000","message":"Similarly this will be refreshing the host if it changed. I *think* we would want to continue on and try the non-local delete after this, if we hit this. Because it would mean we failed a local delete attempt because the host changed under us, which would mean the instance was unshelved at the same time we tried to local delete it.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":1655,"context_line":"                    quotas.commit()"},{"line_number":1656,"context_line":"                    return"},{"line_number":1657,"context_line":"                except exception.ObjectActionError:"},{"line_number":1658,"context_line":"                    instance.refresh()"},{"line_number":1659,"context_line":""},{"line_number":1660,"context_line":"            else:"},{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_b9c00a2c","line":1658,"in_reply_to":"7aa08908_b3a389b1","updated":"2016-06-09 16:44:39.000000000","message":"Done","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"6d5a1fce0e99c7224ba38d7070bd246c3960d52f","unresolved":false,"context_lines":[{"line_number":1655,"context_line":"                    quotas.commit()"},{"line_number":1656,"context_line":"                    return"},{"line_number":1657,"context_line":"                except exception.ObjectActionError:"},{"line_number":1658,"context_line":"                    instance.refresh()"},{"line_number":1659,"context_line":""},{"line_number":1660,"context_line":"            else:"},{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_2ee33c07","line":1658,"in_reply_to":"7aa08908_b3a389b1","updated":"2016-06-09 13:08:15.000000000","message":"Same here, reset local_delete or just eliminate the else on line 1660 like you mentioned?","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":1657,"context_line":"                except exception.ObjectActionError:"},{"line_number":1658,"context_line":"                    instance.refresh()"},{"line_number":1659,"context_line":""},{"line_number":1660,"context_line":"            else:"},{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"},{"line_number":1662,"context_line":"                                           task_states.SOFT_DELETING):"},{"line_number":1663,"context_line":"                    LOG.info(_LI(\u0027Instance is already in deleting state, \u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_1389bdc9","line":1660,"updated":"2016-06-07 22:20:31.000000000","message":"I think maybe we don\u0027t want an else statement based on my comment above","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":1657,"context_line":"                except exception.ObjectActionError:"},{"line_number":1658,"context_line":"                    instance.refresh()"},{"line_number":1659,"context_line":""},{"line_number":1660,"context_line":"            else:"},{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"},{"line_number":1662,"context_line":"                                           task_states.SOFT_DELETING):"},{"line_number":1663,"context_line":"                    LOG.info(_LI(\u0027Instance is already in deleting state, \u0027"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_06a579b4","line":1660,"in_reply_to":"7aa08908_1389bdc9","updated":"2016-06-09 16:44:39.000000000","message":"Done","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"},{"line_number":1662,"context_line":"                                           task_states.SOFT_DELETING):"},{"line_number":1663,"context_line":"                    LOG.info(_LI(\u0027Instance is already in deleting state, \u0027"},{"line_number":1664,"context_line":"                                 \u0027ignoring this request.\u0027),"},{"line_number":1665,"context_line":"                             instance\u003dinstance)"},{"line_number":1666,"context_line":"                    quotas.rollback()"},{"line_number":1667,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_b32d4900","line":1664,"range":{"start_line":1664,"start_character":55,"end_line":1664,"end_character":56},"updated":"2016-06-07 22:20:31.000000000","message":"Unrelated change, maybe from the old base from long ago","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":1661,"context_line":"                if original_task_state in (task_states.DELETING,"},{"line_number":1662,"context_line":"                                           task_states.SOFT_DELETING):"},{"line_number":1663,"context_line":"                    LOG.info(_LI(\u0027Instance is already in deleting state, \u0027"},{"line_number":1664,"context_line":"                                 \u0027ignoring this request.\u0027),"},{"line_number":1665,"context_line":"                             instance\u003dinstance)"},{"line_number":1666,"context_line":"                    quotas.rollback()"},{"line_number":1667,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_c3ba430a","line":1664,"range":{"start_line":1664,"start_character":55,"end_line":1664,"end_character":56},"in_reply_to":"7aa08908_b32d4900","updated":"2016-06-09 16:44:39.000000000","message":"Done","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"6d5a1fce0e99c7224ba38d7070bd246c3960d52f","unresolved":false,"context_lines":[{"line_number":1694,"context_line":"        if not instance.host:"},{"line_number":1695,"context_line":"            if instance.vm_state \u003d\u003d vm_states.ERROR:"},{"line_number":1696,"context_line":"                return True"},{"line_number":1697,"context_line":"            return False"},{"line_number":1698,"context_line":"        else:"},{"line_number":1699,"context_line":"            try:"},{"line_number":1700,"context_line":"                # Check if compute service is up."}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_ae29ec8b","line":1697,"updated":"2016-06-09 13:08:15.000000000","message":"Returning False for this case seems odd to me but maybe I\u0027m just not understanding the reasoning. But looking at this and how _is_local_delete is used on line 1633 above the check on instance.host seems contradictory. If False is returned here then line 1633 is essentially \"if not instance.host and instance.host\" since this check is the same as \"if instance.host return True\".\n\nBut for a method called is_local_delete I would definitely expect an instance without a host to case a local delete.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"65115e89e244ef1cb4281cac760ca84f86ff2971","unresolved":false,"context_lines":[{"line_number":1694,"context_line":"        if not instance.host:"},{"line_number":1695,"context_line":"            if instance.vm_state \u003d\u003d vm_states.ERROR:"},{"line_number":1696,"context_line":"                return True"},{"line_number":1697,"context_line":"            return False"},{"line_number":1698,"context_line":"        else:"},{"line_number":1699,"context_line":"            try:"},{"line_number":1700,"context_line":"                # Check if compute service is up."}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_6869a625","line":1697,"in_reply_to":"7aa08908_26b995ed","updated":"2016-06-09 18:02:30.000000000","message":"I think a different method name is needed then. If instance.host is None then there is no choice but to do a local delete. I can understand why you exclude that here but the method name is confusing. Something like should_attempt_local_delete perhaps. There\u0027s probably a more appropriate name than that.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0b0173f3d9eb7d5f8595276242cbb41be22630cf","unresolved":false,"context_lines":[{"line_number":1694,"context_line":"        if not instance.host:"},{"line_number":1695,"context_line":"            if instance.vm_state \u003d\u003d vm_states.ERROR:"},{"line_number":1696,"context_line":"                return True"},{"line_number":1697,"context_line":"            return False"},{"line_number":1698,"context_line":"        else:"},{"line_number":1699,"context_line":"            try:"},{"line_number":1700,"context_line":"                # Check if compute service is up."}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_f8a9e57c","line":1697,"in_reply_to":"7aa08908_6869a625","updated":"2016-06-09 21:09:15.000000000","message":"Since instance.host is set by compute, there is a window where instance.host is None, yet things have already been set into motion in compute, so local delete isn\u0027t okay as it deletes things out from under compute. Aside: thinking about it, I\u0027m not sure how that resulted in tempest failures.\n\nSo I\u0027m not sure how to name this better. It\u0027s really \"is a local delete given the current state of things\"","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":1694,"context_line":"        if not instance.host:"},{"line_number":1695,"context_line":"            if instance.vm_state \u003d\u003d vm_states.ERROR:"},{"line_number":1696,"context_line":"                return True"},{"line_number":1697,"context_line":"            return False"},{"line_number":1698,"context_line":"        else:"},{"line_number":1699,"context_line":"            try:"},{"line_number":1700,"context_line":"                # Check if compute service is up."}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_26b995ed","line":1697,"in_reply_to":"7aa08908_ae29ec8b","updated":"2016-06-09 16:44:39.000000000","message":"The intention of this method is return a boolean indicating whether or not the _local_delete method should be called.\n\nThis particular block is only going to allow _local_delete to be called when not instance.host and the VM is in ERROR.  This is intentionally excluding all other instance.host \u003d\u003d None cases to avoid the race conditions of attempting local deletes on BUILDING instances and other instances that caused gate failures.  These gate failures caused Ankit Agrawal\u0027s previous two commit attempts of a similar patch to be reverted.\n\nAs noted in the commit message this patch set is not attempting to address those race conditions and therefore is not claiming to fix one or more of the bugs that the original commit/reverts fixed.","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":1702,"context_line":"                    context.elevated(), instance.host)"},{"line_number":1703,"context_line":"                return not self.servicegroup_api.service_is_up(service)"},{"line_number":1704,"context_line":"            except exception.ComputeHostNotFound:"},{"line_number":1705,"context_line":"                pass"},{"line_number":1706,"context_line":"        return True"},{"line_number":1707,"context_line":""},{"line_number":1708,"context_line":"    def _confirm_resize_on_deleting(self, context, instance):"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_f69dff0a","line":1705,"updated":"2016-06-07 22:20:31.000000000","message":"This is different than what exists now but I think it\u0027s correct in that only Service.get_by_compute_host raises ComputeHostNotFound in the old block","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0673c9c4e69dfbab8b0b971eb9db23e84bb54e85","unresolved":false,"context_lines":[{"line_number":1634,"context_line":"                quotas.rollback()"},{"line_number":1635,"context_line":""},{"line_number":1636,"context_line":"            cb(context, instance, bdms,"},{"line_number":1637,"context_line":"               reservations\u003dquotas.reservations)"},{"line_number":1638,"context_line":""},{"line_number":1639,"context_line":"        except exception.InstanceNotFound:"},{"line_number":1640,"context_line":"            # NOTE(comstud): Race condition. Instance already gone."}],"source_content_type":"text/x-python","patch_set":13,"id":"7aa08908_5c72a6ae","line":1637,"updated":"2016-06-09 21:21:38.000000000","message":"Hm, we should be testing that everything falls through as expected in the face of ObjectActionErrors. I can do a follow-up patch for that","commit_id":"b7f83337658181f0e7117c7f3b07f69856ffe405"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6b725d102a164a895e8d0d82edd956f129862439","unresolved":false,"context_lines":[{"line_number":1633,"context_line":"            if instance.vm_state \u003d\u003d vm_states.SOFT_DELETED:"},{"line_number":1634,"context_line":"                quotas.rollback()"},{"line_number":1635,"context_line":""},{"line_number":1636,"context_line":"            cb(context, instance, bdms,"},{"line_number":1637,"context_line":"               reservations\u003dquotas.reservations)"},{"line_number":1638,"context_line":""},{"line_number":1639,"context_line":"        except exception.InstanceNotFound:"},{"line_number":1640,"context_line":"            # NOTE(comstud): Race condition. Instance already gone."}],"source_content_type":"text/x-python","patch_set":13,"id":"3aaa91ec_cb377c4b","line":1637,"range":{"start_line":1636,"start_character":12,"end_line":1637,"end_character":48},"updated":"2016-06-29 19:41:50.000000000","message":"This blows up:\n\nhttp://logs.openstack.org/94/334094/4/check/gate-tempest-dsvm-neutron-full/1200ad2/logs/screen-n-api.txt.gz?level\u003dTRACE#_2016-06-29_14_51_39_286\n\nBecause cb is _do_delete which defaults local\u003dFalse.","commit_id":"b7f83337658181f0e7117c7f3b07f69856ffe405"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":11029,"context_line":"        rules \u003d {\"compute:delete\": []}"},{"line_number":11030,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11031,"context_line":""},{"line_number":11032,"context_line":"        with mock.patch.object(self.compute_api, \u0027_local_delete\u0027):"},{"line_number":11033,"context_line":"            self.compute_api.delete(self.context, instance)"},{"line_number":11034,"context_line":""},{"line_number":11035,"context_line":"    def test_create_fail(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_5bf9c87f","line":11032,"range":{"start_line":11032,"start_character":8,"end_line":11032,"end_character":66},"updated":"2016-03-08 17:24:55.000000000","message":"why do we need this mock now and it was not required before this change?","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"44029f46a4fac81aac06abed6004165204e37bc6","unresolved":false,"context_lines":[{"line_number":11029,"context_line":"        rules \u003d {\"compute:delete\": []}"},{"line_number":11030,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11031,"context_line":""},{"line_number":11032,"context_line":"        with mock.patch.object(self.compute_api, \u0027_local_delete\u0027):"},{"line_number":11033,"context_line":"            self.compute_api.delete(self.context, instance)"},{"line_number":11034,"context_line":""},{"line_number":11035,"context_line":"    def test_create_fail(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_4e1f04b7","line":11032,"range":{"start_line":11032,"start_character":8,"end_line":11032,"end_character":66},"in_reply_to":"5aef4532_5bf9c87f","updated":"2016-03-09 16:37:47.000000000","message":"I don\u0027t know.  This was part of the last merged patch set which was then reverted, which I picked up and modified to scope the fix and hopefully avoid the issues in gate.\n\nThe test case passes without this mock so I will remove it in the next patch set.","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":11073,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11074,"context_line":""},{"line_number":11075,"context_line":"        self.compute_api.delete(self.context, instance)"},{"line_number":11076,"context_line":""},{"line_number":11077,"context_line":"    def test_create_fail(self):"},{"line_number":11078,"context_line":"        rules \u003d {\"compute:create\": [[\"false:false\"]]}"},{"line_number":11079,"context_line":"        self.policy.set_rules(rules)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_899e5fb6","side":"PARENT","line":11076,"updated":"2016-06-07 01:19:07.000000000","message":"This looks like an unrelated change that needs to be removed from this patch","commit_id":"aecefcb9dd5403a0d0a83e9f95d49dcea22e5fc4"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"17602fb33f0248e73f7cc6d7f931499491a0b812","unresolved":false,"context_lines":[{"line_number":11073,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11074,"context_line":""},{"line_number":11075,"context_line":"        self.compute_api.delete(self.context, instance)"},{"line_number":11076,"context_line":""},{"line_number":11077,"context_line":"    def test_create_fail(self):"},{"line_number":11078,"context_line":"        rules \u003d {\"compute:create\": [[\"false:false\"]]}"},{"line_number":11079,"context_line":"        self.policy.set_rules(rules)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_5bc8a522","side":"PARENT","line":11076,"in_reply_to":"7aa08908_899e5fb6","updated":"2016-06-07 11:59:59.000000000","message":"Done.\nThis patch set is 7 months old and hasn\u0027t been rebased in about 3 months.  This line went away when I rebased to address your other comments.","commit_id":"aecefcb9dd5403a0d0a83e9f95d49dcea22e5fc4"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2b44a1b562352481110868800e5796f49032d089","unresolved":false,"context_lines":[{"line_number":11143,"context_line":"        # reset rules to allow deletion"},{"line_number":11144,"context_line":"        rules \u003d {\"compute:delete\": []}"},{"line_number":11145,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11146,"context_line":""},{"line_number":11147,"context_line":"    def test_create_fail(self):"},{"line_number":11148,"context_line":"        rules \u003d {\"compute:create\": [[\"false:false\"]]}"},{"line_number":11149,"context_line":"        self.policy.set_rules(rules)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_d60f2386","line":11146,"updated":"2016-06-07 22:20:31.000000000","message":"Still have the unrelated change removing these lines. You\u0027ll need to add them back into your version of test_compute.py. You can check your commit using \u0027git show -1\u0027 to eyeball for extraneous changes","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"eb0d2f15feda776b45f673bc85bafe2e0da0c9e0","unresolved":false,"context_lines":[{"line_number":11143,"context_line":"        # reset rules to allow deletion"},{"line_number":11144,"context_line":"        rules \u003d {\"compute:delete\": []}"},{"line_number":11145,"context_line":"        self.policy.set_rules(rules)"},{"line_number":11146,"context_line":""},{"line_number":11147,"context_line":"    def test_create_fail(self):"},{"line_number":11148,"context_line":"        rules \u003d {\"compute:create\": [[\"false:false\"]]}"},{"line_number":11149,"context_line":"        self.policy.set_rules(rules)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_430db336","line":11146,"in_reply_to":"7aa08908_d60f2386","updated":"2016-06-09 16:44:39.000000000","message":"Done","commit_id":"531f38b913f0e2294468af2ade02f3696a7e31fa"}],"nova/tests/unit/compute/test_compute_api.py":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"9332a14739aa825fba737f059d54c0c0cdd922f8","unresolved":false,"context_lines":[{"line_number":1115,"context_line":"                                            tzinfo\u003diso8601.iso8601.Utc())"},{"line_number":1116,"context_line":"            updates[\u0027deleted_at\u0027] \u003d delete_time"},{"line_number":1117,"context_line":"            updates[\u0027deleted\u0027] \u003d True"},{"line_number":1118,"context_line":""},{"line_number":1119,"context_line":"            fake_inst \u003d fake_instance.fake_db_instance(**updates)"},{"line_number":1120,"context_line":"            db.instance_destroy(self.context, inst.uuid,"},{"line_number":1121,"context_line":"                                constraint\u003d\u0027constraint\u0027).AndReturn(fake_inst)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_7bee44b5","line":1118,"updated":"2016-03-08 17:24:55.000000000","message":"nit: please remove it","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"44029f46a4fac81aac06abed6004165204e37bc6","unresolved":false,"context_lines":[{"line_number":1115,"context_line":"                                            tzinfo\u003diso8601.iso8601.Utc())"},{"line_number":1116,"context_line":"            updates[\u0027deleted_at\u0027] \u003d delete_time"},{"line_number":1117,"context_line":"            updates[\u0027deleted\u0027] \u003d True"},{"line_number":1118,"context_line":""},{"line_number":1119,"context_line":"            fake_inst \u003d fake_instance.fake_db_instance(**updates)"},{"line_number":1120,"context_line":"            db.instance_destroy(self.context, inst.uuid,"},{"line_number":1121,"context_line":"                                constraint\u003d\u0027constraint\u0027).AndReturn(fake_inst)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5aef4532_1eba5b0a","line":1118,"in_reply_to":"5aef4532_7bee44b5","updated":"2016-03-09 16:37:47.000000000","message":"Done","commit_id":"a090ae04f22ce23151def665e4b07db9cc6e3908"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"4a4e3789f5f7c714cf84b1fd6f778081413f9cc7","unresolved":false,"context_lines":[{"line_number":3532,"context_line":"        mock_terminate_conn.assert_called_once_with(self.context,"},{"line_number":3533,"context_line":"                                                    volume_id, connector)"},{"line_number":3534,"context_line":"        bdm_destroy.assert_called_once_with()"},{"line_number":3535,"context_line":""},{"line_number":3536,"context_line":""},{"line_number":3537,"context_line":"class ComputeAPIAPICellUnitTestCase(_ComputeAPIUnitTestMixIn,"},{"line_number":3538,"context_line":"                                    test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3afc51ec_ec46f04d","line":3535,"updated":"2016-03-15 10:53:31.000000000","message":"I\u0027d like to see some unit tests for the _is_local_delete method, we should tests that the logic is correct and having some tests protect us in the future if someone makes any change to that method","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"4756e25be1d879ed23a1697dc0e804ba42eca57a","unresolved":false,"context_lines":[{"line_number":3532,"context_line":"        mock_terminate_conn.assert_called_once_with(self.context,"},{"line_number":3533,"context_line":"                                                    volume_id, connector)"},{"line_number":3534,"context_line":"        bdm_destroy.assert_called_once_with()"},{"line_number":3535,"context_line":""},{"line_number":3536,"context_line":""},{"line_number":3537,"context_line":"class ComputeAPIAPICellUnitTestCase(_ComputeAPIUnitTestMixIn,"},{"line_number":3538,"context_line":"                                    test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":10,"id":"1af94dfe_eaaac018","line":3535,"in_reply_to":"3afc51ec_ec46f04d","updated":"2016-03-15 18:29:40.000000000","message":"Done","commit_id":"1f62c3f882f5b2a307da44fb48701ccaa81b1432"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":3548,"context_line":"    @mock.patch.object(compute_utils, \u0027notify_about_instance_usage\u0027)"},{"line_number":3549,"context_line":"    @mock.patch.object(objects.BlockDeviceMapping, \u0027destroy\u0027)"},{"line_number":3550,"context_line":"    @mock.patch.object(objects.Instance, \u0027destroy\u0027)"},{"line_number":3551,"context_line":"    def test_delete_volume_backed_instance_in_error_state("},{"line_number":3552,"context_line":"            self, mock_instance_destroy, bdm_destroy,"},{"line_number":3553,"context_line":"            notify_about_instance_usage, mock_reserve,"},{"line_number":3554,"context_line":"            mock_save, mock_elevated, bdm_get_by_instance_uuid):"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_e909e32d","line":3551,"updated":"2016-06-07 01:19:07.000000000","message":"This test is covering the case where instance.host is not set and the instance is in error state. I think we also need to cover the case where instance.host is not set and the instance is SHELVED_OFFLOADED. I suggest renaming this method similar to \"_test_delete_volume_backed\" with an extra parameter to take the vm_state and have the two tests \"test_delete_volume_backed_instance_in_error_state\" and \"test_delete_volume_backed_shelved_offloaded\" call the private method with the respective vm_states","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"e8fa0b2ee1d26e3f42442becefa1b2c7a06d73aa","unresolved":false,"context_lines":[{"line_number":3548,"context_line":"    @mock.patch.object(compute_utils, \u0027notify_about_instance_usage\u0027)"},{"line_number":3549,"context_line":"    @mock.patch.object(objects.BlockDeviceMapping, \u0027destroy\u0027)"},{"line_number":3550,"context_line":"    @mock.patch.object(objects.Instance, \u0027destroy\u0027)"},{"line_number":3551,"context_line":"    def test_delete_volume_backed_instance_in_error_state("},{"line_number":3552,"context_line":"            self, mock_instance_destroy, bdm_destroy,"},{"line_number":3553,"context_line":"            notify_about_instance_usage, mock_reserve,"},{"line_number":3554,"context_line":"            mock_save, mock_elevated, bdm_get_by_instance_uuid):"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_6a600ca6","line":3551,"in_reply_to":"7aa08908_e909e32d","updated":"2016-06-07 18:33:30.000000000","message":"Done","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":3560,"context_line":"                 \u0027delete_on_termination\u0027: False}))]"},{"line_number":3561,"context_line":"        reservations \u003d [\u0027fake-resv\u0027]"},{"line_number":3562,"context_line":""},{"line_number":3563,"context_line":"        delete_time \u003d datetime.datetime(1955, 11, 5, 9, 30,"},{"line_number":3564,"context_line":"                                        tzinfo\u003diso8601.iso8601.Utc())"},{"line_number":3565,"context_line":"        updates \u003d {\u0027deleted_at\u0027: delete_time,"},{"line_number":3566,"context_line":"                   \u0027deleted\u0027: True}"},{"line_number":3567,"context_line":"        fake_inst \u003d fake_instance.fake_instance_obj(self.context, **updates)"},{"line_number":3568,"context_line":"        mock_instance_destroy.return_value \u003d fake_inst"},{"line_number":3569,"context_line":"        bdm_get_by_instance_uuid.return_value \u003d bdms"},{"line_number":3570,"context_line":"        mock_reserve.return_value \u003d reservations"},{"line_number":3571,"context_line":"        mock_elevated.return_value \u003d self.context"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_aed08154","line":3568,"range":{"start_line":3563,"start_character":8,"end_line":3568,"end_character":54},"updated":"2016-06-07 01:19:07.000000000","message":"I don\u0027t think you need any of this","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"17602fb33f0248e73f7cc6d7f931499491a0b812","unresolved":false,"context_lines":[{"line_number":3560,"context_line":"                 \u0027delete_on_termination\u0027: False}))]"},{"line_number":3561,"context_line":"        reservations \u003d [\u0027fake-resv\u0027]"},{"line_number":3562,"context_line":""},{"line_number":3563,"context_line":"        delete_time \u003d datetime.datetime(1955, 11, 5, 9, 30,"},{"line_number":3564,"context_line":"                                        tzinfo\u003diso8601.iso8601.Utc())"},{"line_number":3565,"context_line":"        updates \u003d {\u0027deleted_at\u0027: delete_time,"},{"line_number":3566,"context_line":"                   \u0027deleted\u0027: True}"},{"line_number":3567,"context_line":"        fake_inst \u003d fake_instance.fake_instance_obj(self.context, **updates)"},{"line_number":3568,"context_line":"        mock_instance_destroy.return_value \u003d fake_inst"},{"line_number":3569,"context_line":"        bdm_get_by_instance_uuid.return_value \u003d bdms"},{"line_number":3570,"context_line":"        mock_reserve.return_value \u003d reservations"},{"line_number":3571,"context_line":"        mock_elevated.return_value \u003d self.context"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_9ba04d45","line":3568,"range":{"start_line":3563,"start_character":8,"end_line":3568,"end_character":54},"in_reply_to":"7aa08908_aed08154","updated":"2016-06-07 11:59:59.000000000","message":"Done","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":3570,"context_line":"        mock_reserve.return_value \u003d reservations"},{"line_number":3571,"context_line":"        mock_elevated.return_value \u003d self.context"},{"line_number":3572,"context_line":""},{"line_number":3573,"context_line":"        params \u003d {\u0027host\u0027: \u0027\u0027, \u0027vm_state\u0027: vm_states.ERROR}"},{"line_number":3574,"context_line":"        inst \u003d self._create_instance_obj(params\u003dparams)"},{"line_number":3575,"context_line":"        inst._context \u003d self.context"},{"line_number":3576,"context_line":"        connector \u003d {\u0027ip\u0027: \u0027127.0.0.1\u0027, \u0027initiator\u0027: \u0027iqn.fake\u0027}"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_c90ce73d","line":3573,"range":{"start_line":3573,"start_character":26,"end_line":3573,"end_character":28},"updated":"2016-06-07 01:19:07.000000000","message":"I think it would be more correct to use None here","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"17602fb33f0248e73f7cc6d7f931499491a0b812","unresolved":false,"context_lines":[{"line_number":3570,"context_line":"        mock_reserve.return_value \u003d reservations"},{"line_number":3571,"context_line":"        mock_elevated.return_value \u003d self.context"},{"line_number":3572,"context_line":""},{"line_number":3573,"context_line":"        params \u003d {\u0027host\u0027: \u0027\u0027, \u0027vm_state\u0027: vm_states.ERROR}"},{"line_number":3574,"context_line":"        inst \u003d self._create_instance_obj(params\u003dparams)"},{"line_number":3575,"context_line":"        inst._context \u003d self.context"},{"line_number":3576,"context_line":"        connector \u003d {\u0027ip\u0027: \u0027127.0.0.1\u0027, \u0027initiator\u0027: \u0027iqn.fake\u0027}"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_7ba3e14f","line":3573,"range":{"start_line":3573,"start_character":26,"end_line":3573,"end_character":28},"in_reply_to":"7aa08908_c90ce73d","updated":"2016-06-07 11:59:59.000000000","message":"Done","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":3572,"context_line":""},{"line_number":3573,"context_line":"        params \u003d {\u0027host\u0027: \u0027\u0027, \u0027vm_state\u0027: vm_states.ERROR}"},{"line_number":3574,"context_line":"        inst \u003d self._create_instance_obj(params\u003dparams)"},{"line_number":3575,"context_line":"        inst._context \u003d self.context"},{"line_number":3576,"context_line":"        connector \u003d {\u0027ip\u0027: \u0027127.0.0.1\u0027, \u0027initiator\u0027: \u0027iqn.fake\u0027}"},{"line_number":3577,"context_line":""},{"line_number":3578,"context_line":"        with mock.patch.object(self.compute_api.network_api,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_ae0521c3","line":3575,"range":{"start_line":3575,"start_character":8,"end_line":3575,"end_character":36},"updated":"2016-06-07 01:19:07.000000000","message":"You don\u0027t need this (it already does this in _create_instance_obj)","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"17602fb33f0248e73f7cc6d7f931499491a0b812","unresolved":false,"context_lines":[{"line_number":3572,"context_line":""},{"line_number":3573,"context_line":"        params \u003d {\u0027host\u0027: \u0027\u0027, \u0027vm_state\u0027: vm_states.ERROR}"},{"line_number":3574,"context_line":"        inst \u003d self._create_instance_obj(params\u003dparams)"},{"line_number":3575,"context_line":"        inst._context \u003d self.context"},{"line_number":3576,"context_line":"        connector \u003d {\u0027ip\u0027: \u0027127.0.0.1\u0027, \u0027initiator\u0027: \u0027iqn.fake\u0027}"},{"line_number":3577,"context_line":""},{"line_number":3578,"context_line":"        with mock.patch.object(self.compute_api.network_api,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_db8ad5be","line":3575,"range":{"start_line":3575,"start_character":8,"end_line":3575,"end_character":36},"in_reply_to":"7aa08908_ae0521c3","updated":"2016-06-07 11:59:59.000000000","message":"Done","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"64785c57547ce5a68fb254a82f4b5a71ff0fcc90","unresolved":false,"context_lines":[{"line_number":3581,"context_line":"                              \u0027terminate_connection\u0027) as mock_terminate_conn, \\"},{"line_number":3582,"context_line":"            mock.patch.object(self.compute_api.volume_api,"},{"line_number":3583,"context_line":"                              \u0027detach\u0027) as mock_detach:"},{"line_number":3584,"context_line":"            self.compute_api.delete(self.context, inst)"},{"line_number":3585,"context_line":""},{"line_number":3586,"context_line":"        mock_deallocate.assert_called_once_with(self.context, inst)"},{"line_number":3587,"context_line":"        mock_detach.assert_called_once_with(self.context, volume_id,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_ee83a9ca","line":3584,"updated":"2016-06-07 01:19:07.000000000","message":"This should be indented one more level to differentiate from the with statement indention [1]. Alternatively, line up all of the \"mock\" like the other tests (one space more)\n\n[1] https://www.python.org/dev/peps/pep-0008/#indentation","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"},{"author":{"_account_id":13562,"name":"Samuel Matzek","email":"smatzek@us.ibm.com","username":"smatzek"},"change_message_id":"17602fb33f0248e73f7cc6d7f931499491a0b812","unresolved":false,"context_lines":[{"line_number":3581,"context_line":"                              \u0027terminate_connection\u0027) as mock_terminate_conn, \\"},{"line_number":3582,"context_line":"            mock.patch.object(self.compute_api.volume_api,"},{"line_number":3583,"context_line":"                              \u0027detach\u0027) as mock_detach:"},{"line_number":3584,"context_line":"            self.compute_api.delete(self.context, inst)"},{"line_number":3585,"context_line":""},{"line_number":3586,"context_line":"        mock_deallocate.assert_called_once_with(self.context, inst)"},{"line_number":3587,"context_line":"        mock_detach.assert_called_once_with(self.context, volume_id,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7aa08908_5bbd4567","line":3584,"in_reply_to":"7aa08908_ee83a9ca","updated":"2016-06-07 11:59:59.000000000","message":"Done","commit_id":"e519813516ea447cfa67b526c588e34695c3cd89"}]}