)]}'
{"octavia/controller/worker/tasks/network_tasks.py":[{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"1f3c98c64b1349b4468ec6088d008041cf779851","unresolved":false,"context_lines":[{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        try:"},{"line_number":358,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":359,"context_line":"        except Exception as e:"},{"line_number":360,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":361,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":362,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_87b74f87","line":359,"updated":"2017-12-03 18:11:57.000000000","message":"This seems very broad.  Are there cases we want the exception to raise?","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"b7b9de24b8edf887368743bb9042e5cb3c215134","unresolved":false,"context_lines":[{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        try:"},{"line_number":358,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":359,"context_line":"        except Exception as e:"},{"line_number":360,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":361,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":362,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_d71e4d0b","line":359,"in_reply_to":"df87a7cf_02356dda","updated":"2017-12-04 21:19:44.000000000","message":"VIP is a bit of a special case in that it could be an important IP address for the tenant.  If we just log this and continue it doesn\u0027t highlight for the user or the operator that the address was not released.  This could lead to the inability for the tenant to use that IP for another purpose or LB.\nI lean towards letting this escalate to ERROR.\nInterested to hear other opinions however.","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":25239,"name":"Bar Elharar","email":"bar@redhat.com","username":"belharar"},"change_message_id":"578921298d3c5c50e70db50b4cf27e8d22edff22","unresolved":false,"context_lines":[{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        try:"},{"line_number":358,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":359,"context_line":"        except Exception as e:"},{"line_number":360,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":361,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":362,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_02356dda","line":359,"in_reply_to":"df87a7cf_87b74f87","updated":"2017-12-03 19:50:52.000000000","message":"DeallocateVIP is included in two flows: get_{cascade_,}delete_load_balancer_flow.\n\nMy reasoning is that the behavior should be as close to AllocateVIP\u0027s revert (L334) as possible. In both cases, there\u0027re dangling resources due for clean-up that are dependent on that task\u0027s completion.\nInteresting to note is that the succeeding tasks for DeallocateVIP in the delete flow are all database accesses, no network stuff.\n\nWe could try to implement machanism to defer the exceptions (and reraise them later, exiting the flow), but either way, I would argue that so deep into the flow, we better off trying to complete the flow no matter what.\n\nNonetheless, is there specific exception(s) you would rather exclude and raise immediately?","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":10273,"name":"Adam Harwell","email":"flux.adam@gmail.com","username":"rm_you"},"change_message_id":"8e462baaf9748340a1fccaedd1abc576903d5935","unresolved":false,"context_lines":[{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        try:"},{"line_number":358,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":359,"context_line":"        except Exception as e:"},{"line_number":360,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":361,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":362,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_d3f3b472","line":359,"in_reply_to":"df87a7cf_d71e4d0b","updated":"2017-12-05 08:51:24.000000000","message":"Well, I think technically if the VIP belongs to the tenant, they can re-bind it elsewhere whether we disassociate it or not... That is actually one of the things we were worried about, I thought!\n\nAnyway, this should be easy to test -- create a LB with a tenant port as the VIP, and then just ... try to rebind the port as the tenant to something else, and see what happens?\n\nSince this is part of the delete_load_balancer process, if we let it succeed, the LB will be gone and so it won\u0027t be trying to steal the port back on failovers or anything, so that\u0027s actually a bonus. :P","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"5572ce0047e1328a277cdcc7efd79756dde949ed","unresolved":false,"context_lines":[{"line_number":361,"context_line":""},{"line_number":362,"context_line":"        try:"},{"line_number":363,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":364,"context_line":"        except Exception as e:"},{"line_number":365,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":366,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":367,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":8,"id":"bf8cb3f7_b2a09ea7","line":364,"updated":"2017-12-21 00:38:18.000000000","message":"This method does a few things:\n1. It deletes the base (vrrp/frontend) port owned by octavia.\n2. It removes any security groups octavia added to the VIP port.\n3. Deletes the security group created for the VIP.\n4. If octavia created the port, it deletes it.\n\nI argue that if any one of these things fail we should indicate to the tenant that there has been an ERROR by reverting the flow. This patch makes it a silent failure.\n\nThere are a few side effects of the silent failure:\n1. A port (vip port) may be left in the tenant account (using quota) even if it was created by octavia.\n2. Security groups the tenant may not be aware of would still be on the VIP. They could manually remove them as they own the port, but they would have to know to do so.\n3. It could leave an in-use port in the octavia project.\n\nWhen users are doing ansible, heat, or the like and expecting certain states if there is not an error I think we should be raising the error to user visibility.","commit_id":"cba74bc000e058ed04dc75a11a9adc5b8b54bf12"},{"author":{"_account_id":25239,"name":"Bar Elharar","email":"bar@redhat.com","username":"belharar"},"change_message_id":"41731fbff42b25c3ae5e2d8ac8ec041277ee7f70","unresolved":false,"context_lines":[{"line_number":361,"context_line":""},{"line_number":362,"context_line":"        try:"},{"line_number":363,"context_line":"            self.network_driver.deallocate_vip(vip)"},{"line_number":364,"context_line":"        except Exception as e:"},{"line_number":365,"context_line":"            LOG.error(\"Failed to deallocate VIP.  Resources may still \""},{"line_number":366,"context_line":"                      \"be in use from vip: %(vip)s due to error: %(except)s\","},{"line_number":367,"context_line":"                      {\u0027vip\u0027: vip.ip_address, \u0027except\u0027: e})"}],"source_content_type":"text/x-python","patch_set":8,"id":"bf8cb3f7_2d742835","line":364,"in_reply_to":"bf8cb3f7_b2a09ea7","updated":"2017-12-21 12:54:33.000000000","message":"I get your argument for raising an user-visible error, but I disagree that we should revert the delete flows due to a failure in the deallocate_vip task, for reasons I have mentioned in earlier comments.\n\nIn other words,\nThe side effects of this suppress clause are:\n1. Log an error\n2. Do not raise the exception\n3. Do not revert flow\nWhile what I want is: $1 \u0026 $3 without $2.\nNot sure whether it can be achieved without changing the flow.\n\nNote that any security group removal error will currently not raise a user-visible exception, due to another suppress clause, added by this patch in the network_driver. (ref: https://review.openstack.org/#/c/523931/8/octavia/network/drivers/neutron/allowed_address_pairs.py #L304)\nTherefore, removing that suppress code will only partially address your concerns.","commit_id":"cba74bc000e058ed04dc75a11a9adc5b8b54bf12"}],"octavia/network/drivers/neutron/allowed_address_pairs.py":[{"author":{"_account_id":6579,"name":"Nir Magnezi","email":"nmagnezi@redhat.com","username":"nmagnezi"},"change_message_id":"58c85898113eb6765c2b5971a339b58f0efc0c48","unresolved":false,"context_lines":[{"line_number":232,"context_line":"                         \"already deleted\", sec_grp)"},{"line_number":233,"context_line":"                return"},{"line_number":234,"context_line":"            except neutron_client_exceptions.Conflict:"},{"line_number":235,"context_line":"                message \u003d (\"Security group %s is still in use, and will not \""},{"line_number":236,"context_line":"                           \"be removed.\", sec_grp)"},{"line_number":237,"context_line":"                raise base.DeallocateVIPException(message)"},{"line_number":238,"context_line":"            except Exception:"},{"line_number":239,"context_line":"                LOG.warning(\"Attempt %(attempt)s to remove security group \""}],"source_content_type":"text/x-python","patch_set":2,"id":"df87a7cf_cc057c7c","line":236,"range":{"start_line":235,"start_character":0,"end_line":236,"end_character":50},"updated":"2017-12-03 14:37:14.000000000","message":"this does not embed the sec group uuid in the string, as it should:\n\nERROR octavia.network.drivers.neutron.allowed_address_pairs [-] (\u0027Security group %s is still in use, and will not be removed.\u0027, u\u002728ca8b80-f449-4f73-b18d-7d11233266ec\u0027): DeallocateVIPException: (\u0027Security group %s is still in use, and will not be removed.\u0027, u\u002728ca8b80-f449-4f73-b18d-7d11233266ec\u0027)","commit_id":"43dae8e28cb758d8887d5ffb25c7ac355114e30b"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"1f3c98c64b1349b4468ec6088d008041cf779851","unresolved":false,"context_lines":[{"line_number":242,"context_line":"                    {\u0027attempt\u0027: attempts, \u0027sg\u0027: sec_grp,"},{"line_number":243,"context_line":"                     \u0027msg\u0027: getattr(e, \u0027message\u0027, \u0027NeutronException\u0027),"},{"line_number":244,"context_line":"                     })"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        message \u003d _(\"All attempts to remove security group {0} have \""},{"line_number":247,"context_line":"                    \"failed.\").format(sec_grp)"},{"line_number":248,"context_line":"        raise base.DeallocateVIPException(message)"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_c7bd5769","line":245,"updated":"2017-12-03 18:11:57.000000000","message":"We should still have a catch-all except clause here that logs other failures and continues to try.","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":11628,"name":"Michael Johnson","email":"johnsomor@gmail.com","username":"johnsom"},"change_message_id":"b7b9de24b8edf887368743bb9042e5cb3c215134","unresolved":false,"context_lines":[{"line_number":242,"context_line":"                    {\u0027attempt\u0027: attempts, \u0027sg\u0027: sec_grp,"},{"line_number":243,"context_line":"                     \u0027msg\u0027: getattr(e, \u0027message\u0027, \u0027NeutronException\u0027),"},{"line_number":244,"context_line":"                     })"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        message \u003d _(\"All attempts to remove security group {0} have \""},{"line_number":247,"context_line":"                    \"failed.\").format(sec_grp)"},{"line_number":248,"context_line":"        raise base.DeallocateVIPException(message)"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_0a8808e5","line":245,"in_reply_to":"df87a7cf_42405536","updated":"2017-12-04 21:19:44.000000000","message":"Well, before we had a catch-all exception handler that logged, but let the retry loop continue.\nThis is an advantage in that if some non-neutron exception occurs it will keep retrying.\nIt was the outer call that worried me by having a catch-all.","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"},{"author":{"_account_id":25239,"name":"Bar Elharar","email":"bar@redhat.com","username":"belharar"},"change_message_id":"578921298d3c5c50e70db50b4cf27e8d22edff22","unresolved":false,"context_lines":[{"line_number":242,"context_line":"                    {\u0027attempt\u0027: attempts, \u0027sg\u0027: sec_grp,"},{"line_number":243,"context_line":"                     \u0027msg\u0027: getattr(e, \u0027message\u0027, \u0027NeutronException\u0027),"},{"line_number":244,"context_line":"                     })"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        message \u003d _(\"All attempts to remove security group {0} have \""},{"line_number":247,"context_line":"                    \"failed.\").format(sec_grp)"},{"line_number":248,"context_line":"        raise base.DeallocateVIPException(message)"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_42405536","line":245,"in_reply_to":"df87a7cf_c7bd5769","updated":"2017-12-03 19:50:52.000000000","message":"Can you please elaborate? What scenario would retry-after-non-neutron-exception tackle?\nTo my understanding, the catch-all clause was too broad in that context.\nNote that I _do_ log non-Neutron exceptions, when I catch them in the DeallocateVIP task\u0027s try-except.","commit_id":"4a8cde13da02d046069975fa022bbd99eff5fb3d"}]}