)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"0ef74ca480781ed7bb87859ac9f3e8cab46008c7","unresolved":false,"context_lines":[{"line_number":4659,"context_line":"        if CONF.host \u003d\u003d instance.host:"},{"line_number":4660,"context_line":"            connection_info \u003d self._driver_detach_volume(context, instance,"},{"line_number":4661,"context_line":"                                                         bdm)"},{"line_number":4662,"context_line":"        else:"},{"line_number":4663,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4664,"context_line":""},{"line_number":4665,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab6814e_2da618ae","line":4662,"updated":"2016-05-20 12:11:16.000000000","message":"This actually highlights that we\u0027ve not detached volumes on the source host when rebuilding remotely at all in the past. IMHO that\u0027s a separate bug that I\u0027ll address in a follow up patch using self.compute_rpcapi.remove_volume_connection as used by _rollback_live_migration.","commit_id":"12a968c89f77865c7f527861d4d12646118eb3c7"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1bb4ed66505b23555704c990582ea38229770ce9","unresolved":false,"context_lines":[{"line_number":4681,"context_line":"        else:"},{"line_number":4682,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4683,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4684,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4685,"context_line":""},{"line_number":4686,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4687,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3aaa91ec_35903943","line":4684,"updated":"2016-06-23 13:45:20.000000000","message":"What about this that happens in _driver_detach_volume?\n\nif connection_info and \u0027serial\u0027 not in connection_info:\n            connection_info[\u0027serial\u0027] \u003d volume_id","commit_id":"c6916224f2e88f1a602d6d95c833742d00bc2311"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"aacb735b1db90f4fc3e01f1eba2a3d094f456876","unresolved":false,"context_lines":[{"line_number":4681,"context_line":"        else:"},{"line_number":4682,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4683,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4684,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4685,"context_line":""},{"line_number":4686,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4687,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3aaa91ec_7cae2982","line":4684,"in_reply_to":"3aaa91ec_35903943","updated":"2016-06-23 14:37:01.000000000","message":"That isn\u0027t required as by this point we know that no call to driver.detach_volume is going to be made. Also I can\u0027t find any reference to the use or serial to detach in either Nova or os-brick so I\u0027m not sure that the above is still required.","commit_id":"c6916224f2e88f1a602d6d95c833742d00bc2311"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"11b3816edc91a06fa4582564a62454bcbf4818df","unresolved":false,"context_lines":[{"line_number":4718,"context_line":"            connection_info \u003d self._driver_detach_volume(context, instance,"},{"line_number":4719,"context_line":"                                                         bdm)"},{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_17322ea7","line":4721,"range":{"start_line":4721,"start_character":72,"end_line":4721,"end_character":78},"updated":"2016-07-12 13:50:11.000000000","message":"should be \"cinder\"","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fa6c6cc3590ca45fd9285a693cb0ad158853d5bb","unresolved":false,"context_lines":[{"line_number":4718,"context_line":"            connection_info \u003d self._driver_detach_volume(context, instance,"},{"line_number":4719,"context_line":"                                                         bdm)"},{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_25de1411","line":4721,"range":{"start_line":4721,"start_character":72,"end_line":4721,"end_character":78},"in_reply_to":"1aa78d24_17322ea7","updated":"2016-07-12 14:16:13.000000000","message":"I would log something in this case that we\u0027re not going to attempt to disconnect the volume on the host since it\u0027s changed. Debug log should be fine if destroy_bdm is False, since it\u0027s expected for rebuild/evacuate. If destroy_bdm is True and the host changed, then it\u0027s actually an error, so maybe we need to add that to the host comparison conditional?","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":4718,"context_line":"            connection_info \u003d self._driver_detach_volume(context, instance,"},{"line_number":4719,"context_line":"                                                         bdm)"},{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_1adece10","line":4721,"range":{"start_line":4721,"start_character":72,"end_line":4721,"end_character":78},"in_reply_to":"1aa78d24_25de1411","updated":"2016-07-12 15:36:37.000000000","message":"Done","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"aab9a8cb72f702c4e82c4aa2fabc8d8355aac306","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_daa72ba5","line":4723,"updated":"2016-07-12 14:02:34.000000000","message":"This is duplicated from inside _driver_detach_volume(). It confused me because I thought maybe this would be *different* connection info than what that method returned, but... it isn\u0027t.\n\nSo I think this would be clearer if you refactored just a tad here. Load the connection info first, pass it to _driver_detach_volume() if we\u0027re going to run it, and then carry on.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_ba1a1ac4","line":4723,"in_reply_to":"1aa78d24_0531585a","updated":"2016-07-12 15:36:37.000000000","message":"I\u0027m not sure that I follow, if we are not local to the instance then we can\u0027t do this cleanup via _driver_detach_volume right?","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_3ae90ab8","line":4723,"in_reply_to":"1aa78d24_2589f426","updated":"2016-07-12 15:36:37.000000000","message":"Done","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":7789,"name":"John Wood","email":"john.wood@rackspace.com","username":"woodster"},"change_message_id":"40e5b48541fe44056975a856021e7c680a979ff7","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_4e0ff838","line":4723,"in_reply_to":"1aa78d24_ba1a1ac4","updated":"2016-07-12 16:21:27.000000000","message":"That\u0027s correct, so I\u0027d agree with Matt that the else path here seems like a logic error condition of some sort. I also don\u0027t think we should destroy bdms in this case as this could complicate clean up efforts later.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_5ae4d6de","line":4723,"in_reply_to":"1aa78d24_daa72ba5","updated":"2016-07-12 15:36:37.000000000","message":"Done","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fa6c6cc3590ca45fd9285a693cb0ad158853d5bb","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_2589f426","line":4723,"in_reply_to":"1aa78d24_daa72ba5","updated":"2016-07-12 14:16:13.000000000","message":"That would work to cleanup the duplication. Note you\u0027d have to use the version from _driver_detach_volume which checks for the \u0027serial\u0027 key and adds it if not set, which I asked about in an earlier patchset. Lee said it wasn\u0027t used though, and even the note from Vish around that says it\u0027s not currently used (I don\u0027t know when that was current, probably ages ago), so maybe now would be a good time to just remove it if we split that part out.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":7789,"name":"John Wood","email":"john.wood@rackspace.com","username":"woodster"},"change_message_id":"d7b2063adb3ce0793eee0fbecfe4cd9817572de5","unresolved":false,"context_lines":[{"line_number":4720,"context_line":"        else:"},{"line_number":4721,"context_line":"            # Otherwise fetch the connection_info from the bdm to allow cidner"},{"line_number":4722,"context_line":"            # to terminate the connection to the volume from the previous host."},{"line_number":4723,"context_line":"            connection_info \u003d jsonutils.loads(bdm.connection_info)"},{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_0531585a","line":4723,"in_reply_to":"1aa78d24_daa72ba5","updated":"2016-07-12 14:16:22.000000000","message":"Well, the _driver_detach_volume call also cleans up VBDs and so forth on the hypervisor (necessary before the detach can happen, on Xen at least). If that clean up is a pre-condition to this else branch being called, then no worries though.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fa6c6cc3590ca45fd9285a693cb0ad158853d5bb","unresolved":false,"context_lines":[{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""},{"line_number":4727,"context_line":"        if connection_info and not destroy_bdm and ("},{"line_number":4728,"context_line":"           connector.get(\u0027host\u0027) !\u003d instance.host):"},{"line_number":4729,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":4730,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":4731,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":4732,"context_line":"            # is for the expected host)."},{"line_number":4733,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":4734,"context_line":"            if not stashed_connector:"},{"line_number":4735,"context_line":"                # Volume was attached before we began stashing connectors"},{"line_number":4736,"context_line":"                LOG.warning(_LW(\"Host mismatch detected, but stashed \""},{"line_number":4737,"context_line":"                                \"volume connector not found. Instance host is \""},{"line_number":4738,"context_line":"                                \"%(ihost)s, but volume connector host is \""},{"line_number":4739,"context_line":"                                \"%(chost)s.\"),"},{"line_number":4740,"context_line":"                            {\u0027ihost\u0027: instance.host,"},{"line_number":4741,"context_line":"                             \u0027chost\u0027: connector.get(\u0027host\u0027)})"},{"line_number":4742,"context_line":"            elif stashed_connector.get(\u0027host\u0027) !\u003d instance.host:"},{"line_number":4743,"context_line":"                # Unexpected error. The stashed connector is also not matching"},{"line_number":4744,"context_line":"                # the needed instance host."},{"line_number":4745,"context_line":"                LOG.error(_LE(\"Host mismatch detected in stashed volume \""},{"line_number":4746,"context_line":"                              \"connector. Will use local volume connector. \""},{"line_number":4747,"context_line":"                              \"Instance host is %(ihost)s. Local volume \""},{"line_number":4748,"context_line":"                              \"connector host is %(chost)s. Stashed volume \""},{"line_number":4749,"context_line":"                              \"connector host is %(schost)s.\"),"},{"line_number":4750,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4751,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4752,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4753,"context_line":"            else:"},{"line_number":4754,"context_line":"                # Fix found. Use stashed connector."},{"line_number":4755,"context_line":"                LOG.debug(\"Host mismatch detected. Found usable stashed \""},{"line_number":4756,"context_line":"                          \"volume connector. Instance host is %(ihost)s. \""},{"line_number":4757,"context_line":"                          \"Local volume connector host was %(chost)s. \""},{"line_number":4758,"context_line":"                          \"Stashed volume connector host is %(schost)s.\","},{"line_number":4759,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4760,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4761,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4762,"context_line":"                connector \u003d stashed_connector"},{"line_number":4763,"context_line":""},{"line_number":4764,"context_line":"        self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4765,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_252c5401","line":4762,"range":{"start_line":4727,"start_character":8,"end_line":4762,"end_character":45},"updated":"2016-07-12 14:16:13.000000000","message":"This was all added by Mark Sturdevant in https://review.openstack.org/#/c/266095/ so I\u0027ve added him to this review to make sure that the change being proposed here doesn\u0027t break the volume drivers he was using, which I think was at least HP 3PAR, but there were some others I think.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""},{"line_number":4727,"context_line":"        if connection_info and not destroy_bdm and ("},{"line_number":4728,"context_line":"           connector.get(\u0027host\u0027) !\u003d instance.host):"},{"line_number":4729,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":4730,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":4731,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":4732,"context_line":"            # is for the expected host)."},{"line_number":4733,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":4734,"context_line":"            if not stashed_connector:"},{"line_number":4735,"context_line":"                # Volume was attached before we began stashing connectors"},{"line_number":4736,"context_line":"                LOG.warning(_LW(\"Host mismatch detected, but stashed \""},{"line_number":4737,"context_line":"                                \"volume connector not found. Instance host is \""},{"line_number":4738,"context_line":"                                \"%(ihost)s, but volume connector host is \""},{"line_number":4739,"context_line":"                                \"%(chost)s.\"),"},{"line_number":4740,"context_line":"                            {\u0027ihost\u0027: instance.host,"},{"line_number":4741,"context_line":"                             \u0027chost\u0027: connector.get(\u0027host\u0027)})"},{"line_number":4742,"context_line":"            elif stashed_connector.get(\u0027host\u0027) !\u003d instance.host:"},{"line_number":4743,"context_line":"                # Unexpected error. The stashed connector is also not matching"},{"line_number":4744,"context_line":"                # the needed instance host."},{"line_number":4745,"context_line":"                LOG.error(_LE(\"Host mismatch detected in stashed volume \""},{"line_number":4746,"context_line":"                              \"connector. Will use local volume connector. \""},{"line_number":4747,"context_line":"                              \"Instance host is %(ihost)s. Local volume \""},{"line_number":4748,"context_line":"                              \"connector host is %(chost)s. Stashed volume \""},{"line_number":4749,"context_line":"                              \"connector host is %(schost)s.\"),"},{"line_number":4750,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4751,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4752,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4753,"context_line":"            else:"},{"line_number":4754,"context_line":"                # Fix found. Use stashed connector."},{"line_number":4755,"context_line":"                LOG.debug(\"Host mismatch detected. Found usable stashed \""},{"line_number":4756,"context_line":"                          \"volume connector. Instance host is %(ihost)s. \""},{"line_number":4757,"context_line":"                          \"Local volume connector host was %(chost)s. \""},{"line_number":4758,"context_line":"                          \"Stashed volume connector host is %(schost)s.\","},{"line_number":4759,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4760,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4761,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4762,"context_line":"                connector \u003d stashed_connector"},{"line_number":4763,"context_line":""},{"line_number":4764,"context_line":"        self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4765,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_9df91883","line":4762,"range":{"start_line":4727,"start_character":8,"end_line":4762,"end_character":45},"in_reply_to":"1aa78d24_252c5401","updated":"2016-07-12 15:36:37.000000000","message":"Thanks Matt, I can\u0027t think it will but I\u0027m happy to be corrected by Mark if I\u0027ve missed something.","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":11047,"name":"Mark Sturdevant","email":"mark.sturdevant@ibm.com","username":"markstur"},"change_message_id":"0efe942e4fc7e8551f1c31d8e3d20b6aa0d51cb0","unresolved":false,"context_lines":[{"line_number":4724,"context_line":""},{"line_number":4725,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4726,"context_line":""},{"line_number":4727,"context_line":"        if connection_info and not destroy_bdm and ("},{"line_number":4728,"context_line":"           connector.get(\u0027host\u0027) !\u003d instance.host):"},{"line_number":4729,"context_line":"            # If the volume is attached to another host (evacuate) then"},{"line_number":4730,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":4731,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":4732,"context_line":"            # is for the expected host)."},{"line_number":4733,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":4734,"context_line":"            if not stashed_connector:"},{"line_number":4735,"context_line":"                # Volume was attached before we began stashing connectors"},{"line_number":4736,"context_line":"                LOG.warning(_LW(\"Host mismatch detected, but stashed \""},{"line_number":4737,"context_line":"                                \"volume connector not found. Instance host is \""},{"line_number":4738,"context_line":"                                \"%(ihost)s, but volume connector host is \""},{"line_number":4739,"context_line":"                                \"%(chost)s.\"),"},{"line_number":4740,"context_line":"                            {\u0027ihost\u0027: instance.host,"},{"line_number":4741,"context_line":"                             \u0027chost\u0027: connector.get(\u0027host\u0027)})"},{"line_number":4742,"context_line":"            elif stashed_connector.get(\u0027host\u0027) !\u003d instance.host:"},{"line_number":4743,"context_line":"                # Unexpected error. The stashed connector is also not matching"},{"line_number":4744,"context_line":"                # the needed instance host."},{"line_number":4745,"context_line":"                LOG.error(_LE(\"Host mismatch detected in stashed volume \""},{"line_number":4746,"context_line":"                              \"connector. Will use local volume connector. \""},{"line_number":4747,"context_line":"                              \"Instance host is %(ihost)s. Local volume \""},{"line_number":4748,"context_line":"                              \"connector host is %(chost)s. Stashed volume \""},{"line_number":4749,"context_line":"                              \"connector host is %(schost)s.\"),"},{"line_number":4750,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4751,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4752,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4753,"context_line":"            else:"},{"line_number":4754,"context_line":"                # Fix found. Use stashed connector."},{"line_number":4755,"context_line":"                LOG.debug(\"Host mismatch detected. Found usable stashed \""},{"line_number":4756,"context_line":"                          \"volume connector. Instance host is %(ihost)s. \""},{"line_number":4757,"context_line":"                          \"Local volume connector host was %(chost)s. \""},{"line_number":4758,"context_line":"                          \"Stashed volume connector host is %(schost)s.\","},{"line_number":4759,"context_line":"                          {\u0027ihost\u0027: instance.host,"},{"line_number":4760,"context_line":"                           \u0027chost\u0027: connector.get(\u0027host\u0027),"},{"line_number":4761,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":4762,"context_line":"                connector \u003d stashed_connector"},{"line_number":4763,"context_line":""},{"line_number":4764,"context_line":"        self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4765,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"dada55a8_a7d1fdfd","line":4762,"range":{"start_line":4727,"start_character":8,"end_line":4762,"end_character":45},"in_reply_to":"1aa78d24_9df91883","updated":"2016-07-18 16:39:13.000000000","message":"Looks good. My part still gets connection_info, connector and destroy_bdm as before. It just got better by not relying on _driver_detach_volume to load and return. By the way this can affect all drivers. It\u0027ll send them the original stashed connector instead of building a new one on the wrong host (after evacuate).","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"aab9a8cb72f702c4e82c4aa2fabc8d8355aac306","unresolved":false,"context_lines":[{"line_number":4770,"context_line":"        self._notify_about_instance_usage("},{"line_number":4771,"context_line":"            context, instance, \"volume.detach\", extra_usage_info\u003dinfo)"},{"line_number":4772,"context_line":"        self.volume_api.detach(context.elevated(), volume_id, instance.uuid,"},{"line_number":4773,"context_line":"                               attachment_id)"},{"line_number":4774,"context_line":""},{"line_number":4775,"context_line":"    @wrap_exception()"},{"line_number":4776,"context_line":"    @wrap_instance_fault"}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_7a32775f","line":4773,"updated":"2016-07-12 14:02:34.000000000","message":"IMHO, all this code shouldn\u0027t be in compute manager at all. Having a code path where one compute terminates the volume connection for another seems like a recipe for disaster. Not that we can correct that for this bug fix, but just for the record...","commit_id":"6f69d12110cc04532b104a36100f5a50443a5a15"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"af6928a5c2ca7b6ccc2341e0d36255dc43d9271a","unresolved":false,"context_lines":[{"line_number":4746,"context_line":"            self._driver_detach_volume(context, instance, bdm, connection_info)"},{"line_number":4747,"context_line":"        elif not destroy_bdm:"},{"line_number":4748,"context_line":"            LOG.debug(\"Skipping _driver_detach_volume during remote rebuild.\")"},{"line_number":4749,"context_line":"        elif destroy_bdm:"},{"line_number":4750,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""},{"line_number":4751,"context_line":"                          \"%(vol_id)s due to the instance being registered to \""},{"line_number":4752,"context_line":"                          \"the remote host %(inst_host)s.\"),"}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_55c5d88b","line":4749,"range":{"start_line":4749,"start_character":8,"end_line":4749,"end_character":24},"updated":"2016-07-26 14:00:56.000000000","message":"This is just \"else\" right?","commit_id":"e62ba533db212699cbb815aafa559424e6fe7f58"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9d09fac1771fea8c078b00c396960525ab25f1e6","unresolved":false,"context_lines":[{"line_number":4745,"context_line":"            # instance is currently associated with the local compute host."},{"line_number":4746,"context_line":"            self._driver_detach_volume(context, instance, bdm, connection_info)"},{"line_number":4747,"context_line":"        elif not destroy_bdm:"},{"line_number":4748,"context_line":"            LOG.debug(\"Skipping _driver_detach_volume during remote rebuild.\")"},{"line_number":4749,"context_line":"        elif destroy_bdm:"},{"line_number":4750,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""},{"line_number":4751,"context_line":"                          \"%(vol_id)s due to the instance being registered to \""}],"source_content_type":"text/x-python","patch_set":18,"id":"bacf61ea_5a091331","line":4748,"range":{"start_line":4748,"start_character":12,"end_line":4748,"end_character":21},"updated":"2016-07-27 19:10:56.000000000","message":"Pass instance\u003dinstance here for context.","commit_id":"8b8348e43aa27a3a022ac8861d0661029f8ac423"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9d09fac1771fea8c078b00c396960525ab25f1e6","unresolved":false,"context_lines":[{"line_number":4747,"context_line":"        elif not destroy_bdm:"},{"line_number":4748,"context_line":"            LOG.debug(\"Skipping _driver_detach_volume during remote rebuild.\")"},{"line_number":4749,"context_line":"        elif destroy_bdm:"},{"line_number":4750,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""},{"line_number":4751,"context_line":"                          \"%(vol_id)s due to the instance being registered to \""},{"line_number":4752,"context_line":"                          \"the remote host %(inst_host)s.\"),"},{"line_number":4753,"context_line":"                          {\u0027vol_id\u0027: volume_id, \u0027inst_host\u0027: instance.host})"}],"source_content_type":"text/x-python","patch_set":18,"id":"bacf61ea_3a125f63","line":4750,"range":{"start_line":4750,"start_character":12,"end_line":4750,"end_character":21},"updated":"2016-07-27 19:10:56.000000000","message":"There isn\u0027t a context for the instance in this error message, so pass the instance\u003dinstance kwarg to LOG.error to get context.","commit_id":"8b8348e43aa27a3a022ac8861d0661029f8ac423"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1d72bc88c7785a4dc8d138c485ea1861987aaaf1","unresolved":false,"context_lines":[{"line_number":4751,"context_line":"            LOG.error(_LE(\"Unable to call for a driver detach of volume \""},{"line_number":4752,"context_line":"                          \"%(vol_id)s due to the instance being registered to \""},{"line_number":4753,"context_line":"                          \"the remote host %(inst_host)s.\"),"},{"line_number":4754,"context_line":"                          {\u0027vol_id\u0027: volume_id, \u0027inst_host\u0027: instance.host},"},{"line_number":4755,"context_line":"                      instance\u003dinstance)"},{"line_number":4756,"context_line":""},{"line_number":4757,"context_line":"        if connection_info and not destroy_bdm and ("}],"source_content_type":"text/x-python","patch_set":19,"id":"bacf61ea_e08c9837","line":4754,"updated":"2016-07-27 19:34:56.000000000","message":"nit: alignment, but meh","commit_id":"917d18672134906015d1ffa6a4340ab46b5b083d"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"aab9a8cb72f702c4e82c4aa2fabc8d8355aac306","unresolved":false,"context_lines":[{"line_number":11377,"context_line":"            result[\"detached\"] \u003d volume[\"id\"] \u003d\u003d \u0027fake_volume_id\u0027"},{"line_number":11378,"context_line":"        self.stubs.Set(cinder.API, \"detach\", fake_detach)"},{"line_number":11379,"context_line":""},{"line_number":11380,"context_line":"        self.mox.StubOutWithMock(self.compute, \u0027_driver_detach_volume\u0027)"},{"line_number":11381,"context_line":"        self.compute._driver_detach_volume(mox.IsA(self.context),"},{"line_number":11382,"context_line":"                                           mox.IsA(instance_obj.Instance),"},{"line_number":11383,"context_line":"                                           mox.IsA(objects.BlockDeviceMapping))"}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_da71eb0d","side":"PARENT","line":11380,"updated":"2016-07-12 14:02:34.000000000","message":"Actually you don\u0027t want to remove this. You want to leave it mocked but not mock-call it. That confirms that the code you\u0027re running doesn\u0027t call it, which is what you want to test.","commit_id":"2d551ce83d186ad70ca69b1a6f9454906b4b3448"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"4a6a3bd03fbbcb5c35364e67b891b01e67a46828","unresolved":false,"context_lines":[{"line_number":11377,"context_line":"            result[\"detached\"] \u003d volume[\"id\"] \u003d\u003d \u0027fake_volume_id\u0027"},{"line_number":11378,"context_line":"        self.stubs.Set(cinder.API, \"detach\", fake_detach)"},{"line_number":11379,"context_line":""},{"line_number":11380,"context_line":"        self.mox.StubOutWithMock(self.compute, \u0027_driver_detach_volume\u0027)"},{"line_number":11381,"context_line":"        self.compute._driver_detach_volume(mox.IsA(self.context),"},{"line_number":11382,"context_line":"                                           mox.IsA(instance_obj.Instance),"},{"line_number":11383,"context_line":"                                           mox.IsA(objects.BlockDeviceMapping))"}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_da55661e","side":"PARENT","line":11380,"in_reply_to":"1aa78d24_da71eb0d","updated":"2016-07-12 15:36:37.000000000","message":"Done","commit_id":"2d551ce83d186ad70ca69b1a6f9454906b4b3448"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"bbd937d1aaa6f40dbfd2a25e3055545f2cc61a01","unresolved":false,"context_lines":[{"line_number":11215,"context_line":"            self.assertEqual(instance[\u0027host\u0027], \u0027fake_host_2\u0027)"},{"line_number":11216,"context_line":"            self.assertTrue(mock_inst.called)"},{"line_number":11217,"context_line":""},{"line_number":11218,"context_line":""},{"line_number":11219,"context_line":"    @mock.patch.object(cinder.API, \u0027detach\u0027)"},{"line_number":11220,"context_line":"    @mock.patch.object(compute_manager.ComputeManager, \u0027_prep_block_device\u0027)"},{"line_number":11221,"context_line":"    @mock.patch.object(compute_manager.ComputeManager, \u0027_driver_detach_volume\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"1aa78d24_dd55601e","line":11218,"range":{"start_line":11218,"start_character":0,"end_line":11218,"end_character":0},"updated":"2016-07-12 15:40:03.000000000","message":"urgh","commit_id":"65198f5515501894f27bceba7c1fb3db073a34b4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"af6928a5c2ca7b6ccc2341e0d36255dc43d9271a","unresolved":false,"context_lines":[{"line_number":11262,"context_line":"        mock_drv_detach.assert_called_once_with("},{"line_number":11263,"context_line":"            test.MatchType(context.RequestContext),"},{"line_number":11264,"context_line":"            test.MatchType(objects.Instance),"},{"line_number":11265,"context_line":"            test.MatchType(objects.BlockDeviceMapping))"},{"line_number":11266,"context_line":"        # make sure volumes attach, detach are called"},{"line_number":11267,"context_line":"        mock_detach.assert_called_once_with("},{"line_number":11268,"context_line":"            test.MatchType(context.RequestContext),"}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_15e520a3","side":"PARENT","line":11265,"updated":"2016-07-26 14:00:56.000000000","message":"Well, now that this is mock instead of mox, this just leaves an orphaned mock with no validation. Before, mox would complain if it was called if you didn\u0027t mock-call it. However, mock will not unless you explicitly do it. So, either remove the mock decorator, or assert it was not called to make sure your change is working:\n\n self.assertFalse(mock_drv_attach.called)","commit_id":"a147215196219680360ee4bd2af165d3e3507dd4"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbe9bee5aee64195d18b806dae3a81400d4a7fb9","unresolved":false,"context_lines":[{"line_number":1285,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1286,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"},{"line_number":1287,"context_line":"            #             migration, so we should still disconnect even if"},{"line_number":1288,"context_line":"            #             the instance doesn\u0027t exist here anymore."},{"line_number":1289,"context_line":"            error_code \u003d ex.get_error_code()"},{"line_number":1290,"context_line":"            if error_code \u003d\u003d libvirt.VIR_ERR_NO_DOMAIN:"},{"line_number":1291,"context_line":"                # NOTE(vish):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bab6814e_4ee11d9a","line":1288,"updated":"2016-05-19 10:21:51.000000000","message":"If this comment were correct (that this method is called to cleanup post live migration) then the code below would be wrong, because we would want to detach the volume despite the compute host not matching.\n\nHOWEVER, I\u0027ve looked through the post live migration cleanup, and I don\u0027t think it does actually call this method to remove volumes. It calls terminate_connection from compute.manager, then driver.cleanup, which calls _disconnect_volume directly.\n\nIOW, I think this comment is wrong, and your patch is good. It\u0027s bloody confusing, though.","commit_id":"fef8044f61acc3cf235bc14c95e03bc23d49e50a"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"55ad3c4ac64141245ef2cdf7b34f349daf0508a7","unresolved":false,"context_lines":[{"line_number":1285,"context_line":"        except libvirt.libvirtError as ex:"},{"line_number":1286,"context_line":"            # NOTE(vish): This is called to cleanup volumes after live"},{"line_number":1287,"context_line":"            #             migration, so we should still disconnect even if"},{"line_number":1288,"context_line":"            #             the instance doesn\u0027t exist here anymore."},{"line_number":1289,"context_line":"            error_code \u003d ex.get_error_code()"},{"line_number":1290,"context_line":"            if error_code \u003d\u003d libvirt.VIR_ERR_NO_DOMAIN:"},{"line_number":1291,"context_line":"                # NOTE(vish):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bab6814e_044d2a3e","line":1288,"in_reply_to":"bab6814e_4ee11d9a","updated":"2016-05-19 13:07:24.000000000","message":"Yeah the comment is wrong, I\u0027ve just tested with a devstack env and disconnect_volume is called correctly on the src host once the migration completes even with the latest patch to the compute layer. I\u0027ll clean that comment up in another commit.","commit_id":"fef8044f61acc3cf235bc14c95e03bc23d49e50a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f497f7ca46386b4b7109b8cfed99e7a4af501b31","unresolved":false,"context_lines":[{"line_number":1295,"context_line":"                raise"},{"line_number":1296,"context_line":""},{"line_number":1297,"context_line":"        connector \u003d self.get_volume_connector(instance)"},{"line_number":1298,"context_line":"        if connector.get(\u0027host\u0027) \u003d\u003d instance.host:"},{"line_number":1299,"context_line":"            self._disconnect_volume(connection_info, disk_dev)"},{"line_number":1300,"context_line":""},{"line_number":1301,"context_line":"    def attach_interface(self, instance, image_meta, vif):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bab6814e_e9b44bf3","line":1298,"updated":"2016-05-19 10:33:07.000000000","message":"AFAICT the dance self.get_volume_connector(instance).get(\u0027host\u0027) is equivalent to CONF.host, but a lot more convoluted and expensive! Can you simplify that?","commit_id":"fef8044f61acc3cf235bc14c95e03bc23d49e50a"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"55ad3c4ac64141245ef2cdf7b34f349daf0508a7","unresolved":false,"context_lines":[{"line_number":1295,"context_line":"                raise"},{"line_number":1296,"context_line":""},{"line_number":1297,"context_line":"        connector \u003d self.get_volume_connector(instance)"},{"line_number":1298,"context_line":"        if connector.get(\u0027host\u0027) \u003d\u003d instance.host:"},{"line_number":1299,"context_line":"            self._disconnect_volume(connection_info, disk_dev)"},{"line_number":1300,"context_line":""},{"line_number":1301,"context_line":"    def attach_interface(self, instance, image_meta, vif):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bab6814e_a4809e47","line":1298,"in_reply_to":"bab6814e_e9b44bf3","updated":"2016-05-19 13:07:24.000000000","message":"Done","commit_id":"fef8044f61acc3cf235bc14c95e03bc23d49e50a"}]}