)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"d67606a2d88c76de13cc0bc0fbf12ce7066d568a","unresolved":false,"context_lines":[{"line_number":56,"context_line":"Co-Authored-By: melanie witt \u003cmelwittt@gmail.com\u003e"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"Closes-Bug: 1404867"},{"line_number":59,"context_line":"Closes-Bug: 1408527"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"Change-Id: I4dc6c8bd3bb6c135f8a698af41f5d0e026c39117"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"5a3905b3_880a18d1","line":59,"range":{"start_line":59,"start_character":12,"end_line":59,"end_character":19},"updated":"2017-01-19 21:10:10.000000000","message":"Please add this bug too: #1594604. A patch for the bug (https://review.openstack.org/#/c/333579/) was abandoned due to this patch that covers the the two bugs in addition. The fix is badly needed.\n\nIs there any way for the core reviewers to look at this and move this forward? Thanks.","commit_id":"169be5140b7fd6d5fec62d400be680c4283a660f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":10,"context_line":"scheduled. However, the exception handling routine in compute manager"},{"line_number":11,"context_line":"[1] will set instance.host \u003d None and set instance.vm_state \u003d ERROR"},{"line_number":12,"context_line":"if the instance fails to build on the compute host. If that happens, we"},{"line_number":13,"context_line":"end up with an instance with host \u003d None, vm_state \u003d ERROR, and ports"},{"line_number":14,"context_line":"and volumes still allocated on the compute host (if the instance had"},{"line_number":15,"context_line":"networking and volumes)."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Ideally, in this case, we would want to do a terminate instance over"},{"line_number":18,"context_line":"RPC to have the compute node clean up the instance and its ports and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_da1d3074","line":15,"range":{"start_line":13,"start_character":60,"end_line":15,"end_character":24},"updated":"2018-02-15 15:04:56.000000000","message":"Well, the compute manager does have the network (ports) and volume cleanup here:\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L1921-L1924\n\nWhy isn\u0027t _cleanup_allocated_networks unbinding the ports on failure?\n\nWe know that _cleanup_volumes fails to detach the volume because it simply tries to delete the volume, which fails because it might be attached, which is being fixed here:\n\nhttps://review.openstack.org/#/c/528385/\n\nBut I\u0027d like some more information on why the compute fails to unbind the ports. We should be calling this:\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/network/neutronv2/api.py#L1248\n\nWhich should properly unbind ports that were provided on the server create request, and delete ports that nova created on the user\u0027s behalf. Is that not working?","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":10,"context_line":"scheduled. However, the exception handling routine in compute manager"},{"line_number":11,"context_line":"[1] will set instance.host \u003d None and set instance.vm_state \u003d ERROR"},{"line_number":12,"context_line":"if the instance fails to build on the compute host. If that happens, we"},{"line_number":13,"context_line":"end up with an instance with host \u003d None, vm_state \u003d ERROR, and ports"},{"line_number":14,"context_line":"and volumes still allocated on the compute host (if the instance had"},{"line_number":15,"context_line":"networking and volumes)."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Ideally, in this case, we would want to do a terminate instance over"},{"line_number":18,"context_line":"RPC to have the compute node clean up the instance and its ports and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_74fe6caf","line":15,"range":{"start_line":13,"start_character":60,"end_line":15,"end_character":24},"in_reply_to":"3fa0c359_691779ef","updated":"2018-02-15 16:39:55.000000000","message":"Yeah I figured the ports mentions in the commit message were purely for completeness, but it doesn\u0027t really have anything to do with the major bug we\u0027re trying to resolve with orphaned volumes, so IMO it\u0027s a bit distracting to include it.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":10,"context_line":"scheduled. However, the exception handling routine in compute manager"},{"line_number":11,"context_line":"[1] will set instance.host \u003d None and set instance.vm_state \u003d ERROR"},{"line_number":12,"context_line":"if the instance fails to build on the compute host. If that happens, we"},{"line_number":13,"context_line":"end up with an instance with host \u003d None, vm_state \u003d ERROR, and ports"},{"line_number":14,"context_line":"and volumes still allocated on the compute host (if the instance had"},{"line_number":15,"context_line":"networking and volumes)."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Ideally, in this case, we would want to do a terminate instance over"},{"line_number":18,"context_line":"RPC to have the compute node clean up the instance and its ports and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_3d31a4ed","line":15,"range":{"start_line":13,"start_character":60,"end_line":15,"end_character":24},"in_reply_to":"3fa0c359_74fe6caf","updated":"2018-02-15 20:36:38.000000000","message":"Yeah, sorry for the confusion here. I was indeed trying to be complete and call out that this patch will cause ERROR and SHELVED_OFFLOADED instances with instance.host \u003d None to go down the _local_delete path, which will attempt to clean up ports and volumes.\n\nThe more I\u0027m learning about these related bugs, the more I\u0027m realizing this patch of mine is really the \"last line of defense\" that will attempt to clean up loose ends if all of the other cleanups in compute/manager and conductor/manager have somehow failed or were interrupted.\n\nI\u0027ll rewrite the commit message to be clear about that.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":10,"context_line":"scheduled. However, the exception handling routine in compute manager"},{"line_number":11,"context_line":"[1] will set instance.host \u003d None and set instance.vm_state \u003d ERROR"},{"line_number":12,"context_line":"if the instance fails to build on the compute host. If that happens, we"},{"line_number":13,"context_line":"end up with an instance with host \u003d None, vm_state \u003d ERROR, and ports"},{"line_number":14,"context_line":"and volumes still allocated on the compute host (if the instance had"},{"line_number":15,"context_line":"networking and volumes)."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Ideally, in this case, we would want to do a terminate instance over"},{"line_number":18,"context_line":"RPC to have the compute node clean up the instance and its ports and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_691779ef","line":15,"range":{"start_line":13,"start_character":60,"end_line":15,"end_character":24},"in_reply_to":"3fa0c359_da1d3074","updated":"2018-02-15 15:56:49.000000000","message":"I think ports will likely not need to be cleaned up as nova-compute creates the ports and it will delete them if things fail.  If the instance never got scheduled in the first place, then the ports were never created.\n\nRegardless, if this instance does end up getting deleted locally, it does clean up and deallocate instance in here:\n\nhttps://github.com/openstack/nova/blob/b5bd19c6/nova/compute/api.py#L2055-L2056","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So, the best we can do is run through the \"local delete\" routine in the"},{"line_number":23,"context_line":"compute api to delete the instance record from the database and free up"},{"line_number":24,"context_line":"ports and volumes. Then, when the reap task runs on the compute host"},{"line_number":25,"context_line":"the instance virt domain lives on, it will detect that the instance"},{"line_number":26,"context_line":"record has been deleted from the database and will clean up the"},{"line_number":27,"context_line":"orphaned virt domain."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"This adds some logic around deleting the instance when we expect"},{"line_number":30,"context_line":"instance.host to be None."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_00756a98","line":27,"range":{"start_line":24,"start_character":19,"end_line":27,"end_character":21},"updated":"2018-02-15 15:04:56.000000000","message":"We shouldn\u0027t even have a guest on the hypervisor if driver.spawn() failed. Maybe there is some other scenario where we can fail after the guest is created but before we consider the build complete?","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So, the best we can do is run through the \"local delete\" routine in the"},{"line_number":23,"context_line":"compute api to delete the instance record from the database and free up"},{"line_number":24,"context_line":"ports and volumes. Then, when the reap task runs on the compute host"},{"line_number":25,"context_line":"the instance virt domain lives on, it will detect that the instance"},{"line_number":26,"context_line":"record has been deleted from the database and will clean up the"},{"line_number":27,"context_line":"orphaned virt domain."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"This adds some logic around deleting the instance when we expect"},{"line_number":30,"context_line":"instance.host to be None."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_092d9545","line":27,"range":{"start_line":24,"start_character":19,"end_line":27,"end_character":21},"in_reply_to":"3fa0c359_00756a98","updated":"2018-02-15 15:56:49.000000000","message":"I agree, this should never be the case.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So, the best we can do is run through the \"local delete\" routine in the"},{"line_number":23,"context_line":"compute api to delete the instance record from the database and free up"},{"line_number":24,"context_line":"ports and volumes. Then, when the reap task runs on the compute host"},{"line_number":25,"context_line":"the instance virt domain lives on, it will detect that the instance"},{"line_number":26,"context_line":"record has been deleted from the database and will clean up the"},{"line_number":27,"context_line":"orphaned virt domain."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"This adds some logic around deleting the instance when we expect"},{"line_number":30,"context_line":"instance.host to be None."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_9dca389f","line":27,"range":{"start_line":24,"start_character":19,"end_line":27,"end_character":21},"in_reply_to":"3fa0c359_092d9545","updated":"2018-02-15 20:36:38.000000000","message":"Yeah ... we really shouldn\u0027t. It even says so here:\n\nhttps://github.com/openstack/nova/blob/82db86c/nova/compute/manager.py#L1921\n\nI probably shouldn\u0027t mention it in the commit message since it\u0027s supposed to not be possible. I\u0027ll remove this part.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"So, the best we can do is run through the \"local delete\" routine in the"},{"line_number":23,"context_line":"compute api to delete the instance record from the database and free up"},{"line_number":24,"context_line":"ports and volumes. Then, when the reap task runs on the compute host"},{"line_number":25,"context_line":"the instance virt domain lives on, it will detect that the instance"},{"line_number":26,"context_line":"record has been deleted from the database and will clean up the"},{"line_number":27,"context_line":"orphaned virt domain."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"This adds some logic around deleting the instance when we expect"},{"line_number":30,"context_line":"instance.host to be None."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_e02953f2","line":27,"range":{"start_line":24,"start_character":19,"end_line":27,"end_character":21},"in_reply_to":"3fa0c359_9dca389f","updated":"2018-02-15 21:50:57.000000000","message":"Yeah I think we really don\u0027t want to have stuff in the local delete API code that assumes, \"well, I did my best, hopefully the reap task in the compute takes care of the rest\" because that worries me. Best to just not mention that stuff. If we\u0027re not cleaning up stuff in the compute we should definitely be opening bugs for those.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":30,"context_line":"instance.host to be None."},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"  1. If the instance is not in ERROR or SHELVED_OFFLOADED state, we"},{"line_number":33,"context_line":"     expect instance.host to be set to a compute host. So, if we find"},{"line_number":34,"context_line":"     instance.host \u003d None in states other than ERROR or"},{"line_number":35,"context_line":"     SHELVED_OFFLOADED, we consider the instance to have failed"},{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_c03f8285","line":35,"range":{"start_line":33,"start_character":55,"end_line":35,"end_character":22},"updated":"2018-02-15 15:04:56.000000000","message":"When can this happen?\n\nOne way could be that we\u0027re still building the instance (it\u0027s going through scheduling) and the user attempts to delete it while it\u0027s building. That\u0027s the first big block in the \u0027local delete\u0027 code which attempts to remove a build request and delete the instance from cell0. But in that case, we\u0027d still likely orphan volumes that were reserved by the API before casting to conductor to do scheduling, pick a host, and create the instance in the cell that the chosen host is in.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":30,"context_line":"instance.host to be None."},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"  1. If the instance is not in ERROR or SHELVED_OFFLOADED state, we"},{"line_number":33,"context_line":"     expect instance.host to be set to a compute host. So, if we find"},{"line_number":34,"context_line":"     instance.host \u003d None in states other than ERROR or"},{"line_number":35,"context_line":"     SHELVED_OFFLOADED, we consider the instance to have failed"},{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_6026a319","line":35,"range":{"start_line":33,"start_character":55,"end_line":35,"end_character":22},"in_reply_to":"3fa0c359_c03f8285","updated":"2018-02-15 21:50:57.000000000","message":"I\u0027ve got a fix for this here:\n\nhttps://review.openstack.org/#/c/545132/\n\nNot sure how you want to wordsmith that.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"  2. If the instance is in ERROR or SHELVED_OFFLOADED state, we expect"},{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_00048ac5","line":40,"range":{"start_line":39,"start_character":5,"end_line":40,"end_character":29},"updated":"2018-02-15 15:04:56.000000000","message":"This is pretty dicey. There are plenty of cases that the instance could be in ERROR state with the host set, like if reboot fails:\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L3220","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"  2. If the instance is in ERROR or SHELVED_OFFLOADED state, we expect"},{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_dd5e80e8","line":40,"range":{"start_line":39,"start_character":5,"end_line":40,"end_character":29},"in_reply_to":"3fa0c359_00048ac5","updated":"2018-02-15 20:36:38.000000000","message":"Yeah, but that case is already covered. The code already does the right thing when host is set. In this patch, we\u0027re trying to cover the case where instance.host \u003d None and we bypass local delete because we think there can\u0027t be ports or volumes.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5e806978bb871df58bb571445f7f2962fac21877","unresolved":false,"context_lines":[{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"  2. If the instance is in ERROR or SHELVED_OFFLOADED state, we expect"},{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_568d6d79","line":40,"range":{"start_line":39,"start_character":5,"end_line":40,"end_character":29},"in_reply_to":"3fa0c359_7bb0948c","updated":"2018-02-15 22:54:55.000000000","message":"Sure, we do expect it for that very specific scenario, but my point is that just because the instance is in ERROR state doesn\u0027t mean we should expect the instance.host is None - like my failed reboot example.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"805251bf5c79ca17f1fc9d71be00017c22bd26dd","unresolved":false,"context_lines":[{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"  2. If the instance is in ERROR or SHELVED_OFFLOADED state, we expect"},{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_7bb0948c","line":40,"range":{"start_line":39,"start_character":5,"end_line":40,"end_character":29},"in_reply_to":"3fa0c359_a0810bc6","updated":"2018-02-15 22:44:50.000000000","message":"We do expect instance.host \u003d None when vm_state ERROR because the compute manager actively sets instance.host \u003d None at the same time it sets vm_state \u003d ERROR:\n\nhttps://github.com/openstack/nova/blob/bfae5f28a4c3b39f7978d5f3015c1b32be81215d/nova/compute/manager.py#L1916-L1917\n\nThat\u0027s how this bug started, when compute manager started setting instance.host \u003d None during an error.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":36,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":37,"context_line":"     simply destroy the instance database record and return."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"  2. If the instance is in ERROR or SHELVED_OFFLOADED state, we expect"},{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_a0810bc6","line":40,"range":{"start_line":39,"start_character":5,"end_line":40,"end_character":29},"in_reply_to":"3fa0c359_dd5e80e8","updated":"2018-02-15 21:50:57.000000000","message":"Sure, but I take issue with the \"we expect instance.host to be None\" in here when the vm_state is ERROR, because we shouldn\u0027t expect the instance.host to be None in that case. SHELVED_OFFLOADED, yes totally.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"},{"line_number":44,"context_line":"     have vifs plugged and volumes attached at the virt domain level"},{"line_number":45,"context_line":"     and will not be cleaned up until the next periodic reap task"},{"line_number":46,"context_line":"     interval fires."},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"Co-Authored-By: Ankit Agrawal \u003cankit11.agrawal@nttdata.com\u003e"},{"line_number":49,"context_line":"Co-Authored-By: Samuel Matzek \u003csmatzek@us.ibm.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_c026a269","line":46,"range":{"start_line":43,"start_character":22,"end_line":46,"end_character":20},"updated":"2018-02-15 15:04:56.000000000","message":"OK you\u0027re talking about https://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L7350 and CONF.running_deleted_instance_action does default to \u0027reap\u0027.\n\nStill, if the instance.host is not None, we should be casting to compute to do the proper cleanup regardless of the ERROR state.\n\nLooking at how _has_been_scheduled works:\n\n    def _has_been_scheduled(self, instance):\n        # NOTE(melwitt): When an instance build fails in the compute manager,\n        # the instance host and node are set to None and the vm_state is set\n        # to ERROR. In the case, the instance with host \u003d None has actually\n        # been scheduled and has resources allocated on the compute node.\n        if instance.host or instance.vm_state in (vm_states.SHELVED_OFFLOADED,\n                                                  vm_states.ERROR):\n            return True\n        return False\n\nWe will bypass the local delete if instance.host is set and RPC cast to the compute (assuming it\u0027s not down), so that\u0027s good. I think the commit message is just confusing in this regard.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":40,"context_line":"     instance.host to be None even though there could be ports and"},{"line_number":41,"context_line":"     volumes allocated. In this case, run through the _local_delete"},{"line_number":42,"context_line":"     routine to clean up ports and volumes and delete the instance"},{"line_number":43,"context_line":"     database record. Note that instances in this situation will still"},{"line_number":44,"context_line":"     have vifs plugged and volumes attached at the virt domain level"},{"line_number":45,"context_line":"     and will not be cleaned up until the next periodic reap task"},{"line_number":46,"context_line":"     interval fires."},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"Co-Authored-By: Ankit Agrawal \u003cankit11.agrawal@nttdata.com\u003e"},{"line_number":49,"context_line":"Co-Authored-By: Samuel Matzek \u003csmatzek@us.ibm.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_5df7d0d5","line":46,"range":{"start_line":43,"start_character":22,"end_line":46,"end_character":20},"in_reply_to":"3fa0c359_c026a269","updated":"2018-02-15 20:36:38.000000000","message":"Yeah, I\u0027ll rewrite the commit message again and try to find a better name for the function.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"Closes-Bug: 1404867"},{"line_number":53,"context_line":"Closes-Bug: 1408527"},{"line_number":54,"context_line":"Closes-Bug: 1594604"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"[1] https://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L1927-L1929"},{"line_number":57,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_9d0d58ea","line":54,"range":{"start_line":54,"start_character":0,"end_line":54,"end_character":19},"updated":"2018-02-15 20:36:38.000000000","message":"This should be removed. I think this bug was fixed back in mitaka by:\n\nhttps://review.openstack.org/#/c/243477","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"Closes-Bug: 1404867"},{"line_number":53,"context_line":"Closes-Bug: 1408527"},{"line_number":54,"context_line":"Closes-Bug: 1594604"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"[1] https://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L1927-L1929"},{"line_number":57,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3fa0c359_c08697dc","line":54,"range":{"start_line":54,"start_character":0,"end_line":54,"end_character":19},"in_reply_to":"3fa0c359_9d0d58ea","updated":"2018-02-15 21:50:57.000000000","message":"Sure, and it\u0027s still going strong:\n\nhttps://review.openstack.org/#/c/520248/","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2d2a9a2fc7121de9e056ac3e0c7aeb16f1b9ffea","unresolved":false,"context_lines":[{"line_number":19,"context_line":"  1. If the instance is not in ERROR or SHELVED_OFFLOADED state, we"},{"line_number":20,"context_line":"     expect instance.host to be set to a compute host. So, if we find"},{"line_number":21,"context_line":"     instance.host \u003d None in states other than ERROR or"},{"line_number":22,"context_line":"     SHELVED_OFFLOADED, we consider the instance to have failed"},{"line_number":23,"context_line":"     scheduling and not require ports or volumes to be freed, and we"},{"line_number":24,"context_line":"     simply destroy the instance database record and return. This is"},{"line_number":25,"context_line":"     the \"delete while booting\" scenario."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"  2. If the instance is in ERROR because of a failed build or is"},{"line_number":28,"context_line":"     SHELVED_OFFLOADED, we expect instance.host to be None even though"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"3fa0c359_23cf2c80","line":25,"range":{"start_line":22,"start_character":24,"end_line":25,"end_character":41},"updated":"2018-02-16 23:12:15.000000000","message":"Yeah so we shouldn\u0027t really consider the instance to have failed scheduling - if the instance failed scheduling, it should be in ERROR state.\n\nWe can hit the code where instance.host is None and the status is \u0027building\u0027, before the instance is created in a cell, and the instance is deleted via the build request, and we fail to cleanup volumes - that\u0027s my fix later in the series.\n\nThat code doesn\u0027t need to worry about cleaning up ports because it can only happen (1) before we get to the compute and (2) ports are only allocated/bound to the instance on the compute.","commit_id":"c6f0d694ec71daf6156a3f55763025454b32ff3d"}],"nova/compute/api.py":[{"author":{"_account_id":6685,"name":"Baodong (Robert) Li","email":"baoli@cisco.com","username":"baoli"},"change_message_id":"288f54b1cf55617e1581bcfc021691111299cfb8","unresolved":false,"context_lines":[{"line_number":2042,"context_line":"                LOG.debug(\u0027Compute host %s not found during service up check, \u0027"},{"line_number":2043,"context_line":"                          \u0027going to local delete instance\u0027, instance.host,"},{"line_number":2044,"context_line":"                          instance\u003dinstance)"},{"line_number":2045,"context_line":"                pass"},{"line_number":2046,"context_line":""},{"line_number":2047,"context_line":"            if is_local_delete:"},{"line_number":2048,"context_line":"                # If instance is in shelved_offloaded state or compute node"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f201791_dcf07c6a","line":2045,"range":{"start_line":2045,"start_character":16,"end_line":2045,"end_character":20},"updated":"2017-06-22 13:55:25.000000000","message":"remove pass","commit_id":"9e93a2b359b72db1f4c8139f9bb17aeabc46efbb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"87737069c6e75080cb2e9c1ccb75e5730ff323d4","unresolved":false,"context_lines":[{"line_number":2042,"context_line":"                LOG.debug(\u0027Compute host %s not found during service up check, \u0027"},{"line_number":2043,"context_line":"                          \u0027going to local delete instance\u0027, instance.host,"},{"line_number":2044,"context_line":"                          instance\u003dinstance)"},{"line_number":2045,"context_line":"                pass"},{"line_number":2046,"context_line":""},{"line_number":2047,"context_line":"            if is_local_delete:"},{"line_number":2048,"context_line":"                # If instance is in shelved_offloaded state or compute node"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f201791_5199ee6b","line":2045,"range":{"start_line":2045,"start_character":16,"end_line":2045,"end_character":20},"in_reply_to":"5f201791_dcf07c6a","updated":"2017-06-22 17:24:24.000000000","message":"Oops, thanks.","commit_id":"9e93a2b359b72db1f4c8139f9bb17aeabc46efbb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"80a491769a48c9ccc91a7371c2ce10fa0aa4b1ac","unresolved":false,"context_lines":[{"line_number":2007,"context_line":"                          \u0027the instance host %(instance_host)s.\u0027,"},{"line_number":2008,"context_line":"                          {\u0027connector_host\u0027: connector.get(\u0027host\u0027),"},{"line_number":2009,"context_line":"                           \u0027instance_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":2010,"context_line":"                if (instance.host is None and"},{"line_number":2011,"context_line":"                        self._has_been_scheduled(instance)):"},{"line_number":2012,"context_line":"                    LOG.debug(\u0027Allowing use of stashed volume connector with \u0027"},{"line_number":2013,"context_line":"                              \u0027instance host None because instance with \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_e330a1eb","line":2010,"range":{"start_line":2010,"start_character":16,"end_line":2010,"end_character":18},"updated":"2018-02-14 18:54:18.000000000","message":"This should probably be elif.","commit_id":"dc2b6d540ea97fc626d49e33a086ebc50db324fc"},{"author":{"_account_id":14084,"name":"Einst Crazy","email":"yu.changcai@99cloud.net","username":"Einst"},"change_message_id":"f76259118b7ba6a35e27f99dacae75d870c87026","unresolved":false,"context_lines":[{"line_number":2007,"context_line":"                          \u0027the instance host %(instance_host)s.\u0027,"},{"line_number":2008,"context_line":"                          {\u0027connector_host\u0027: connector.get(\u0027host\u0027),"},{"line_number":2009,"context_line":"                           \u0027instance_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":2010,"context_line":"                if (instance.host is None and"},{"line_number":2011,"context_line":"                        self._has_been_scheduled(instance)):"},{"line_number":2012,"context_line":"                    LOG.debug(\u0027Allowing use of stashed volume connector with \u0027"},{"line_number":2013,"context_line":"                              \u0027instance host None because instance with \u0027"},{"line_number":2014,"context_line":"                              \u0027vm_state %(vm_state)s has been scheduled in \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_2e43f743","line":2011,"range":{"start_line":2010,"start_character":20,"end_line":2011,"end_character":58},"updated":"2018-02-13 14:56:46.000000000","message":"self._has_been_scheduled(instance)\nis enough","commit_id":"dc2b6d540ea97fc626d49e33a086ebc50db324fc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"80a491769a48c9ccc91a7371c2ce10fa0aa4b1ac","unresolved":false,"context_lines":[{"line_number":2007,"context_line":"                          \u0027the instance host %(instance_host)s.\u0027,"},{"line_number":2008,"context_line":"                          {\u0027connector_host\u0027: connector.get(\u0027host\u0027),"},{"line_number":2009,"context_line":"                           \u0027instance_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":2010,"context_line":"                if (instance.host is None and"},{"line_number":2011,"context_line":"                        self._has_been_scheduled(instance)):"},{"line_number":2012,"context_line":"                    LOG.debug(\u0027Allowing use of stashed volume connector with \u0027"},{"line_number":2013,"context_line":"                              \u0027instance host None because instance with \u0027"},{"line_number":2014,"context_line":"                              \u0027vm_state %(vm_state)s has been scheduled in \u0027"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa0c359_c361e5d5","line":2011,"range":{"start_line":2010,"start_character":20,"end_line":2011,"end_character":58},"in_reply_to":"3fa0c359_2e43f743","updated":"2018-02-14 18:54:18.000000000","message":"I think we need to also check for None, else we would defeat the purpose of the check on L2003. We want to use the stashed connector if the host matches *or* if there is no host set and the instance has been scheduled before. We don\u0027t want to use the stashed connector in the case of connector.get(\u0027host\u0027) !\u003d instance.host and the host is not None. The None case is a special case for an instance that has been scheduled in the past but has had its host set to None because of an ERROR or being SHELVED_OFFLOADED.","commit_id":"dc2b6d540ea97fc626d49e33a086ebc50db324fc"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":1703,"context_line":"                    return None, None"},{"line_number":1704,"context_line":"        return cell, instance"},{"line_number":1705,"context_line":""},{"line_number":1706,"context_line":"    def _delete_while_booting(self, context, instance):"},{"line_number":1707,"context_line":"        \"\"\"Handle deletion if the instance has not reached a cell yet"},{"line_number":1708,"context_line":""},{"line_number":1709,"context_line":"        Deletion before an instance reaches a cell needs to be handled"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_66180675","line":1706,"range":{"start_line":1706,"start_character":4,"end_line":1706,"end_character":55},"updated":"2018-02-15 15:56:49.000000000","message":"Could we add:\n\n self._local_cleanup_bdm_volumes(bdms, instance, context)\n\nSomewhere here so that we can clean up volumes which were reserved?","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":1703,"context_line":"                    return None, None"},{"line_number":1704,"context_line":"        return cell, instance"},{"line_number":1705,"context_line":""},{"line_number":1706,"context_line":"    def _delete_while_booting(self, context, instance):"},{"line_number":1707,"context_line":"        \"\"\"Handle deletion if the instance has not reached a cell yet"},{"line_number":1708,"context_line":""},{"line_number":1709,"context_line":"        Deletion before an instance reaches a cell needs to be handled"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_74ec2cf3","line":1706,"range":{"start_line":1706,"start_character":4,"end_line":1706,"end_character":55},"in_reply_to":"3fa0c359_66180675","updated":"2018-02-15 16:39:55.000000000","message":"Yeah possibly as a separate patch - like I said, we can get the bdms off the build request. We also have a fixture we can use for a functional regression test which blocks the rpc cast to conductor, so that we can simulate the case that we\u0027ve done the stuff in the API and then delete the instance (really the build request) so we can make sure things get cleaned up properly. I can start working on that functional regression test too.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":1703,"context_line":"                    return None, None"},{"line_number":1704,"context_line":"        return cell, instance"},{"line_number":1705,"context_line":""},{"line_number":1706,"context_line":"    def _delete_while_booting(self, context, instance):"},{"line_number":1707,"context_line":"        \"\"\"Handle deletion if the instance has not reached a cell yet"},{"line_number":1708,"context_line":""},{"line_number":1709,"context_line":"        Deletion before an instance reaches a cell needs to be handled"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_2075fbfa","line":1706,"range":{"start_line":1706,"start_character":4,"end_line":1706,"end_character":55},"in_reply_to":"3fa0c359_74ec2cf3","updated":"2018-02-15 21:50:57.000000000","message":"https://review.openstack.org/#/c/520248/","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":1787,"context_line":"        has_been_scheduled \u003d self._has_been_scheduled(instance)"},{"line_number":1788,"context_line":"        if not has_been_scheduled:"},{"line_number":1789,"context_line":"            try:"},{"line_number":1790,"context_line":"                if self._delete_while_booting(context, instance):"},{"line_number":1791,"context_line":"                    return"},{"line_number":1792,"context_line":"                # If instance.host was not set it\u0027s possible that the Instance"},{"line_number":1793,"context_line":"                # object here was pulled from a BuildRequest object and is not"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_404e12c2","line":1790,"range":{"start_line":1790,"start_character":24,"end_line":1790,"end_character":45},"updated":"2018-02-15 15:04:56.000000000","message":"If we got here and deleted the build request before conductor created the instance/bdms in a cell, and we\u0027re doing boot from volume where the API reserved the volume, then we\u0027re still going to orphan the volume because _delete_while_booting doesn\u0027t unreserve the volume (nor does conductor when it finds out that the build request is deleted once it goes to create the instance mapping). So we\u0027ve still got a gap here. Don\u0027t know if we just TODO that for now or what. The BuildRequest does have the BDMs so we could reconstruct the volumes that we need to unreserve.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":1787,"context_line":"        has_been_scheduled \u003d self._has_been_scheduled(instance)"},{"line_number":1788,"context_line":"        if not has_been_scheduled:"},{"line_number":1789,"context_line":"            try:"},{"line_number":1790,"context_line":"                if self._delete_while_booting(context, instance):"},{"line_number":1791,"context_line":"                    return"},{"line_number":1792,"context_line":"                # If instance.host was not set it\u0027s possible that the Instance"},{"line_number":1793,"context_line":"                # object here was pulled from a BuildRequest object and is not"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_a37280e2","line":1790,"range":{"start_line":1790,"start_character":24,"end_line":1790,"end_character":45},"in_reply_to":"3fa0c359_404e12c2","updated":"2018-02-15 15:56:49.000000000","message":"You\u0027re right about this, if the instance gets deleted while it isn\u0027t assigned to a cell yet, it does not go through the clean-up stages.  I\u0027ve suggested a fix above (though not sure about how clean it is)","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":1796,"context_line":"                # properly. A lookup is attempted which will either return a"},{"line_number":1797,"context_line":"                # full Instance or None if not found. If not found then it\u0027s"},{"line_number":1798,"context_line":"                # acceptable to skip the rest of the delete processing."},{"line_number":1799,"context_line":"                cell, instance \u003d self._lookup_instance(context, instance.uuid)"},{"line_number":1800,"context_line":"                if cell and instance:"},{"line_number":1801,"context_line":"                    try:"},{"line_number":1802,"context_line":"                        # Now destroy the instance from the cell it lives in."},{"line_number":1803,"context_line":"                        with compute_utils.notify_about_instance_delete("},{"line_number":1804,"context_line":"                                self.notifier, context, instance):"},{"line_number":1805,"context_line":"                            instance.destroy()"},{"line_number":1806,"context_line":"                    except exception.InstanceNotFound:"},{"line_number":1807,"context_line":"                        pass"},{"line_number":1808,"context_line":"                    # The instance was deleted or is already gone."},{"line_number":1809,"context_line":"                    return"},{"line_number":1810,"context_line":"                if not instance:"},{"line_number":1811,"context_line":"                    # Instance is already deleted."},{"line_number":1812,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_80873ab7","line":1809,"range":{"start_line":1799,"start_character":16,"end_line":1809,"end_character":26},"updated":"2018-02-15 15:04:56.000000000","message":"This is the cell0 case right? Scheduling in conductor fails and the instance is created in cell0. Here again we don\u0027t do anything about unreserving the volume, we just return.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":1796,"context_line":"                # properly. A lookup is attempted which will either return a"},{"line_number":1797,"context_line":"                # full Instance or None if not found. If not found then it\u0027s"},{"line_number":1798,"context_line":"                # acceptable to skip the rest of the delete processing."},{"line_number":1799,"context_line":"                cell, instance \u003d self._lookup_instance(context, instance.uuid)"},{"line_number":1800,"context_line":"                if cell and instance:"},{"line_number":1801,"context_line":"                    try:"},{"line_number":1802,"context_line":"                        # Now destroy the instance from the cell it lives in."},{"line_number":1803,"context_line":"                        with compute_utils.notify_about_instance_delete("},{"line_number":1804,"context_line":"                                self.notifier, context, instance):"},{"line_number":1805,"context_line":"                            instance.destroy()"},{"line_number":1806,"context_line":"                    except exception.InstanceNotFound:"},{"line_number":1807,"context_line":"                        pass"},{"line_number":1808,"context_line":"                    # The instance was deleted or is already gone."},{"line_number":1809,"context_line":"                    return"},{"line_number":1810,"context_line":"                if not instance:"},{"line_number":1811,"context_line":"                    # Instance is already deleted."},{"line_number":1812,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_a61dae10","line":1809,"range":{"start_line":1799,"start_character":16,"end_line":1809,"end_character":26},"in_reply_to":"3fa0c359_80873ab7","updated":"2018-02-15 15:56:49.000000000","message":"This is not the case from what I understand, as `has_been_scheduled` will be True if an instance failed to schedule and ended up in cell0.\n\ninstance.host will be empty, so self._has_been_scheduled(instance) will return True, \"if not True\" \u003d\u003e this block will be skipped.\n\nIf the instance is scheduled and ACTIVE, then this block will be processed, but now that I think about it, does that mean because of the return, it will not clean-up BDMs (or does the compute service do this?)","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":1796,"context_line":"                # properly. A lookup is attempted which will either return a"},{"line_number":1797,"context_line":"                # full Instance or None if not found. If not found then it\u0027s"},{"line_number":1798,"context_line":"                # acceptable to skip the rest of the delete processing."},{"line_number":1799,"context_line":"                cell, instance \u003d self._lookup_instance(context, instance.uuid)"},{"line_number":1800,"context_line":"                if cell and instance:"},{"line_number":1801,"context_line":"                    try:"},{"line_number":1802,"context_line":"                        # Now destroy the instance from the cell it lives in."},{"line_number":1803,"context_line":"                        with compute_utils.notify_about_instance_delete("},{"line_number":1804,"context_line":"                                self.notifier, context, instance):"},{"line_number":1805,"context_line":"                            instance.destroy()"},{"line_number":1806,"context_line":"                    except exception.InstanceNotFound:"},{"line_number":1807,"context_line":"                        pass"},{"line_number":1808,"context_line":"                    # The instance was deleted or is already gone."},{"line_number":1809,"context_line":"                    return"},{"line_number":1810,"context_line":"                if not instance:"},{"line_number":1811,"context_line":"                    # Instance is already deleted."},{"line_number":1812,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_b4907450","line":1809,"range":{"start_line":1799,"start_character":16,"end_line":1809,"end_character":26},"in_reply_to":"3fa0c359_a61dae10","updated":"2018-02-15 16:39:55.000000000","message":"\u003e This is not the case from what I understand, as `has_been_scheduled` will be True if an instance failed to schedule and ended up in cell0.\n\ninstance.host will be empty, so self._has_been_scheduled(instance) will return True, \"if not True\" \u003d\u003e this block will be skipped.\n\n_has_been_scheduled returns returns True if instance.host is None, so the opposite of what you said.\n\nBut I think you clarified one issue for me, which is the ERROR state. So when the instance is in cell0, instance.host is None and vm_state\u003d\u0027ERROR\u0027 which means _has_been_scheduled returns True, so we don\u0027t get into this block. That was the major issue for me here.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":1823,"context_line":"                    # Instance is already deleted"},{"line_number":1824,"context_line":"                    return"},{"line_number":1825,"context_line":""},{"line_number":1826,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1827,"context_line":"                context, instance.uuid)"},{"line_number":1828,"context_line":""},{"line_number":1829,"context_line":"        # At these states an instance has a snapshot associate."},{"line_number":1830,"context_line":"        if instance.vm_state in (vm_states.SHELVED,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_63d6b8f4","line":1827,"range":{"start_line":1826,"start_character":8,"end_line":1827,"end_character":39},"updated":"2018-02-15 15:04:56.000000000","message":"If the BDMs are stored in cell0, and we pulled the instance from cell0 above, but didn\u0027t destroy it for some reason, we should need to be using a cell0-targeted context here...so again I\u0027m wondering how mnaser\u0027s functional test is working, unless it\u0027s chalked up to a problem with our cell db fixture...","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":1823,"context_line":"                    # Instance is already deleted"},{"line_number":1824,"context_line":"                    return"},{"line_number":1825,"context_line":""},{"line_number":1826,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1827,"context_line":"                context, instance.uuid)"},{"line_number":1828,"context_line":""},{"line_number":1829,"context_line":"        # At these states an instance has a snapshot associate."},{"line_number":1830,"context_line":"        if instance.vm_state in (vm_states.SHELVED,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_c6db1a4d","line":1827,"range":{"start_line":1826,"start_character":8,"end_line":1827,"end_character":39},"in_reply_to":"3fa0c359_63d6b8f4","updated":"2018-02-15 15:56:49.000000000","message":"The instance does not get destroyed at this stage as per the comment above if I understand correctly","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":1823,"context_line":"                    # Instance is already deleted"},{"line_number":1824,"context_line":"                    return"},{"line_number":1825,"context_line":""},{"line_number":1826,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1827,"context_line":"                context, instance.uuid)"},{"line_number":1828,"context_line":""},{"line_number":1829,"context_line":"        # At these states an instance has a snapshot associate."},{"line_number":1830,"context_line":"        if instance.vm_state in (vm_states.SHELVED,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_5476d040","line":1827,"range":{"start_line":1826,"start_character":8,"end_line":1827,"end_character":39},"in_reply_to":"3fa0c359_c6db1a4d","updated":"2018-02-15 16:39:55.000000000","message":"Yeah, so in this case, with my corrected understanding, we have a real instance in cell0 and when the API looked up that instance here:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/api.py#L2233\n\nIt targeted the context to cell0 so that\u0027s how we\u0027re able to pull the BDMs from cell0.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":1946,"context_line":"        # NOTE(melwitt): When an instance build fails in the compute manager,"},{"line_number":1947,"context_line":"        # the instance host and node are set to None and the vm_state is set"},{"line_number":1948,"context_line":"        # to ERROR. In the case, the instance with host \u003d None has actually"},{"line_number":1949,"context_line":"        # been scheduled and has resources allocated on the compute node."},{"line_number":1950,"context_line":"        if instance.host or instance.vm_state in (vm_states.SHELVED_OFFLOADED,"},{"line_number":1951,"context_line":"                                                  vm_states.ERROR):"},{"line_number":1952,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_2351900f","line":1949,"range":{"start_line":1949,"start_character":25,"end_line":1949,"end_character":73},"updated":"2018-02-15 15:04:56.000000000","message":"This is not true. There are two things:\n\n1. The claim would be aborted if an exception was raised while in this context manager:\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L2007\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/compute/claims.py#L124\n\n2. The allocation, created by the scheduler, would be deleted here:\n\nhttps://github.com/openstack/nova/blob/55ea961/nova/compute/manager.py#L1773","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":1946,"context_line":"        # NOTE(melwitt): When an instance build fails in the compute manager,"},{"line_number":1947,"context_line":"        # the instance host and node are set to None and the vm_state is set"},{"line_number":1948,"context_line":"        # to ERROR. In the case, the instance with host \u003d None has actually"},{"line_number":1949,"context_line":"        # been scheduled and has resources allocated on the compute node."},{"line_number":1950,"context_line":"        if instance.host or instance.vm_state in (vm_states.SHELVED_OFFLOADED,"},{"line_number":1951,"context_line":"                                                  vm_states.ERROR):"},{"line_number":1952,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_dd2c404c","line":1949,"range":{"start_line":1949,"start_character":25,"end_line":1949,"end_character":73},"in_reply_to":"3fa0c359_2351900f","updated":"2018-02-15 20:36:38.000000000","message":"By \"resources allocated\" I meant ports and/or volumes.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6e4a275e56a0549d7375623756019718e3aafa79","unresolved":false,"context_lines":[{"line_number":1946,"context_line":"        # NOTE(melwitt): When an instance build fails in the compute manager,"},{"line_number":1947,"context_line":"        # the instance host and node are set to None and the vm_state is set"},{"line_number":1948,"context_line":"        # to ERROR. In the case, the instance with host \u003d None has actually"},{"line_number":1949,"context_line":"        # been scheduled and has resources allocated on the compute node."},{"line_number":1950,"context_line":"        if instance.host or instance.vm_state in (vm_states.SHELVED_OFFLOADED,"},{"line_number":1951,"context_line":"                                                  vm_states.ERROR):"},{"line_number":1952,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_c05f7770","line":1949,"range":{"start_line":1949,"start_character":25,"end_line":1949,"end_character":73},"in_reply_to":"3fa0c359_dd2c404c","updated":"2018-02-15 21:50:57.000000000","message":"OK, \u0027resources allocated\u0027 to me is claims and allocations in Placement, so can we re-word this to be more specific?","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"6015bc14225650f49ea7bd8c26945392be543be6","unresolved":false,"context_lines":[{"line_number":2026,"context_line":"                try:"},{"line_number":2027,"context_line":"                    if bdm.attachment_id:"},{"line_number":2028,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2029,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2030,"context_line":"                    else:"},{"line_number":2031,"context_line":"                        connector \u003d self._get_stashed_volume_connector("},{"line_number":2032,"context_line":"                            bdm, instance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_b2c15a31","line":2029,"updated":"2018-02-16 05:44:36.000000000","message":"From doing some testing with devstack, I\u0027ve found that this indeed is a no-op with status 200 if the volume isn\u0027t attached. BUT, it doesn\u0027t set the volume status back to \u0027available\u0027, it stays \u0027reserved\u0027 which causes the volume delete on L2063 to fail.\n\nThis is strange because the call that made the volume \u0027reserved\u0027 was volume_api.attachment_create. Why does volume_api.attachment_delete not reverse that?\n\nREQ: curl -g -i -X DELETE http://127.0.0.1/volume/v3/bf31b39813684fc68fdd30f807da9deb/attachments/f566a447-1f25-45f6-a0f0-65726139c409 -H \"OpenStack-API-Version: volume 3.44\"\nRESP: [200]\nRESP BODY: {\"attachments\": [{\"status\": \"reserved\", \"instance\": \"b505e206-be88-41d2-8329-b17320036be6\", \"id\": \"3db96a40-7cea-4260-b8f4-b129797cbddf\", \"volume_id\": \"6f11839e-a1a7-4fb3-b22f-943db027912f\"}, {\"status\": \"reserved\", \"instance\": \"540a2334-ded5-4d94-b125-92546e05665a\", \"id\": \"5b8d9d1f-937b-4e1a-bdef-6757fb3578b2\", \"volume_id\": \"6f11839e-a1a7-4fb3-b22f-943db027912f\"}, {\"status\": \"reserved\", \"instance\": \"072d7211-3965-4da2-b223-f8c573ea91d0\", \"id\": \"9fa494af-15f1-4dc4-937f-915bd9bc2c84\", \"volume_id\": \"6f11839e-a1a7-4fb3-b22f-943db027912f\"}, {\"status\": \"reserved\", \"instance\": \"a7585478-579a-4f9f-8040-ef4ad8dcaa27\", \"id\": \"c52cf27e-e4f6-40a4-852f-53878a486e92\", \"volume_id\": \"6f11839e-a1a7-4fb3-b22f-943db027912f\"}]}\n\nThe scenario I\u0027m doing in devstack is to do BFV with an already existing volume + shutdown\u003dremove and making it fail on the compute host by stopping libvirtd. The instance ends up in ERROR state in cell1 with a volume in \u0027reserved\u0027 state.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c01887df1760a324284a8a6813476ffcf6739f71","unresolved":false,"context_lines":[{"line_number":2026,"context_line":"                try:"},{"line_number":2027,"context_line":"                    if bdm.attachment_id:"},{"line_number":2028,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2029,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2030,"context_line":"                    else:"},{"line_number":2031,"context_line":"                        connector \u003d self._get_stashed_volume_connector("},{"line_number":2032,"context_line":"                            bdm, instance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_43459814","line":2029,"in_reply_to":"3fa0c359_b2c15a31","updated":"2018-02-16 22:59:57.000000000","message":"(4:53:22 PM) mriedem: melwitt: did you figure this out? https://review.openstack.org/#/c/340614/18/nova/compute/api.py@2029\n(4:53:27 PM) mriedem: you had \u003e1 attachment right\u003e\n(4:53:27 PM) mriedem: ?\n(4:53:41 PM) mriedem: and that\u0027s why the volume status wasn\u0027t changing to \u0027available\u0027?\n(4:55:45 PM) melwitt: mriedem: I had multiple attachments because I was having trouble getting the code path to hit in devstack. so I tried the scenario multiple times with the same volume by reset-state on it\n(4:55:51 PM) melwitt: and didn\u0027t notice it was building up attachments\n(4:56:20 PM) melwitt: so once I started from a clean slate, new volume and did the scenario, it worked as expected. the volume actually had no attachments in the fresh volume case. it was \u0027reserved\u0027 with no attachments\n(4:56:47 PM) melwitt: then the attachment_delete change it from \u0027reserved\u0027 -\u003e \u0027available\u0027, then the volume_api.delete deleted the volume properly\n(4:57:40 PM) melwitt: so I think all is well","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"6015bc14225650f49ea7bd8c26945392be543be6","unresolved":false,"context_lines":[{"line_number":2042,"context_line":"                        # connection made in the first place this is just"},{"line_number":2043,"context_line":"                        # cleaning up the volume state in the Cinder DB."},{"line_number":2044,"context_line":"                        self.volume_api.detach(elevated, bdm.volume_id,"},{"line_number":2045,"context_line":"                                               instance.uuid)"},{"line_number":2046,"context_line":"                except Exception as exc:"},{"line_number":2047,"context_line":"                    err_str \u003d \"Ignoring volume cleanup failure due to %s\""},{"line_number":2048,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_b2a71a50","line":2045,"updated":"2018-02-16 05:44:36.000000000","message":"What about this? Does this remove \u0027reserved\u0027 state? Or does it remove the \u0027attaching\u0027 state? I\u0027m not sure what the state the volume would be in with the old style attachment.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c01887df1760a324284a8a6813476ffcf6739f71","unresolved":false,"context_lines":[{"line_number":2042,"context_line":"                        # connection made in the first place this is just"},{"line_number":2043,"context_line":"                        # cleaning up the volume state in the Cinder DB."},{"line_number":2044,"context_line":"                        self.volume_api.detach(elevated, bdm.volume_id,"},{"line_number":2045,"context_line":"                                               instance.uuid)"},{"line_number":2046,"context_line":"                except Exception as exc:"},{"line_number":2047,"context_line":"                    err_str \u003d \"Ignoring volume cleanup failure due to %s\""},{"line_number":2048,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_c339a8a3","line":2045,"in_reply_to":"3fa0c359_b2a71a50","updated":"2018-02-16 22:59:57.000000000","message":"I believe it handles either? It should put the volume back into available status:\n\nhttps://github.com/openstack/cinder/blob/c100c8f1888f8963ed5cf81661e52889d765d2b2/cinder/volume/manager.py#L1320\n\nhttps://github.com/openstack/cinder/blob/c100c8f1888f8963ed5cf81661e52889d765d2b2/cinder/volume/manager.py#L1371\n\nhttps://github.com/openstack/cinder/blob/c100c8f1888f8963ed5cf81661e52889d765d2b2/cinder/objects/volume.py#L551\n\nhttps://github.com/openstack/cinder/blob/c100c8f1888f8963ed5cf81661e52889d765d2b2/cinder/db/sqlalchemy/api.py#L1794\n\nI get lost reading ^ but I think it\u0027s the same idea.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":2048,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"},{"line_number":2049,"context_line":"                # This block handles the following case:"},{"line_number":2050,"context_line":"                # 1. Instance scheduled to host and fails on the host."},{"line_number":2051,"context_line":"                # 2. Compute manager\u0027s cleanup calls terminate_connection"},{"line_number":2052,"context_line":"                #    and detach if the spawn made it that far."},{"line_number":2053,"context_line":"                # 3. Instance fails to boot on all other reschedule attempts."},{"line_number":2054,"context_line":"                # 4. Instance is left in error state with no assigned host."},{"line_number":2055,"context_line":"                # 5. Volumes in the instance\u0027s BDMs are left in the available"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_f4b59cce","line":2052,"range":{"start_line":2051,"start_character":21,"end_line":2052,"end_character":62},"updated":"2018-02-15 16:39:55.000000000","message":"This doesn\u0027t actually happen today, maybe that\u0027s where my confusion is coming from. The compute manager calls _cleanup_volumes which only tries to delete the volume if bdm.delete_on_termination is True, but that will fail if the volume is attached (so after _prep_block_devices but then let\u0027s say driver.spawn() failed). That\u0027s what Ameeda\u0027s patch is trying to fix.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a81fc6d8db87261f9be57dbeb4fcf622d0a574","unresolved":false,"context_lines":[{"line_number":2048,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"},{"line_number":2049,"context_line":"                # This block handles the following case:"},{"line_number":2050,"context_line":"                # 1. Instance scheduled to host and fails on the host."},{"line_number":2051,"context_line":"                # 2. Compute manager\u0027s cleanup calls terminate_connection"},{"line_number":2052,"context_line":"                #    and detach if the spawn made it that far."},{"line_number":2053,"context_line":"                # 3. Instance fails to boot on all other reschedule attempts."},{"line_number":2054,"context_line":"                # 4. Instance is left in error state with no assigned host."},{"line_number":2055,"context_line":"                # 5. Volumes in the instance\u0027s BDMs are left in the available"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_3d1764f0","line":2052,"range":{"start_line":2051,"start_character":21,"end_line":2052,"end_character":62},"in_reply_to":"3fa0c359_f4b59cce","updated":"2018-02-15 20:36:38.000000000","message":"This is a really old comment, this patch has been up since 2015, so I\u0027ll try to correct all of this.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"11c5ad44c6d75a75dc1144ab42283232cc1a545e","unresolved":false,"context_lines":[{"line_number":2050,"context_line":"                # 1. Instance scheduled to host and fails on the host."},{"line_number":2051,"context_line":"                # 2. Compute manager\u0027s cleanup calls terminate_connection"},{"line_number":2052,"context_line":"                #    and detach if the spawn made it that far."},{"line_number":2053,"context_line":"                # 3. Instance fails to boot on all other reschedule attempts."},{"line_number":2054,"context_line":"                # 4. Instance is left in error state with no assigned host."},{"line_number":2055,"context_line":"                # 5. Volumes in the instance\u0027s BDMs are left in the available"},{"line_number":2056,"context_line":"                #    state."}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_34e1c4d1","line":2053,"range":{"start_line":2053,"start_character":18,"end_line":2053,"end_character":77},"updated":"2018-02-15 16:39:55.000000000","message":"note to self, if _prep_block_devices fails in the compute, we don\u0027t reschedule, we abort the build.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":2052,"context_line":"                #    and detach if the spawn made it that far."},{"line_number":2053,"context_line":"                # 3. Instance fails to boot on all other reschedule attempts."},{"line_number":2054,"context_line":"                # 4. Instance is left in error state with no assigned host."},{"line_number":2055,"context_line":"                # 5. Volumes in the instance\u0027s BDMs are left in the available"},{"line_number":2056,"context_line":"                #    state."},{"line_number":2057,"context_line":"                # When this is the case the terminate_connection and detach"},{"line_number":2058,"context_line":"                # calls above will fail. We attempt the volume delete in a"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_43be1c00","line":2055,"range":{"start_line":2055,"start_character":68,"end_line":2055,"end_character":77},"updated":"2018-02-15 15:04:56.000000000","message":"attaching? Or you\u0027re saying the compute cleaned up the volume appropriate so it\u0027s detached and \u0027available\u0027 so when the API tried to detach it again, that failed because it\u0027s already detached? I think Cinder actually changed that in their API so that subsequent attempts to detach an available volume are noops:\n\nI11b7c45bb6570ce11e13e487cf1136ca2551036b\n\n^ has been around since Newton.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"38981b9d0ad5f69454677e28d70b6b382702e8c3","unresolved":false,"context_lines":[{"line_number":2052,"context_line":"                #    and detach if the spawn made it that far."},{"line_number":2053,"context_line":"                # 3. Instance fails to boot on all other reschedule attempts."},{"line_number":2054,"context_line":"                # 4. Instance is left in error state with no assigned host."},{"line_number":2055,"context_line":"                # 5. Volumes in the instance\u0027s BDMs are left in the available"},{"line_number":2056,"context_line":"                #    state."},{"line_number":2057,"context_line":"                # When this is the case the terminate_connection and detach"},{"line_number":2058,"context_line":"                # calls above will fail. We attempt the volume delete in a"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_26bc3eba","line":2055,"range":{"start_line":2055,"start_character":68,"end_line":2055,"end_character":77},"in_reply_to":"3fa0c359_43be1c00","updated":"2018-02-15 15:56:49.000000000","message":"I think this might be \u0027available\u0027 here.  The scenario covered for the clean-up below is BFV where Nova creates the volume (so the volume was never in an \u0027attaching\u0027 state at the API layer, as it\u0027s created by Nova)","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"6015bc14225650f49ea7bd8c26945392be543be6","unresolved":false,"context_lines":[{"line_number":2060,"context_line":"                # storage leak."},{"line_number":2061,"context_line":"                try:"},{"line_number":2062,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":2063,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":2064,"context_line":"                except Exception as exc:"},{"line_number":2065,"context_line":"                    LOG.warning(\"Ignoring volume cleanup failure due to %s\","},{"line_number":2066,"context_line":"                                exc, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_720af28a","line":2063,"updated":"2018-02-16 05:44:36.000000000","message":"I do find a problem here though -- because the volume is in state \u0027reserved\u0027, the delete is rejected with a 400:\n\nREQ: curl -g -i -X DELETE http://127.0.0.1/volume/v3/bf31b39813684fc68fdd30f807da9deb/volumes/6f11839e-a1a7-4fb3-b22f-943db027912f\n\n(HTTP 400) (Request-ID: req-c0661300-fa00-425c-bf85-9079a9b1515c): InvalidInput: Invalid input received: Invalid volume: Volume status must be available or error or error_restoring or error_extending or error_managing and must not be migrating, attached, belong to a group, have snapshots or be disassociated from snapshots after volume transfer.\n\nAdmittedly, I\u0027m testing an unusual case here with BFV + already existing volume + delete_on_termination. My original goal was to test if we still need the new try-except block this patch introduced back before mitaka, when detach probably used to return an error code if there were no attachments.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c01887df1760a324284a8a6813476ffcf6739f71","unresolved":false,"context_lines":[{"line_number":2060,"context_line":"                # storage leak."},{"line_number":2061,"context_line":"                try:"},{"line_number":2062,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":2063,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":2064,"context_line":"                except Exception as exc:"},{"line_number":2065,"context_line":"                    LOG.warning(\"Ignoring volume cleanup failure due to %s\","},{"line_number":2066,"context_line":"                                exc, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_436ef896","line":2063,"in_reply_to":"3fa0c359_720af28a","updated":"2018-02-16 22:59:57.000000000","message":"I\u0027m assuming this was failing for the same reasons above for detach - if you can\u0027t detach to get the volume into \u0027available\u0027 status, the delete won\u0027t work either because of those status checks on the volume.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"377bca556de27e8ee670f0681df1b721bb9efeda","unresolved":false,"context_lines":[{"line_number":1881,"context_line":"                    # The instance\u0027s host likely changed under us as"},{"line_number":1882,"context_line":"                    # this instance could be building and has since been"},{"line_number":1883,"context_line":"                    # scheduled. Continue with attempts to delete it."},{"line_number":1884,"context_line":"                    # NOTE(melwitt): Often instance.host is None after the"},{"line_number":1885,"context_line":"                    # refresh because the claim was aborted on the compute"},{"line_number":1886,"context_line":"                    # host."},{"line_number":1887,"context_line":"                    LOG.debug(\u0027Refreshing instance because: %s\u0027, ex,"},{"line_number":1888,"context_line":"                              instance\u003dinstance)"},{"line_number":1889,"context_line":"                    instance.refresh()"}],"source_content_type":"text/x-python","patch_set":19,"id":"3fa0c359_355e94e6","line":1886,"range":{"start_line":1884,"start_character":20,"end_line":1886,"end_character":27},"updated":"2018-02-16 06:17:10.000000000","message":"Sigh, I don\u0027t think this is true anymore but I didn\u0027t notice it while I was fixing other things.","commit_id":"ceb33f71c04b9f22c67d0a931462f8409f06f0a9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1d5299a27866d81764286ee2de54ba265f517575","unresolved":false,"context_lines":[{"line_number":2046,"context_line":"                        # NOTE(melwitt): This will fail with a 400 if the"},{"line_number":2047,"context_line":"                        # volume has \u0027reserved\u0027 status and"},{"line_number":2048,"context_line":"                        # volume_api.attachment_delete does not remove the"},{"line_number":2049,"context_line":"                        # \u0027reserved\u0027 status."},{"line_number":2050,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"},{"line_number":2051,"context_line":"                except Exception as exc:"},{"line_number":2052,"context_line":"                    LOG.warning(\"Ignoring volume cleanup failure due to %s\","}],"source_content_type":"text/x-python","patch_set":19,"id":"3fa0c359_4f03880e","line":2049,"updated":"2018-02-16 15:11:15.000000000","message":"I need to remove this because in my test I wasn\u0027t noticing that I had multiple attachments on my test volume (because I had been reusing the same volume through several failed attempts to hit this code path). When I created a fresh volume and tried it again, I got a \u0027reserved\u0027 volume with no attachments with my instance. Then, when I deleted the instance, the volume was also deleted successfully. Thanks to mnaser for noticing the problem in my example.","commit_id":"ceb33f71c04b9f22c67d0a931462f8409f06f0a9"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"83fd38dd971497e6a35b42cff8513a679d30f60e","unresolved":false,"context_lines":[{"line_number":1784,"context_line":"        # in error state), the instance has been scheduled and sent to a"},{"line_number":1785,"context_line":"        # cell/compute which means it was pulled from the cell db."},{"line_number":1786,"context_line":"        # Normal delete should be attempted."},{"line_number":1787,"context_line":"        may_have_ports_or_volumes \u003d self._may_have_ports_or_volumes(instance)"},{"line_number":1788,"context_line":"        if not instance.host and not may_have_ports_or_volumes:"},{"line_number":1789,"context_line":"            try:"},{"line_number":1790,"context_line":"                if self._delete_while_booting(context, instance):"}],"source_content_type":"text/x-python","patch_set":20,"id":"3fa0c359_80c6fd48","line":1787,"range":{"start_line":1787,"start_character":8,"end_line":1787,"end_character":33},"updated":"2018-02-20 15:14:20.000000000","message":"I love the name ;)","commit_id":"c6f0d694ec71daf6156a3f55763025454b32ff3d"}],"nova/tests/functional/regressions/test_bug_1404867.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7679578bce789c4341bc43889d006265f1cd2e42","unresolved":false,"context_lines":[{"line_number":80,"context_line":""},{"line_number":81,"context_line":"        # The volume should no longer be reserved as the deletion of the"},{"line_number":82,"context_line":"        # server should have released all the resources."},{"line_number":83,"context_line":"        self.assertNotIn(volume_id, self.cinder.reserved_volumes)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @mock.patch(\u0027nova.objects.service.get_minimum_version_all_cells\u0027,"},{"line_number":86,"context_line":"                return_value\u003dcompute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3fa0c359_83a18498","line":83,"updated":"2018-02-15 15:04:56.000000000","message":"I\u0027m actually not sure how this (or the one below) are working given what I\u0027ve pointed out in the API code in the case that the instance is in cell0, the API will destroy the instance in cell0 and from what I can tell, fail to even lookup BDMs.","commit_id":"a2bc83ea44b10b52a1403fb0b35ab96a178d1be0"}]}