)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a3abafe7_f96733b3","updated":"2025-10-24 15:08:37.000000000","message":"Logically LGTM, just some things that I think make it a little easier to follow the flow.","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"5341822c5ec2bb9147e390d3bbc06d32df4ebd47","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7b024621_0c9d5771","in_reply_to":"a3abafe7_f96733b3","updated":"2025-10-24 19:01:25.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"}],"ovn_bgp_agent/drivers/openstack/utils/wire.py":[{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":438,"context_line":"    columns[\u0027external_ids\u0027] \u003d {"},{"line_number":439,"context_line":"        constants.OVN_BGP_OWNER_EXT_ID_KEY:"},{"line_number":440,"context_line":"            constants.OVN_BGP_OWNER_EXT_ID_VALUE}"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"    return [ovn_idl.lr_policy_add(constants.OVN_CLUSTER_ROUTER, priority,"},{"line_number":443,"context_line":"                                  match, action, may_exist\u003dTrue, **columns)]"},{"line_number":444,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"49c1f74b_28fd3a30","line":441,"updated":"2025-10-24 15:08:37.000000000","message":"Given the below comment about `nexthop` being deprecated, shouldn\u0027t we always create `columns \u003d {\u0027nexthops\u0027: next_hops}` then we don\u0027t need the `else` and can also inline the `external_ids`:\n\n```suggestion\ncolumns \u003d {\n    \u0027nexthops\u0027: next_hops,\n    \u0027external_ids\": {\n        constants.OVN_BGP_OWNER_EXT_ID_KEY:\n            constants.OVN_BGP_OWNER_EXT_ID_VALUE\n    }\n}\n```","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":438,"context_line":"    columns[\u0027external_ids\u0027] \u003d {"},{"line_number":439,"context_line":"        constants.OVN_BGP_OWNER_EXT_ID_KEY:"},{"line_number":440,"context_line":"            constants.OVN_BGP_OWNER_EXT_ID_VALUE}"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"    return [ovn_idl.lr_policy_add(constants.OVN_CLUSTER_ROUTER, priority,"},{"line_number":443,"context_line":"                                  match, action, may_exist\u003dTrue, **columns)]"},{"line_number":444,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"46add154_2beee4ee","line":441,"in_reply_to":"49c1f74b_28fd3a30","updated":"2025-10-24 17:47:32.000000000","message":"Agreed, considering OVN prioritizes the plural version we can simply use it without using the singular form.\n\nhttps://github.com/ovn-org/ovn/blob/v25.09.0/northd/northd.c#L14068-L14070","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":573,"context_line":"        ).execute(check_error\u003dTrue)"},{"line_number":574,"context_line":"        router_uuid \u003d getattr(router_row, \u0027uuid\u0027, None)"},{"line_number":575,"context_line":"    except Exception:"},{"line_number":576,"context_line":"        router_row \u003d None"},{"line_number":577,"context_line":""},{"line_number":578,"context_line":"    def _is_cluster_router(rtr):"},{"line_number":579,"context_line":"        if not rtr:"}],"source_content_type":"text/x-python","patch_set":1,"id":"e2579bc6_e1363219","line":576,"updated":"2025-10-24 15:08:37.000000000","message":"I would turn this into a helper as well:\n\n```suggestion\ndef _discover_router() -\u003e str | None:\n    try:\n        row \u003d ovn_idl.lr_get(\n            constants.OVN_CLUSTER_ROUTER\n        ).execute(check_error\u003dTrue)\n        return getattr(row, \u0027uuid\u0027, None)\n    except Exception:\n        return None\n        \nrouter_uuid \u003d _discover_router()\n```","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":573,"context_line":"        ).execute(check_error\u003dTrue)"},{"line_number":574,"context_line":"        router_uuid \u003d getattr(router_row, \u0027uuid\u0027, None)"},{"line_number":575,"context_line":"    except Exception:"},{"line_number":576,"context_line":"        router_row \u003d None"},{"line_number":577,"context_line":""},{"line_number":578,"context_line":"    def _is_cluster_router(rtr):"},{"line_number":579,"context_line":"        if not rtr:"}],"source_content_type":"text/x-python","patch_set":1,"id":"fbca34b7_8c9daeca","line":576,"in_reply_to":"e2579bc6_e1363219","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":588,"context_line":"            return True"},{"line_number":589,"context_line":"        return False"},{"line_number":590,"context_line":""},{"line_number":591,"context_line":"    def _owned_by_agent(row):"},{"line_number":592,"context_line":"        ext_ids \u003d getattr(row, \u0027external_ids\u0027, None)"},{"line_number":593,"context_line":"        if not ext_ids:"},{"line_number":594,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"5b95215b_02b08b18","line":591,"updated":"2025-10-24 15:08:37.000000000","message":"Just to keep the naming consistant with boolean funcs:\n\n```suggestion\n    def _is_owned_by_agent(row):\n```","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":588,"context_line":"            return True"},{"line_number":589,"context_line":"        return False"},{"line_number":590,"context_line":""},{"line_number":591,"context_line":"    def _owned_by_agent(row):"},{"line_number":592,"context_line":"        ext_ids \u003d getattr(row, \u0027external_ids\u0027, None)"},{"line_number":593,"context_line":"        if not ext_ids:"},{"line_number":594,"context_line":"            return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"ac5d73c6_12e4deea","line":591,"in_reply_to":"5b95215b_02b08b18","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":632,"context_line":"                continue"},{"line_number":633,"context_line":"            if not _owned_by_agent(row):"},{"line_number":634,"context_line":"                continue"},{"line_number":635,"context_line":"            nexthops \u003d getattr(row, \u0027nexthops\u0027, None) or []"},{"line_number":636,"context_line":"            # The plural form wins when present and the"},{"line_number":637,"context_line":"            # singular one is deprecated."},{"line_number":638,"context_line":"            # https://github.com/ovn-org/ovn/blob/v25.09.0/northd/northd.c#L14068-L14070"}],"source_content_type":"text/x-python","patch_set":1,"id":"48d9fbe9_44632580","line":635,"updated":"2025-10-24 15:08:37.000000000","message":"I think this can be collapsed with the check below (i would still keep the comment):\n\n```suggestion\nrow_hops \u003d set(\n    getattr(\n        row, \u0027nexthops\u0027, getattr(row, \u0027nexthop\u0027, [])\n    )\n)\n```\n\nIts complicated enough that i think a small helper might be warranted to make it easier to read here:\n\n```\ndef _get_nexthops(row) -\u003e set[str]\n    # The plural form wins when present and the\n    # singular one is deprecated.\n    # https://github.com/ovn-org/ovn/blob/v25.09.0/northd/northd.c#L14068-L14070\n    return set(getattr(row, \u0027nexthops\u0027, getattr(row, \u0027nexthop\u0027, []))\n```","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":632,"context_line":"                continue"},{"line_number":633,"context_line":"            if not _owned_by_agent(row):"},{"line_number":634,"context_line":"                continue"},{"line_number":635,"context_line":"            nexthops \u003d getattr(row, \u0027nexthops\u0027, None) or []"},{"line_number":636,"context_line":"            # The plural form wins when present and the"},{"line_number":637,"context_line":"            # singular one is deprecated."},{"line_number":638,"context_line":"            # https://github.com/ovn-org/ovn/blob/v25.09.0/northd/northd.c#L14068-L14070"}],"source_content_type":"text/x-python","patch_set":1,"id":"5ad0f1b7_df513ecf","line":635,"in_reply_to":"48d9fbe9_44632580","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":665,"context_line":"            \u0027Logical_Router_Static_Route\u0027"},{"line_number":666,"context_line":"        ).execute(check_error\u003dTrue)"},{"line_number":667,"context_line":"    except Exception:"},{"line_number":668,"context_line":"        routes \u003d None"},{"line_number":669,"context_line":"        LOG.exception("},{"line_number":670,"context_line":"            \"Failed to list Logical_Router_Static_Route rows for %s\","},{"line_number":671,"context_line":"            constants.OVN_CLUSTER_ROUTER,"}],"source_content_type":"text/x-python","patch_set":1,"id":"cd0d658f_1fa197a2","line":668,"updated":"2025-10-24 15:08:37.000000000","message":"This could be \n```suggestion\n        routes \u003d []\n```\n\nand then the `if routes` check below is not needed.","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":665,"context_line":"            \u0027Logical_Router_Static_Route\u0027"},{"line_number":666,"context_line":"        ).execute(check_error\u003dTrue)"},{"line_number":667,"context_line":"    except Exception:"},{"line_number":668,"context_line":"        routes \u003d None"},{"line_number":669,"context_line":"        LOG.exception("},{"line_number":670,"context_line":"            \"Failed to list Logical_Router_Static_Route rows for %s\","},{"line_number":671,"context_line":"            constants.OVN_CLUSTER_ROUTER,"}],"source_content_type":"text/x-python","patch_set":1,"id":"dd4b797a_b8f81ebd","line":668,"in_reply_to":"cd0d658f_1fa197a2","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":675,"context_line":"            if getattr(row, \u0027ip_prefix\u0027, None) !\u003d \u00270.0.0.0/0\u0027:"},{"line_number":676,"context_line":"                continue"},{"line_number":677,"context_line":"            logical_router \u003d next("},{"line_number":678,"context_line":"                iter(getattr(row, \u0027logical_router\u0027, [])), None)"},{"line_number":679,"context_line":"            if not _is_cluster_router(logical_router):"},{"line_number":680,"context_line":"                continue"},{"line_number":681,"context_line":"            if not _owned_by_agent(row):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5c17bbdd_c99a87d4","line":678,"updated":"2025-10-24 15:08:37.000000000","message":"this iterator extraction is duplicated with above, I think making it a helper will also make it easier to understand:\n\n```suggestion\nlogical_router \u003d _get_logical_router(row)\n```","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":675,"context_line":"            if getattr(row, \u0027ip_prefix\u0027, None) !\u003d \u00270.0.0.0/0\u0027:"},{"line_number":676,"context_line":"                continue"},{"line_number":677,"context_line":"            logical_router \u003d next("},{"line_number":678,"context_line":"                iter(getattr(row, \u0027logical_router\u0027, [])), None)"},{"line_number":679,"context_line":"            if not _is_cluster_router(logical_router):"},{"line_number":680,"context_line":"                continue"},{"line_number":681,"context_line":"            if not _owned_by_agent(row):"}],"source_content_type":"text/x-python","patch_set":1,"id":"666b0730_16deb902","line":678,"in_reply_to":"5c17bbdd_c99a87d4","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":758,"context_line":"                ip, cr_lrp_port,"},{"line_number":759,"context_line":"            )"},{"line_number":760,"context_line":""},{"line_number":761,"context_line":"    return True"},{"line_number":762,"context_line":""},{"line_number":763,"context_line":""},{"line_number":764,"context_line":"def _cleanup_wiring_underlay(idl, bridge_mappings, ovs_flows, exposed_ips,"}],"source_content_type":"text/x-python","patch_set":1,"id":"d82cc278_cb82a108","line":761,"updated":"2025-10-24 15:08:37.000000000","message":"We only ever `return True` so i\u0027m not sure what the use of adding it to the  cleanup phase is.","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"da9d1faf9e23849d299c59945b6fa1d640cc7e4d","unresolved":false,"context_lines":[{"line_number":758,"context_line":"                ip, cr_lrp_port,"},{"line_number":759,"context_line":"            )"},{"line_number":760,"context_line":""},{"line_number":761,"context_line":"    return True"},{"line_number":762,"context_line":""},{"line_number":763,"context_line":""},{"line_number":764,"context_line":"def _cleanup_wiring_underlay(idl, bridge_mappings, ovs_flows, exposed_ips,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3b0bfff4_bb6bab72","line":761,"in_reply_to":"d82cc278_cb82a108","updated":"2025-10-24 17:47:32.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"}],"ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_wire.py":[{"author":{"_account_id":385,"name":"Jason Kölker","email":"jason@koelker.net","username":"jason-koelker"},"change_message_id":"0d43bb6e636d5d89e1b6db1668d5fffb424e343f","unresolved":true,"context_lines":[{"line_number":254,"context_line":"            columns \u003d {\u0027nexthops\u0027: next_hops}"},{"line_number":255,"context_line":"        elif len(next_hops) \u003d\u003d 1:"},{"line_number":256,"context_line":"            columns \u003d {\u0027nexthop\u0027: next_hops[0]}"},{"line_number":257,"context_line":"        else:"},{"line_number":258,"context_line":"            columns \u003d {}"},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"        columns[\u0027external_ids\u0027] \u003d {"}],"source_content_type":"text/x-python","patch_set":1,"id":"fd128614_3c6d6bff","line":257,"updated":"2025-10-24 15:08:37.000000000","message":"Same here with `nexthop`","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"},{"author":{"_account_id":24824,"name":"Dmitrii Shcherbakov","username":"dmitriis"},"change_message_id":"5341822c5ec2bb9147e390d3bbc06d32df4ebd47","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            columns \u003d {\u0027nexthops\u0027: next_hops}"},{"line_number":255,"context_line":"        elif len(next_hops) \u003d\u003d 1:"},{"line_number":256,"context_line":"            columns \u003d {\u0027nexthop\u0027: next_hops[0]}"},{"line_number":257,"context_line":"        else:"},{"line_number":258,"context_line":"            columns \u003d {}"},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"        columns[\u0027external_ids\u0027] \u003d {"}],"source_content_type":"text/x-python","patch_set":1,"id":"a3fe8e64_57f4482a","line":257,"in_reply_to":"fd128614_3c6d6bff","updated":"2025-10-24 19:01:25.000000000","message":"Acknowledged","commit_id":"3229a533a54694c1386efd9d9b31b756af88ee66"}]}