)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"ead7ebe81a958ef40d7d2ac2801384769b7263f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e2093a1f_0ba2127a","updated":"2023-05-23 08:19:30.000000000","message":"I agree with Slawek to use 409.\nThe user has a permission to delete the port (allowed by the RBAC policy) but the condition(s) for the port deletion are not satisfied (the port is used by another resource (FIP)), so 409 conflict sounds better.\n\nIn normal user scenario, a FIP is disassociated when the associated port is deleted (if it can be disassociated), but FIP is owned by another project and it cannot be disassociated. I think it is similar to a case where a router has attached port(s), so Conlict makes sense to me.\n\nNote that a case where admin associates a FIP with a port created by a regular user is a kind of special case (at least it is beyond self-service usecase), so I think admin and a regular user need better communications for such case","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"bf62c8bd78169b48c6f5ed791e378367356a7794","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"ebcc8708_f3a5f4e0","updated":"2023-05-22 08:52:34.000000000","message":"I\u0027m not sure if 403 is the best error returned here. IIUC it will be returned when user will want to remove fixed port (port e.g. attached to the VM) which have FIP associated to it also. In such case telling user that he is forbiden to delete port may be misleading IMO.\nMaybe 409 Conflict would be better in this case? Or PortInUse from https://github.com/openstack/neutron-lib/blob/master/neutron_lib/exceptions/__init__.py#L234 ?","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"78cbcac1bb21cb4cfcde389a405a27ac8c6e537b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c4eb963d_477190ac","updated":"2023-05-04 06:08:20.000000000","message":"LGTM","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"fc3d32fa45a07aa17e9081847bfe234d101af0e1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"2ba52094_9cc6ee24","updated":"2023-05-15 19:21:46.000000000","message":"So I have a simple question - why can\u0027t the port owner just disassociate the floating IP in this case? I guess I\u0027m trying to think of other relationships like this and what we allow in those cases.\n\nFor example, here\u0027s a scenario that is similar:\n\n1) User creates a new subnet\n2) Admin user creates a port on that subnet\n3) User can delete that port\n\nI guess 3) works because the port has the user\u0027s project_id and not the Admins.\n\nSo maybe this is an RBAC question?","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"7ee6e9f1f88ae7368af708d0483ef74b0853d3d9","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5cf9df37_a9a824ad","in_reply_to":"01ee97d7_868c1902","updated":"2023-05-16 13:32:45.000000000","message":"[1] meeting logs (I don\u0027t remember if it was drivers or networking meeting)\n\n[1] https://meetings.opendev.org/meetings/networking/2023/networking.2023-04-25-14.01.log.html","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"b2e70db89673074ceb153b49d26fede94202057c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"144504e7_3cc24bcb","in_reply_to":"26a3f42e_52caf122","updated":"2023-05-23 10:55:30.000000000","message":"It seems that 5 (409) - 1 (403) is a great result for a football match 😄\n\nRight now I\u0027ll upload a patch triggering a new exception, and also add it to neutron-lib, but until a new release and its bump, let\u0027s keep the code here too. \n\nThanks for the feedback guys","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"f0cdbc09c05ba17b9cee4660f152edde5f0e3bff","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"384f5ee2_39a97ea3","in_reply_to":"2ba52094_9cc6ee24","updated":"2023-05-16 07:38:14.000000000","message":"Thx Brian for take a look, IMO your comment aligns perfectly with the patch, indeed the patch reflects the decision made during the drivers meeting when we discussed this BZ. \n\nEssentially, the aim is to notify the user that they lack the necessary permissions to delete the port due to its association with a FIP (by an admin user). Consequently, the user will need to reach out to an admin user to make a decision (perhaps the admin\u0027s decision will involve associating the project_id during the next operation to prevent the user from having to contact them). So the main motivation behind this change is to transition from a vague 500 ServerError to an error message that provides the user with more information.","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b006e5e0f561a85d2c967fd592f0a5fa004437bc","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"01ee97d7_868c1902","in_reply_to":"384f5ee2_39a97ea3","updated":"2023-05-16 12:30:20.000000000","message":"Hi Fernando, thanks for the response. I didn\u0027t know this had been discussed at the drivers meeting. I guess since the user owns the port I would have figured they could delete the FIP association, and not have to involve an admin. I will have to find the meeting notes. This at least makes the error better though.","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"529e343ce1638652c4d2dfff5d4f9820e87f2ade","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"dff55f5f_3f73bda6","in_reply_to":"41fde0af_ca9d6fc4","updated":"2023-05-23 07:26:45.000000000","message":"IMO 409 is better as there is actually conflict with deletion of port as it has FIP which is still using it and can\u0027t be disassociate. IMO that\u0027s conflict 😊","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e8f4957926519e342f76b4524f60c370f1f35c2a","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"ab6238e2_20388417","in_reply_to":"5cf9df37_a9a824ad","updated":"2023-05-16 14:48:15.000000000","message":"Thanks for the link, I can see Rodolfo originally agreed with me, but then Slawek pointed out the FIP object is owned by the admin, so we should not allow a \"normal\" user to update it. Makes sense.","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"8f9d54d8195bce046abf950a6f0636f4b4bd7621","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"171f781c_19827583","in_reply_to":"dff55f5f_3f73bda6","updated":"2023-05-23 07:57:18.000000000","message":"I agree with Slawek on this. 409 is a better exception here and we are already using it in similar exceptions (as commented, PortInUse, that is very similar to this one). I would use HTTPConflict rather than HTTPForbidden.\n\nThe port cannot be deleted because there is a policy against it (because is forbidden) but because there is a conflict in the status of the port that doesn\u0027t allow the requested change.","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"96baba985eaea4550c3f1688c6261b36afae907e","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e998fc55_617204c8","in_reply_to":"dff55f5f_3f73bda6","updated":"2023-05-23 08:19:01.000000000","message":"My first idea was also Conflict, but PortInUse sounds much more user friendly, even a child exception is ok like FipPortInUse(): msg\u003d\u0027Unable to delete port \u003cx\u003e on network \u003cy\u003e, as port still has FloatingIP attached!\u0027\n\nNot sure what a regular tenant can know about the existing floating IPs created by admin.","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"2e14caea95911c47de92125914853c52d500082d","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"26a3f42e_52caf122","in_reply_to":"e998fc55_617204c8","updated":"2023-05-23 08:29:02.000000000","message":"\u003e My first idea was also Conflict, but PortInUse sounds much more user friendly, even a child exception is ok like FipPortInUse(): msg\u003d\u0027Unable to delete port \u003cx\u003e on network \u003cy\u003e, as port still has FloatingIP attached!\u0027\n\u003e \n\u003e Not sure what a regular tenant can know about the existing floating IPs created by admin.\n\nNote that InUse exception (a parent class of PortInUse) is finally mapped to 409 HTTP \nresponse code [1], so it is same from POV of the neutron API.\n\nIn normal scenarios, if a FIP is owned by a same project, the FIP will be disassociated when the associated port is deleted, so \"port still has FloatingIP attached\u0027 may be confusing, but perhaps it is enough because it is a special scenario and admin and user need to have better communications for such usage.\n\n[1] https://opendev.org/openstack/neutron-lib/src/commit/c5ca1ddf420b827e4684dee6a6495475014a91e3/neutron_lib/api/faults.py#L22","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"2da80be83add0c2b70fe9466bc5ce5244f41c8b3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"41fde0af_ca9d6fc4","in_reply_to":"ebcc8708_f3a5f4e0","updated":"2023-05-22 15:34:50.000000000","message":"With this patch the error code will be 403 and the msg received by user is:\n\nFailed to delete port with name or ID \u0027XXXX\u0027: ForbiddenException: 403: Client Error for url: http://..., Not authorized.\n\nUpon examining the HTTP response codes:\n\n403 [1]: IMO I think we can address it with this responde code, it covers the issue and also provides a Not authorized msg to the user (root cause), Moreover, its definition encompasses problems related to insufficient rights to access a resource.\n\n409 [2]: It talks about the status of the resource, so IMO we are not suffering a already bind/collision/conflict status issue over the port. The PortInUse exception is using this one as base,  with the following msg \u0027Failed to delete port with name or ID \u0027XXXX\u0027: ConflictException: 409: Client Error for url: http://..., Unable to complete operation on port \u003cX\u003e for network \u003cY\u003e. Port already has an attached device \u003cZ\u003e.\u0027\n\nSo IMHO the best code will be 403 but the PortInUse msg contains more info. wdyt?\n\n[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403\n[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/409","commit_id":"a811def8d783004ebd218166d5280ac575c0a09e"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"8a8654ad5eb7f6fd90c46a75c4ca1727db20443e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"d0f5e0e5_aff7abe7","updated":"2023-05-23 19:18:55.000000000","message":"recheck openstack-tox-cover unrelated","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"10881a30da011ba01bd38810f549ff2e48ee705d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"e8b81209_00ae4d64","updated":"2023-05-30 08:11:50.000000000","message":"recheck neutron-ovn-rally-task timeout","commit_id":"9f6f6d5082b4341529144e992d5293675146ae88"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"3ef940cb8093c27fabc52d768fb54931f8f8c2c9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"2d8645ee_e09c5ab7","updated":"2023-05-29 08:07:25.000000000","message":"recheck neutron-ovn-tempest-ipv6-only-ovs-release unrelated","commit_id":"9f6f6d5082b4341529144e992d5293675146ae88"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"654b8fdb26d6978bc2a82848e018b7deef0f646d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"8a95ee27_9ee29c8e","updated":"2023-05-30 10:59:07.000000000","message":"recheck neutron-ovs-tempest-multinode-full unrelated","commit_id":"9f6f6d5082b4341529144e992d5293675146ae88"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"881bcb4219ac9b9252b0e1ccc4eff0cbc8e58e67","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"bec074a6_83b48645","updated":"2023-05-26 11:36:46.000000000","message":"recheck neutron-tempest-plugin-designate-scenario dns timed out unrelated","commit_id":"9f6f6d5082b4341529144e992d5293675146ae88"}],"neutron/common/ovn/exceptions.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"0eb996cf2d334e829a5644ea8e148af0a997ea28","unresolved":true,"context_lines":[{"line_number":42,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":43,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":44,"context_line":"    message \u003d _(\u0027Unable to complete the operation on port \"%(port_id)s\" \u0027"},{"line_number":45,"context_line":"                \u0027because port still has associated floating IP.\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"6672807f_3a3c02c3","line":45,"updated":"2023-05-26 07:53:57.000000000","message":"The exception is not specific to OVN now, so common/ovn/exceptions.py still sounds tricky. As neutron-lib change is already prepared, the location does not matter much, but it is still better to choose the appropriate location.\nIMHO we can define this in db/l3_db.py itself (or extensions/l3.py).","commit_id":"3946908810607bb8946034389b6c9f6435bcce77"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"176f359029170742b6ff1188a571185d8e46d3a7","unresolved":false,"context_lines":[{"line_number":42,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":43,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":44,"context_line":"    message \u003d _(\u0027Unable to complete the operation on port \"%(port_id)s\" \u0027"},{"line_number":45,"context_line":"                \u0027because port still has associated floating IP.\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"dd49c49d_737f2473","line":45,"in_reply_to":"6672807f_3a3c02c3","updated":"2023-05-26 09:52:16.000000000","message":"ack, lets we put directly in db/l3_db.py in order to keep it visible and remind us to remove it once neutron-lib is updated.","commit_id":"3946908810607bb8946034389b6c9f6435bcce77"}],"neutron/db/l3_db.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"efc9c3dda501f1b162d2b2fd0c37dc14567bd6ef","unresolved":true,"context_lines":[{"line_number":1796,"context_line":"                floating_ip_objs_admin \u003d l3_obj.FloatingIP.get_objects("},{"line_number":1797,"context_line":"                    context.elevated(), fixed_port_id\u003dport_id)"},{"line_number":1798,"context_line":"                if floating_ip_objs_admin !\u003d floating_ip_objs:"},{"line_number":1799,"context_line":"                    raise ovn_l3_exc.FipAssociated(port_id\u003dport_id)"},{"line_number":1800,"context_line":""},{"line_number":1801,"context_line":"            router_ids \u003d {fip.router_id for fip in floating_ip_objs}"},{"line_number":1802,"context_line":"            old_fips \u003d {fip.id: self._make_floatingip_dict(fip)"}],"source_content_type":"text/x-python","patch_set":5,"id":"374f70b0_eccdabb5","line":1799,"range":{"start_line":1799,"start_character":26,"end_line":1799,"end_character":36},"updated":"2023-05-24 14:37:21.000000000","message":"it sounds tricky that the DB layer consumes an exception from ovn_l3 because ovn_l3 depends on the L3 DB module. Although FipAttached exception in ovn_l3 module will be moved to neutron-lib, I think it is better to define this exception in more common module.","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"af4a45ee5013830ae9cd23b39848f658aee5c89b","unresolved":false,"context_lines":[{"line_number":1796,"context_line":"                floating_ip_objs_admin \u003d l3_obj.FloatingIP.get_objects("},{"line_number":1797,"context_line":"                    context.elevated(), fixed_port_id\u003dport_id)"},{"line_number":1798,"context_line":"                if floating_ip_objs_admin !\u003d floating_ip_objs:"},{"line_number":1799,"context_line":"                    raise ovn_l3_exc.FipAssociated(port_id\u003dport_id)"},{"line_number":1800,"context_line":""},{"line_number":1801,"context_line":"            router_ids \u003d {fip.router_id for fip in floating_ip_objs}"},{"line_number":1802,"context_line":"            old_fips \u003d {fip.id: self._make_floatingip_dict(fip)"}],"source_content_type":"text/x-python","patch_set":5,"id":"aaa564dd_07d532a8","line":1799,"range":{"start_line":1799,"start_character":26,"end_line":1799,"end_character":36},"in_reply_to":"374f70b0_eccdabb5","updated":"2023-05-25 12:09:58.000000000","message":"Yep, lets we move to neutron/common/ovn/exceptions, in any case it is already in n-lib prepared [1]\n\n[1] https://review.opendev.org/c/openstack/neutron-lib/+/883901","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"0eb996cf2d334e829a5644ea8e148af0a997ea28","unresolved":false,"context_lines":[{"line_number":1795,"context_line":"            if not context.is_admin:"},{"line_number":1796,"context_line":"                floating_ip_objs_admin \u003d l3_obj.FloatingIP.get_objects("},{"line_number":1797,"context_line":"                    context.elevated(), fixed_port_id\u003dport_id)"},{"line_number":1798,"context_line":"                if floating_ip_objs_admin !\u003d floating_ip_objs:"},{"line_number":1799,"context_line":"                    raise ovn_exc.FipAssociated(port_id\u003dport_id)"},{"line_number":1800,"context_line":""},{"line_number":1801,"context_line":"            router_ids \u003d {fip.router_id for fip in floating_ip_objs}"}],"source_content_type":"text/x-python","patch_set":7,"id":"3869a8b5_048edfb3","line":1798,"range":{"start_line":1798,"start_character":19,"end_line":1798,"end_character":62},"updated":"2023-05-26 07:53:57.000000000","message":"NOTE: Generally speaking, comparing arrays of objects can be a heavy operation potentially if there are many resources. However, one port can be associated to one floating IP, so the size of the arrays here (floating_ip_objs*) is 1 at most, so I am okay with this operation.","commit_id":"3946908810607bb8946034389b6c9f6435bcce77"}],"neutron/services/ovn_l3/exceptions.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"efc9c3dda501f1b162d2b2fd0c37dc14567bd6ef","unresolved":true,"context_lines":[{"line_number":24,"context_line":"# TODO(froyo): Move this exception to neutron-lib as soon as possible, and when"},{"line_number":25,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":26,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":27,"context_line":"    message \u003d _(\"Unable to complete operation on port %(port_id)s \""},{"line_number":28,"context_line":"                \"because port still has associated Floating IP.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"0a88ed02_0472ac7e","line":27,"updated":"2023-05-24 14:37:21.000000000","message":"operation -\u003e the operation","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"af4a45ee5013830ae9cd23b39848f658aee5c89b","unresolved":false,"context_lines":[{"line_number":24,"context_line":"# TODO(froyo): Move this exception to neutron-lib as soon as possible, and when"},{"line_number":25,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":26,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":27,"context_line":"    message \u003d _(\"Unable to complete operation on port %(port_id)s \""},{"line_number":28,"context_line":"                \"because port still has associated Floating IP.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"cd639e7a_8136d228","line":27,"in_reply_to":"0a88ed02_0472ac7e","updated":"2023-05-25 12:09:58.000000000","message":"ok, I will change here, and in the n-lib patch [1]. (but don\u0027t ask me to fix the other 9 except that will require the \u0027the\u0027 addition :P )\n\n[1] https://review.opendev.org/c/openstack/neutron-lib/+/883901","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"84b9ffc25fb7a9e4c38928f37497d1950f77bde7","unresolved":false,"context_lines":[{"line_number":24,"context_line":"# TODO(froyo): Move this exception to neutron-lib as soon as possible, and when"},{"line_number":25,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":26,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":27,"context_line":"    message \u003d _(\"Unable to complete operation on port %(port_id)s \""},{"line_number":28,"context_line":"                \"because port still has associated Floating IP.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"cb6f930f_5216b37d","line":27,"in_reply_to":"cd639e7a_8136d228","updated":"2023-05-26 07:55:29.000000000","message":"yeah, we can focus on the code we touch.","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"efc9c3dda501f1b162d2b2fd0c37dc14567bd6ef","unresolved":true,"context_lines":[{"line_number":25,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":26,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":27,"context_line":"    message \u003d _(\"Unable to complete operation on port %(port_id)s \""},{"line_number":28,"context_line":"                \"because port still has associated Floating IP.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"f2900d6c_97b0955d","line":28,"range":{"start_line":28,"start_character":51,"end_line":28,"end_character":62},"updated":"2023-05-24 14:37:21.000000000","message":"There is no need to capitalize a floating IP. Let\u0027s use \"floating IP\" consistenly in user-visible messages.\n\n  because the port is associated with a floating IP.","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"af4a45ee5013830ae9cd23b39848f658aee5c89b","unresolved":false,"context_lines":[{"line_number":25,"context_line":"# a new release is created and pointed to in the requirements remove this code."},{"line_number":26,"context_line":"class FipAssociated(n_exc.InUse):"},{"line_number":27,"context_line":"    message \u003d _(\"Unable to complete operation on port %(port_id)s \""},{"line_number":28,"context_line":"                \"because port still has associated Floating IP.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"a6db41ff_ae27405e","line":28,"range":{"start_line":28,"start_character":51,"end_line":28,"end_character":62},"in_reply_to":"f2900d6c_97b0955d","updated":"2023-05-25 12:09:58.000000000","message":"Done","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"}],"neutron/services/ovn_l3/plugin.py":[{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"1209fad577548cb9dc587f69e3e34fb4426793b9","unresolved":true,"context_lines":[{"line_number":313,"context_line":"            fips_tenant \u003d self.get_floatingips(context,"},{"line_number":314,"context_line":"                                               filters\u003d{\u0027port_id\u0027: [port_id]})"},{"line_number":315,"context_line":"            if fips !\u003d fips_tenant:"},{"line_number":316,"context_line":"                # NOTE(froyo): it is important to generate a precise 4XX error"},{"line_number":317,"context_line":"                # message that prompts the tenant user to contact an admin,"},{"line_number":318,"context_line":"                # rather than a 500 error message."},{"line_number":319,"context_line":"                raise ovn_l3_exc.FipAssociated(port_id\u003dport_id)"},{"line_number":320,"context_line":"        return fips"},{"line_number":321,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"6572e976_28caa23b","line":318,"range":{"start_line":316,"start_character":0,"end_line":318,"end_character":50},"updated":"2023-05-24 02:17:20.000000000","message":"Not sure if this comment is needed here. I also dont see where such message would be shown to the end user. Unless I am reading something wrong","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"be339df1ec2d411fb6477b070d9d87272dd0ef39","unresolved":true,"context_lines":[{"line_number":313,"context_line":"            fips_tenant \u003d self.get_floatingips(context,"},{"line_number":314,"context_line":"                                               filters\u003d{\u0027port_id\u0027: [port_id]})"},{"line_number":315,"context_line":"            if fips !\u003d fips_tenant:"},{"line_number":316,"context_line":"                # NOTE(froyo): it is important to generate a precise 4XX error"},{"line_number":317,"context_line":"                # message that prompts the tenant user to contact an admin,"},{"line_number":318,"context_line":"                # rather than a 500 error message."},{"line_number":319,"context_line":"                raise ovn_l3_exc.FipAssociated(port_id\u003dport_id)"},{"line_number":320,"context_line":"        return fips"},{"line_number":321,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"b02fdbda_1692f62d","line":318,"range":{"start_line":316,"start_character":0,"end_line":318,"end_character":50},"in_reply_to":"6572e976_28caa23b","updated":"2023-05-24 09:32:25.000000000","message":"The msg is associated to the new exception FipAssociated. This is the output of the command pre and post this patch (--debug flag):\n\nPre-patch:\nRESP: [500]\nRESP BODY: {\"NeutronError\": {\"type\": \"HTTPInternalServerError\", \"message\": \"Request Failed: internal server error while processing your request.\", \"detail\": \"\"}}\nFailed to delete port with name or ID \u0027...\u0027: HttpException: 500: Server Error for url: http://..., Request Failed: internal server error while processing your request.\n\n\nPost-patch:\nRESP: [409] \nRESP BODY: {\"NeutronError\": {\"type\": \"FipAssociated\", \"message\": \"Unable to complete operation on port ... because port still has associated Floating IP.\", \"detail\": \"\"}}\nFailed to delete port with name or ID \u0027...\u0027: ConflictException: 409: Client Error for url: http://..., Unable to complete operation on port ... because port still has associated Floating IP.\n\nBut anyway, the comment in code is mainly motivated to ensure that a future developer will understand that we send a particular exception to cover this use case, I would not link code comments to what we return to the user.","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"efc9c3dda501f1b162d2b2fd0c37dc14567bd6ef","unresolved":true,"context_lines":[{"line_number":321,"context_line":""},{"line_number":322,"context_line":"    def disassociate_floatingips(self, context, port_id, do_notify\u003dTrue):"},{"line_number":323,"context_line":"        # NOTE(froyo): To ensure that a FIP assigned by an admin user"},{"line_number":324,"context_line":"        # cannot be disassociated by a tenant user"},{"line_number":325,"context_line":"        fips \u003d self._get_floatingips_validating_access(context, port_id)"},{"line_number":326,"context_line":"        router_ids \u003d super(OVNL3RouterPlugin, self).disassociate_floatingips("},{"line_number":327,"context_line":"            context, port_id, do_notify)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1e201bf0_1b1737e3","line":324,"updated":"2023-05-24 14:37:21.000000000","message":"nit: I think this comment can be moved to _get_floatingips_validating_access() as this is the purpose of the new method.","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"af4a45ee5013830ae9cd23b39848f658aee5c89b","unresolved":false,"context_lines":[{"line_number":321,"context_line":""},{"line_number":322,"context_line":"    def disassociate_floatingips(self, context, port_id, do_notify\u003dTrue):"},{"line_number":323,"context_line":"        # NOTE(froyo): To ensure that a FIP assigned by an admin user"},{"line_number":324,"context_line":"        # cannot be disassociated by a tenant user"},{"line_number":325,"context_line":"        fips \u003d self._get_floatingips_validating_access(context, port_id)"},{"line_number":326,"context_line":"        router_ids \u003d super(OVNL3RouterPlugin, self).disassociate_floatingips("},{"line_number":327,"context_line":"            context, port_id, do_notify)"}],"source_content_type":"text/x-python","patch_set":5,"id":"c89733fe_db134fdf","line":324,"in_reply_to":"1e201bf0_1b1737e3","updated":"2023-05-25 12:09:58.000000000","message":"Done","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"efc9c3dda501f1b162d2b2fd0c37dc14567bd6ef","unresolved":true,"context_lines":[{"line_number":324,"context_line":"        # cannot be disassociated by a tenant user"},{"line_number":325,"context_line":"        fips \u003d self._get_floatingips_validating_access(context, port_id)"},{"line_number":326,"context_line":"        router_ids \u003d super(OVNL3RouterPlugin, self).disassociate_floatingips("},{"line_number":327,"context_line":"            context, port_id, do_notify)"},{"line_number":328,"context_line":"        for fip in fips:"},{"line_number":329,"context_line":"            router_id \u003d fip.get(\u0027router_id\u0027)"},{"line_number":330,"context_line":"            fixed_ip_address \u003d fip.get(\u0027fixed_ip_address\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"259ee5ca_305bd52e","line":327,"updated":"2023-05-24 14:37:21.000000000","message":"The class here OVNL3RouterPlugin inherits l3_db. L3_NAT_dbonly_mixin indirectly\n(more precisely, OVNL3RouterPlugin-\u003e l3_gwmode_db.L3_NAT_db_mixin-\u003e l3_gwmode_db.L3_NAT_dbonly_mixin-\u003e l3_db.L3_NAT_dbonly_mixin).\n\ndisassociate_floatingips() in L3_NAT_dbonly_mixin in l3_db.py and _get_floatingips_validating_access() here do the same check.\n\nIsn\u0027t the check in disassociate_floatingips() in L3_NAT_dbonly_mixin in l3_db.py enough?","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"af4a45ee5013830ae9cd23b39848f658aee5c89b","unresolved":false,"context_lines":[{"line_number":324,"context_line":"        # cannot be disassociated by a tenant user"},{"line_number":325,"context_line":"        fips \u003d self._get_floatingips_validating_access(context, port_id)"},{"line_number":326,"context_line":"        router_ids \u003d super(OVNL3RouterPlugin, self).disassociate_floatingips("},{"line_number":327,"context_line":"            context, port_id, do_notify)"},{"line_number":328,"context_line":"        for fip in fips:"},{"line_number":329,"context_line":"            router_id \u003d fip.get(\u0027router_id\u0027)"},{"line_number":330,"context_line":"            fixed_ip_address \u003d fip.get(\u0027fixed_ip_address\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"2297c664_650773ff","line":327,"in_reply_to":"259ee5ca_305bd52e","updated":"2023-05-25 12:09:58.000000000","message":"Yep, I think you are right, at this way we detect the issue before request the L3_DB plugin, but we can trust on the L3_DB raise exception and save one query removing the _get_floatingips_validating_access method (it does two select according to the tenant context and the elevated one). So let me remove the code from the OVNL3RouterPlugin. \n\ngood catch!","commit_id":"c16c4cae012aa671f2e7fecf80bd4bbdb669d9d4"}]}