)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"59937f152d996536a054f83186206c762cd4894a","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":8,"id":"4b664b3c_afc282a5","updated":"2026-05-26 09:47:12.000000000","message":"Please fix that imports in one of the files and it is good for me. But I am not FRR nor BGP EVPN expert so I will rely on the reviews from others for sure :)","commit_id":"cad7e2d4acd26ccc8fa4f2ee7b2f8975537808fd"}],"neutron/agent/linux/evpn_router/frr/frr_driver.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"ba1baeec1a0e488826f704a9a21f4565b2666697","unresolved":true,"context_lines":[{"line_number":151,"context_line":"        return std_err in (\u0027\u0027, f\u0027Cannot find device \"{vrf_name}\u0027)"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"class FrrVtyshDriver(interface.EVPNRouterDriver):"},{"line_number":155,"context_line":"    \"\"\"Apply EVPN configuration via vtysh."},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    Uses EVPNRouterConfig for portable fields."}],"source_content_type":"text/x-python","patch_set":1,"id":"efd71746_34e4230f","line":154,"updated":"2026-05-11 18:33:54.000000000","message":"This class seems almost stateless except the vrf_handler. The whole code structure seems to me rather procedural than OOP. I think there are some good design patterns to generate a config (file). Just by a quick look I think the most suitable is the Builder Pattern: https://en.wikipedia.org/wiki/Builder_pattern\n\nWith that, I think it can be easier to extend its functionality to be able to provide custom config files and the code would be more readable as it would look something like\n```\nFrrConfigBuilder().add_router(asn\u003d1234).add_address_family(param1, param2).add_interface()\n```\n\nThis would be more object-oriented and each section can be extended as we like.","commit_id":"e3e1484dee7922890ad17a2a86acf0c7f5697a35"},{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"2cff442abe7152aeba2e697b6372d9f278f408b0","unresolved":false,"context_lines":[{"line_number":151,"context_line":"        return std_err in (\u0027\u0027, f\u0027Cannot find device \"{vrf_name}\u0027)"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"class FrrVtyshDriver(interface.EVPNRouterDriver):"},{"line_number":155,"context_line":"    \"\"\"Apply EVPN configuration via vtysh."},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    Uses EVPNRouterConfig for portable fields."}],"source_content_type":"text/x-python","patch_set":1,"id":"08948af9_3c253ebe","line":154,"in_reply_to":"efd71746_34e4230f","updated":"2026-05-19 19:47:20.000000000","message":"Done","commit_id":"e3e1484dee7922890ad17a2a86acf0c7f5697a35"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"5405dde801345dea985fb758173f1ae5ff2d0938","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"a2bedd35_4a65968a","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"updated":"2026-05-21 20:18:48.000000000","message":"Is this still necessary if this is called from the FSM?","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"9eed2db638aa84df73331fd7ae708ca27e038d5e","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"2a9892a5_1eae9273","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"in_reply_to":"042137fb_6af7bf2b","updated":"2026-05-21 20:54:37.000000000","message":"Even if generic, this is also unnecessary, at least not for FRR\u0027s benefit.  FRR listens for VRF creation/deletion via netlink and can manage without the VRF existing.","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"d974a7b63ee6b834e43bffc05d047979c86d5a2f","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"800d4909_93833f40","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"in_reply_to":"2a9892a5_1eae9273","updated":"2026-05-21 23:43:38.000000000","message":"Through my experimentation I discovered that frr does care about the state of vrf on a system. For example, if you try to remove `evpnvrf-11` reference from frr.config but `evpnvrf-11` vrf still exists, you will get following error\n```\n1097741|zebra] done\n% Configuration failed.\n\ncommit failed session-id 15 on Unknown-FD-16 req-id 3 source-ds: candidate target-ds: running validate-only: 0: reason: \u0027Failed to create cfgdata: Only inactive VRFs can be deleted\u0027\n```\nI encountered similar error when for example I was trying to add vrf into frr.config but that vrf did not exists.\n\nThat is the reason I created the `EVPNRouterVrfHandler`. It basically defines a contract that you must pass to FrrVtyshDriver which will handle vrf.\n\nFor example, in functional testing I will do something like this(simplification):\n```\nclass FunctionalTestVRFHandler(interface.VRFHandler):\n    def ensure_vrf_exists(self, vrf_name):\n        utils.execute([ip, link, add, type, vrf, {vrf_name}])\n\n    def ensure_vrf_deleted(self, vrf_name):\n         utils.execute([ip, link, del, {vrf_name}])\n```\n\nIn FSM case, I might just create a dummy handler since the FSM state machine will make sure that `frr_driver.create_evpn_router` is always called after vrf netlink was detected\n\n```\nclass FSMEVPNRouterVrfHandler(interface.VRFHandle):\n    def ensure_vrf_exists(self, vrf_name):\n        # just pass, FSM state enforces that vrf present and active\n        pass\n\n    def ensure_vrf_deleted(self, vrf_name):\n        pass\n\n```\n\nThis implementation keeps my `FrrVtyshDriver` generic and can be used by anyone.\n\nBut I am open to other implementation, this is just my take on it","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"25983533a3722bd314d2009a64a4bb8e852e1c72","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"788e44fe_61180f7c","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"in_reply_to":"800d4909_93833f40","updated":"2026-05-29 19:08:54.000000000","message":"FRR keeps two different states for a VRF, VRF_ACTIVE (in kernel) and VRF_CONFIGURED (in frr.conf).  FRR monitors VRF creation by listening for netlink messages.  When FRR learns through netlink that a VRF is created, FRR adds the vrf configuration.  FRR does not allow for the vrf configuration to be deleted if the VRF devices still exists in the kernel.","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"25983533a3722bd314d2009a64a4bb8e852e1c72","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1e7a92a0_847d177e","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"in_reply_to":"800d4909_93833f40","updated":"2026-05-29 19:08:54.000000000","message":"On my setup, when the VRF device does not exist in the kernel, FRR allows for VRF configuration to be added.\n\nHowever, there seems to be some asymmetry on delete.  For each VRF, FRR keeps two states, VRF_ACTIVE and VRF_CONFIGURED.  Both states need to be unset for the VRF configuration to be deleted.\n\nFRR monitors netlink messages for VRF create and delete.  Once FRR detects that a VRF is created, FRR adds the VRF configuration automatically, sets the VRF state to VRF_ACTIVE and only unsets it until the VRF device is deleted from the kernel. This VRF that FRR automatically adds to it configuration does not have the state VRF_CONFIGURED.  So, FRR complains when using configuration to delete the VRF at this point, when VRF_ACTIVE but not VRF_CONFIGURED.\n\nWhen the VRF device exists in the kernel, triggering FRR to automatically add the VRF configuration, and the user subsequently adds the VNI configuration to the VRF (first entering the VRF context and then adding VNI configuration), the VRF is considered VRF_CONFIGURED.  At this point, the VRF is both VRF_ACTIVE and VRF_CONFIGURED.  At this point, if FRR detects that the VRF device is removed, FRR does not complain but does not remove the VRF configuration.\n\nSo on my setup, create can occur in either order, VRF create in the kernel first or VRF create in FRR configuration first.  Clean delete can only occur in one order, VRF delete from the kernel first followed by VRF delete from FRR configuration.\n\nIMO, whether to check for VRF device in the kernel depends on what we want the code to be.  If this code is purely intended to transmit configuration to FRR, then it shouldn\u0027t check for VRF device.  If this code is intended configure certain things, then it should check for VRF device.","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"957f7d9356cd4c6d238b03555154d734de28fe81","unresolved":true,"context_lines":[{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def create_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":272,"context_line":"        LOG.debug(\"Creating EVPN router: %s\", config)"},{"line_number":273,"context_line":"        self.vrf_handler.ensure_vrf_exists(config.vrf_name)"},{"line_number":274,"context_line":"        self._create_bgp_router(config)"},{"line_number":275,"context_line":"        add_evpn_router_cmd \u003d self.cmd_builder.add_evpn_router_cmds(config)"},{"line_number":276,"context_line":"        self.executor.execute_cmds(add_evpn_router_cmd)"}],"source_content_type":"text/x-python","patch_set":4,"id":"042137fb_6af7bf2b","line":273,"range":{"start_line":273,"start_character":8,"end_line":273,"end_character":59},"in_reply_to":"a2bedd35_4a65968a","updated":"2026-05-21 20:20:46.000000000","message":"Although this driver will be used by FSM, I wanted to keep it generic unaware who will use it.","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"5405dde801345dea985fb758173f1ae5ff2d0938","unresolved":true,"context_lines":[{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    def delete_evpn_router(self, config: interface.EVPNRouterConfig) -\u003e None:"},{"line_number":279,"context_line":"        LOG.debug(\"Deleting EVPN router: %s\", config)"},{"line_number":280,"context_line":"        self.vrf_handler.ensure_vrf_deleted(config.vrf_name)"},{"line_number":281,"context_line":"        self.executor.execute_cmds("},{"line_number":282,"context_line":"            self.cmd_builder.delete_evpn_router_cmds(config))"}],"source_content_type":"text/x-python","patch_set":4,"id":"011fea84_a8669e53","line":280,"range":{"start_line":280,"start_character":8,"end_line":280,"end_character":60},"updated":"2026-05-21 20:18:48.000000000","message":"Same question. Is this still necessary if this is called from the FSM?","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"1f7dfe067dd5f8d4f86cc62b32685516e6015a31","unresolved":true,"context_lines":[{"line_number":162,"context_line":""},{"line_number":163,"context_line":"class FrrVtyshExecutor:"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def __init__(self, namespace: str | None \u003d None):"},{"line_number":166,"context_line":"        self._namespace \u003d namespace"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _execute_vtysh(self, vtysh_args: list[str]) -\u003e str:"}],"source_content_type":"text/x-python","patch_set":5,"id":"c3eed4ad_5398960b","line":165,"range":{"start_line":165,"start_character":22,"end_line":165,"end_character":51},"updated":"2026-05-23 02:57:07.000000000","message":"I am undecided if passing namespace here is a good idea. The namespace here is purely because tests run frr in a namespace. In production we will run frr in the default namespace. I dont like mixing \"test business\" in production code. What is your thought about it.\n\nAnother idea would be to overwrite `FrrVtyshExecutor` class which just adds namespace, something like this\n\n```\nclass FrrVtyshExecutorNamespaced(FrrVtyshExecutor):\n      def __init__(self, namespace):\n          super().__init__(additional_args\u003d[\u0027-N\u0027, namespace])\n```\n\nor another idea is to simply define a generic method in `FrrVtyshExecutor` which takes any  additional arguments that get appended to each command.","commit_id":"80d22355030d5ea9618c296880c388d799f2f588"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"59937f152d996536a054f83186206c762cd4894a","unresolved":true,"context_lines":[{"line_number":162,"context_line":""},{"line_number":163,"context_line":"class FrrVtyshExecutor:"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def __init__(self, namespace: str | None \u003d None):"},{"line_number":166,"context_line":"        self._namespace \u003d namespace"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _execute_vtysh(self, vtysh_args: list[str]) -\u003e str:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9c575085_d780a699","line":165,"range":{"start_line":165,"start_character":22,"end_line":165,"end_character":51},"in_reply_to":"c3eed4ad_5398960b","updated":"2026-05-26 09:47:12.000000000","message":"I\u0027m fine either way","commit_id":"80d22355030d5ea9618c296880c388d799f2f588"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"59937f152d996536a054f83186206c762cd4894a","unresolved":true,"context_lines":[{"line_number":25,"context_line":"from neutron.agent.linux import utils as linux_utils"},{"line_number":26,"context_line":"from neutron_lib import exceptions"},{"line_number":27,"context_line":"from oslo_log import log as logging"},{"line_number":28,"context_line":"from oslo_serialization import jsonutils"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"LOG \u003d logging.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":8,"id":"b706aa77_c417289e","line":28,"updated":"2026-05-26 09:47:12.000000000","message":"please divide imports into 3 different groups: imports from stdlib, import of 3rd party libs and finally imports of neutron modules. This is common pattern we use in Neutron and other OpenStack projects also","commit_id":"cad7e2d4acd26ccc8fa4f2ee7b2f8975537808fd"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"59937f152d996536a054f83186206c762cd4894a","unresolved":true,"context_lines":[{"line_number":191,"context_line":"            f.flush()"},{"line_number":192,"context_line":"            temp_path \u003d f.name"},{"line_number":193,"context_line":"            try:"},{"line_number":194,"context_line":"                self._execute_vtysh([\u0027--dryrun\u0027, \u0027-f\u0027, temp_path])"},{"line_number":195,"context_line":"            except exceptions.ProcessExecutionError as err:"},{"line_number":196,"context_line":"                raise frr_exceptions.FrrDryrunError("},{"line_number":197,"context_line":"                    \"FRR syntatic validity failed for \""}],"source_content_type":"text/x-python","patch_set":8,"id":"c611de1b_0e1a287e","line":194,"updated":"2026-05-26 09:47:12.000000000","message":"do we need to run that test always? Maybe only if debug is enabled or something like that?","commit_id":"cad7e2d4acd26ccc8fa4f2ee7b2f8975537808fd"}],"neutron/agent/linux/evpn_router/frr/templates.py":[{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"5405dde801345dea985fb758173f1ae5ff2d0938","unresolved":true,"context_lines":[{"line_number":103,"context_line":"EVPN_AF_IPV4_UNICAST \u003d \"\"\"\\"},{"line_number":104,"context_line":" address-family ipv4 unicast"},{"line_number":105,"context_line":"  redistribute kernel"},{"line_number":106,"context_line":"  redistribute connected"},{"line_number":107,"context_line":" exit-address-family"},{"line_number":108,"context_line":"\"\"\""},{"line_number":109,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"c0e49424_1eb0da4d","line":106,"range":{"start_line":106,"start_character":2,"end_line":106,"end_character":24},"updated":"2026-05-21 20:18:48.000000000","message":"Is this necessary?","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"},{"author":{"_account_id":38298,"name":"Helen Chen","display_name":"Helen Chen","email":"ichen@redhat.com","username":"ingwherchen"},"change_message_id":"5405dde801345dea985fb758173f1ae5ff2d0938","unresolved":true,"context_lines":[{"line_number":110,"context_line":"EVPN_AF_IPV6_UNICAST \u003d \"\"\"\\"},{"line_number":111,"context_line":" address-family ipv6 unicast"},{"line_number":112,"context_line":"  redistribute kernel"},{"line_number":113,"context_line":"  redistribute connected"},{"line_number":114,"context_line":" exit-address-family"},{"line_number":115,"context_line":"\"\"\""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"4a3c32d7_a43a38e6","line":113,"range":{"start_line":113,"start_character":2,"end_line":113,"end_character":24},"updated":"2026-05-21 20:18:48.000000000","message":"Is this necessary?","commit_id":"a8fbd7681b2c290f3f06e17b7c06db1735551e1e"}]}