)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7e1d91fbf6f8d607af844d0c28694cd6eacef704","unresolved":false,"context_lines":[{"line_number":24,"context_line":"modification on the destination node. As well as working round this"},{"line_number":25,"context_line":"specific issue, it also avoids a redundant cinder call in all cases."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"Fixes: bug #1754716"},{"line_number":28,"context_line":"Change-Id: I0390c9ff51f49b063f736ca6ef868a4fa782ede5"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"df7087c5_b7d9faf3","line":27,"range":{"start_line":27,"start_character":0,"end_line":27,"end_character":19},"updated":"2018-03-13 11:04:21.000000000","message":"Not sure if this will link correctly this way. I think it\u0027s normally:\n\n  Closes-Bug: #1754716","commit_id":"01affd8112dbcb19c520f5d8005dec66040ffb41"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f42b480fb172315a578ba79544b78663d1dde0c4","unresolved":false,"context_lines":[{"line_number":24,"context_line":"modification on the destination node. As well as working round this"},{"line_number":25,"context_line":"specific issue, it also avoids a redundant cinder call in all cases."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"Fixes: bug #1754716"},{"line_number":28,"context_line":"Change-Id: I0390c9ff51f49b063f736ca6ef868a4fa782ede5"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"df7087c5_a696a692","line":27,"range":{"start_line":27,"start_character":0,"end_line":27,"end_character":19},"in_reply_to":"df7087c5_b7d9faf3","updated":"2018-03-13 13:44:31.000000000","message":"I agree it should be Closes-Bug","commit_id":"01affd8112dbcb19c520f5d8005dec66040ffb41"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":11,"context_line":"node. This means that after migration completes the BDM no longer has a"},{"line_number":12,"context_line":"reference to the original connection_info to do the detach on the source"},{"line_number":13,"context_line":"host. To address this, change I3dfb75eb added a second call to"},{"line_number":14,"context_line":"initialize_connection on the source host to re-fetch the original"},{"line_number":15,"context_line":"connection_info before calling disconnect."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Unfortunately the cinder driver interface does not strictly require that"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df7087c5_1cd2705d","line":14,"range":{"start_line":14,"start_character":57,"end_line":14,"end_character":65},"updated":"2018-03-21 18:45:52.000000000","message":"s/original/source host/\n\nThe original is gone and the BDM.connection_info at this point that we\u0027re trying to use it in post_live_migration is from the destination host, so the code calls cinder\u0027s initialize_connection API to get a new connection_info based on the source host connector, but it might not be the same as the original connection_info used to connect the volume on the source host.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":29,"context_line":"original connection_info. This required considering the cinder v2 and v3"},{"line_number":30,"context_line":"use cases, and reconstructing the multipath_id which was written to"},{"line_number":31,"context_line":"connection_info by the libvirt fibrechannel volume connector on"},{"line_number":32,"context_line":"connection. These things are not necessary when we just use the original"},{"line_number":33,"context_line":"data unmodified."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Other drivers affected are Xenapi and HyperV. Xenapi doesn\u0027t touch"},{"line_number":36,"context_line":"volumes in post_live_migration, so is unaffected. HyperV did not"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df7087c5_9c4a0025","line":33,"range":{"start_line":32,"start_character":12,"end_line":33,"end_character":16},"updated":"2018-03-21 18:45:52.000000000","message":"Well, I don\u0027t think any of this was a problem for the new v3 flow since the connection_info is stored in the per-host cinder attachment records, nova just keeps a pointer to the attachment IDs.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e337f5b7934a6156358589e110af5608774f5628","unresolved":false,"context_lines":[{"line_number":29,"context_line":"original connection_info. This required considering the cinder v2 and v3"},{"line_number":30,"context_line":"use cases, and reconstructing the multipath_id which was written to"},{"line_number":31,"context_line":"connection_info by the libvirt fibrechannel volume connector on"},{"line_number":32,"context_line":"connection. These things are not necessary when we just use the original"},{"line_number":33,"context_line":"data unmodified."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Other drivers affected are Xenapi and HyperV. Xenapi doesn\u0027t touch"},{"line_number":36,"context_line":"volumes in post_live_migration, so is unaffected. HyperV did not"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"bf659307_7d0f08e1","line":33,"range":{"start_line":32,"start_character":12,"end_line":33,"end_character":16},"in_reply_to":"df7087c5_9c4a0025","updated":"2018-03-27 09:28:10.000000000","message":"Actually I think the fibrechannel driver could be a problem even in the v3 case because we hack multipath_id in there in Nova, which Cinder wouldn\u0027t know anything about:\n\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/volume/fibrechannel.py#L59-L60\n\nFrom the comment in disconnect_volume, it looks like getting rid of this has been a TODO list item since the move to os-brick in Liberty.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":32,"context_line":"connection. These things are not necessary when we just use the original"},{"line_number":33,"context_line":"data unmodified."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Other drivers affected are Xenapi and HyperV. Xenapi doesn\u0027t touch"},{"line_number":36,"context_line":"volumes in post_live_migration, so is unaffected. HyperV did not"},{"line_number":37,"context_line":"previously account for differences in connection_info between source and"},{"line_number":38,"context_line":"destination, so was likely previously broken. This change should fix it."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df7087c5_fc5c5cee","line":35,"updated":"2018-03-21 18:45:52.000000000","message":"PowerVM also has a migrate data object, but it\u0027s just for their out of tree driver at this time since the in-tree driver doesn\u0027t yet support live migration.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"5f65e5633e073ce6987bcf36392eff3832da3fa9","unresolved":false,"context_lines":[{"line_number":33,"context_line":"data unmodified."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Other drivers affected are Xenapi and HyperV. Xenapi doesn\u0027t touch"},{"line_number":36,"context_line":"volumes in post_live_migration, so is unaffected. HyperV did not"},{"line_number":37,"context_line":"previously account for differences in connection_info between source and"},{"line_number":38,"context_line":"destination, so was likely previously broken. This change should fix it."},{"line_number":39,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df7087c5_1311bb5d","line":36,"updated":"2018-03-14 10:03:43.000000000","message":"XenAPI is affected and unaffected at the same time? Schrödinger\u0027s XenAPI? :D\n\nIf you have to re-spin for some reason, might want to just elide the first sentence.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Other drivers affected are Xenapi and HyperV. Xenapi doesn\u0027t touch"},{"line_number":36,"context_line":"volumes in post_live_migration, so is unaffected. HyperV did not"},{"line_number":37,"context_line":"previously account for differences in connection_info between source and"},{"line_number":38,"context_line":"destination, so was likely previously broken. This change should fix it."},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"Closes-Bug: #1754716"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df7087c5_3c0894d7","line":37,"range":{"start_line":37,"start_character":0,"end_line":37,"end_character":69},"updated":"2018-03-21 18:45:52.000000000","message":"OK so the workaround from I3dfb75eb never extended to hyper-v. Good to know.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"21c694689cf58da7bc5eaeab67890014a5df1e65","unresolved":false,"context_lines":[{"line_number":37,"context_line":"previously account for differences in connection_info between source and"},{"line_number":38,"context_line":"destination, so was likely previously broken. This change should fix it."},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"Closes-Bug: #1754716"},{"line_number":41,"context_line":"Change-Id: I0390c9ff51f49b063f736ca6ef868a4fa782ede5"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"9fdfeff1_dfb410f5","line":40,"updated":"2019-02-06 23:27:02.000000000","message":"Sounds like this should also close bug 1814245.","commit_id":"0411f3235e34b03edfbe305bc04e89db4c2f4a96"}],"nova/compute/manager.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7e1d91fbf6f8d607af844d0c28694cd6eacef704","unresolved":false,"context_lines":[{"line_number":6214,"context_line":"        :param migrate_data: if not None, it is a dict which has data"},{"line_number":6215,"context_line":"        :param source_bdms: BDMs prior to modification by the destination"},{"line_number":6216,"context_line":"                            compute host. Set by _do_live_migration and not"},{"line_number":6217,"context_line":"                            part of the callback interface, so this is never"},{"line_number":6218,"context_line":"                            None"},{"line_number":6219,"context_line":"        required for live migration without shared storage"},{"line_number":6220,"context_line":""},{"line_number":6221,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"df7087c5_77a60256","line":6218,"range":{"start_line":6217,"start_character":60,"end_line":6218,"end_character":32},"updated":"2018-03-13 11:04:21.000000000","message":"If this is never None, can we change the order of the paramters so we don\u0027t have a default parameter of None? :)","commit_id":"01affd8112dbcb19c520f5d8005dec66040ffb41"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"731dad5feb77561e87dc29a82db2f9bcc81add0d","unresolved":false,"context_lines":[{"line_number":6214,"context_line":"        :param migrate_data: if not None, it is a dict which has data"},{"line_number":6215,"context_line":"        :param source_bdms: BDMs prior to modification by the destination"},{"line_number":6216,"context_line":"                            compute host. Set by _do_live_migration and not"},{"line_number":6217,"context_line":"                            part of the callback interface, so this is never"},{"line_number":6218,"context_line":"                            None"},{"line_number":6219,"context_line":"        required for live migration without shared storage"},{"line_number":6220,"context_line":""},{"line_number":6221,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"df7087c5_51d8a61c","line":6218,"range":{"start_line":6217,"start_character":60,"end_line":6218,"end_character":32},"in_reply_to":"df7087c5_66c08e2b","updated":"2018-03-13 14:02:53.000000000","message":"Actually my first pass made it a positional. Because of the way functools.partial works it has to be the first positional argument. This works fine unless the method raises an exception (which fortunately it did in my testing), in which which case @wrap_exception breaks because it expects the first positional argument to be a RequestContext.","commit_id":"01affd8112dbcb19c520f5d8005dec66040ffb41"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f42b480fb172315a578ba79544b78663d1dde0c4","unresolved":false,"context_lines":[{"line_number":6214,"context_line":"        :param migrate_data: if not None, it is a dict which has data"},{"line_number":6215,"context_line":"        :param source_bdms: BDMs prior to modification by the destination"},{"line_number":6216,"context_line":"                            compute host. Set by _do_live_migration and not"},{"line_number":6217,"context_line":"                            part of the callback interface, so this is never"},{"line_number":6218,"context_line":"                            None"},{"line_number":6219,"context_line":"        required for live migration without shared storage"},{"line_number":6220,"context_line":""},{"line_number":6221,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"df7087c5_66c08e2b","line":6218,"range":{"start_line":6217,"start_character":60,"end_line":6218,"end_character":32},"in_reply_to":"df7087c5_77a60256","updated":"2018-03-13 13:44:31.000000000","message":"In general it\u0027s not a good idea, since drivers may be passing optional arguments as positional, although in this case since we are using the functools.partial we could do it without introducing any bugs.\n\nI prefer this implementation to the alternative.","commit_id":"01affd8112dbcb19c520f5d8005dec66040ffb41"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fb6ac43c4a410dd4f4c437a4501d207d5f07ae3c","unresolved":false,"context_lines":[{"line_number":6235,"context_line":"                    # cinder v3.44 api flow - delete the old attachment"},{"line_number":6236,"context_line":"                    # for the source host"},{"line_number":6237,"context_line":"                    old_attachment_id \u003d \\"},{"line_number":6238,"context_line":"                        migrate_data.old_vol_attachment_ids[bdm.volume_id]"},{"line_number":6239,"context_line":"                    self.volume_api.attachment_delete(ctxt, old_attachment_id)"},{"line_number":6240,"context_line":""},{"line_number":6241,"context_line":"        # Releasing vlan."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_2ccf07b1","side":"PARENT","line":6238,"updated":"2018-03-21 15:31:04.000000000","message":"So, this was clearly purpose-built for the new attach flow and this exact sort of problem. I wonder if it might be best to leave this in place so that if/when we can remove the old-style attachment stuff, we\u0027re no longer needing to kinda hack around the problem with partial() in the way you have it?\n\nOtherwise, I expect this basically makes old_attachment_id useless (which isn\u0027t necessarily a bad thing, but the authors added it instead of doing this, so...)","commit_id":"b1173396edc1f364ae00d774a0dbb25de12408c8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"65e951a6dc7041398b3f976428fae90c4bb3b03e","unresolved":false,"context_lines":[{"line_number":6235,"context_line":"                    # cinder v3.44 api flow - delete the old attachment"},{"line_number":6236,"context_line":"                    # for the source host"},{"line_number":6237,"context_line":"                    old_attachment_id \u003d \\"},{"line_number":6238,"context_line":"                        migrate_data.old_vol_attachment_ids[bdm.volume_id]"},{"line_number":6239,"context_line":"                    self.volume_api.attachment_delete(ctxt, old_attachment_id)"},{"line_number":6240,"context_line":""},{"line_number":6241,"context_line":"        # Releasing vlan."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_d7037841","side":"PARENT","line":6238,"in_reply_to":"df7087c5_2ccf07b1","updated":"2018-03-21 16:20:59.000000000","message":"I changed it just to make the 2 branches consistent. As this is the \u0027original\u0027 data rather than what was sent and received I guess it\u0027s also slightly more reliable, although I don\u0027t imagine that\u0027s important at this stage.\n\nI\u0027m ambivalent, tbh. I suspect it\u0027s the way it is because it used the previous behaviour as a template. Changing it here makes it a little more obvious that it is redundant, but I don\u0027t think it\u0027s important.","commit_id":"b1173396edc1f364ae00d774a0dbb25de12408c8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6235,"context_line":"                    # cinder v3.44 api flow - delete the old attachment"},{"line_number":6236,"context_line":"                    # for the source host"},{"line_number":6237,"context_line":"                    old_attachment_id \u003d \\"},{"line_number":6238,"context_line":"                        migrate_data.old_vol_attachment_ids[bdm.volume_id]"},{"line_number":6239,"context_line":"                    self.volume_api.attachment_delete(ctxt, old_attachment_id)"},{"line_number":6240,"context_line":""},{"line_number":6241,"context_line":"        # Releasing vlan."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_9c276013","side":"PARENT","line":6238,"in_reply_to":"df7087c5_d7037841","updated":"2018-03-21 18:45:52.000000000","message":"I\u0027m also fairly ambivalent about this. One the one hand, it sucks that we did all of this plumbing to stash stuff for the new flow with live migration just to not need it, but on the other hand I can\u0027t think of a great excuse for needing to keep it when this should be sufficient.\n\nWe also don\u0027t have any mixed compute issues here as far as I can tell.\n\nIf we\u0027re talking to an older dest compute, it\u0027s putting stuff in migrate_data and we don\u0027t care since we\u0027re not using it now. If the source compute is old, it\u0027s just business as usual and nothing we can do since post_live_migration runs on the source host and the bug will be there if you\u0027re using a cinder driver that doesn\u0027t treat initialize_connection as idempotent (or you don\u0027t have this fix in stable yet).","commit_id":"b1173396edc1f364ae00d774a0dbb25de12408c8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":5683,"context_line":"            old_volume_id, new_volume_id)"},{"line_number":5684,"context_line":""},{"line_number":5685,"context_line":"    @wrap_exception()"},{"line_number":5686,"context_line":"    def remove_volume_connection(self, context, volume_id, instance):"},{"line_number":5687,"context_line":"        \"\"\"Remove the volume connection on this host"},{"line_number":5688,"context_line":""},{"line_number":5689,"context_line":"        Detach the volume from this instance on this host, and if this is"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_bcaca435","line":5686,"updated":"2018-03-21 18:45:52.000000000","message":"If we got through pre_live_migration and the BDM connection_info and attachment_id are now pointing at the destination host, but then live migration fails, the rollback method will call this to cleanup volume attachments on the dest host. In this case I think we\u0027re OK since we\u0027re on the dest host and the BDMs also \"look\" like they are for the dest host.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":5944,"context_line":"                        context, bdm.volume_id, bdm.instance_uuid,"},{"line_number":5945,"context_line":"                        connector\u003dconnector, mountpoint\u003dbdm.device_name)"},{"line_number":5946,"context_line":""},{"line_number":5947,"context_line":"                    # save current attachment so we can detach it on success,"},{"line_number":5948,"context_line":"                    # or restore it on a rollback."},{"line_number":5949,"context_line":"                    migrate_data.old_vol_attachment_ids[bdm.volume_id] \u003d \\"},{"line_number":5950,"context_line":"                        bdm.attachment_id"},{"line_number":5951,"context_line":""},{"line_number":5952,"context_line":"                    # update the bdm with the new attachment_id."},{"line_number":5953,"context_line":"                    bdm.attachment_id \u003d attach_ref[\u0027id\u0027]"},{"line_number":5954,"context_line":"                    bdm.save()"},{"line_number":5955,"context_line":"        except Exception:"},{"line_number":5956,"context_line":"            # If we raise, migrate_data with the updated attachment ids"},{"line_number":5957,"context_line":"            # will not be returned to the source host for rollback."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_1c2b50eb","line":5954,"range":{"start_line":5947,"start_character":20,"end_line":5954,"end_character":30},"updated":"2018-03-21 18:45:52.000000000","message":"Review note: this is where the switch-a-roo happens on the BDM.attachment_id to point at the destination host attachment, and the source host attachment_id is stashed off in the migrate_data object, which is now no longer needed in the _post_live_migration method on the source host.\n\nQuestion: should we put a TODO here to remove this in S (or when we drop the old style flow) since it\u0027s not being used on the source in post_live_migration now? Although it is still being used in _rollback_live_migration.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e8b36ae3a3a3b2a7761a57d38ddee6102297f09e","unresolved":false,"context_lines":[{"line_number":5944,"context_line":"                        context, bdm.volume_id, bdm.instance_uuid,"},{"line_number":5945,"context_line":"                        connector\u003dconnector, mountpoint\u003dbdm.device_name)"},{"line_number":5946,"context_line":""},{"line_number":5947,"context_line":"                    # save current attachment so we can detach it on success,"},{"line_number":5948,"context_line":"                    # or restore it on a rollback."},{"line_number":5949,"context_line":"                    migrate_data.old_vol_attachment_ids[bdm.volume_id] \u003d \\"},{"line_number":5950,"context_line":"                        bdm.attachment_id"},{"line_number":5951,"context_line":""},{"line_number":5952,"context_line":"                    # update the bdm with the new attachment_id."},{"line_number":5953,"context_line":"                    bdm.attachment_id \u003d attach_ref[\u0027id\u0027]"},{"line_number":5954,"context_line":"                    bdm.save()"},{"line_number":5955,"context_line":"        except Exception:"},{"line_number":5956,"context_line":"            # If we raise, migrate_data with the updated attachment ids"},{"line_number":5957,"context_line":"            # will not be returned to the source host for rollback."}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_cf2ba579","line":5954,"range":{"start_line":5947,"start_character":20,"end_line":5954,"end_character":30},"in_reply_to":"df7087c5_1c2b50eb","updated":"2018-07-05 16:01:36.000000000","message":"Assume you\u0027re just referring to removing old_vol_attachment_ids from migrate_data? Presumably we still need to set the new attachment_id on the BDM?\n\nYeah, I think it\u0027s worth commenting that as it\u0027s weird if you\u0027re following the code that it no longer appears used. I\u0027ll also add a comment to the object.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":5967,"context_line":"                        bdm.save()"},{"line_number":5968,"context_line":""},{"line_number":5969,"context_line":"        block_device_info \u003d self._get_instance_block_device_info("},{"line_number":5970,"context_line":"                            context, instance, refresh_conn_info\u003dTrue,"},{"line_number":5971,"context_line":"                            bdms\u003dbdms)"},{"line_number":5972,"context_line":""},{"line_number":5973,"context_line":"        network_info \u003d self.network_api.get_instance_nw_info(context, instance)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_9ccf607e","line":5970,"range":{"start_line":5970,"start_character":47,"end_line":5970,"end_character":69},"updated":"2018-03-21 18:45:52.000000000","message":"Review note: this is where the BDM.connection_info gets updated for the destination host. That\u0027s because refresh_conn_info\u003dTrue makes it run through this code:\n\nhttps://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/block_device.py#L625\n\nAnd that @update_db decorator saves the changes to the underlying BlockDeviceMapping object (wrapped in the DriverVolumeBlockDevice object).\n\nIt might be worth adding a comment on this call for posterity that this updates the BDM.connection_info for the destination host.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e8b36ae3a3a3b2a7761a57d38ddee6102297f09e","unresolved":false,"context_lines":[{"line_number":5967,"context_line":"                        bdm.save()"},{"line_number":5968,"context_line":""},{"line_number":5969,"context_line":"        block_device_info \u003d self._get_instance_block_device_info("},{"line_number":5970,"context_line":"                            context, instance, refresh_conn_info\u003dTrue,"},{"line_number":5971,"context_line":"                            bdms\u003dbdms)"},{"line_number":5972,"context_line":""},{"line_number":5973,"context_line":"        network_info \u003d self.network_api.get_instance_nw_info(context, instance)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_fd241b10","line":5970,"range":{"start_line":5970,"start_character":47,"end_line":5970,"end_character":69},"in_reply_to":"df7087c5_9ccf607e","updated":"2018-07-05 16:01:36.000000000","message":"Note added.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":5979,"context_line":"            action\u003dfields.NotificationAction.LIVE_MIGRATION_PRE,"},{"line_number":5980,"context_line":"            phase\u003dfields.NotificationPhase.START)"},{"line_number":5981,"context_line":""},{"line_number":5982,"context_line":"        migrate_data \u003d self.driver.pre_live_migration(context,"},{"line_number":5983,"context_line":"                                       instance,"},{"line_number":5984,"context_line":"                                       block_device_info,"},{"line_number":5985,"context_line":"                                       network_info,"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_7c160cf4","line":5982,"updated":"2018-03-21 18:45:52.000000000","message":"Review note: this is where the driver connects the volumes on the destination host.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6026,"context_line":"        # reporting"},{"line_number":6027,"context_line":"        self._set_migration_status(migration, \u0027preparing\u0027)"},{"line_number":6028,"context_line":""},{"line_number":6029,"context_line":"        # NOTE(mdbooth): pre_live_migration will update connection_info and"},{"line_number":6030,"context_line":"        # attachment_id on all volumes BDMS to reflect the new destination"},{"line_number":6031,"context_line":"        # host attachment. We fetch BDMs before that to retain connection_info"},{"line_number":6032,"context_line":"        # and attachment_id relating to the source host for post migration"},{"line_number":6033,"context_line":"        # cleanup."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_bc8fc42c","line":6030,"range":{"start_line":6029,"start_character":72,"end_line":6030,"end_character":23},"updated":"2018-03-21 18:45:52.000000000","message":"The attachment_id part here is true going back to Queens. If/when you backport this fix, that won\u0027t be true for any computes before Queens.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fb6ac43c4a410dd4f4c437a4501d207d5f07ae3c","unresolved":false,"context_lines":[{"line_number":6027,"context_line":"        self._set_migration_status(migration, \u0027preparing\u0027)"},{"line_number":6028,"context_line":""},{"line_number":6029,"context_line":"        # NOTE(mdbooth): pre_live_migration will update connection_info and"},{"line_number":6030,"context_line":"        # attachment_id on all volumes BDMS to reflect the new destination"},{"line_number":6031,"context_line":"        # host attachment. We fetch BDMs before that to retain connection_info"},{"line_number":6032,"context_line":"        # and attachment_id relating to the source host for post migration"},{"line_number":6033,"context_line":"        # cleanup."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_91767006","line":6030,"range":{"start_line":6030,"start_character":31,"end_line":6030,"end_character":43},"updated":"2018-03-21 15:31:04.000000000","message":"volume BDMs","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e8b36ae3a3a3b2a7761a57d38ddee6102297f09e","unresolved":false,"context_lines":[{"line_number":6026,"context_line":"        # reporting"},{"line_number":6027,"context_line":"        self._set_migration_status(migration, \u0027preparing\u0027)"},{"line_number":6028,"context_line":""},{"line_number":6029,"context_line":"        # NOTE(mdbooth): pre_live_migration will update connection_info and"},{"line_number":6030,"context_line":"        # attachment_id on all volumes BDMS to reflect the new destination"},{"line_number":6031,"context_line":"        # host attachment. We fetch BDMs before that to retain connection_info"},{"line_number":6032,"context_line":"        # and attachment_id relating to the source host for post migration"},{"line_number":6033,"context_line":"        # cleanup."}],"source_content_type":"text/x-python","patch_set":6,"id":"5f7c97a3_fd85fbae","line":6030,"range":{"start_line":6029,"start_character":72,"end_line":6030,"end_character":23},"in_reply_to":"df7087c5_bc8fc42c","updated":"2018-07-05 16:01:36.000000000","message":"ACK. Backport will be slightly messy, but the legacy side should at least remain the same.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6029,"context_line":"        # NOTE(mdbooth): pre_live_migration will update connection_info and"},{"line_number":6030,"context_line":"        # attachment_id on all volumes BDMS to reflect the new destination"},{"line_number":6031,"context_line":"        # host attachment. We fetch BDMs before that to retain connection_info"},{"line_number":6032,"context_line":"        # and attachment_id relating to the source host for post migration"},{"line_number":6033,"context_line":"        # cleanup."},{"line_number":6034,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6035,"context_line":"                context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_3c8df430","line":6032,"range":{"start_line":6032,"start_character":14,"end_line":6032,"end_character":27},"updated":"2018-03-21 18:45:52.000000000","message":"same","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fb6ac43c4a410dd4f4c437a4501d207d5f07ae3c","unresolved":false,"context_lines":[{"line_number":6034,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6035,"context_line":"                context, instance.uuid)"},{"line_number":6036,"context_line":"        post_live_migration \u003d functools.partial(self._post_live_migration,"},{"line_number":6037,"context_line":"                                                source_bdms\u003dsource_bdms)"},{"line_number":6038,"context_line":""},{"line_number":6039,"context_line":"        try:"},{"line_number":6040,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_31bbe472","line":6037,"updated":"2018-03-21 15:31:04.000000000","message":"I don\u0027t love this since the driver explicitly calls post and thinks it knows the full calling convention. We probably could convert the convention to just the driver calling post with no arguments and partial() our way to everything we need.\n\nBut, maybe not worth it for this bug fix.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"65e951a6dc7041398b3f976428fae90c4bb3b03e","unresolved":false,"context_lines":[{"line_number":6034,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6035,"context_line":"                context, instance.uuid)"},{"line_number":6036,"context_line":"        post_live_migration \u003d functools.partial(self._post_live_migration,"},{"line_number":6037,"context_line":"                                                source_bdms\u003dsource_bdms)"},{"line_number":6038,"context_line":""},{"line_number":6039,"context_line":"        try:"},{"line_number":6040,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_571448a0","line":6037,"in_reply_to":"df7087c5_31bbe472","updated":"2018-03-21 16:20:59.000000000","message":"Agreed. This was actually expediency to avoid changing the signature with all the potential for error. My preference would be to partial() everything that can be partial()ed and simplify the interface.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6034,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6035,"context_line":"                context, instance.uuid)"},{"line_number":6036,"context_line":"        post_live_migration \u003d functools.partial(self._post_live_migration,"},{"line_number":6037,"context_line":"                                                source_bdms\u003dsource_bdms)"},{"line_number":6038,"context_line":""},{"line_number":6039,"context_line":"        try:"},{"line_number":6040,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_dcd078cc","line":6037,"in_reply_to":"df7087c5_571448a0","updated":"2018-03-21 18:45:52.000000000","message":"I\u0027m not generally a fan of partials anyway, but this is admittedly a nice little way around the problem for now.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6472,"context_line":"        # NOTE(tr3buchet): setup networks on source host (really it\u0027s re-setup)"},{"line_number":6473,"context_line":"        self.network_api.setup_networks_on_host(context, instance, self.host)"},{"line_number":6474,"context_line":""},{"line_number":6475,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6476,"context_line":"                context, instance.uuid)"},{"line_number":6477,"context_line":"        for bdm in bdms:"},{"line_number":6478,"context_line":"            if bdm.is_volume:"},{"line_number":6479,"context_line":"                # remove the connection on the destination host"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_dc6cf809","line":6476,"range":{"start_line":6475,"start_character":8,"end_line":6476,"end_character":39},"updated":"2018-03-21 18:45:52.000000000","message":"These will be the BDMs that have the dest host information (connection_info and attachment_id) from pre_live_migration.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6480,"context_line":"                self.compute_rpcapi.remove_volume_connection("},{"line_number":6481,"context_line":"                        context, instance, bdm.volume_id, dest)"},{"line_number":6482,"context_line":""},{"line_number":6483,"context_line":"                if bdm.attachment_id:"},{"line_number":6484,"context_line":"                    # 3.44 cinder api flow. Set the bdm\u0027s"},{"line_number":6485,"context_line":"                    # attachment_id to the old attachment of the source"},{"line_number":6486,"context_line":"                    # host. If old_attachments is not there, then"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_5c80e88b","line":6483,"range":{"start_line":6483,"start_character":16,"end_line":6483,"end_character":36},"updated":"2018-03-21 18:45:52.000000000","message":"This will be the dest host attachment_id.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6488,"context_line":"                    old_attachments \u003d migrate_data.old_vol_attachment_ids \\"},{"line_number":6489,"context_line":"                        if \u0027old_vol_attachment_ids\u0027 in migrate_data else None"},{"line_number":6490,"context_line":"                    if old_attachments and bdm.volume_id in old_attachments:"},{"line_number":6491,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":6492,"context_line":"                                                          bdm.attachment_id)"},{"line_number":6493,"context_line":"                        bdm.attachment_id \u003d old_attachments[bdm.volume_id]"},{"line_number":6494,"context_line":"                        bdm.save()"},{"line_number":6495,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_dc139865","line":6492,"range":{"start_line":6491,"start_character":24,"end_line":6492,"end_character":76},"updated":"2018-03-21 18:45:52.000000000","message":"This is deleting the dest host attachment in cinder. We have to do that here because remove_volume_connection on the dest host doesn\u0027t do it. I can\u0027t really remember why at this point, it came up in the review during Queens.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":6490,"context_line":"                    if old_attachments and bdm.volume_id in old_attachments:"},{"line_number":6491,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":6492,"context_line":"                                                          bdm.attachment_id)"},{"line_number":6493,"context_line":"                        bdm.attachment_id \u003d old_attachments[bdm.volume_id]"},{"line_number":6494,"context_line":"                        bdm.save()"},{"line_number":6495,"context_line":""},{"line_number":6496,"context_line":"        self._notify_about_instance_usage(context, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_1c31100c","line":6493,"range":{"start_line":6493,"start_character":24,"end_line":6493,"end_character":74},"updated":"2018-03-21 18:45:52.000000000","message":"This changes the bdm.attachment_id back to the source host attachment_id, but the connection_info on the BDM at this point would still be wrong (it would be pointing at the dest host connection_info). That could cause problems later if we tried to detach this volume, because that flow eventually gets here:\n\nhttps://github.com/openstack/nova/blob/2ec8c49f6cb4a0e7dba217e824c20d9c703d2105/nova/virt/block_device.py#L285\n\nAnd then we\u0027re working with the wrong connection_info (from the dest host) and that could screw up the disconnect in os-brick, along with anything about encrypted volumes stored in the connection_info.\n\nSo maybe we need to pass the source_bdms to _rollback_live_migration also?","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f9849d9fc0c4027a30fee6cf6d642852e839f8a8","unresolved":false,"context_lines":[{"line_number":6490,"context_line":"                    if old_attachments and bdm.volume_id in old_attachments:"},{"line_number":6491,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":6492,"context_line":"                                                          bdm.attachment_id)"},{"line_number":6493,"context_line":"                        bdm.attachment_id \u003d old_attachments[bdm.volume_id]"},{"line_number":6494,"context_line":"                        bdm.save()"},{"line_number":6495,"context_line":""},{"line_number":6496,"context_line":"        self._notify_about_instance_usage(context, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f6a8fd7_d3c95578","line":6493,"range":{"start_line":6493,"start_character":24,"end_line":6493,"end_character":74},"in_reply_to":"bf659307_9d5f7c71","updated":"2018-04-18 19:48:47.000000000","message":"\u003e I don\u0027t mind combining the patches if you like.\n\nI\u0027m OK with them being separate to keep the review sane, but I probably won\u0027t vote on this patch until Ic4bbc075 is ready for review since I\u0027d like to see them merge close together.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"edd27a5160f10fc28fc0e884aa323b850b902ed4","unresolved":false,"context_lines":[{"line_number":6490,"context_line":"                    if old_attachments and bdm.volume_id in old_attachments:"},{"line_number":6491,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":6492,"context_line":"                                                          bdm.attachment_id)"},{"line_number":6493,"context_line":"                        bdm.attachment_id \u003d old_attachments[bdm.volume_id]"},{"line_number":6494,"context_line":"                        bdm.save()"},{"line_number":6495,"context_line":""},{"line_number":6496,"context_line":"        self._notify_about_instance_usage(context, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf659307_bd31200e","line":6493,"range":{"start_line":6493,"start_character":24,"end_line":6493,"end_character":74},"in_reply_to":"bf659307_9d5f7c71","updated":"2018-03-27 09:30:29.000000000","message":"Cut/paste fail, that\u0027s change Ic4bbc075.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e337f5b7934a6156358589e110af5608774f5628","unresolved":false,"context_lines":[{"line_number":6490,"context_line":"                    if old_attachments and bdm.volume_id in old_attachments:"},{"line_number":6491,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":6492,"context_line":"                                                          bdm.attachment_id)"},{"line_number":6493,"context_line":"                        bdm.attachment_id \u003d old_attachments[bdm.volume_id]"},{"line_number":6494,"context_line":"                        bdm.save()"},{"line_number":6495,"context_line":""},{"line_number":6496,"context_line":"        self._notify_about_instance_usage(context, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf659307_9d5f7c71","line":6493,"range":{"start_line":6493,"start_character":24,"end_line":6493,"end_character":74},"in_reply_to":"df7087c5_1c31100c","updated":"2018-03-27 09:28:10.000000000","message":"I also noticed this and I wrote change e6cdd169 to address it. However, I think it\u0027s a (related, but) different issue. I suspect it\u0027s also always been broken so I wanted to get this one squared away first.\n\nI don\u0027t mind combining the patches if you like.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e8b36ae3a3a3b2a7761a57d38ddee6102297f09e","unresolved":false,"context_lines":[{"line_number":1654,"context_line":"                                        bdms\u003dNone):"},{"line_number":1655,"context_line":"        \"\"\"Transform block devices to the driver block_device format.\"\"\""},{"line_number":1656,"context_line":""},{"line_number":1657,"context_line":"        if bdms is None:"},{"line_number":1658,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1659,"context_line":"                    context, instance.uuid)"},{"line_number":1660,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance, bdms)"}],"source_content_type":"text/x-python","patch_set":7,"id":"5f7c97a3_b2400d34","line":1657,"updated":"2018-07-05 16:01:36.000000000","message":"Reviewer note: this is a related change! We had tests passing an empty list of (source) BDMs which were failing when we tried to re-fetch those BDMs from the DB. This is potentially also a bug in non-test code.","commit_id":"39e787bcd5c53020c170ca697e9a68ea751070ab"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d86069097b7025a58a6e9d68a65333e5f6590230","unresolved":false,"context_lines":[{"line_number":1800,"context_line":"                                        bdms\u003dNone):"},{"line_number":1801,"context_line":"        \"\"\"Transform block devices to the driver block_device format.\"\"\""},{"line_number":1802,"context_line":""},{"line_number":1803,"context_line":"        if bdms is None:"},{"line_number":1804,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1805,"context_line":"                    context, instance.uuid)"},{"line_number":1806,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance, bdms)"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_14b468d0","line":1803,"updated":"2019-02-12 16:09:28.000000000","message":"This is OK but bdms is probably never an empty list in practice, just test code (which I\u0027ve hit that before in tests passing an empty list). Even a non-volume-backed server should have a single image BDM (source_type\u003dimage destination_type\u003dlocal I think). Anyway, this is fine and more explicit given the kwarg default.","commit_id":"57a869bfbb6a9ff6804ae4f24e086de2892d3c05"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e0d7f48af1943df4a57834c44fbf6a79c0e9bc6b","unresolved":false,"context_lines":[{"line_number":6292,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6293,"context_line":"                context, instance.uuid)"},{"line_number":6294,"context_line":"        post_live_migration \u003d functools.partial(self._post_live_migration,"},{"line_number":6295,"context_line":"                                                source_bdms\u003dsource_bdms)"},{"line_number":6296,"context_line":""},{"line_number":6297,"context_line":"        try:"},{"line_number":6298,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_9767fab7","line":6295,"updated":"2019-02-12 16:28:21.000000000","message":"Why are we doing this instead of just modifying how this gets called? Hiding wrappers around all our function calls makes it super hard to know how a thing is supposed to be called from looking at the actual call site and function signature. If we couldn\u0027t control the layer in the middle, the partial() is a handy hack, but this just seems sloppy to me.","commit_id":"57a869bfbb6a9ff6804ae4f24e086de2892d3c05"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"1b11b9344afe71d51f9be02ea46a048cc8f8ba24","unresolved":false,"context_lines":[{"line_number":6371,"context_line":"        # host attachment. We fetch BDMs before that to retain connection_info"},{"line_number":6372,"context_line":"        # and attachment_id relating to the source host for post migration"},{"line_number":6373,"context_line":"        # cleanup."},{"line_number":6374,"context_line":"        source_bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6375,"context_line":"                context, instance.uuid)"},{"line_number":6376,"context_line":"        post_live_migration \u003d functools.partial(self._post_live_migration,"},{"line_number":6377,"context_line":"                                                source_bdms\u003dsource_bdms)"},{"line_number":6378,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"9fdfeff1_84281886","line":6375,"range":{"start_line":6374,"start_character":0,"end_line":6375,"end_character":39},"updated":"2019-02-13 09:09:52.000000000","message":"This is causing the functional failures, this needs to happen at the start of this live migration method.","commit_id":"c3abb1bb0c9c7d8d42da535843a7f309dbafa03b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c67ea92cd11a29fbc753e4a546d98c171b7390d8","unresolved":false,"context_lines":[{"line_number":6289,"context_line":"        try:"},{"line_number":6290,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"},{"line_number":6291,"context_line":"                    migrate_data.block_migration):"},{"line_number":6292,"context_line":"                block_device_info \u003d self._get_instance_block_device_info("},{"line_number":6293,"context_line":"                    context, instance)"},{"line_number":6294,"context_line":"                disk \u003d self.driver.get_instance_disk_info("},{"line_number":6295,"context_line":"                    instance, block_device_info\u003dblock_device_info)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_b019d2d7","line":6292,"range":{"start_line":6292,"start_character":41,"end_line":6292,"end_character":72},"updated":"2019-02-13 13:12:37.000000000","message":"You can pass bdms\u003dsource_bdms to this and avoid the extra DB query in this method.","commit_id":"22f0c384d71f3f9bf52c5b98d99289961a5120c6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"e0911dd4a4c804add371a42240c428102d92ca69","unresolved":false,"context_lines":[{"line_number":6289,"context_line":"        try:"},{"line_number":6290,"context_line":"            if (\u0027block_migration\u0027 in migrate_data and"},{"line_number":6291,"context_line":"                    migrate_data.block_migration):"},{"line_number":6292,"context_line":"                block_device_info \u003d self._get_instance_block_device_info("},{"line_number":6293,"context_line":"                    context, instance)"},{"line_number":6294,"context_line":"                disk \u003d self.driver.get_instance_disk_info("},{"line_number":6295,"context_line":"                    instance, block_device_info\u003dblock_device_info)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_041f4308","line":6292,"range":{"start_line":6292,"start_character":41,"end_line":6292,"end_character":72},"in_reply_to":"9fdfeff1_b019d2d7","updated":"2019-02-13 13:42:03.000000000","message":"Done","commit_id":"22f0c384d71f3f9bf52c5b98d99289961a5120c6"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fb6ac43c4a410dd4f4c437a4501d207d5f07ae3c","unresolved":false,"context_lines":[{"line_number":6588,"context_line":"                                              source_bdms\u003dbdms)"},{"line_number":6589,"context_line":""},{"line_number":6590,"context_line":"            terminate_connection.assert_called_once_with("},{"line_number":6591,"context_line":"                    c, uuids.volume_id, \u0027fake-connector\u0027)"},{"line_number":6592,"context_line":""},{"line_number":6593,"context_line":"    @mock.patch.object(objects.ComputeNode,"},{"line_number":6594,"context_line":"                       \u0027get_by_host_and_nodename\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_6c02df0b","line":6591,"updated":"2018-03-21 15:31:04.000000000","message":"I think this is the only place you assert that we\u0027re actually calling terminate on the new set. Maybe that\u0027s enough, I guess.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d86069097b7025a58a6e9d68a65333e5f6590230","unresolved":false,"context_lines":[{"line_number":6426,"context_line":"            is_shared_block_storage\u003dFalse,"},{"line_number":6427,"context_line":"            migration\u003dmigration_obj,"},{"line_number":6428,"context_line":"            block_migration\u003dFalse)"},{"line_number":6429,"context_line":"        bdms \u003d objects.BlockDeviceMappingList(objects\u003d[])"},{"line_number":6430,"context_line":""},{"line_number":6431,"context_line":"        with test.nested("},{"line_number":6432,"context_line":"            mock.patch.object("}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_74d7f4ef","line":6429,"range":{"start_line":6429,"start_character":54,"end_line":6429,"end_character":56},"updated":"2019-02-12 16:09:28.000000000","message":"This is why you needed to add that \"if bdms is None\" check.","commit_id":"57a869bfbb6a9ff6804ae4f24e086de2892d3c05"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fb6ac43c4a410dd4f4c437a4501d207d5f07ae3c","unresolved":false,"context_lines":[{"line_number":6963,"context_line":"        @mock.patch.object(self.compute, \u0027compute_rpcapi\u0027)"},{"line_number":6964,"context_line":"        @mock.patch.object(self.compute, \u0027_notify_about_instance_usage\u0027)"},{"line_number":6965,"context_line":"        @mock.patch.object(self.compute, \u0027network_api\u0027)"},{"line_number":6966,"context_line":"        @mock.patch.object(self.compute, \u0027_get_instance_block_device_info\u0027)"},{"line_number":6967,"context_line":"        def _do_call(bdi, nwapi, notify, rpc, update):"},{"line_number":6968,"context_line":"            bdms \u003d objects.BlockDeviceMappingList(objects\u003d[])"},{"line_number":6969,"context_line":"            return self.compute._post_live_migration(self.context,"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_8c1bb319","line":6966,"updated":"2018-03-21 15:31:04.000000000","message":"I\u0027m missing why you need to mock this out now.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"65e951a6dc7041398b3f976428fae90c4bb3b03e","unresolved":false,"context_lines":[{"line_number":6963,"context_line":"        @mock.patch.object(self.compute, \u0027compute_rpcapi\u0027)"},{"line_number":6964,"context_line":"        @mock.patch.object(self.compute, \u0027_notify_about_instance_usage\u0027)"},{"line_number":6965,"context_line":"        @mock.patch.object(self.compute, \u0027network_api\u0027)"},{"line_number":6966,"context_line":"        @mock.patch.object(self.compute, \u0027_get_instance_block_device_info\u0027)"},{"line_number":6967,"context_line":"        def _do_call(bdi, nwapi, notify, rpc, update):"},{"line_number":6968,"context_line":"            bdms \u003d objects.BlockDeviceMappingList(objects\u003d[])"},{"line_number":6969,"context_line":"            return self.compute._post_live_migration(self.context,"}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_32f682fb","line":6966,"in_reply_to":"df7087c5_8c1bb319","updated":"2018-03-21 16:20:59.000000000","message":"Good point, seems it\u0027s not required. Can\u0027t remember why I did this. I\u0027ll take it out when I respin.","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d86069097b7025a58a6e9d68a65333e5f6590230","unresolved":false,"context_lines":[{"line_number":7438,"context_line":"        \"\"\""},{"line_number":7439,"context_line":"        migrate_data \u003d objects.LibvirtLiveMigrateData("},{"line_number":7440,"context_line":"            wait_for_vif_plugged\u003dTrue)"},{"line_number":7441,"context_line":"        mock_get_bdms.return_value \u003d objects.BlockDeviceMappingList(objects\u003d[])"},{"line_number":7442,"context_line":"        mock_pre_live_mig.return_value \u003d migrate_data"},{"line_number":7443,"context_line":"        self.instance.info_cache \u003d objects.InstanceInfoCache("},{"line_number":7444,"context_line":"            network_info\u003dnetwork_model.NetworkInfo(["}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_1410e8a7","line":7441,"updated":"2019-02-12 16:09:28.000000000","message":"Could have done this on the mock definition above:\n\n@mock.patch(\u0027nova.objects.BlockDeviceMappingList.get_by_instance_uuid\u0027,\n            return_value\u003dobjects.BlockDeviceMappingList())","commit_id":"57a869bfbb6a9ff6804ae4f24e086de2892d3c05"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b1b9b67710725410a5becc62fbcaa81ebd9072c6","unresolved":false,"context_lines":[{"line_number":7714,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":7715,"context_line":"                block_device_info)"},{"line_number":7716,"context_line":"        for vol in block_device_mapping:"},{"line_number":7717,"context_line":"            # NOTE(mdbooth): The block_device_info we were passed was"},{"line_number":7718,"context_line":"            # initialized with BDMs from the source host before they were"},{"line_number":7719,"context_line":"            # updated to point to the destination. We can safely use this to"},{"line_number":7720,"context_line":"            # disconnect the source without re-fetching."}],"source_content_type":"text/x-python","patch_set":6,"id":"df7087c5_1f282284","line":7717,"range":{"start_line":7717,"start_character":54,"end_line":7717,"end_character":58},"updated":"2018-03-21 18:45:52.000000000","message":"\"we are passed is\"","commit_id":"a7a5ec65a8e55e8284050269ccc54d5c7b3552f2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d86069097b7025a58a6e9d68a65333e5f6590230","unresolved":false,"context_lines":[{"line_number":7967,"context_line":"            # initialized with BDMs from the source host before they were"},{"line_number":7968,"context_line":"            # updated to point to the destination. We can safely use this to"},{"line_number":7969,"context_line":"            # disconnect the source without re-fetching."},{"line_number":7970,"context_line":"            self._disconnect_volume(context, vol[\u0027connection_info\u0027], instance)"},{"line_number":7971,"context_line":""},{"line_number":7972,"context_line":"    def post_live_migration_at_source(self, context, instance, network_info):"},{"line_number":7973,"context_line":"        \"\"\"Unplug VIFs from networks at source."}],"source_content_type":"text/x-python","patch_set":10,"id":"9fdfeff1_14ec688d","line":7970,"range":{"start_line":7970,"start_character":45,"end_line":7970,"end_character":67},"updated":"2019-02-12 16:09:28.000000000","message":"If vol is a BlockDeviceMapping object we should use dot notation for attribute access, but I see we used dict like syntax before as well.","commit_id":"57a869bfbb6a9ff6804ae4f24e086de2892d3c05"}]}