)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b74af77d5f25598fe15ffce9b7fb4b8a03ad2a46","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"f619cb93_3e7b0472","updated":"2024-08-27 19:25:36.000000000","message":"Is there any maintenance task work that needs to be done as well?","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"88c24e37b4840479746657cebf4f41a8d2334e60","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"f2f8ce14_f80fdf5d","updated":"2024-08-30 00:14:12.000000000","message":"Thanks for the review! Please see my responses in line","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"faf543315b964a693414611dd434a9bea04d0182","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"fd3cdfd4_4ced7f61","updated":"2024-08-29 09:08:33.000000000","message":"This change deserves:\n* A LP bug, to make a short description of this feature.\n* A release note.","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"548528d3db557c1db625749aa9fd992a74512210","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"ba732212_37e72d05","in_reply_to":"f619cb93_3e7b0472","updated":"2024-08-27 20:04:27.000000000","message":"We merged it already https://review.opendev.org/c/openstack/neutron/+/918151","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"56fe53fb7a6fdb3704d3de82360f14ed6e4fa5a9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"fa915553_a9142ad3","updated":"2024-09-03 11:08:55.000000000","message":"Thanks Miguel","commit_id":"44cbbba369ad12bfdc8276319c7bcea173ddaa96"}],"neutron/services/ovn_l3/service_providers/user_defined.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"a7b1a94132f664732b265f8f3f90d6cfb412771a","unresolved":true,"context_lines":[{"line_number":228,"context_line":"                                    context, router_id\u003drouter_id,"},{"line_number":229,"context_line":"                                    port_type\u003dconst.DEVICE_OWNER_ROUTER_INTF)]"},{"line_number":230,"context_line":"            router_ports \u003d [self.l3plugin._plugin.get_port(context, pid)"},{"line_number":231,"context_line":"                            for pid in router_ports_ids]"},{"line_number":232,"context_line":"        for rp in router_ports:"},{"line_number":233,"context_line":"            for ip in rp[\u0027fixed_ips\u0027]:"},{"line_number":234,"context_line":"                if ip[\u0027subnet_id\u0027] \u003d\u003d subnet_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"1cc64c20_880e9750","line":231,"updated":"2024-08-23 15:13:36.000000000","message":"I think this is fine, I\u0027m just trying to think if we need to make the get_port() call given we have the RP object? And I\u0027m not a DB expert 😊\n\nLooking at a few other places it looks like this:\n\n    objs \u003d router.RouterPort.get_objects(\n        context, router_id\u003drouter_id,\n        port_type\u003dconst.DEVICE_OWNER_ROUTER_INTF)\n\n    router_ports \u003d [self.l3_plugin._plugin._make_port_dict(rp.db_obj.port)\n                    for rp in objs]\n\nTo me that looks like it saves one DB transaction, right?","commit_id":"aeed73b63752de5cc8b75620e4c1071b7f457fb5"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"53feb4dded2f69f5d2ece8f6b027f5e16bdc2167","unresolved":false,"context_lines":[{"line_number":228,"context_line":"                                    context, router_id\u003drouter_id,"},{"line_number":229,"context_line":"                                    port_type\u003dconst.DEVICE_OWNER_ROUTER_INTF)]"},{"line_number":230,"context_line":"            router_ports \u003d [self.l3plugin._plugin.get_port(context, pid)"},{"line_number":231,"context_line":"                            for pid in router_ports_ids]"},{"line_number":232,"context_line":"        for rp in router_ports:"},{"line_number":233,"context_line":"            for ip in rp[\u0027fixed_ips\u0027]:"},{"line_number":234,"context_line":"                if ip[\u0027subnet_id\u0027] \u003d\u003d subnet_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"94c742c1_c04a77f2","line":231,"in_reply_to":"1cc64c20_880e9750","updated":"2024-08-25 23:09:10.000000000","message":"Yes! Good optimization. I added LOG statements to compares both ways to get router_ports and I get the same result: https://paste.openstack.org/show/bN6cfob50MwypYq1rD6Q/.","commit_id":"aeed73b63752de5cc8b75620e4c1071b7f457fb5"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"faf543315b964a693414611dd434a9bea04d0182","unresolved":true,"context_lines":[{"line_number":250,"context_line":"        # If the revision row exists, the interface is being removed by port"},{"line_number":251,"context_line":"        # and the port has fixed ips in more than one subnet, then the port has"},{"line_number":252,"context_line":"        # been recreated in a previous notification of this event. Do nothing"},{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"}],"source_content_type":"text/x-python","patch_set":9,"id":"9d32889d_b22934ee","line":254,"range":{"start_line":253,"start_character":8,"end_line":254,"end_character":18},"updated":"2024-08-29 09:08:33.000000000","message":"Why not checking the LSP itself?","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"26d62e1fbd9aaf4d0c0393551aa8e5ac4e97efa2","unresolved":true,"context_lines":[{"line_number":250,"context_line":"        # If the revision row exists, the interface is being removed by port"},{"line_number":251,"context_line":"        # and the port has fixed ips in more than one subnet, then the port has"},{"line_number":252,"context_line":"        # been recreated in a previous notification of this event. Do nothing"},{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"}],"source_content_type":"text/x-python","patch_set":9,"id":"bd24b8cb_725a87b8","line":254,"range":{"start_line":253,"start_character":8,"end_line":254,"end_character":18},"in_reply_to":"1a39851d_c0c48c62","updated":"2024-08-30 14:07:43.000000000","message":"Using the nb_idl from the ovn client is perfectly fine, we do this in the code right now (in the OVNMechanismDriver).\n\nThe revision number is a register in the Neutron DB; the LSP is on the OVN DB. Not only is faster to do a lookup in OVN DB but also safer. So you can directly use the code you provided.","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"88c24e37b4840479746657cebf4f41a8d2334e60","unresolved":true,"context_lines":[{"line_number":250,"context_line":"        # If the revision row exists, the interface is being removed by port"},{"line_number":251,"context_line":"        # and the port has fixed ips in more than one subnet, then the port has"},{"line_number":252,"context_line":"        # been recreated in a previous notification of this event. Do nothing"},{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"}],"source_content_type":"text/x-python","patch_set":9,"id":"1a39851d_c0c48c62","line":254,"range":{"start_line":253,"start_character":8,"end_line":254,"end_character":18},"in_reply_to":"9d32889d_b22934ee","updated":"2024-08-30 00:14:12.000000000","message":"In your comment to line 256 below you point out, quite correctly, that \n _ovn_client.create_port creates the initial revision. In fact, _ovn_client.create_port accomplishes the creation of the initial revision by calling ovn_revision_numbers_db.bump_revision as its very last step, after the transaction to create the LSP has been committed to the NBDB. So the existence of the revision number is a reliable indication that the corresponding LSP also exists. \n\nAdditionally, to retrieve the revision number, there is the straightforward call in line 253 just above. I tried to find something as straightforward in the code base to retrieve the LSP and didn\u0027t find anything. So I looked at the ovn_client here: https://github.com/openstack/neutron/blob/d73cc2eff6481b8b02a608c57aa204006a0f8b4c/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py#L726. Translating this to my code, it would look as:\n\n    nbdb_idl \u003d self.l3plugin._ovn_client._nb_idl\n    if nbdb_idl.lookup(\u0027Logical_Switch_Port\u0027, port[\u0027id\u0027], default\u003dNone):\n        return\n        \nIs this what you had in mind? If yes, how is this better than what I already have? If not, what did you have in mind? I\u0027ll gladly add any suggestion that is an improvement on what I have.\n\nEven more, is this driver\u0027s level of abstraction the right place to be making calls to the nbdb idl, bypassing the ovn client?","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"d808350dd773735f8d150c4dc5c9656413dc497a","unresolved":false,"context_lines":[{"line_number":250,"context_line":"        # If the revision row exists, the interface is being removed by port"},{"line_number":251,"context_line":"        # and the port has fixed ips in more than one subnet, then the port has"},{"line_number":252,"context_line":"        # been recreated in a previous notification of this event. Do nothing"},{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a312c4f_56554dc7","line":254,"range":{"start_line":253,"start_character":8,"end_line":254,"end_character":18},"in_reply_to":"bd24b8cb_725a87b8","updated":"2024-08-30 15:01:10.000000000","message":"Done","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"faf543315b964a693414611dd434a9bea04d0182","unresolved":true,"context_lines":[{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"},{"line_number":258,"context_line":"            std_attr_id\u003dport[\u0027standard_attr_id\u0027])"},{"line_number":259,"context_line":"        self.l3plugin._ovn_client.create_port(context, port)"},{"line_number":260,"context_line":"        LOG.debug(\u0027Recreated OVN LSP for port with id %s before removing \u0027"},{"line_number":261,"context_line":"                  \u0027interface for user defined flavor router with id %s\u0027,"}],"source_content_type":"text/x-python","patch_set":9,"id":"e6f576e5_b765a973","line":258,"range":{"start_line":256,"start_character":8,"end_line":258,"end_character":49},"updated":"2024-08-29 09:08:33.000000000","message":"Why is this needed? The ``_ovn_client.create_port`` creates the initial revision.","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"88c24e37b4840479746657cebf4f41a8d2334e60","unresolved":false,"context_lines":[{"line_number":253,"context_line":"        if ovn_revision_numbers_db.get_revision_row(context, port[\u0027id\u0027]):"},{"line_number":254,"context_line":"            return"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"        ovn_revision_numbers_db.create_initial_revision("},{"line_number":257,"context_line":"            context, port[\u0027id\u0027], ovn_const.TYPE_PORTS,"},{"line_number":258,"context_line":"            std_attr_id\u003dport[\u0027standard_attr_id\u0027])"},{"line_number":259,"context_line":"        self.l3plugin._ovn_client.create_port(context, port)"},{"line_number":260,"context_line":"        LOG.debug(\u0027Recreated OVN LSP for port with id %s before removing \u0027"},{"line_number":261,"context_line":"                  \u0027interface for user defined flavor router with id %s\u0027,"}],"source_content_type":"text/x-python","patch_set":9,"id":"ee35c857_24fd8658","line":258,"range":{"start_line":256,"start_character":8,"end_line":258,"end_character":49},"in_reply_to":"e6f576e5_b765a973","updated":"2024-08-30 00:14:12.000000000","message":"You are right. Removed","commit_id":"d9f838ef20638cbdc7df545114cfa641be4c433e"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"080d112ca9cbb61fa1d603371f36aca5b03ebc89","unresolved":true,"context_lines":[{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    @registry.receives(resources.ROUTER_INTERFACE, [events.AFTER_CREATE])"},{"line_number":207,"context_line":"    def _process_add_router_interface(self, resource, event, trigger, payload):"},{"line_number":208,"context_line":"        router \u003d payload.states[0]"},{"line_number":209,"context_line":"        context \u003d payload.context"},{"line_number":210,"context_line":"        if not self._is_user_defined_provider(context, router):"},{"line_number":211,"context_line":"            return"},{"line_number":212,"context_line":"        port \u003d payload.metadata[\u0027port\u0027]"},{"line_number":213,"context_line":"        subnets \u003d payload.metadata[\u0027subnets\u0027]"},{"line_number":214,"context_line":"        router_interface_info \u003d self.l3plugin._make_router_interface_info("},{"line_number":215,"context_line":"            router.id, port[\u0027tenant_id\u0027], port[\u0027id\u0027], port[\u0027network_id\u0027],"},{"line_number":216,"context_line":"            subnets[-1][\u0027id\u0027], [subnet[\u0027id\u0027] for subnet in subnets])"},{"line_number":217,"context_line":"        self.l3plugin._ovn_client.delete_port(context, port[\u0027id\u0027],"},{"line_number":218,"context_line":"                                              port_object\u003dport)"},{"line_number":219,"context_line":"        LOG.debug(\u0027Got request to add interface %s to a user defined flavor \u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"d6351c81_0bb924dd","line":216,"range":{"start_line":208,"start_character":8,"end_line":216,"end_character":68},"updated":"2024-08-30 14:27:43.000000000","message":"nitty nit: this all is the same as in super()._process_add_router_interface(...)","commit_id":"31e5697e476e38ec85a34ae06e3fa21183c7e25e"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"87370e834367d90caa8a3ecce8cc37014086afb3","unresolved":false,"context_lines":[{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    @registry.receives(resources.ROUTER_INTERFACE, [events.AFTER_CREATE])"},{"line_number":207,"context_line":"    def _process_add_router_interface(self, resource, event, trigger, payload):"},{"line_number":208,"context_line":"        router \u003d payload.states[0]"},{"line_number":209,"context_line":"        context \u003d payload.context"},{"line_number":210,"context_line":"        if not self._is_user_defined_provider(context, router):"},{"line_number":211,"context_line":"            return"},{"line_number":212,"context_line":"        port \u003d payload.metadata[\u0027port\u0027]"},{"line_number":213,"context_line":"        subnets \u003d payload.metadata[\u0027subnets\u0027]"},{"line_number":214,"context_line":"        router_interface_info \u003d self.l3plugin._make_router_interface_info("},{"line_number":215,"context_line":"            router.id, port[\u0027tenant_id\u0027], port[\u0027id\u0027], port[\u0027network_id\u0027],"},{"line_number":216,"context_line":"            subnets[-1][\u0027id\u0027], [subnet[\u0027id\u0027] for subnet in subnets])"},{"line_number":217,"context_line":"        self.l3plugin._ovn_client.delete_port(context, port[\u0027id\u0027],"},{"line_number":218,"context_line":"                                              port_object\u003dport)"},{"line_number":219,"context_line":"        LOG.debug(\u0027Got request to add interface %s to a user defined flavor \u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"eb4ad443_d15277d1","line":216,"range":{"start_line":208,"start_character":8,"end_line":216,"end_character":68},"in_reply_to":"d6351c81_0bb924dd","updated":"2024-08-30 15:07:23.000000000","message":"Yeah, I could refactor this, but how to handle the return in the middle without complicating it. I originally went for readability here. I\u0027ll pass. Thanks anyway","commit_id":"31e5697e476e38ec85a34ae06e3fa21183c7e25e"}]}