)]}'
{"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0d708ec9363be835feffe985c1d837395ec3c9b0","unresolved":false,"context_lines":[{"line_number":16570,"context_line":"            mox.IsA(objects.Flavor),"},{"line_number":16571,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":16572,"context_line":"            self.drvr._host).AndReturn(expected)"},{"line_number":16573,"context_line":"        self.mox.StubOutWithMock(self.drvr, \u0027_build_device_metadata\u0027)"},{"line_number":16574,"context_line":"        self.drvr._build_device_metadata(self.context, instance).AndReturn("},{"line_number":16575,"context_line":"            objects.InstanceDeviceMetadata())"},{"line_number":16576,"context_line":"        self.mox.StubOutWithMock(objects.Instance, \u0027save\u0027)"},{"line_number":16577,"context_line":"        objects.Instance.save()"},{"line_number":16578,"context_line":"        domain.attachDeviceFlags(expected.to_xml(), flags\u003dexpected_flags)"},{"line_number":16579,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_22f1c8cd","line":16576,"range":{"start_line":16573,"start_character":0,"end_line":16576,"end_character":58},"updated":"2017-03-10 16:33:42.000000000","message":"This might not be possible, but the general rule of thumb is not to add anymore mox code, instead preferring to use mock. I\u0027m not going to block on this for now, but it might be worth rewording if possible","commit_id":"8de76cb907a72f0d6e27383d40fb84aa982c83d8"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"164755f5029360767bd34f3eafee779c37a38ced","unresolved":false,"context_lines":[{"line_number":16570,"context_line":"            mox.IsA(objects.Flavor),"},{"line_number":16571,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":16572,"context_line":"            self.drvr._host).AndReturn(expected)"},{"line_number":16573,"context_line":"        self.mox.StubOutWithMock(self.drvr, \u0027_build_device_metadata\u0027)"},{"line_number":16574,"context_line":"        self.drvr._build_device_metadata(self.context, instance).AndReturn("},{"line_number":16575,"context_line":"            objects.InstanceDeviceMetadata())"},{"line_number":16576,"context_line":"        self.mox.StubOutWithMock(objects.Instance, \u0027save\u0027)"},{"line_number":16577,"context_line":"        objects.Instance.save()"},{"line_number":16578,"context_line":"        domain.attachDeviceFlags(expected.to_xml(), flags\u003dexpected_flags)"},{"line_number":16579,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_5649e72f","line":16576,"range":{"start_line":16573,"start_character":0,"end_line":16576,"end_character":58},"in_reply_to":"9a30ddce_22f1c8cd","updated":"2017-03-13 15:48:26.000000000","message":"My understanding was that it\u0027s OK to add mox code as long as we\u0027re only modifying an existing test method. I have precedent of this sort of thing somewhere, if only I could remember the actual review it was in ;)","commit_id":"8de76cb907a72f0d6e27383d40fb84aa982c83d8"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"efb8612681ec23978b06c729a7b365422b4251b3","unresolved":false,"context_lines":[{"line_number":16570,"context_line":"            mox.IsA(objects.Flavor),"},{"line_number":16571,"context_line":"            CONF.libvirt.virt_type,"},{"line_number":16572,"context_line":"            self.drvr._host).AndReturn(expected)"},{"line_number":16573,"context_line":"        self.mox.StubOutWithMock(self.drvr, \u0027_build_device_metadata\u0027)"},{"line_number":16574,"context_line":"        self.drvr._build_device_metadata(self.context, instance).AndReturn("},{"line_number":16575,"context_line":"            objects.InstanceDeviceMetadata())"},{"line_number":16576,"context_line":"        self.mox.StubOutWithMock(objects.Instance, \u0027save\u0027)"},{"line_number":16577,"context_line":"        objects.Instance.save()"},{"line_number":16578,"context_line":"        domain.attachDeviceFlags(expected.to_xml(), flags\u003dexpected_flags)"},{"line_number":16579,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_ec1e8c5d","line":16576,"range":{"start_line":16573,"start_character":0,"end_line":16576,"end_character":58},"in_reply_to":"9a30ddce_5649e72f","updated":"2017-03-13 16:04:28.000000000","message":"I still do this for tests that are overwhelmingly mox-based where using mock would be onerous. I\u0027m not sure this meets that test, tbh, but I\u0027m not overly concerned with such things, FWIW.","commit_id":"8de76cb907a72f0d6e27383d40fb84aa982c83d8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"22fd04cd8284e68eb14897df06f6f5d4f4d25d0c","unresolved":false,"context_lines":[{"line_number":16742,"context_line":"                        \u003c/interface\u003e"},{"line_number":16743,"context_line":"                    \u003c/devices\u003e"},{"line_number":16744,"context_line":"                \u003c/domain\u003e\"\"\")"},{"line_number":16745,"context_line":"        self.mox.StubOutWithMock(host.Host, \u0027get_domain\u0027)"},{"line_number":16746,"context_line":"        self.mox.StubOutWithMock(self.drvr.firewall_driver,"},{"line_number":16747,"context_line":"                                 \u0027setup_basic_filtering\u0027)"},{"line_number":16748,"context_line":"        self.mox.StubOutWithMock(domain, \u0027attachDeviceFlags\u0027)"},{"line_number":16749,"context_line":"        self.mox.StubOutWithMock(domain, \u0027info\u0027)"},{"line_number":16750,"context_line":""},{"line_number":16751,"context_line":"        host.Host.get_domain(instance).AndReturn(domain)"},{"line_number":16752,"context_line":"        domain.info().AndReturn([power_state.RUNNING, 1, 2, 3, 4])"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_830424c8","line":16749,"range":{"start_line":16745,"start_character":8,"end_line":16749,"end_character":48},"updated":"2017-05-17 12:38:12.000000000","message":"Please use mock with new tests, we\u0027re trying to get away from mox.","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"453aec3aab100fec772ad1e864e6aaf4a7d338b7","unresolved":false,"context_lines":[{"line_number":16742,"context_line":"                        \u003c/interface\u003e"},{"line_number":16743,"context_line":"                    \u003c/devices\u003e"},{"line_number":16744,"context_line":"                \u003c/domain\u003e\"\"\")"},{"line_number":16745,"context_line":"        self.mox.StubOutWithMock(host.Host, \u0027get_domain\u0027)"},{"line_number":16746,"context_line":"        self.mox.StubOutWithMock(self.drvr.firewall_driver,"},{"line_number":16747,"context_line":"                                 \u0027setup_basic_filtering\u0027)"},{"line_number":16748,"context_line":"        self.mox.StubOutWithMock(domain, \u0027attachDeviceFlags\u0027)"},{"line_number":16749,"context_line":"        self.mox.StubOutWithMock(domain, \u0027info\u0027)"},{"line_number":16750,"context_line":""},{"line_number":16751,"context_line":"        host.Host.get_domain(instance).AndReturn(domain)"},{"line_number":16752,"context_line":"        domain.info().AndReturn([power_state.RUNNING, 1, 2, 3, 4])"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_50a929dd","line":16749,"range":{"start_line":16745,"start_character":8,"end_line":16749,"end_character":48},"in_reply_to":"ff0f0b1f_830424c8","updated":"2017-05-17 16:44:16.000000000","message":"Done","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"}],"nova/virt/driver.py":[{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3681d8d9f3be3354d2bf11bc045c54d3cd0caefa","unresolved":false,"context_lines":[{"line_number":491,"context_line":"        :param nova.network.model.NetworkInfo vif:"},{"line_number":492,"context_line":"            The object which has the information about the interface to attach."},{"line_number":493,"context_line":"        :param tag:"},{"line_number":494,"context_line":"            Optional. Indicates that the interface to be attached was tagged by"},{"line_number":495,"context_line":"            the user with a device tag."},{"line_number":496,"context_line":""},{"line_number":497,"context_line":"        :raise nova.exception.NovaException: If the attach fails."},{"line_number":498,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_f1bfc9a7","line":495,"range":{"start_line":494,"start_character":22,"end_line":495,"end_character":39},"updated":"2017-01-11 13:28:53.000000000","message":"if I read this comment, I wonder what type is tag. By looking at the libvirt impl, I first thought it was a boolean, but when looking at the test, it\u0027s the tag value itself.\n\nGiven it\u0027s an important driver interface, could we have a better docstring here ?","commit_id":"4203b1b726ec8db3900ff87c26750fb3e27d8914"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0fc8ea07b7794e927a0cc559c74b1d3708de084d","unresolved":false,"context_lines":[{"line_number":491,"context_line":"        :param nova.network.model.NetworkInfo vif:"},{"line_number":492,"context_line":"            The object which has the information about the interface to attach."},{"line_number":493,"context_line":"        :param tag:"},{"line_number":494,"context_line":"            Optional. Indicates that the interface to be attached was tagged by"},{"line_number":495,"context_line":"            the user with a device tag."},{"line_number":496,"context_line":""},{"line_number":497,"context_line":"        :raise nova.exception.NovaException: If the attach fails."},{"line_number":498,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_b4d288fe","line":495,"range":{"start_line":494,"start_character":22,"end_line":495,"end_character":39},"in_reply_to":"ba5201f7_f1bfc9a7","updated":"2017-01-11 15:35:39.000000000","message":"Done","commit_id":"4203b1b726ec8db3900ff87c26750fb3e27d8914"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0d708ec9363be835feffe985c1d837395ec3c9b0","unresolved":false,"context_lines":[{"line_number":493,"context_line":"            The object which has the information about the interface to attach."},{"line_number":494,"context_line":"        :param tag:"},{"line_number":495,"context_line":"            Optional string. The device tag, if any, set by the user on the"},{"line_number":496,"context_line":"            interface to be attached."},{"line_number":497,"context_line":""},{"line_number":498,"context_line":"        :raise nova.exception.NovaException: If the attach fails."},{"line_number":499,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_c55ab6f1","line":496,"updated":"2017-03-10 16:33:42.000000000","message":"If you respin this, could you add a note about why you pass this in per your comments on PS10. Either here or in the libvirt subclassed function","commit_id":"8de76cb907a72f0d6e27383d40fb84aa982c83d8"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"164755f5029360767bd34f3eafee779c37a38ced","unresolved":false,"context_lines":[{"line_number":493,"context_line":"            The object which has the information about the interface to attach."},{"line_number":494,"context_line":"        :param tag:"},{"line_number":495,"context_line":"            Optional string. The device tag, if any, set by the user on the"},{"line_number":496,"context_line":"            interface to be attached."},{"line_number":497,"context_line":""},{"line_number":498,"context_line":"        :raise nova.exception.NovaException: If the attach fails."},{"line_number":499,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_428cc6d9","line":496,"in_reply_to":"9a30ddce_c55ab6f1","updated":"2017-03-13 15:48:26.000000000","message":"Removed the tag param entirely, since (as discussed on IRC), instance.save() is smart enough not to call the DB unless device_metadata has actually changed. Needlessly building device_metadata we can live with if it reduces code complexity, needlessly calling the database less so.","commit_id":"8de76cb907a72f0d6e27383d40fb84aa982c83d8"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7accbbd93657398adfb6af228704e451a27fb1be","unresolved":false,"context_lines":[{"line_number":1214,"context_line":"            # the upper layers having to call the driver through RPC,"},{"line_number":1215,"context_line":"            # especially since the code path from attach_volume in the compute"},{"line_number":1216,"context_line":"            # api is pretty convoluted"},{"line_number":1217,"context_line":"            instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1218,"context_line":"                context, instance)"},{"line_number":1219,"context_line":"            instance.save()"},{"line_number":1220,"context_line":"        except Exception as ex:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba5201f7_d48a451a","line":1217,"updated":"2017-01-07 18:12:15.000000000","message":"Is it not possible to pass the tag down from the API side like in the attach_interface case so that we don\u0027t have to rebuild this if the volume being attached wasn\u0027t actually tagged?","commit_id":"56515028cc0d4bc7c9d52fdc7fd6aecba4afb751"},{"author":{"_account_id":8802,"name":"Vladik Romanovsky","email":"vromanso@redhat.com","username":"vladikr"},"change_message_id":"b2160f9f4feeb4c98a1023d8f1649732f53aaf75","unresolved":false,"context_lines":[{"line_number":7676,"context_line":"        # methods that call this function."},{"line_number":7677,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":7678,"context_line":"            context, instance.uuid)"},{"line_number":7679,"context_line":"        tagged_bdms \u003d {bdm.volume_id: bdm for bdm in bdms if bdm.tag}"},{"line_number":7680,"context_line":""},{"line_number":7681,"context_line":"        devices \u003d []"},{"line_number":7682,"context_line":"        guest \u003d self._host.get_guest(instance)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba5201f7_51ca8e50","line":7679,"range":{"start_line":7679,"start_character":27,"end_line":7679,"end_character":36},"updated":"2017-01-10 13:49:04.000000000","message":"I\u0027m puzzled about this change. Config classes will show a device name (target_dev) that the guest will see for a specific  device - why are you changing it here to a volume_id? Also, on line 7705, how do you intend to get a serial from a dict of volume_ids? \nWhat am I missing here?","commit_id":"48ea1d7b09acebf67f1697b4e2e511bec139c869"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3681d8d9f3be3354d2bf11bc045c54d3cd0caefa","unresolved":false,"context_lines":[{"line_number":1366,"context_line":"            state \u003d guest.get_power_state(self._host)"},{"line_number":1367,"context_line":"            live \u003d state in (power_state.RUNNING, power_state.PAUSED)"},{"line_number":1368,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1369,"context_line":"            if tag:"},{"line_number":1370,"context_line":"                instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1371,"context_line":"                    context, instance)"},{"line_number":1372,"context_line":"                instance.save()"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_f1ed09b8","line":1369,"updated":"2017-01-11 13:28:53.000000000","message":"why do you need to get the tag value if you just want to signal to libvirt that you need to update the device metadata in case the interface was tagged ?\n\nCould we instead not modify the signature and try to get the tag by the vif config?","commit_id":"4203b1b726ec8db3900ff87c26750fb3e27d8914"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0fc8ea07b7794e927a0cc559c74b1d3708de084d","unresolved":false,"context_lines":[{"line_number":1366,"context_line":"            state \u003d guest.get_power_state(self._host)"},{"line_number":1367,"context_line":"            live \u003d state in (power_state.RUNNING, power_state.PAUSED)"},{"line_number":1368,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1369,"context_line":"            if tag:"},{"line_number":1370,"context_line":"                instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1371,"context_line":"                    context, instance)"},{"line_number":1372,"context_line":"                instance.save()"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_14bfd4aa","line":1369,"in_reply_to":"ba5201f7_f1ed09b8","updated":"2017-01-11 15:35:39.000000000","message":"As discussed in IRC, I agree that\u0027s it\u0027s not pretty, but I\u0027m not sure I have a better idea. We could add a \u0027tag\u0027 field to vif, but that would imply changing it in os-vif as well I believe...","commit_id":"4203b1b726ec8db3900ff87c26750fb3e27d8914"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"270f9bf915da88458eb9b188c73ce9f7a3698026","unresolved":false,"context_lines":[{"line_number":1367,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1368,"context_line":"            if tag:"},{"line_number":1369,"context_line":"                instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1370,"context_line":"                    context, instance)"},{"line_number":1371,"context_line":"                instance.save()"},{"line_number":1372,"context_line":"        except libvirt.libvirtError:"},{"line_number":1373,"context_line":"            LOG.error(_LE(\u0027attaching network adapter failed.\u0027),"}],"source_content_type":"text/x-python","patch_set":10,"id":"5a3905b3_8f31b15c","line":1370,"updated":"2017-01-23 18:38:45.000000000","message":"Using libvirt as the example, I don\u0027t really understand why we\u0027re passing the tag into here. We don\u0027t use it for anything other than as a trigger to regenerate device metadata. We could just always do that here since we know we\u0027ve tweaked the devices anyway.\n\nAlso, presumably we\u0027re depending on the tag being written to a VirtualInterface object in the database already so that our _build_device_metadata() method will see and use it?","commit_id":"c631dad2f945b7869406346516106a824fe4635b"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"dd754db2e9adb73ed19c2acb2546060281e8289a","unresolved":false,"context_lines":[{"line_number":1367,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1368,"context_line":"            if tag:"},{"line_number":1369,"context_line":"                instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1370,"context_line":"                    context, instance)"},{"line_number":1371,"context_line":"                instance.save()"},{"line_number":1372,"context_line":"        except libvirt.libvirtError:"},{"line_number":1373,"context_line":"            LOG.error(_LE(\u0027attaching network adapter failed.\u0027),"}],"source_content_type":"text/x-python","patch_set":10,"id":"5a3905b3_aae65be5","line":1370,"in_reply_to":"5a3905b3_8f31b15c","updated":"2017-01-23 18:57:30.000000000","message":"_build_device_metadata was only being called on instance spawn previously. We could just call it here every time, but that\u0027d be wasteful since not every interface attachment will be tagged (and we only exposed tagged devices in the metadata). My thinking was therefore to add the tag parameter to indicate that the attachment is tagged and we need to rebuild the device metadata.","commit_id":"c631dad2f945b7869406346516106a824fe4635b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9350cabb0d600ed4ef85c62fda84e03d62544eea","unresolved":false,"context_lines":[{"line_number":1400,"context_line":"            state \u003d guest.get_power_state(self._host)"},{"line_number":1401,"context_line":"            live \u003d state in (power_state.RUNNING, power_state.PAUSED)"},{"line_number":1402,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1403,"context_line":"            # NOTE(artom) If we\u0027re attaching with a device role tag, we need to"},{"line_number":1404,"context_line":"            # rebuild device_metadata. If we\u0027re attaching without a role"},{"line_number":1405,"context_line":"            # tag, we\u0027re rebuilding it here needlessly anyways. This isn\u0027t a"},{"line_number":1406,"context_line":"            # massive deal, and it helps reduce code complexity by not having"},{"line_number":1407,"context_line":"            # to indicate to the virt driver that the attach is tagged. The"},{"line_number":1408,"context_line":"            # really important optimization of not calling the database unless"},{"line_number":1409,"context_line":"            # device_metadata has actually changed is done for us by"},{"line_number":1410,"context_line":"            # instance.save()."},{"line_number":1411,"context_line":"            instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1412,"context_line":"                context, instance)"},{"line_number":1413,"context_line":"            instance.save()"},{"line_number":1414,"context_line":"        except libvirt.libvirtError:"},{"line_number":1415,"context_line":"            LOG.error(_LE(\u0027attaching network adapter failed.\u0027),"},{"line_number":1416,"context_line":"                     instance\u003dinstance, exc_info\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":34,"id":"1f013ff3_6b2884e1","line":1413,"range":{"start_line":1403,"start_character":0,"end_line":1413,"end_character":27},"updated":"2017-05-16 19:24:26.000000000","message":"Technically this should come outside the try/except which is handling libvirtError, which I don\u0027t think we\u0027d get in this code, right? If _build_device_metadata fails should it be caught and raise InterfaceAttachFailed so the compute manager code can rollback the attachment?\n\nI guess the compute manager code will catch any NovaException and deallocate the port:\n\nhttps://github.com/openstack/nova/blob/d966d33227acfca5329beac39f86059ae1ac69ad/nova/compute/manager.py#L5161\n\nThat doesn\u0027t directly detach it from the guest though - I think what would happen is deallocating the port might trigger neutron to send a vif-deleted event to nova which would detach the interface from the guest:\n\nhttps://github.com/openstack/nova/blob/d966d33227acfca5329beac39f86059ae1ac69ad/nova/compute/manager.py#L6862\n\nAssuming we get a vif-deleted event; I\u0027m not really sure what happens if the port was provided to attach to nova, it wouldn\u0027t get deleted then, just unbound, which might give a network-changed event but we don\u0027t detach in that case:\n\nhttps://github.com/openstack/nova/blob/d966d33227acfca5329beac39f86059ae1ac69ad/nova/compute/manager.py#L6854\n\nSo would it be cleanest to just rollback the attach in the guest here if something fails when building the device metadata? Or just do that before we attach to the guest, and do instance.save() after that?","commit_id":"4727d38142c1f37a2052a53df23397ecaa6f6661"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"bca7b6a24b0fee1a9863e8436c5427aac2748498","unresolved":false,"context_lines":[{"line_number":1400,"context_line":"            state \u003d guest.get_power_state(self._host)"},{"line_number":1401,"context_line":"            live \u003d state in (power_state.RUNNING, power_state.PAUSED)"},{"line_number":1402,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1403,"context_line":"            # NOTE(artom) If we\u0027re attaching with a device role tag, we need to"},{"line_number":1404,"context_line":"            # rebuild device_metadata. If we\u0027re attaching without a role"},{"line_number":1405,"context_line":"            # tag, we\u0027re rebuilding it here needlessly anyways. This isn\u0027t a"},{"line_number":1406,"context_line":"            # massive deal, and it helps reduce code complexity by not having"},{"line_number":1407,"context_line":"            # to indicate to the virt driver that the attach is tagged. The"},{"line_number":1408,"context_line":"            # really important optimization of not calling the database unless"},{"line_number":1409,"context_line":"            # device_metadata has actually changed is done for us by"},{"line_number":1410,"context_line":"            # instance.save()."},{"line_number":1411,"context_line":"            instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1412,"context_line":"                context, instance)"},{"line_number":1413,"context_line":"            instance.save()"},{"line_number":1414,"context_line":"        except libvirt.libvirtError:"},{"line_number":1415,"context_line":"            LOG.error(_LE(\u0027attaching network adapter failed.\u0027),"},{"line_number":1416,"context_line":"                     instance\u003dinstance, exc_info\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":34,"id":"1f013ff3_5917d9a4","line":1413,"range":{"start_line":1403,"start_character":0,"end_line":1413,"end_character":27},"in_reply_to":"1f013ff3_6b2884e1","updated":"2017-05-17 01:45:11.000000000","message":"After IRC discussion, I\u0027ve put the device metadata stuff into its own try block, and if it fails we call detach_interface to cleanup.","commit_id":"4727d38142c1f37a2052a53df23397ecaa6f6661"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"22fd04cd8284e68eb14897df06f6f5d4f4d25d0c","unresolved":false,"context_lines":[{"line_number":1401,"context_line":"            live \u003d state in (power_state.RUNNING, power_state.PAUSED)"},{"line_number":1402,"context_line":"            guest.attach_device(cfg, persistent\u003dTrue, live\u003dlive)"},{"line_number":1403,"context_line":"        except libvirt.libvirtError:"},{"line_number":1404,"context_line":"            LOG.error(_LE(\u0027attaching network adapter failed.\u0027),"},{"line_number":1405,"context_line":"                     instance\u003dinstance, exc_info\u003dTrue)"},{"line_number":1406,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":1407,"context_line":"            raise exception.InterfaceAttachFailed("},{"line_number":1408,"context_line":"                    instance_uuid\u003dinstance.uuid)"},{"line_number":1409,"context_line":"        try:"},{"line_number":1410,"context_line":"            # NOTE(artom) If we\u0027re attaching with a device role tag, we need to"},{"line_number":1411,"context_line":"            # rebuild device_metadata. If we\u0027re attaching without a role"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_0076cad9","line":1408,"range":{"start_line":1404,"start_character":12,"end_line":1408,"end_character":48},"updated":"2017-05-17 12:38:12.000000000","message":"Unrelated, but we should separately fix this by putting it into an excutils.save_and_reraise_exception in case unplug fails, we\u0027d lose the original error.","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"22fd04cd8284e68eb14897df06f6f5d4f4d25d0c","unresolved":false,"context_lines":[{"line_number":1423,"context_line":"            # successfully but building and/or saving the device metadata"},{"line_number":1424,"context_line":"            # failed. Just unplugging the vif is therefore not enough cleanup,"},{"line_number":1425,"context_line":"            # we need to detach the interface."},{"line_number":1426,"context_line":"            LOG.error(\u0027interface attached successfully but building and/or \u0027"},{"line_number":1427,"context_line":"                      \u0027saving device metadata failed\u0027)"},{"line_number":1428,"context_line":"            self.detach_interface(context, instance, vif)"},{"line_number":1429,"context_line":"            raise exception.InterfaceAttachFailed("},{"line_number":1430,"context_line":"                    instance_uuid\u003dinstance.uuid)"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_c3533ce2","line":1427,"range":{"start_line":1426,"start_character":12,"end_line":1427,"end_character":54},"updated":"2017-05-17 12:38:12.000000000","message":"We should pass the instance kwarg on this for context in the logs, and in this case we should probably either use LOG.exception so we get the traceback, or use exc_info\u003dTrue like above.","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"453aec3aab100fec772ad1e864e6aaf4a7d338b7","unresolved":false,"context_lines":[{"line_number":1423,"context_line":"            # successfully but building and/or saving the device metadata"},{"line_number":1424,"context_line":"            # failed. Just unplugging the vif is therefore not enough cleanup,"},{"line_number":1425,"context_line":"            # we need to detach the interface."},{"line_number":1426,"context_line":"            LOG.error(\u0027interface attached successfully but building and/or \u0027"},{"line_number":1427,"context_line":"                      \u0027saving device metadata failed\u0027)"},{"line_number":1428,"context_line":"            self.detach_interface(context, instance, vif)"},{"line_number":1429,"context_line":"            raise exception.InterfaceAttachFailed("},{"line_number":1430,"context_line":"                    instance_uuid\u003dinstance.uuid)"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_5727e365","line":1427,"range":{"start_line":1426,"start_character":12,"end_line":1427,"end_character":54},"in_reply_to":"ff0f0b1f_c3533ce2","updated":"2017-05-17 16:44:16.000000000","message":"Done","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"22fd04cd8284e68eb14897df06f6f5d4f4d25d0c","unresolved":false,"context_lines":[{"line_number":1425,"context_line":"            # we need to detach the interface."},{"line_number":1426,"context_line":"            LOG.error(\u0027interface attached successfully but building and/or \u0027"},{"line_number":1427,"context_line":"                      \u0027saving device metadata failed\u0027)"},{"line_number":1428,"context_line":"            self.detach_interface(context, instance, vif)"},{"line_number":1429,"context_line":"            raise exception.InterfaceAttachFailed("},{"line_number":1430,"context_line":"                    instance_uuid\u003dinstance.uuid)"},{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    def detach_interface(self, context, instance, vif):"},{"line_number":1433,"context_line":"        guest \u003d self._host.get_guest(instance)"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_23d0b850","line":1430,"range":{"start_line":1428,"start_character":12,"end_line":1430,"end_character":48},"updated":"2017-05-17 12:38:12.000000000","message":"Can we use excutils.save_and_reraise_exception here? There is a way to raise a new exception with that context manager.","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"453aec3aab100fec772ad1e864e6aaf4a7d338b7","unresolved":false,"context_lines":[{"line_number":1425,"context_line":"            # we need to detach the interface."},{"line_number":1426,"context_line":"            LOG.error(\u0027interface attached successfully but building and/or \u0027"},{"line_number":1427,"context_line":"                      \u0027saving device metadata failed\u0027)"},{"line_number":1428,"context_line":"            self.detach_interface(context, instance, vif)"},{"line_number":1429,"context_line":"            raise exception.InterfaceAttachFailed("},{"line_number":1430,"context_line":"                    instance_uuid\u003dinstance.uuid)"},{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    def detach_interface(self, context, instance, vif):"},{"line_number":1433,"context_line":"        guest \u003d self._host.get_guest(instance)"}],"source_content_type":"text/x-python","patch_set":35,"id":"ff0f0b1f_57dc8345","line":1430,"range":{"start_line":1428,"start_character":12,"end_line":1430,"end_character":48},"in_reply_to":"ff0f0b1f_23d0b850","updated":"2017-05-17 16:44:16.000000000","message":"Wait, what\u0027s the point of using save_and_reraise if we\u0027re just going to raise a new one? Since the compute manager catches NovaException we\u0027re OK with a simple save_and_reraise here, no?","commit_id":"f55aabb4e3aec533d8a557ba130fd51ea00c0e04"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a5f5078b14c6b9dd81d29fe10c3d8ecbb75dd714","unresolved":false,"context_lines":[{"line_number":1418,"context_line":"            instance.device_metadata \u003d self._build_device_metadata("},{"line_number":1419,"context_line":"                context, instance)"},{"line_number":1420,"context_line":"            instance.save()"},{"line_number":1421,"context_line":"        except Exception:"},{"line_number":1422,"context_line":"            # NOTE(artom) If we fail here it means the interface attached"},{"line_number":1423,"context_line":"            # successfully but building and/or saving the device metadata"},{"line_number":1424,"context_line":"            # failed. Just unplugging the vif is therefore not enough cleanup,"}],"source_content_type":"text/x-python","patch_set":38,"id":"ff0f0b1f_8e40bd09","line":1421,"updated":"2017-05-17 19:23:40.000000000","message":"By not raising InterfaceAttachFailed, if whatever this exception is is not a NovaException, it won\u0027t be handled in the compute manager:\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L5161\n\nAnd we won\u0027t deallocate the port. I\u0027m thinking coding errors, like TypeError or something. In that case the interface might be detached from the guest but the port wouldn\u0027t be deallocated/unbound before recording the instance fault, and the port would be bound to the instance even though it\u0027s detached from the guest.","commit_id":"faa965f11d6374052a9d692b3a26fb30457f2e8c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"036828f41d740903ce960c37fb3898115c9fa0ef","unresolved":false,"context_lines":[{"line_number":1423,"context_line":"            # successfully but building and/or saving the device metadata"},{"line_number":1424,"context_line":"            # failed. Just unplugging the vif is therefore not enough cleanup,"},{"line_number":1425,"context_line":"            # we need to detach the interface."},{"line_number":1426,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":1427,"context_line":"                LOG.error(\u0027Interface attached successfully but building \u0027"},{"line_number":1428,"context_line":"                          \u0027and/or saving device metadata failed.\u0027,"},{"line_number":1429,"context_line":"                          instance\u003dinstance, exc_info\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":39,"id":"ff0f0b1f_b10ac487","line":1426,"updated":"2017-05-17 20:19:42.000000000","message":"I suppose this should use reraise\u003dFalse so the original exception isn\u0027t logged when the new exception is raised, especially since we log the traceback before calling detach_interface.","commit_id":"bf52f3cd9b5fc30102446ba548fa20c24584b15b"}],"nova/virt/xenapi/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a8ab53d26e72331a6619971e68c9a6895898d6e9","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        \"supports_migrate_to_same_host\": False,"},{"line_number":73,"context_line":"        \"supports_attach_interface\": True,"},{"line_number":74,"context_line":"        \"supports_device_tagging\": False,"},{"line_number":75,"context_line":"        \"supports_tagged_attach_interface\": False,"},{"line_number":76,"context_line":"    }"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def __init__(self, virtapi, read_only\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":29,"id":"5ff73747_e1092c2c","line":75,"updated":"2017-04-26 15:40:08.000000000","message":"The vmware and ironic drivers also override capabilities, why aren\u0027t you updating their capabilities dict in this change?","commit_id":"2d30f40b8919e3f4d7d53abb83f63f3c4c0430fa"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0c6a450c2cdeea94d8d0c9cb37fa158a6426bf04","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        \"supports_migrate_to_same_host\": False,"},{"line_number":73,"context_line":"        \"supports_attach_interface\": True,"},{"line_number":74,"context_line":"        \"supports_device_tagging\": False,"},{"line_number":75,"context_line":"        \"supports_tagged_attach_interface\": False,"},{"line_number":76,"context_line":"    }"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def __init__(self, virtapi, read_only\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":29,"id":"5ff73747_0188986e","line":75,"in_reply_to":"5ff73747_e1092c2c","updated":"2017-04-26 15:42:54.000000000","message":"Alternatively, if the compute manager code, or whatever checking this, is just doing capabilities.get(\"supports_tagged_attach_interface\", False) then we\u0027d also be OK for the default.","commit_id":"2d30f40b8919e3f4d7d53abb83f63f3c4c0430fa"}]}