)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"999c585adcd6b25566513b2290b84cc462c9b983","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Only unplug vif after the device is detached from libvirt"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a potential race between unplugging a vif and removing the"},{"line_number":10,"context_line":"related device from the domain XML. During macvtap (hw_veb) unplug nova"},{"line_number":11,"context_line":"tries to reset the MAC and VLAN config of the VF used for the macvtap."},{"line_number":12,"context_line":"However as it is done before the macvtap device is removed from the"},{"line_number":13,"context_line":"domain, libvirt still feels responsible to the VF too and restores the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9f560f44_188f8a23","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":36},"updated":"2020-09-08 09:16:13.000000000","message":"that\u0027s where a functional test could have be good for reproducing the race and making sure we fix it.","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"49b6be82a65fd6cc227dc62fcc4b4baee689136c","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Only unplug vif after the device is detached from libvirt"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a potential race between unplugging a vif and removing the"},{"line_number":10,"context_line":"related device from the domain XML. During macvtap (hw_veb) unplug nova"},{"line_number":11,"context_line":"tries to reset the MAC and VLAN config of the VF used for the macvtap."},{"line_number":12,"context_line":"However as it is done before the macvtap device is removed from the"},{"line_number":13,"context_line":"domain, libvirt still feels responsible to the VF too and restores the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9f560f44_1b84a45d","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":36},"in_reply_to":"9f560f44_188f8a23","updated":"2020-09-08 09:54:24.000000000","message":"functional tests cant repoduce this race.\nwe need a real libvirt deamon to see it.\n\nwhat is happening is that libvirt observes the interface is no longer present on the ovs bridge and it readds it after we do unplug.\n\nso by moving unplug to after the libvirt interface detach we can prevent that race between nova/os-vif unplugging the interface form ovs and libvirt readding it.\n\n\nif you think about the sequncing this also makes sense.\n\nwe plug the interface into the network backend before attaching it or booting the vm via libvirt\nso it make sense that we would do the reverse and update libvirt before unpluging the interface when we do detach or vm destroy.","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"f3e35a2f9dd8007b3c5444b6e2113236c99a8bcd","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Only unplug vif after the device is detached from libvirt"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a potential race between unplugging a vif and removing the"},{"line_number":10,"context_line":"related device from the domain XML. During macvtap (hw_veb) unplug nova"},{"line_number":11,"context_line":"tries to reset the MAC and VLAN config of the VF used for the macvtap."},{"line_number":12,"context_line":"However as it is done before the macvtap device is removed from the"},{"line_number":13,"context_line":"domain, libvirt still feels responsible to the VF too and restores the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"9f560f44_96408b37","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":36},"in_reply_to":"9f560f44_1b84a45d","updated":"2020-09-08 10:53:57.000000000","message":"Yeah, we cannot really reproduce it without maken a too smart libvirt stub","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3c49507ccff36a5e5922aef5ba2d87d3e4eaa31e","unresolved":false,"context_lines":[{"line_number":2286,"context_line":"                            \u0027the device is no longer found on the guest.\u0027,"},{"line_number":2287,"context_line":"                            {\u0027mac\u0027: mac}, instance\u003dinstance)"},{"line_number":2288,"context_line":""},{"line_number":2289,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2290,"context_line":""},{"line_number":2291,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2292,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_d76781e8","line":2289,"range":{"start_line":2289,"start_character":11,"end_line":2289,"end_character":49},"updated":"2020-08-05 20:24:58.000000000","message":"maybe just put this in a finally block? so its allways called regardelss of if an excption is raised or not.\n\nthat way you only need to have unplug in one place?","commit_id":"660e7b0b2464bd79e4ffe3317a86948252791ded"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"635044e994bf59d386a195576bb46df45d36ce1c","unresolved":false,"context_lines":[{"line_number":2286,"context_line":"                            \u0027the device is no longer found on the guest.\u0027,"},{"line_number":2287,"context_line":"                            {\u0027mac\u0027: mac}, instance\u003dinstance)"},{"line_number":2288,"context_line":""},{"line_number":2289,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2290,"context_line":""},{"line_number":2291,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2292,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_b790d24f","line":2289,"range":{"start_line":2289,"start_character":11,"end_line":2289,"end_character":49},"in_reply_to":"9f560f44_17d38b02","updated":"2020-08-27 12:27:23.000000000","message":"Done","commit_id":"660e7b0b2464bd79e4ffe3317a86948252791ded"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"71da939d7eadfda934767c8de4d32ee501c3f618","unresolved":false,"context_lines":[{"line_number":2286,"context_line":"                            \u0027the device is no longer found on the guest.\u0027,"},{"line_number":2287,"context_line":"                            {\u0027mac\u0027: mac}, instance\u003dinstance)"},{"line_number":2288,"context_line":""},{"line_number":2289,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2290,"context_line":""},{"line_number":2291,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2292,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f560f44_17d38b02","line":2289,"range":{"start_line":2289,"start_character":11,"end_line":2289,"end_character":49},"in_reply_to":"9f560f44_d76781e8","updated":"2020-08-26 18:20:44.000000000","message":"+1","commit_id":"660e7b0b2464bd79e4ffe3317a86948252791ded"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"e2691dbfb8750436117932e54b5dafc1d78295af","unresolved":false,"context_lines":[{"line_number":2244,"context_line":"            # we can be racing against Neutron deleting the port and"},{"line_number":2245,"context_line":"            # sending the vif-deleted event which then triggers a call to"},{"line_number":2246,"context_line":"            # detach the interface, so if the interface is not found then"},{"line_number":2247,"context_line":"            # we can just log it as a warning."},{"line_number":2248,"context_line":"            if not interface:"},{"line_number":2249,"context_line":"                mac \u003d vif.get(\u0027address\u0027)"},{"line_number":2250,"context_line":"                # The interface is gone so just log it as a warning."}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_5b1e1c5f","line":2247,"updated":"2020-09-08 09:54:40.000000000","message":"note to self : this above comment is still valid, we could still delete the instance and go there without unpluging the VIF first.","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"e2691dbfb8750436117932e54b5dafc1d78295af","unresolved":false,"context_lines":[{"line_number":2326,"context_line":"            # race and leave the detached device configured. Also even if we"},{"line_number":2327,"context_line":"            # are failed to detach due to race conditions the unplug is"},{"line_number":2328,"context_line":"            # necessary for the same reason"},{"line_number":2329,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2330,"context_line":""},{"line_number":2331,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2332,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_bbdf7864","line":2329,"updated":"2020-09-08 09:54:40.000000000","message":"OK, I verified all the codepath and what you write seems correct.","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a41df22c749e180f274fe032d194e82c2fc406c0","unresolved":false,"context_lines":[{"line_number":2326,"context_line":"            # race and leave the detached device configured. Also even if we"},{"line_number":2327,"context_line":"            # are failed to detach due to race conditions the unplug is"},{"line_number":2328,"context_line":"            # necessary for the same reason"},{"line_number":2329,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2330,"context_line":""},{"line_number":2331,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2332,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_8f5b2d20","line":2329,"in_reply_to":"9f560f44_0f70bda3","updated":"2020-09-09 20:05:25.000000000","message":"\u003e Please excuse the late drive by review, but what about all of the\n \u003e error handling above? Can `unplug` raise an exception that should\n \u003e be handled and potentially logged rather than re-raised? And what\n \u003e if the device detach above fails, do you still want to unplug here?\n \u003e In other words, should this be an `else` block rather than a\n \u003e `finally` block?\n\nI guess the comment says:\n\n\u003e Also even if we are failed to detach due to race conditions the unplug is necessary for the same reason\n\nSo OK, but still seems weird.","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"57a8db931ac11901bcd269a07d72272cce3bf7b8","unresolved":false,"context_lines":[{"line_number":2326,"context_line":"            # race and leave the detached device configured. Also even if we"},{"line_number":2327,"context_line":"            # are failed to detach due to race conditions the unplug is"},{"line_number":2328,"context_line":"            # necessary for the same reason"},{"line_number":2329,"context_line":"            self.vif_driver.unplug(instance, vif)"},{"line_number":2330,"context_line":""},{"line_number":2331,"context_line":"    def _create_snapshot_metadata(self, image_meta, instance,"},{"line_number":2332,"context_line":"                                  img_fmt, snp_name):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_0f70bda3","line":2329,"in_reply_to":"9f560f44_bbdf7864","updated":"2020-09-09 20:03:33.000000000","message":"Please excuse the late drive by review, but what about all of the error handling above? Can `unplug` raise an exception that should be handled and potentially logged rather than re-raised? And what if the device detach above fails, do you still want to unplug here? In other words, should this be an `else` block rather than a `finally` block?","commit_id":"7e8a9786dc3d330b79970f398e73fa596dc9445a"}]}