)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":10,"context_line":"destination if they were never connected in the first place and"},{"line_number":11,"context_line":"2) avoid trying to disconnect volumes on the destination using block"},{"line_number":12,"context_line":"device info for the source."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Details:"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"  * Only remotely disconnect volumes on the destination if the failure"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ee706fc4_6157082c","line":13,"updated":"2025-04-22 22:53:27.000000000","message":"this is the impoartnt sequence diagram \nhttps://docs.openstack.org/nova/latest/reference/live-migration.html","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d766019f2b65e01d61e394fcc3b95b82ebc11d68","unresolved":true,"context_lines":[{"line_number":20,"context_line":"    connection_info in the database is not guaranteed to for the"},{"line_number":21,"context_line":"    destination because a failure could have happened before the new"},{"line_number":22,"context_line":"    info was saved. In this scenario, there is no way to reliably"},{"line_number":23,"context_line":"    disconnect volumes in the destination remotely from the source."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"  * Due to the first point, this adds exception handling to disconnect"},{"line_number":26,"context_line":"    the volumes while still on the source while the destination"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"31f1fde3_60a5b006","line":23,"updated":"2025-04-11 01:20:12.000000000","message":"\"before re-raising and returning\"\n\n\"is not guaranteed to reference the destination\"\n\n\"a failure could have happened after the Cinder attachment was created but before the new connection_info was saved back to the database\"\n\n\"there is no way to reliably disconnect volumes in the destination remotely from the source because the destination connection_info needed to do it might not be available\"","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1e88250937dcce5f161e29996c59d753765efac0","unresolved":false,"context_lines":[{"line_number":20,"context_line":"    connection_info in the database is not guaranteed to for the"},{"line_number":21,"context_line":"    destination because a failure could have happened before the new"},{"line_number":22,"context_line":"    info was saved. In this scenario, there is no way to reliably"},{"line_number":23,"context_line":"    disconnect volumes in the destination remotely from the source."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"  * Due to the first point, this adds exception handling to disconnect"},{"line_number":26,"context_line":"    the volumes while still on the source while the destination"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"765a36bd_776ac276","line":23,"in_reply_to":"31f1fde3_60a5b006","updated":"2025-04-29 01:19:26.000000000","message":"Done","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d766019f2b65e01d61e394fcc3b95b82ebc11d68","unresolved":true,"context_lines":[{"line_number":25,"context_line":"  * Due to the first point, this adds exception handling to disconnect"},{"line_number":26,"context_line":"    the volumes while still on the source while the destination"},{"line_number":27,"context_line":"    connection_info is still available instead of trying to do it"},{"line_number":28,"context_line":"    remotely from the source afterward."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"  * Omit volume block_device_info when calling"},{"line_number":31,"context_line":"    rollback_live_migration_on_destination() because volume BDM records"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"dc7fa71d_dc9237b2","line":28,"updated":"2025-04-11 01:20:12.000000000","message":"\"while still on the destination, while the destination connection_info is still available\"","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1e88250937dcce5f161e29996c59d753765efac0","unresolved":false,"context_lines":[{"line_number":25,"context_line":"  * Due to the first point, this adds exception handling to disconnect"},{"line_number":26,"context_line":"    the volumes while still on the source while the destination"},{"line_number":27,"context_line":"    connection_info is still available instead of trying to do it"},{"line_number":28,"context_line":"    remotely from the source afterward."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"  * Omit volume block_device_info when calling"},{"line_number":31,"context_line":"    rollback_live_migration_on_destination() because volume BDM records"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7305b38a_c9296607","line":28,"in_reply_to":"dc7fa71d_dc9237b2","updated":"2025-04-29 01:19:26.000000000","message":"Done","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d766019f2b65e01d61e394fcc3b95b82ebc11d68","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    have already been rolled back to contain info for the source by"},{"line_number":33,"context_line":"    that point. This avoids volume disconnection attempts with the"},{"line_number":34,"context_line":"    source connection_info during driver.destroy() and subequently"},{"line_number":35,"context_line":"    driver.cleanup() on the destination."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"Closes-Bug: #1899835"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"a81c3830_63c2cba6","line":35,"updated":"2025-04-11 01:20:12.000000000","message":"\"Do not pass Cinder volume block_device_info when calling\"\n\n\"Not passing volume block_device_info will prevent driver.destroy() and subsequently driver.cleanup() from attempting to disconnect volumes on the destination using connection_info for the source\"","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1e88250937dcce5f161e29996c59d753765efac0","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    have already been rolled back to contain info for the source by"},{"line_number":33,"context_line":"    that point. This avoids volume disconnection attempts with the"},{"line_number":34,"context_line":"    source connection_info during driver.destroy() and subequently"},{"line_number":35,"context_line":"    driver.cleanup() on the destination."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"Closes-Bug: #1899835"},{"line_number":38,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"4ce40dd1_a676962a","line":35,"in_reply_to":"a81c3830_63c2cba6","updated":"2025-04-29 01:19:26.000000000","message":"Done","commit_id":"88aea1591b82a587beb127a87b20572895875081"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"904142f8f844cbe1209e70a6e97b8bd388ed9053","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c860323b_090a9b0a","updated":"2025-04-09 08:36:22.000000000","message":"As far as I understand this patch, the general idea is similar to Lee\u0027s approach. However, not using a field in the migrate_data object seems simpler and also makes backporting easier.\n\nFrom the workflow perspective, modifying the _rollback_live_migration method looks like the right place to apply this change.\n\nThe patch does what is described in the commit message, passes all the tests and the reproducer.\n\nIn my opinion, your approach makes sense, and I don\u0027t see any issues with it. That said, this part of the code is quite tricky, and although I\u0027ve recently been exposed to live migration, I’m not sure I fully grasp every aspect of it yet.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4a68920e6bc5655483f78fb2b7e0b4e56ad54c0e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"6b033b1d_2a2b820d","updated":"2025-04-09 17:55:48.000000000","message":"Thanks all for looking and for your input. I will work on it more.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b7bc48c73c04d7a31e53bb74e6cd831d3a41a9f9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c9ccf28c_80c1df53","updated":"2025-04-08 17:01:05.000000000","message":"recheck test timeout due to still waiting for vif plug event from Neutron\n\nDetails: (ServerDiskConfigTestJSON:test_rebuild_server_with_auto_disk_config) Server 472263d6-8a7b-4655-8454-bc7110118c0d failed to reach ACTIVE status and task state \"None\" within the required time (196 s). Current status: REBUILD. Current task state: rebuild_spawning.\n\nand later\n\n[instance: 472263d6-8a7b-4655-8454-bc7110118c0d] Timeout waiting for [\u0027network-vif-plugged-55a161e3-2f20-44bb-8c68-fd0fc9b1e6d0\u0027] for instance with vm_state active and task_state rebuild_spawning. Event states are: network-vif-plugged-55a161e3-2f20-44bb-8c68-fd0fc9b1e6d0: timed out after 300.00 seconds: eventlet.timeout.Timeout: 300 seconds","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"7661540a_8cd55ac6","updated":"2025-04-22 22:53:27.000000000","message":"+1 for direction but i thing ther are some potential upgrade issues with this approch.\n\nnot that we cant fix them i just am not conviced this patch in its current form handels all the edgcaes fo patched vs unpatch soruce/destiation node combinations.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d766019f2b65e01d61e394fcc3b95b82ebc11d68","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8a846cb6_cf8fb0d9","updated":"2025-04-11 01:20:12.000000000","message":"Changed the approach to handle driver disconnect of volumes on the destination to be performed on the destination host (instead of remotely from the source) while we still have the destination host\u0027s connection_info for the volumes","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"a5ed71ac_6db3feb9","updated":"2025-04-23 03:02:44.000000000","message":"Thanks for the review!","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"693b880a_f039b1b2","updated":"2025-05-07 23:13:39.000000000","message":"... i hate pushing comment i left 8 days ago because i dont knwo if any of these are stil relevent.\n\nill try an do a proper view of this tomorrow","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"dabb2b95867f07101dc385e4d7bf12b1867695fb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e6bfa399_04881098","updated":"2025-05-15 06:35:10.000000000","message":"Looks good to me.","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"222cecc4436ca02d5589a2378cff452ad6ff469c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"45797488_e418741a","updated":"2025-05-16 00:42:16.000000000","message":"recheck Details: {\u0027message\u0027: \u0027Image a4057603-6b77-471f-8995-ff97cbe1334e could not be deleted because it is in use: The image cannot be deleted because it is in use through the backend store outside of Glance.\u003cbr /\u003e\u003cbr /\u003e\\n\\n\\n\u0027, \u0027code\u0027: \u0027409 Conflict\u0027, \u0027title\u0027: \u0027Conflict\u0027}","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3896aa1b570755cf71dd3682097f1d178e5e8de9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"7ab8db69_b3e077d6","updated":"2025-04-29 19:55:20.000000000","message":"recheck https://review.opendev.org/c/openstack/nova/+/948392 has merged","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"58a4e1293524517ae70308f0e6de39ce6d198c66","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"11b21aad_024fc78a","updated":"2025-05-15 20:34:53.000000000","message":"recheck nova.exception.ImageDeleteConflict: Conflict deleting image. Reason: HTTP 409 Conflict: Image a34561ed-6ef7-457c-8845-677f83af10de could not be deleted because it is in use: The image cannot be deleted because it is in use through the backend store outside of Glance..","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f8b309244960a585a3817d82079c9c443802c702","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"60b4b86b_089751e4","updated":"2025-05-15 17:07:53.000000000","message":"recheck ssh timeout, sshd was not up yet at the time of the timeout","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"11dacc19c30d813ac30d109ccafbb92706c2e35d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e1155079_1a4973f8","updated":"2025-05-15 22:30:16.000000000","message":"recheck urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host\u003d\u0027200.225.47.14\u0027, port\u003d9696): Read timed out. (read timeout\u003d90)\n\nunrelated to live migration","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"}],"nova/compute/manager.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"506232feec426f7ad8598d2af238ce9705cc1672","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"20795e07_e417ee01","line":10208,"updated":"2025-04-09 08:15:27.000000000","message":"This assumes that a connection info for the same volume on different hosts will differ. But looking at the connection info dict form devstack I\u0027m not sure what will be a real difference. Also connection_info is storage driver specific so such assumption might be true for some drivers but not for others. I don\u0027t think nova can use this assumption. We see in the recent report that a connection info from the dest might be valid on the source even if mapping to a different volume. So the same way I can imagine that the connection info from the dest is valid on the source mapping to the same volume as well. \n\n//later\nI checked with devstack:\n\nbdm on source:\n```json\n{\n  \"driver_volume_type\": \"iscsi\",\n  \"data\": {\n    \"target_discovered\": false,\n    \"target_portal\": \"192.168.121.142:3260\",\n    \"target_iqn\": \"iqn.2010-10.org.openstack:volume-af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n    \"target_lun\": 0,\n    \"volume_id\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n    \"auth_method\": \"CHAP\",\n    \"auth_username\": \"pPB7M8xSc4AEvhm7KE4S\",\n    \"auth_password\": \"54jEF82Tmi3RRBxQ\",\n    \"encrypted\": false,\n    \"qos_specs\": null,\n    \"access_mode\": \"rw\",\n    \"cacheable\": false,\n    \"enforce_multipath\": true,\n    \"device_path\": \"/dev/sda\"\n  },\n  \"status\": \"reserved\",\n  \"instance\": \"d68c2ff0-b34b-42a7-8704-0644cc6475d9\",\n  \"attached_at\": \"\",\n  \"detached_at\": \"\",\n  \"volume_id\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n  \"serial\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\"\n}\n```\n\nbdm on dest:\n```json\n{\n  \"driver_volume_type\": \"iscsi\",\n  \"data\": {\n    \"target_discovered\": false,\n    \"target_portal\": \"192.168.121.142:3260\",\n    \"target_iqn\": \"iqn.2010-10.org.openstack:volume-af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n    \"target_lun\": 0,\n    \"volume_id\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n    \"auth_method\": \"CHAP\",\n    \"auth_username\": \"pPB7M8xSc4AEvhm7KE4S\",\n    \"auth_password\": \"54jEF82Tmi3RRBxQ\",\n    \"encrypted\": false,\n    \"qos_specs\": null,\n    \"access_mode\": \"rw\",\n    \"cacheable\": false,\n    \"enforce_multipath\": true,\n    \"device_path\": \"/dev/sda\"\n  },\n  \"status\": \"attaching\",\n  \"instance\": \"d68c2ff0-b34b-42a7-8704-0644cc6475d9\",\n  \"attached_at\": \"2025-04-09T07:42:56.000000\",\n  \"detached_at\": \"\",\n  \"volume_id\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\",\n  \"serial\": \"af4aa3d4-53b7-4ee4-809c-a3f743947797\"\n}\n```\nI see two diffs:\n* attached_at is not set after boot but set after live migration\n* status is reserved after boot but attaching after live migration\n\nBoth of these diffs are very arbitrary and even showing independent inconsistencies in the data stored by nova. (I.e. Why don\u0027t we have both bdm in attached state after the lifecycle operation?, or why don\u0027t we have attached_at set after VM boot?) So I\u0027m not really happy about relaying of these diffs to signal that the connection_info is pointing at a different host.\n\n---\n\nCould we use the bdm.attachment_id to differentiate instead? At least I see that is changing during a successful live migration. \n\nIf I understand correctly we create a new attachment then refresh the connection info https://github.com/openstack/nova/blob/6e37eeaeb3cdc64d39d20e49ab71b559a5b5e87b/nova/compute/manager.py#L9288-L9321 So if we see a different attachment_id that is pretty indicative of nova also refreshed the connection_info as well.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"864793caa7076423588c8184306fa8c7116e4e9c","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"4aabfb80_c4c39060","line":10208,"in_reply_to":"0873df4e_ee295a7b","updated":"2025-04-09 18:36:40.000000000","message":"Sorry, I just realized I typo\u0027d this. I meant \"is NOT a good enough mechanism\". Hopefully it was obvious from the context of me going on to explain why I didn\u0027t think so, but just for posterity...","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"aad177c73b191394ffce75aa50df1e8ed42491a5","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3922e66a_904f440e","line":10208,"in_reply_to":"20795e07_e417ee01","updated":"2025-04-09 10:19:38.000000000","message":"I think you\u0027re right, Gibi — the absence of a new attachment_id is highly suspicious and suggests that we might not be removing it as intended.\nIn the worst case, we end up leaving a dangling or invalid attachment, which is arguably safer than mistakenly deleting the source attachment.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c99356b9cd7605d612f403c08992ce5d6d7359af","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"b03299fa_ea345583","line":10208,"in_reply_to":"3922e66a_904f440e","updated":"2025-04-09 12:28:09.000000000","message":"so i belive attchments for a volume are key\u0027d by hot in cinder.\n\nshoudl we consider just calling cidner to get all the attchment for the dest and then disconnect those one by one instead of trying to use our bdms to figure this out","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d766019f2b65e01d61e394fcc3b95b82ebc11d68","unresolved":false,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"19f004bb_3b996e8f","line":10208,"in_reply_to":"4aabfb80_c4c39060","updated":"2025-04-11 01:20:12.000000000","message":"Acknowledged","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4a68920e6bc5655483f78fb2b7e0b4e56ad54c0e","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"0873df4e_ee295a7b","line":10208,"in_reply_to":"b03299fa_ea345583","updated":"2025-04-09 17:55:48.000000000","message":"\u003e Could we use the bdm.attachment_id to differentiate instead? At least I see that is changing during a successful live migration.\n\nThat was my first thought but I think I somehow convinced myself that there could be a way that it would not work i.e. that there could be a situation where the attachment_id was updated but it is not always safe to disconnect on the dest.\n\n(later)\n\nI see the problem is that the attachment is created before the volumes are actually connected [1][2]. So if you fail somewhere between attachment create and _connect_volume in the driver, you will have a new attachment_id but the connection_info in the BDM still refers to the source and you get the same problem of the bug. There is also a code comment stating that some virt drivers will mutate the connection_info during driver pre_live_migration() so it may not be enough to try and save the BDM new attachment_id + refreshed connection_info in a single DB transaction, for example.\n\nI\u0027ll think about this more and come up with something else.\n\nSean\u0027s suggestion might probably be the safest way to go about this and I could see it making sense to do additional API calls or other general expenses given that this is a specific failure case and it\u0027s not like it will be happening \"often\".\n\n```\nnova/compute/manager.py\n\n    def pre_live_migration(self, context, instance, disk, migrate_data):\n        \"\"\"Preparations for live migration at dest host.\n        [...]\n        try:\n            for bdm in bdms:\n                    [...]\n                    attach_ref \u003d self.volume_api.attachment_create(\n                        context, bdm.volume_id, bdm.instance_uuid,\n                        connector\u003dconnector, mountpoint\u003dbdm.device_name)\n                    [...]\n                    # update the bdm with the new attachment_id.\n                    bdm.attachment_id \u003d attach_ref[\u0027id\u0027]\n                    bdm.save()\n            [...]\n            block_device_info \u003d self._get_instance_block_device_info(\n                                context, instance, refresh_conn_info\u003dTrue,\n                                bdms\u003dbdms)\n\n            # The driver pre_live_migration will plug vifs on the host\n            migrate_data \u003d self.driver.pre_live_migration(context,\n                                           instance,\n                                           block_device_info,\n                                           network_info,\n                                           disk,\n                                           migrate_data)\n           [...]        \n```\n\n```\nnova/virt/libvirt/driver.py\n\n    def pre_live_migration(self, context, instance, block_device_info,\n                           network_info, disk_info, migrate_data):\n        [...]\n        # Establishing connection to volume server.\n        block_device_mapping \u003d driver.block_device_info_get_mapping(\n            block_device_info)\n\n        if len(block_device_mapping):\n            LOG.debug(\u0027Connecting volumes before live migration.\u0027,\n                      instance\u003dinstance)\n\n        for bdm in block_device_mapping:\n            connection_info \u003d bdm[\u0027connection_info\u0027]\n            self._connect_volume(context, connection_info, instance)\n        [...]\n```\n\n[1] https://github.com/openstack/nova/blob/6e37eeaeb3cdc64d39d20e49ab71b559a5b5e87b/nova/compute/manager.py#L9303-L9316\n[2] https://github.com/openstack/nova/blob/6e37eeaeb3cdc64d39d20e49ab71b559a5b5e87b/nova/virt/libvirt/driver.py#L11591-L11593","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"647de5fea8f36bc324ec745b69d28fad6886a4e8","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"beb3175f_f2309b7c","line":10208,"in_reply_to":"b03299fa_ea345583","updated":"2025-04-09 16:58:31.000000000","message":"Agreed that \"src !\u003d dest\" is a good enough mechanism to determine whether we need to do something different. I suspect there may be a way to get those to match for multi-attach when we wouldn\u0027t expect them to, if an instance on one or the other host also has that volume attached. Also, as gibi noted, for certain transports they may or may not differ across hosts even when we expect they would.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"c851b62bf04afe003e77e16c0937fbf5d23ffb22","unresolved":true,"context_lines":[{"line_number":10205,"context_line":"            bdm.volume_id: bdm for bdm in original_bdms if bdm.is_volume}"},{"line_number":10206,"context_line":"        for bdm in bdms:"},{"line_number":10207,"context_line":"            original_bdm \u003d original_bdms_by_volid[bdm.volume_id]"},{"line_number":10208,"context_line":"            if bdm.connection_info !\u003d original_bdm.connection_info:"},{"line_number":10209,"context_line":"                bdms_with_new_connection_info.append(bdm)"},{"line_number":10210,"context_line":"        return bdms_with_new_connection_info"},{"line_number":10211,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"df4b4aee_e41f3904","line":10208,"in_reply_to":"beb3175f_f2309b7c","updated":"2025-04-09 17:19:50.000000000","message":"FYI, we did something like that in deny_share() method making sure the share is not  used elsewhere before denying it. https://opendev.org/openstack/nova/src/commit/14ffc8267879d9515d6d93e755e3737d96f3d1d1/nova/compute/manager.py#L4786\nBut of course it adds a sql query.","commit_id":"14ffc8267879d9515d6d93e755e3737d96f3d1d1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":9286,"context_line":""},{"line_number":9287,"context_line":"        connector \u003d None"},{"line_number":9288,"context_line":"        try:"},{"line_number":9289,"context_line":"            for bdm in bdms:"},{"line_number":9290,"context_line":"                if bdm.is_volume and bdm.attachment_id is not None:"},{"line_number":9291,"context_line":"                    # This bdm uses the new cinder v3.44 API."},{"line_number":9292,"context_line":"                    # We will create a new attachment for this"},{"line_number":9293,"context_line":"                    # volume on this migration destination host. The old"},{"line_number":9294,"context_line":"                    # attachment will be deleted on the source host"},{"line_number":9295,"context_line":"                    # when the migration succeeds. The old attachment_id"},{"line_number":9296,"context_line":"                    # is stored in dict with the key being the bdm.volume_id"},{"line_number":9297,"context_line":"                    # so it can be restored on rollback."},{"line_number":9298,"context_line":"                    #"},{"line_number":9299,"context_line":"                    # Also note that attachment_update is not needed as we"},{"line_number":9300,"context_line":"                    # are providing the connector in the create call."},{"line_number":9301,"context_line":"                    if connector is None:"},{"line_number":9302,"context_line":"                        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":9303,"context_line":"                    attach_ref \u003d self.volume_api.attachment_create("},{"line_number":9304,"context_line":"                        context, bdm.volume_id, bdm.instance_uuid,"},{"line_number":9305,"context_line":"                        connector\u003dconnector, mountpoint\u003dbdm.device_name)"},{"line_number":9306,"context_line":""},{"line_number":9307,"context_line":"                    # save current attachment so we can detach it on success,"},{"line_number":9308,"context_line":"                    # or restore it on a rollback."},{"line_number":9309,"context_line":"                    # NOTE(mdbooth): This data is no longer used by the source"},{"line_number":9310,"context_line":"                    # host since change Ibe9215c0. We can\u0027t remove it until we"},{"line_number":9311,"context_line":"                    # are sure the source host has been upgraded."},{"line_number":9312,"context_line":"                    migrate_data.old_vol_attachment_ids[bdm.volume_id] \u003d \\"},{"line_number":9313,"context_line":"                        bdm.attachment_id"},{"line_number":9314,"context_line":""},{"line_number":9315,"context_line":"                    # update the bdm with the new attachment_id."},{"line_number":9316,"context_line":"                    bdm.attachment_id \u003d attach_ref[\u0027id\u0027]"},{"line_number":9317,"context_line":"                    bdm.save()"},{"line_number":9318,"context_line":""},{"line_number":9319,"context_line":"            # Retrieve connection_info for the destination. Note that it is not"},{"line_number":9320,"context_line":"            # saved back to the database yet."}],"source_content_type":"text/x-python","patch_set":2,"id":"8229a291_0e0c3436","line":9317,"range":{"start_line":9289,"start_character":0,"end_line":9317,"end_character":30},"updated":"2025-04-22 22:53:27.000000000","message":"so here we are looping over the bdm and creatign the new attachment for the destioatnion and saivign the old attachment info for the souce in the migration_data.\n\nwe ar esaving the bdms back to the db here too","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8cf51163f76806afbc18de10825442ff4f1e3e7d","unresolved":false,"context_lines":[{"line_number":9286,"context_line":""},{"line_number":9287,"context_line":"        connector \u003d None"},{"line_number":9288,"context_line":"        try:"},{"line_number":9289,"context_line":"            for bdm in bdms:"},{"line_number":9290,"context_line":"                if bdm.is_volume and bdm.attachment_id is not None:"},{"line_number":9291,"context_line":"                    # This bdm uses the new cinder v3.44 API."},{"line_number":9292,"context_line":"                    # We will create a new attachment for this"},{"line_number":9293,"context_line":"                    # volume on this migration destination host. The old"},{"line_number":9294,"context_line":"                    # attachment will be deleted on the source host"},{"line_number":9295,"context_line":"                    # when the migration succeeds. The old attachment_id"},{"line_number":9296,"context_line":"                    # is stored in dict with the key being the bdm.volume_id"},{"line_number":9297,"context_line":"                    # so it can be restored on rollback."},{"line_number":9298,"context_line":"                    #"},{"line_number":9299,"context_line":"                    # Also note that attachment_update is not needed as we"},{"line_number":9300,"context_line":"                    # are providing the connector in the create call."},{"line_number":9301,"context_line":"                    if connector is None:"},{"line_number":9302,"context_line":"                        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":9303,"context_line":"                    attach_ref \u003d self.volume_api.attachment_create("},{"line_number":9304,"context_line":"                        context, bdm.volume_id, bdm.instance_uuid,"},{"line_number":9305,"context_line":"                        connector\u003dconnector, mountpoint\u003dbdm.device_name)"},{"line_number":9306,"context_line":""},{"line_number":9307,"context_line":"                    # save current attachment so we can detach it on success,"},{"line_number":9308,"context_line":"                    # or restore it on a rollback."},{"line_number":9309,"context_line":"                    # NOTE(mdbooth): This data is no longer used by the source"},{"line_number":9310,"context_line":"                    # host since change Ibe9215c0. We can\u0027t remove it until we"},{"line_number":9311,"context_line":"                    # are sure the source host has been upgraded."},{"line_number":9312,"context_line":"                    migrate_data.old_vol_attachment_ids[bdm.volume_id] \u003d \\"},{"line_number":9313,"context_line":"                        bdm.attachment_id"},{"line_number":9314,"context_line":""},{"line_number":9315,"context_line":"                    # update the bdm with the new attachment_id."},{"line_number":9316,"context_line":"                    bdm.attachment_id \u003d attach_ref[\u0027id\u0027]"},{"line_number":9317,"context_line":"                    bdm.save()"},{"line_number":9318,"context_line":""},{"line_number":9319,"context_line":"            # Retrieve connection_info for the destination. Note that it is not"},{"line_number":9320,"context_line":"            # saved back to the database yet."}],"source_content_type":"text/x-python","patch_set":2,"id":"f6b36fcc_e38417d6","line":9317,"range":{"start_line":9289,"start_character":0,"end_line":9317,"end_character":30},"in_reply_to":"8229a291_0e0c3436","updated":"2025-05-15 10:08:42.000000000","message":"Acknowledged","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":9321,"context_line":"            block_device_info \u003d self._get_instance_block_device_info("},{"line_number":9322,"context_line":"                                context, instance, refresh_conn_info\u003dTrue,"},{"line_number":9323,"context_line":"                                bdms\u003dbdms)"},{"line_number":9324,"context_line":""},{"line_number":9325,"context_line":"            # The driver pre_live_migration will plug vifs and connect volumes"},{"line_number":9326,"context_line":"            # on the host"},{"line_number":9327,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"67ef1676_13e9840d","line":9324,"updated":"2025-04-22 22:53:27.000000000","message":"so each bdm that is modifed above is already save back to the db\nso im not sure how ture this is or waht exactly is not saved back.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":false,"context_lines":[{"line_number":9321,"context_line":"            block_device_info \u003d self._get_instance_block_device_info("},{"line_number":9322,"context_line":"                                context, instance, refresh_conn_info\u003dTrue,"},{"line_number":9323,"context_line":"                                bdms\u003dbdms)"},{"line_number":9324,"context_line":""},{"line_number":9325,"context_line":"            # The driver pre_live_migration will plug vifs and connect volumes"},{"line_number":9326,"context_line":"            # on the host"},{"line_number":9327,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a303609_1f06258f","line":9324,"in_reply_to":"045de103_4776a33c","updated":"2025-05-07 23:13:39.000000000","message":"Acknowledged","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":true,"context_lines":[{"line_number":9321,"context_line":"            block_device_info \u003d self._get_instance_block_device_info("},{"line_number":9322,"context_line":"                                context, instance, refresh_conn_info\u003dTrue,"},{"line_number":9323,"context_line":"                                bdms\u003dbdms)"},{"line_number":9324,"context_line":""},{"line_number":9325,"context_line":"            # The driver pre_live_migration will plug vifs and connect volumes"},{"line_number":9326,"context_line":"            # on the host"},{"line_number":9327,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"045de103_4776a33c","line":9324,"in_reply_to":"67ef1676_13e9840d","updated":"2025-04-23 03:02:44.000000000","message":"It\u0027s a bit confusing but the BDM save above it only saving the new attachment UUID into the BDM database record. The new connection_info has not been pulled from Cinder yet. L9321 is where the new connection_info is pulled from Cinder by way of the refresh_conn_info keyword arg.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":9353,"context_line":"                # Ensure this is saved to the database by calling .save()"},{"line_number":9354,"context_line":"                # against the driver BDMs we passed down via block_device_info."},{"line_number":9355,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9356,"context_line":"                    driver_bdm.save()"},{"line_number":9357,"context_line":"            except Exception:"},{"line_number":9358,"context_line":"                # NOTE(melwitt): Try to disconnect any volumes which may have"},{"line_number":9359,"context_line":"                # been connected during driver pre_live_migration(). By the"}],"source_content_type":"text/x-python","patch_set":2,"id":"83be8ee2_7cb03e06","line":9356,"updated":"2025-04-22 22:53:27.000000000","message":"im not sure that hits second save is actlly need with the save on line 9317\n\ni causee we only save the bdm on line 9317 if we are takign the new cinder flow with multiple attachment and this save is were we used to save it when using the old cidner attachemtns api.\n\ni.e. before cinder  v3.44","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":true,"context_lines":[{"line_number":9353,"context_line":"                # Ensure this is saved to the database by calling .save()"},{"line_number":9354,"context_line":"                # against the driver BDMs we passed down via block_device_info."},{"line_number":9355,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9356,"context_line":"                    driver_bdm.save()"},{"line_number":9357,"context_line":"            except Exception:"},{"line_number":9358,"context_line":"                # NOTE(melwitt): Try to disconnect any volumes which may have"},{"line_number":9359,"context_line":"                # been connected during driver pre_live_migration(). By the"}],"source_content_type":"text/x-python","patch_set":2,"id":"8676747e_9e5c0202","line":9356,"in_reply_to":"83be8ee2_7cb03e06","updated":"2025-04-23 03:02:44.000000000","message":"This save is for the new connection_info and the code comment is explaining the reason the save is done after calling driver pre_live_migration() is because it\u0027s apparently part of the virt driver contract that drivers can stash additional metadata into the connection_info by reference. So this will save that new connection_info back to the database.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":false,"context_lines":[{"line_number":9353,"context_line":"                # Ensure this is saved to the database by calling .save()"},{"line_number":9354,"context_line":"                # against the driver BDMs we passed down via block_device_info."},{"line_number":9355,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9356,"context_line":"                    driver_bdm.save()"},{"line_number":9357,"context_line":"            except Exception:"},{"line_number":9358,"context_line":"                # NOTE(melwitt): Try to disconnect any volumes which may have"},{"line_number":9359,"context_line":"                # been connected during driver pre_live_migration(). By the"}],"source_content_type":"text/x-python","patch_set":2,"id":"f9ffaa1f_f11f901c","line":9356,"in_reply_to":"8676747e_9e5c0202","updated":"2025-05-07 23:13:39.000000000","message":"Acknowledged","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"0260f594_879271c9","line":9367,"updated":"2025-04-22 22:53:27.000000000","message":"so this is the main fix. you moved the destintation detach call  to pre-livemigration\ninstead of _rollback_live_migration\n\nthat meanst that this is not incorrect \nhttps://docs.openstack.org/nova/latest/reference/live-migration.html\n\nthe call to remove_volume commeciton  which used to be done via \n_remove_remote_volume_connections is now conditional and can happen in pre-livemigration instead.\n\npartly because updating that will be a pain and partly because i dotn really know how express that in the diagram i think we can leave it as is.\n\nits still mostly correct it just that it can happen earlier if we have not call driver.live_migraiton when the failure happend.\n\nwe are moving the cleanup of the destination voluems form the source host to the destiantion host however.\ngranted the souce host was just calling the destition to have ti do it but it does change the pattern of rpcs by entrily removing the need for the rpc call betwen the souce and dest on roolback.\n\nso over all i think that an improment but just want to make it clear we are moving the cleanup bethween hosts in this specific edgcase.\n\ni think that valid but it might have upgrade implcitions.\ni.e. if the souce is upgraded and hte dest is not then nothing will clean up.\nif the dest is upgraded and the souces is not both will try and clean up.\n\nthere is not much we can do the first case unless we add a compute service version bump and check in rollback_live_migration in addtion to the current if for \"pre-live-migrating\" \n\n\nthe second case of the dest is upgrade/patched but the souce is less of an issue provided we make sure its safe to call _remove_remote_volume_connections if its already been cleaned up in pre_live_migrate.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"6604169c_c8676cd5","line":9367,"in_reply_to":"0260f594_879271c9","updated":"2025-04-23 03:02:44.000000000","message":"That\u0027s a good point about a scenario mid upgrade between old host and new host ... I think we don\u0027t officially support live migration from new host to old host do we? Might not matter if it currently usually \"just works\" and we need to preserve that. Just something to note.\n\nBut yeah, the whole \"remove connections remotely from the source based on info from the database\" is kind of easy to accidentally break with unrelated code changes for bug fixes (which is what happened AFAICT). So it would be nice/ideal if we could clean up the destination on the destination IMHO. (Code was added later to roll back the BDM connection_info to the source connection_info before cleaning up the destination and that broke the remote disconnect logic).\n\nI\u0027ll think through the upgrade cases some more.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"4046aa6857911a2a7fecb7b780e12896f661e033","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"de6221ba_3130df28","line":9367,"in_reply_to":"21a9d6df_2930dfcd","updated":"2025-05-07 16:25:30.000000000","message":"OK thanks. Then the dest cannot rely on the fact that the second disconnect needs to be ignored. So no simplification possible here. \n\nSo it looks good to me what is proposed here.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"225cd960_d974c3fb","line":9367,"in_reply_to":"21a9d6df_2930dfcd","updated":"2025-05-07 23:13:39.000000000","message":"sorry for not getting back to this for now.\n\noffically you are correct we do not supprot live migrationg form newer version fo qemu/libvirt to older ones. for nova you are also not ment to live migrate form new compute to older compute across major release. within a major release that should be supported certenly if the comptue service version is the same. so we are in a grey area here.\n\nthechnically we shoudl try to ensure that this is reasonabel to do to avoid big bang minor updates so its fine for ti to fail but it shoudl fail safely.\n\nin case 2 the dest is updated  so we can modify the code to be a noop.\nim just not sure how to do that check given the current rpc interface.\n\nwe could hope that the cidner backend uses uniqe ides and treat the not foudn as a noop but if the wwns is reused beacuse the iscsi san reuses them we coudl disconenct the wrong voluem\n\nthis really does feel like we shoudl be passign more info to the dest to allow ti to do the right thing.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8cf51163f76806afbc18de10825442ff4f1e3e7d","unresolved":false,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"64a3fae5_0f0cffd3","line":9367,"in_reply_to":"225cd960_d974c3fb","updated":"2025-05-15 10:08:42.000000000","message":"Acknowledged","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0c1222df0b285c0e3f4ca8bb36e7155def7e4dee","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"88eb485e_6c20cce4","line":9367,"in_reply_to":"6604169c_c8676cd5","updated":"2025-04-25 19:33:45.000000000","message":"\u003e so over all i think that an improment but just want to make it clear we are moving the cleanup bethween hosts in this specific edgcase.\n\nThanks for pointing out about the live migration diagram in the docs. I will make sure to update that alongside this to show the edgecase.\n\n\u003e i think that valid but it might have upgrade implcitions.\ni.e. if the souce is upgraded and hte dest is not then nothing will clean up.\nif the dest is upgraded and the souces is not both will try and clean up.\n\u003e\n\u003e there is not much we can do the first case unless we add a compute service version bump and check in rollback_live_migration in addtion to the current if for \"pre-live-migrating\"\n\u003e\n\u003e the second case of the dest is upgrade/patched but the souce is less of an issue provided we make sure its safe to call _remove_remote_volume_connections if its already been cleaned up in pre_live_migrate.\n\nOK, I\u0027m trying to walk through this is in my head. Let me know if you think I\u0027ve missed something about this.\n\n#### First case: if the source is updated and the destination is not\n\n* New behavior: no volumes disconnect on the destination during pre_live_migration\n* Old behavior: RPC sent to disconnect volumes on the destination using connection_info for the source\n* Maybe this is a net positive? Old behavior seems more bad than new behavior\n\n#### Second case: if the destination is updated and the source is not\n\n* New behavior: volumes disconnected while on the destination, then RPC sent to disconnect volumes on the destination using connection_info for the source\n\n* Old behavior: RPC sent to disconnect volumes on the destination using connection_info for the source\n\n* This is more difficult to reason about. The bug worst case scenario is one where the connection_info for the source \"finds\" or \"matches\" a volume on the destination due to how the volume storage backend\u0027s IDs/naming works. And a volume that should *not* be disconnected does get disconnected and then an unrelated VM on the destination suffers. \"Normally\" I would think if you try to disconnect on the dest with info from the source, it would not find anything and thus do nothing damaging. But this bug scenario means that it *can* find something on the dest using info from the source.\n\n   * So if we are thinking about new behavior where volumes would be attempted to disconnect twice ... The first attempt will happen on the destination where it will match whatever IDs and disconnect the volumes. The second attempt which comes in over RPC, I\u0027m thinking it would fail to \"find\" the volumes because those IDs would have been disconnected already and will instead result in a \"not found\" no-op. Which would actually be again an improvement in behavior in that no disconnect on dest with source info should succeed anymore. What do you think?","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"63bb50f09d4dd0202e360b93b9df007ac537cb3e","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"7fbe35cc_bf6565f6","line":9367,"in_reply_to":"7d42908e_5c9960d3","updated":"2025-04-28 09:31:07.000000000","message":"Thanks for the detailed analysis. I agree that the code change does not make the situation worse. I\u0027m OK to accept the compromise that the fix only goes away after all the nodes are updated.\n//later\nI wondered about the second case a bit more. So after the patch nova disconnects the dest volume on the dest, then later the dest gets an RPC to disconnect volumes (based on the potentially wrong, src connection_info). Is it true that at this point the dest nova knows that this disconnect request can be ignored as the disconnect happened already? Or is it not that simple? (I guess so but I haven\u0027t looked)","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e0ce73c30328995738c029d5f4394e7565aafb95","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"21a9d6df_2930dfcd","line":9367,"in_reply_to":"7fbe35cc_bf6565f6","updated":"2025-04-28 16:50:16.000000000","message":"I don\u0027t know of a way to know the disconnect happened already. My understanding (which may be limited):\n\nIn general there is not a way to check \"is connected?\" other than the high-level BDM attachment_id, and by high-level I mean that is a resource ID from Cinder and does not tell anything about a volume connection at a host or VM level. The BDM connection_info has details but it is a freeform dict dependent on Cinder backend with no practical way to tell which host it\u0027s about.\n\nThe volume disconnect stuff at the host or VM level focuses instead on being idempotent and letting a \"not found\" from either layer be a no-op indicating something was already disconnected. And the way in which \"not found\" is determined varies depending on the Cinder volume storage backend.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b9651f278914798ea6fdb4d5c88585d70a283594","unresolved":true,"context_lines":[{"line_number":9364,"context_line":"                for driver_bdm in block_device_info[\u0027block_device_mapping\u0027]:"},{"line_number":9365,"context_line":"                    driver_bdm.driver_detach("},{"line_number":9366,"context_line":"                        context, instance, self.volume_api, self.driver)"},{"line_number":9367,"context_line":"                # Re-raise to perform any remaining rollback actions."},{"line_number":9368,"context_line":"                raise"},{"line_number":9369,"context_line":"        except Exception:"},{"line_number":9370,"context_line":"            # If we raise, migrate_data with the updated attachment ids"}],"source_content_type":"text/x-python","patch_set":2,"id":"7d42908e_5c9960d3","line":9367,"in_reply_to":"88eb485e_6c20cce4","updated":"2025-04-25 19:40:17.000000000","message":"As usual, I think of something after I publish the draft. I\u0027m getting confused about the second case. Maybe what would happen is for both disconnect attempts it would find something to disconnect -- the first one would be OK bc it\u0027s using dest info but the second one would result in the same bug where a wrong volume gets disconnected. If this is the case, I would think the new behavior is net neutral -- no better and no worse?","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":10335,"context_line":"        # BlockDeviceMappingList."},{"line_number":10336,"context_line":"        vol_bdms \u003d [bdm for bdm in bdms if bdm.is_volume]"},{"line_number":10337,"context_line":""},{"line_number":10338,"context_line":"        if not pre_live_migration:"},{"line_number":10339,"context_line":"            # This will do both a driver detach and a Cinder attachment delete."},{"line_number":10340,"context_line":"            # If we are in here due to a pre_live_migration failure, BDMs have"},{"line_number":10341,"context_line":"            # already been rolled back to contain info for the source, so don\u0027t"}],"source_content_type":"text/x-python","patch_set":2,"id":"595596dc_95a639c7","line":10338,"updated":"2025-04-22 22:53:27.000000000","message":"which is why we need this check\nits already done in the prelive migration case but if we fall when we call migaate i.e. any time after pre_live_migration exits succecully we still need to clean up in roolback.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":true,"context_lines":[{"line_number":10335,"context_line":"        # BlockDeviceMappingList."},{"line_number":10336,"context_line":"        vol_bdms \u003d [bdm for bdm in bdms if bdm.is_volume]"},{"line_number":10337,"context_line":""},{"line_number":10338,"context_line":"        if not pre_live_migration:"},{"line_number":10339,"context_line":"            # This will do both a driver detach and a Cinder attachment delete."},{"line_number":10340,"context_line":"            # If we are in here due to a pre_live_migration failure, BDMs have"},{"line_number":10341,"context_line":"            # already been rolled back to contain info for the source, so don\u0027t"}],"source_content_type":"text/x-python","patch_set":2,"id":"d4031466_31f574d8","line":10338,"in_reply_to":"595596dc_95a639c7","updated":"2025-04-23 03:02:44.000000000","message":"Yeah. I interpreted this to be because this _rollback_live_migration is used as a catch all for any failure scenario and what needs to be done is different in different cases. In a way while working on this I thought maybe there should be two (or more) separate rollback methods depending on where a failure happens but maybe that would be a bad idea.\n\nObviously someone else ran into the same problem in the past with \"where did it fail\" and added that pre_live_migration flag.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ebdacb56d1b2a31d874b7e4f3dadebfe12884275","unresolved":true,"context_lines":[{"line_number":10335,"context_line":"        # BlockDeviceMappingList."},{"line_number":10336,"context_line":"        vol_bdms \u003d [bdm for bdm in bdms if bdm.is_volume]"},{"line_number":10337,"context_line":""},{"line_number":10338,"context_line":"        if not pre_live_migration:"},{"line_number":10339,"context_line":"            # This will do both a driver detach and a Cinder attachment delete."},{"line_number":10340,"context_line":"            # If we are in here due to a pre_live_migration failure, BDMs have"},{"line_number":10341,"context_line":"            # already been rolled back to contain info for the source, so don\u0027t"}],"source_content_type":"text/x-python","patch_set":2,"id":"5db604ff_3f77b105","line":10338,"in_reply_to":"9c3d08b1_c53c1a0e","updated":"2025-05-14 12:15:50.000000000","message":"to me its a separate code path because its logically a different edge on the state machine transtion\n\ni.e. rolling back from a pre-migration failure is logically different then an in-progress migration as those are logiclly diffent state and imply idffent work has been done.\n\ntoday we have _cleanup_pre_live_migration jsut call _rollback_live_migration\n\nbut im not sure that is correct. i feell like if we eitehr duplciated the code or had a common funciton it may help but what more problematic is \nthat the delta is lost when callign the dest node. today we cant tell which state we are rolling back from when cleaning up the volumes on the destination.\n\nspliting the cleanup logic without addressing that issue wont really help with this bug so we can park this disucsion for now. i htink it would be eiaser to reaosn about if we had two indepented cleanup funciton for the rolback based on the previous state but keeping them in sync when we add new funcitonalty is also a consideration and likely why the flag was added instead fo keeping them seperate.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e05c0490b16eead152c76a5c1535d7087bb0130e","unresolved":true,"context_lines":[{"line_number":10335,"context_line":"        # BlockDeviceMappingList."},{"line_number":10336,"context_line":"        vol_bdms \u003d [bdm for bdm in bdms if bdm.is_volume]"},{"line_number":10337,"context_line":""},{"line_number":10338,"context_line":"        if not pre_live_migration:"},{"line_number":10339,"context_line":"            # This will do both a driver detach and a Cinder attachment delete."},{"line_number":10340,"context_line":"            # If we are in here due to a pre_live_migration failure, BDMs have"},{"line_number":10341,"context_line":"            # already been rolled back to contain info for the source, so don\u0027t"}],"source_content_type":"text/x-python","patch_set":2,"id":"9c3d08b1_c53c1a0e","line":10338,"in_reply_to":"9ea23da0_992875e5","updated":"2025-05-08 01:55:31.000000000","message":"I\u0027m not sure I understand, because _cleanup_pre_live_migration just calls _rollback_live_migration after setting migration status. That doesn\u0027t seem like a separate path from failing pre_live_migration vs middle of live migration.\n\nThat said, I agree I\u0027m not sure it would be an improvement really. I\u0027m just speaking from the perspective of someone who was trying to differentiate the different steps needed for failing during pre_live_migration vs live migration.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":true,"context_lines":[{"line_number":10335,"context_line":"        # BlockDeviceMappingList."},{"line_number":10336,"context_line":"        vol_bdms \u003d [bdm for bdm in bdms if bdm.is_volume]"},{"line_number":10337,"context_line":""},{"line_number":10338,"context_line":"        if not pre_live_migration:"},{"line_number":10339,"context_line":"            # This will do both a driver detach and a Cinder attachment delete."},{"line_number":10340,"context_line":"            # If we are in here due to a pre_live_migration failure, BDMs have"},{"line_number":10341,"context_line":"            # already been rolled back to contain info for the source, so don\u0027t"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ea23da0_992875e5","line":10338,"in_reply_to":"d4031466_31f574d8","updated":"2025-05-07 23:13:39.000000000","message":"we do have 2 today\n\nwe have this geneicr one _rollback_live_migration\n\nand then there is a seperate one  _cleanup_pre_live_migration\n\nwhich calls _rollback_live_migration internally.\n\nthat how the bool get set to true\n\nso we could refactor that i guess into 3 methods. a common method that\nis shared and then oen for each of pre and live migration failure.\n\nwe dont rollback if we fail in post live migration so we only have 2 states.\n\nim not sure that is actully goign to hlep much however.\n\ndisentageling the logic is going to be tricky.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"7ec79af436027e05ef83b861566b5c13bfb23210","unresolved":true,"context_lines":[{"line_number":10497,"context_line":"            # of a driver detach call to remove_volume_connection()."},{"line_number":10498,"context_line":"            # Set the list for Cinder volumes to empty to avoid attempting to"},{"line_number":10499,"context_line":"            # disconnect volumes during driver.cleanup() on the destination."},{"line_number":10500,"context_line":"            block_device_info[\u0027block_device_mapping\u0027] \u003d []"},{"line_number":10501,"context_line":""},{"line_number":10502,"context_line":"            # free any instance PCI claims done on destination during"},{"line_number":10503,"context_line":"            # check_can_live_migrate_destination()"}],"source_content_type":"text/x-python","patch_set":2,"id":"cc1d5843_242c1525","line":10500,"updated":"2025-04-22 22:53:27.000000000","message":"do we need to protect thsi with somethign like the temperary mutation decorator?\n\nblock_device_info is not an ovo and we never save that back to the db right?\n\nso its proably ok to clear this i just want tot make sure this has no side effect other then the one noted in the comment above.","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8cf51163f76806afbc18de10825442ff4f1e3e7d","unresolved":false,"context_lines":[{"line_number":10497,"context_line":"            # of a driver detach call to remove_volume_connection()."},{"line_number":10498,"context_line":"            # Set the list for Cinder volumes to empty to avoid attempting to"},{"line_number":10499,"context_line":"            # disconnect volumes during driver.cleanup() on the destination."},{"line_number":10500,"context_line":"            block_device_info[\u0027block_device_mapping\u0027] \u003d []"},{"line_number":10501,"context_line":""},{"line_number":10502,"context_line":"            # free any instance PCI claims done on destination during"},{"line_number":10503,"context_line":"            # check_can_live_migrate_destination()"}],"source_content_type":"text/x-python","patch_set":2,"id":"1758fcd6_4105fb68","line":10500,"in_reply_to":"7067e258_60a10522","updated":"2025-05-15 10:08:42.000000000","message":"Acknowledged","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"835fe7f02b0940187cf8d731badbf44f89a09f70","unresolved":true,"context_lines":[{"line_number":10497,"context_line":"            # of a driver detach call to remove_volume_connection()."},{"line_number":10498,"context_line":"            # Set the list for Cinder volumes to empty to avoid attempting to"},{"line_number":10499,"context_line":"            # disconnect volumes during driver.cleanup() on the destination."},{"line_number":10500,"context_line":"            block_device_info[\u0027block_device_mapping\u0027] \u003d []"},{"line_number":10501,"context_line":""},{"line_number":10502,"context_line":"            # free any instance PCI claims done on destination during"},{"line_number":10503,"context_line":"            # check_can_live_migrate_destination()"}],"source_content_type":"text/x-python","patch_set":2,"id":"7067e258_60a10522","line":10500,"in_reply_to":"cc1d5843_242c1525","updated":"2025-04-23 03:02:44.000000000","message":"Hm, yeah. The block_device_info contains lists of DriverBlockDevice which have underlying BDM ovos and could be saved back to the database if someone went into the lists and called save(). I don\u0027t think we could use the temporary mutation decorator in this case because of the lists of indirect objects.\n\nI think at best we could make a copy and pass that instead but it won\u0027t guard against something inside driver.rollback_live_migration_at_destination() saving something back to the database if it wants to. (I think the same is true for the temporary mutation decorator).","commit_id":"88aea1591b82a587beb127a87b20572895875081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8c6d84468c48cd377f07fec2b0a07ef578603d4","unresolved":true,"context_lines":[{"line_number":9440,"context_line":"                            part of the callback interface, so this is never"},{"line_number":9441,"context_line":"                            None"},{"line_number":9442,"context_line":"        \"\"\""},{"line_number":9443,"context_line":"        self._set_migration_status(migration, \u0027error\u0027)"},{"line_number":9444,"context_line":"        # Make sure we set this for _rollback_live_migration()"},{"line_number":9445,"context_line":"        # so it can find it, as expected if it was called later"},{"line_number":9446,"context_line":"        migrate_data.migration \u003d migration"},{"line_number":9447,"context_line":"        self._rollback_live_migration(context, instance, dest,"},{"line_number":9448,"context_line":"                                      migrate_data\u003dmigrate_data,"},{"line_number":9449,"context_line":"                                      source_bdms\u003dsource_bdms,"},{"line_number":9450,"context_line":"                                      pre_live_migration\u003dTrue)"},{"line_number":9451,"context_line":""},{"line_number":9452,"context_line":"    def _do_pre_live_migration_from_source(self, context, dest, instance,"},{"line_number":9453,"context_line":"                                           block_migration, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"88f24b7f_d96d0995","line":9450,"range":{"start_line":9443,"start_character":7,"end_line":9450,"end_character":62},"updated":"2025-05-07 23:13:39.000000000","message":"To be able to check the migration status on the dest and know that an upgraded compute shoudl treat the disconnect rpc as a noop we woudl need to change this to something like this\n\n```suggestion\n        try:\n            self._set_migration_status(migration, \u0027pre-migration-error\u0027)\n            # Make sure we set this for _rollback_live_migration()\n            # so it can find it, as expected if it was called later\n            migrate_data.migration \u003d migration\n            self._rollback_live_migration(\n                context, instance, dest, migrate_data\u003dmigrate_data,\n                source_bdms\u003dsource_bdms, pre_live_migration\u003dTrue)\n        finally:\n            self._set_migration_status(migration, \u0027error\u0027)\n```\n\nin other words defer setign the migration to errror form pre-live migrating until we have done the cleanup. and set it to pre-migration-error instead \n\nthen we can look up the migration on the dest and knwo if we are in pre-migration or not when we are cleaning up.\n\n\nfor better or worse the status filed is a string not an enum\n\nhttps://github.com/openstack/nova/blob/master/nova/objects/migration.py#L73\n\nso we can add a new state.","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e05c0490b16eead152c76a5c1535d7087bb0130e","unresolved":true,"context_lines":[{"line_number":9440,"context_line":"                            part of the callback interface, so this is never"},{"line_number":9441,"context_line":"                            None"},{"line_number":9442,"context_line":"        \"\"\""},{"line_number":9443,"context_line":"        self._set_migration_status(migration, \u0027error\u0027)"},{"line_number":9444,"context_line":"        # Make sure we set this for _rollback_live_migration()"},{"line_number":9445,"context_line":"        # so it can find it, as expected if it was called later"},{"line_number":9446,"context_line":"        migrate_data.migration \u003d migration"},{"line_number":9447,"context_line":"        self._rollback_live_migration(context, instance, dest,"},{"line_number":9448,"context_line":"                                      migrate_data\u003dmigrate_data,"},{"line_number":9449,"context_line":"                                      source_bdms\u003dsource_bdms,"},{"line_number":9450,"context_line":"                                      pre_live_migration\u003dTrue)"},{"line_number":9451,"context_line":""},{"line_number":9452,"context_line":"    def _do_pre_live_migration_from_source(self, context, dest, instance,"},{"line_number":9453,"context_line":"                                           block_migration, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"c904343d_4ccc1450","line":9450,"range":{"start_line":9443,"start_character":7,"end_line":9450,"end_character":62},"in_reply_to":"88f24b7f_d96d0995","updated":"2025-05-08 01:55:31.000000000","message":"Hm, that is an interesting idea. I didn\u0027t consider using migration states as a marker.\n\nDo you think we should do something like that instead of what I currently have proposed? Just want to make sure I understand.","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ebdacb56d1b2a31d874b7e4f3dadebfe12884275","unresolved":true,"context_lines":[{"line_number":9440,"context_line":"                            part of the callback interface, so this is never"},{"line_number":9441,"context_line":"                            None"},{"line_number":9442,"context_line":"        \"\"\""},{"line_number":9443,"context_line":"        self._set_migration_status(migration, \u0027error\u0027)"},{"line_number":9444,"context_line":"        # Make sure we set this for _rollback_live_migration()"},{"line_number":9445,"context_line":"        # so it can find it, as expected if it was called later"},{"line_number":9446,"context_line":"        migrate_data.migration \u003d migration"},{"line_number":9447,"context_line":"        self._rollback_live_migration(context, instance, dest,"},{"line_number":9448,"context_line":"                                      migrate_data\u003dmigrate_data,"},{"line_number":9449,"context_line":"                                      source_bdms\u003dsource_bdms,"},{"line_number":9450,"context_line":"                                      pre_live_migration\u003dTrue)"},{"line_number":9451,"context_line":""},{"line_number":9452,"context_line":"    def _do_pre_live_migration_from_source(self, context, dest, instance,"},{"line_number":9453,"context_line":"                                           block_migration, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa3b1b90_fa2139fa","line":9450,"range":{"start_line":9443,"start_character":7,"end_line":9450,"end_character":62},"in_reply_to":"c904343d_4ccc1450","updated":"2025-05-14 12:15:50.000000000","message":"im unsure. partly oth because im not sure how i feel about potentially backporting a new state.\n\ni think having a new state or an rpc parameter to indicate if its pre-migration or not would help future us deal with similar bugs but perhaps we should proceed with your current approach and consider this a s master only followup change.\n\ni have not had time to review v3 yet so i have not really formed an opipion on the current proposal. i chatted to gibi breifly and they indicated they were more or less ok witht he direction so ill try an load context and review this again in the next day or two.\n\nwe coudl also chat about this in realtiem when your onlien if you think that would help on irc or gmeet. whatever works best.","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8cf51163f76806afbc18de10825442ff4f1e3e7d","unresolved":false,"context_lines":[{"line_number":9440,"context_line":"                            part of the callback interface, so this is never"},{"line_number":9441,"context_line":"                            None"},{"line_number":9442,"context_line":"        \"\"\""},{"line_number":9443,"context_line":"        self._set_migration_status(migration, \u0027error\u0027)"},{"line_number":9444,"context_line":"        # Make sure we set this for _rollback_live_migration()"},{"line_number":9445,"context_line":"        # so it can find it, as expected if it was called later"},{"line_number":9446,"context_line":"        migrate_data.migration \u003d migration"},{"line_number":9447,"context_line":"        self._rollback_live_migration(context, instance, dest,"},{"line_number":9448,"context_line":"                                      migrate_data\u003dmigrate_data,"},{"line_number":9449,"context_line":"                                      source_bdms\u003dsource_bdms,"},{"line_number":9450,"context_line":"                                      pre_live_migration\u003dTrue)"},{"line_number":9451,"context_line":""},{"line_number":9452,"context_line":"    def _do_pre_live_migration_from_source(self, context, dest, instance,"},{"line_number":9453,"context_line":"                                           block_migration, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ad16dbc9_7668ddab","line":9450,"range":{"start_line":9443,"start_character":7,"end_line":9450,"end_character":62},"in_reply_to":"fa3b1b90_fa2139fa","updated":"2025-05-15 10:08:42.000000000","message":"Acknowledged","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"}],"nova/tests/functional/regressions/test_bug_1899835.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8cf51163f76806afbc18de10825442ff4f1e3e7d","unresolved":true,"context_lines":[{"line_number":200,"context_line":"            ctxt, nova_fixtures.CinderFixture.IMAGE_BACKED_VOL,"},{"line_number":201,"context_line":"            instance_uuid\u003dserver[\u0027id\u0027])"},{"line_number":202,"context_line":"        current_connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":203,"context_line":"        self.assertEqual(src_connection_info, current_connection_info)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f57e220_cfa017ef","line":203,"updated":"2025-05-15 10:08:42.000000000","message":"+1\nthis looks pretty good to me.","commit_id":"5a55a78d510b86975f0f4f8f43ee1feef7206244"}]}