)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c0315f0b67fa311af2563f9028e2bea8bf940324","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b7f3e975_11f22867","updated":"2023-11-15 01:23:18.000000000","message":"recheck sqlalchemy job is voting now\n\nI think you should file a bug for this explaining it a little, since if it\u0027s a big win we could backport it.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":35825,"name":"Adam Oswick","email":"adam@adamoswick.co.uk","username":"adamoswick","status":"GoDaddy"},"change_message_id":"5f7dcf43f2183ff67d838bf21d53d1dcbb4cbdd6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"d3fc6a63_2807deb3","in_reply_to":"b7f3e975_11f22867","updated":"2023-11-15 10:23:39.000000000","message":"Created https://bugs.launchpad.net/neutron/+bug/2043564 to track this. Hopefully that goes into enough detail but happy to add more.\n\nIn an environment with a router that has 100 subnets attached, we were seeing router setup time drop from about 100s to around 70s.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"a8f9b4a9df61ddc364767b755e877e391d39c787","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c5376a97_458abb07","updated":"2024-02-21 17:40:23.000000000","message":"This should probably be tested somehow.","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"}],"neutron/agent/linux/ip_lib.py":[{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"beffddcb6b34e3cbba562aa90b61ed596900d227","unresolved":true,"context_lines":[{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"}],"source_content_type":"text/x-python","patch_set":1,"id":"fc5b8535_3ba48ccb","line":1582,"range":{"start_line":1582,"start_character":11,"end_line":1582,"end_character":18},"updated":"2023-11-13 17:07:37.000000000","message":"where does this \"devices\" variable come from now? Before, it was one of the parameters for the function, but now it isn\u0027t. I might be missing something though","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"bc5c1e671a17f2946095d7a173a865fc3b21c0de","unresolved":false,"context_lines":[{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"}],"source_content_type":"text/x-python","patch_set":1,"id":"3b0cc0aa_0e13ddcd","line":1582,"range":{"start_line":1582,"start_character":11,"end_line":1582,"end_character":18},"in_reply_to":"51dafeac_7b453caa","updated":"2023-11-13 17:23:19.000000000","message":"Ohhhh I see now it\u0027s below, sorry :)","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":35825,"name":"Adam Oswick","email":"adam@adamoswick.co.uk","username":"adamoswick","status":"GoDaddy"},"change_message_id":"ea516dca7878ae1c856e2f81a7db4a310319e6cc","unresolved":false,"context_lines":[{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"}],"source_content_type":"text/x-python","patch_set":1,"id":"51dafeac_7b453caa","line":1582,"range":{"start_line":1582,"start_character":11,"end_line":1582,"end_character":18},"in_reply_to":"fc5b8535_3ba48ccb","updated":"2023-11-13 17:16:19.000000000","message":"It\u0027s scoped within the containing list_ip_routes function so any other functions declared within that (e.g. get_device) can access it with it out being passed in directly.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c0315f0b67fa311af2563f9028e2bea8bf940324","unresolved":true,"context_lines":[{"line_number":1597,"context_line":"    table \u003d IP_RULE_TABLES.get(table, table)"},{"line_number":1598,"context_line":"    routes \u003d privileged.list_ip_routes(namespace, ip_version, device\u003ddevice,"},{"line_number":1599,"context_line":"                                       table\u003dtable, **kwargs)"},{"line_number":1600,"context_line":"    devices \u003d {}"},{"line_number":1601,"context_line":"    ret \u003d []"},{"line_number":1602,"context_line":"    for route in routes:"},{"line_number":1603,"context_line":"        cidr \u003d linux_utils.get_attr(route, \u0027RTA_DST\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"38cc95a5_afd9f7c6","line":1600,"updated":"2023-11-15 01:23:18.000000000","message":"So is this whole thing only a win if device\u003d\"some_device\" ? Otherwise instead of one get_link_devices() call we have many?","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":35825,"name":"Adam Oswick","email":"adam@adamoswick.co.uk","username":"adamoswick","status":"GoDaddy"},"change_message_id":"5f7dcf43f2183ff67d838bf21d53d1dcbb4cbdd6","unresolved":false,"context_lines":[{"line_number":1597,"context_line":"    table \u003d IP_RULE_TABLES.get(table, table)"},{"line_number":1598,"context_line":"    routes \u003d privileged.list_ip_routes(namespace, ip_version, device\u003ddevice,"},{"line_number":1599,"context_line":"                                       table\u003dtable, **kwargs)"},{"line_number":1600,"context_line":"    devices \u003d {}"},{"line_number":1601,"context_line":"    ret \u003d []"},{"line_number":1602,"context_line":"    for route in routes:"},{"line_number":1603,"context_line":"        cidr \u003d linux_utils.get_attr(route, \u0027RTA_DST\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"8ccab23f_c45c265a","line":1600,"in_reply_to":"38cc95a5_afd9f7c6","updated":"2023-11-15 10:23:39.000000000","message":"That\u0027s correct however it looks like only caller of this function is IpRouteCommand.list_routes() which always provides the device argument.\n\nIf you think it\u0027s worthwhile then I could update the logic here to do the new method if a device argument is provided, otherwise fetch all the devices and filter as needed.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"ed7842122a3623fc4496e95203d43c22101e07b5","unresolved":false,"context_lines":[{"line_number":1597,"context_line":"    table \u003d IP_RULE_TABLES.get(table, table)"},{"line_number":1598,"context_line":"    routes \u003d privileged.list_ip_routes(namespace, ip_version, device\u003ddevice,"},{"line_number":1599,"context_line":"                                       table\u003dtable, **kwargs)"},{"line_number":1600,"context_line":"    devices \u003d {}"},{"line_number":1601,"context_line":"    ret \u003d []"},{"line_number":1602,"context_line":"    for route in routes:"},{"line_number":1603,"context_line":"        cidr \u003d linux_utils.get_attr(route, \u0027RTA_DST\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"a41ba3bc_61bbd4e9","line":1600,"in_reply_to":"7b4d879c_5f4d8116","updated":"2024-02-21 20:44:43.000000000","message":"I think get_device should be moved outside of the parent function; all arguments made explicit (incl. `devices`); then we can properly target it with some unit tests. While it\u0027s embedded into the parent, it\u0027s impossible to directly test it.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"902730cede76ffcbcf89581363304924b0dc27e9","unresolved":false,"context_lines":[{"line_number":1597,"context_line":"    table \u003d IP_RULE_TABLES.get(table, table)"},{"line_number":1598,"context_line":"    routes \u003d privileged.list_ip_routes(namespace, ip_version, device\u003ddevice,"},{"line_number":1599,"context_line":"                                       table\u003dtable, **kwargs)"},{"line_number":1600,"context_line":"    devices \u003d {}"},{"line_number":1601,"context_line":"    ret \u003d []"},{"line_number":1602,"context_line":"    for route in routes:"},{"line_number":1603,"context_line":"        cidr \u003d linux_utils.get_attr(route, \u0027RTA_DST\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7b4d879c_5f4d8116","line":1600,"in_reply_to":"8ccab23f_c45c265a","updated":"2024-01-10 18:50:43.000000000","message":"Sorry for the slow reply. It does seem like a good change.\n\nI think changing slightly as you mentioned (fetch all if device\u003dNone) is useful, since that way we could at least add a unit test for each case.","commit_id":"087195179f090b837e7c648f601e96fd88abec06"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"5347d80797ac8003ab48d3e1048c39706819b441","unresolved":true,"context_lines":[{"line_number":1578,"context_line":"def list_ip_routes(namespace, ip_version, scope\u003dNone, via\u003dNone, table\u003dNone,"},{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"}],"source_content_type":"text/x-python","patch_set":2,"id":"90170784_cfcf42dd","line":1581,"updated":"2024-02-21 20:36:06.000000000","message":"before the patch, get_device would return None if the device is not in the list of actual interfaces that exist on the system; now, AFAIU, it will crash with IndexError because of `[0]` access to a tuple that may potentially be empty.\n\nLooking at some of call stacks where the result of list_ip_routes is used, I can see that at least some of them can handle `device\u003dNone` gracefully (e.g. some of them lead through `_make_pyroute2_route_args` that handles this case.)\n\nMy concern is that some of the code that calls to `list_ip_routes` may not be affected by unexpected `KeyError` raised. Should we make this code gracefully handle an empty tuple reply from `get_link_devices`?","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"0ea75c84262fb844206659b4cacd7e80d4d715f5","unresolved":true,"context_lines":[{"line_number":1578,"context_line":"def list_ip_routes(namespace, ip_version, scope\u003dNone, via\u003dNone, table\u003dNone,"},{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"}],"source_content_type":"text/x-python","patch_set":2,"id":"88733a44_5195075d","line":1581,"in_reply_to":"90170784_cfcf42dd","updated":"2024-02-21 20:36:55.000000000","message":"s/may not/may now","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"a8f9b4a9df61ddc364767b755e877e391d39c787","unresolved":true,"context_lines":[{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"}],"source_content_type":"text/x-python","patch_set":2,"id":"c6b43bc2_17e9b3cb","line":1582,"range":{"start_line":1582,"start_character":11,"end_line":1582,"end_character":18},"updated":"2024-02-21 17:40:23.000000000","message":"this variable doesn\u0027t exist, no? you renamed the argument.","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"061be10b4ad03cecbeb410321af5b95129bcea3e","unresolved":true,"context_lines":[{"line_number":1579,"context_line":"                   device\u003dNone, **kwargs):"},{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"}],"source_content_type":"text/x-python","patch_set":2,"id":"2a2effe0_255a5743","line":1582,"range":{"start_line":1582,"start_character":11,"end_line":1582,"end_character":18},"in_reply_to":"c6b43bc2_17e9b3cb","updated":"2024-02-21 20:16:38.000000000","message":"Ah sorry, I missed this is a helper function and the variable is implicit scoped.\n\nThat said, I still think it should not be implicit unless we really have to. Can we still pass an argument here?","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"5347d80797ac8003ab48d3e1048c39706819b441","unresolved":true,"context_lines":[{"line_number":1580,"context_line":"    \"\"\"List IP routes\"\"\""},{"line_number":1581,"context_line":"    def get_device(index, namespace):"},{"line_number":1582,"context_line":"        if devices.get(index) is None:"},{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"},{"line_number":1586,"context_line":"        return linux_utils.get_attr(devices[index], \u0027IFLA_IFNAME\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"e6cdc356_43aa6fec","line":1583,"updated":"2024-02-21 20:36:06.000000000","message":"(No action required) I believe get_link_devices could be extended to accept an iterable \u0027index\u0027 argument, then pass it as *index into ip.get_links, to fetch information about all interfaces in one netlink message.\n\n(Then you could build the list of indices of interest first; then fetch in one go; then populate `value[device]` / `value[via]` from the result.)","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"f5bea224c3b8fdd88f6237972ac715d90ca502de","unresolved":true,"context_lines":[{"line_number":1583,"context_line":"            devices[index] \u003d privileged.get_link_devices("},{"line_number":1584,"context_line":"                namespace,"},{"line_number":1585,"context_line":"                index\u003dindex)[0]"},{"line_number":1586,"context_line":"        return linux_utils.get_attr(devices[index], \u0027IFLA_IFNAME\u0027)"},{"line_number":1587,"context_line":""},{"line_number":1588,"context_line":"    def get_proto(proto_number):"},{"line_number":1589,"context_line":"        if isinstance(proto_number, int) and proto_number in rtnl.rt_proto:"}],"source_content_type":"text/x-python","patch_set":2,"id":"716790e2_f49eda67","line":1586,"updated":"2024-02-21 20:42:07.000000000","message":"(No action required)\n\nnit: since the only thing you actually care to cache is the ifname, you could directly store it in devices[index], then you would not need to get_attr it on each entrance into the helper function.\n\nOf course, this is of little consequence, because the work \"wasted\" is so tiny...","commit_id":"1a5e40ae6ae583c65f7d4d184475211783f4f2a1"}]}