)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Brian Haley \u003chaleyb.dev@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-01 17:50:16 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Change OVN client to account for nested networks"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When the OVN driver adds SNAT rules, it only adds them"},{"line_number":10,"context_line":"for network CIDRs directly connected to the internal"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"a4e3467c_f59cdba4","line":7,"updated":"2024-04-19 16:13:06.000000000","message":"I think if we go with the argument listed below and the patch, then a number of follow-ups should be done with the project:\n\n1. separate LP bugs reported for DVR and HA (?) to adjust their behavior for nested networks and to mimic behavior of \"legacy\" routers.\n2. api-ref should be updated, explaining the intent of enable_snat field.\n\nWould you mind following up on these as part of this work?\n\n(BTW this is unfortunate that our routers behave so inconsistently. One way to eventually coalesce on the \"right\" behavior for all of them would be to implement a tempest scenario that would have to pass with different router types and ml2 drivers. Maybe a LP could be reported to add such a test case too.)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Brian Haley \u003chaleyb.dev@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-01 17:50:16 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Change OVN client to account for nested networks"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When the OVN driver adds SNAT rules, it only adds them"},{"line_number":10,"context_line":"for network CIDRs directly connected to the internal"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"0353c5fc_503f5597","line":7,"updated":"2024-04-19 16:13:06.000000000","message":"Please describe the rationale of this behavior change.\n\nI think the rationale is based on prior art / original intent, specifically on the fact that https://bugs.launchpad.net/neutron/+bug/1386041 was implemented early for L3 agent (before it was called \"legacy\"). Even though DVR apparently behaves otherwise, the linked bug suggests that it\u0027s DVR (and HA maybe?) bug, not the legacy bug. (THANKS TERRY WILSON FOR DIGGING THE PATCH!)\n\nFrom this \"original intent\" argument comes the parity argument between ml2/ovs and ml2/ovn. This chain of thought satisfies me.\n\nBut please make it explicit in the commit message (and the code comments?), so that we no longer have to dig code from circa 2013.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Brian Haley \u003chaleyb.dev@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-01 17:50:16 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Change OVN client to account for nested networks"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When the OVN driver adds SNAT rules, it only adds them"},{"line_number":10,"context_line":"for network CIDRs directly connected to the internal"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"fbc2f371_a0637221","line":7,"in_reply_to":"0353c5fc_503f5597","updated":"2024-04-19 21:04:19.000000000","message":"Acknowledged","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Brian Haley \u003chaleyb.dev@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-01 17:50:16 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Change OVN client to account for nested networks"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When the OVN driver adds SNAT rules, it only adds them"},{"line_number":10,"context_line":"for network CIDRs directly connected to the internal"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"ddcc34e8_c214c484","line":7,"in_reply_to":"a4e3467c_f59cdba4","updated":"2024-04-19 21:04:19.000000000","message":"The DVR bug is here, and already has a patch proposed, I might need  to take that one on and get across the finish line.\n\n    https://bugs.launchpad.net/neutron/+bug/2029722\n\nI don\u0027t think HA has the issue per-se as the behavior is just what is inherited from the classes (DVR, legacy).\n\nI can look at the api-ref and docs to see what we can change to clarify this behavior.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":18,"context_line":"nested behind it, no SNAT rule will be added for that"},{"line_number":19,"context_line":"CIDR."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change the code to account for such a scenario by"},{"line_number":22,"context_line":"recursively traversing all attached routers and their"},{"line_number":23,"context_line":"networks, up to a depth of 20 routers. Restarting"},{"line_number":24,"context_line":"neutron-server and running the OVN DB sync tool in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"377000bb_3ad8ca04","line":21,"updated":"2024-04-19 16:13:06.000000000","message":"The recursive implementation is \"icky\", but AFAIU there\u0027s a good reason for this, namely that 0.0.0.0/0 would apply to FIPs (DNAT), as per https://github.com/ovn-org/ovn/pull/123 ? Sadly, I see the patch is closed as not needed (do we know why this is not deemed needed anymore?) If that\u0027s the reason to insert a snat rule per \"reachable\" subnet, then let\u0027s explain it here in commit message and in the comments to the recursive implementation, so that we know when we could revisit it. (when a bug fix happens on OVN side.)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":false,"context_lines":[{"line_number":18,"context_line":"nested behind it, no SNAT rule will be added for that"},{"line_number":19,"context_line":"CIDR."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change the code to account for such a scenario by"},{"line_number":22,"context_line":"recursively traversing all attached routers and their"},{"line_number":23,"context_line":"networks, up to a depth of 20 routers. Restarting"},{"line_number":24,"context_line":"neutron-server and running the OVN DB sync tool in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"54576f48_defafa4d","line":21,"in_reply_to":"0ef8b269_834329d8","updated":"2024-05-15 21:57:23.000000000","message":"Done","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":true,"context_lines":[{"line_number":18,"context_line":"nested behind it, no SNAT rule will be added for that"},{"line_number":19,"context_line":"CIDR."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change the code to account for such a scenario by"},{"line_number":22,"context_line":"recursively traversing all attached routers and their"},{"line_number":23,"context_line":"networks, up to a depth of 20 routers. Restarting"},{"line_number":24,"context_line":"neutron-server and running the OVN DB sync tool in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"0ef8b269_834329d8","line":21,"in_reply_to":"377000bb_3ad8ca04","updated":"2024-04-19 21:04:19.000000000","message":"I was not aware of that change and can\u0027t say I know about the OVN code in question to determine if it\u0027s relevant here.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":29,"context_line":"some of the OVN client code, fixed all instances that"},{"line_number":30,"context_line":"were causing existing tests to fail with this change."},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"Closes-bug: #2051935"},{"line_number":33,"context_line":"Change-Id: I893c23ff3f33abbacc9bc99ca08b9f3d67f40e3b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"10ffdf65_ec515330","line":32,"updated":"2024-04-19 16:13:06.000000000","message":"This is a major change to the router behavior and is worth being mentioned in a release note.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":29,"context_line":"some of the OVN client code, fixed all instances that"},{"line_number":30,"context_line":"were causing existing tests to fail with this change."},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"Closes-bug: #2051935"},{"line_number":33,"context_line":"Change-Id: I893c23ff3f33abbacc9bc99ca08b9f3d67f40e3b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"ac15c6ac_6a7a6e79","line":32,"in_reply_to":"10ffdf65_ec515330","updated":"2024-04-19 21:04:19.000000000","message":"Acknowledged","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d26b79c880657a17b0e4d5db42af22d97f2d37b5","unresolved":true,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"The original intent was for neutron to allow this behavior,"},{"line_number":22,"context_line":"please see https://bugs.launchpad.net/neutron/+bug/1386041"},{"line_number":23,"context_line":"for that description and dicsussion of the reasoning."},{"line_number":24,"context_line":"This change closes the gap with respect to OVN."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change the code to account for such a scenario by"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"330d2367_4a32ef3c","line":23,"range":{"start_line":23,"start_character":25,"end_line":23,"end_character":35},"updated":"2024-04-24 17:30:04.000000000","message":"discussion","commit_id":"392281ef0759487998242c69cf2ded850b0bf83a"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"7f58e387e050046c08d83eb30ea9ebac517b2c42","unresolved":false,"context_lines":[{"line_number":20,"context_line":""},{"line_number":21,"context_line":"The original intent was for neutron to allow this behavior,"},{"line_number":22,"context_line":"please see https://bugs.launchpad.net/neutron/+bug/1386041"},{"line_number":23,"context_line":"for that description and dicsussion of the reasoning."},{"line_number":24,"context_line":"This change closes the gap with respect to OVN."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change the code to account for such a scenario by"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"f460c32b_86285354","line":23,"range":{"start_line":23,"start_character":25,"end_line":23,"end_character":35},"in_reply_to":"330d2367_4a32ef3c","updated":"2024-04-25 22:34:41.000000000","message":"Done","commit_id":"392281ef0759487998242c69cf2ded850b0bf83a"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"f4a70d18502eca0d92dda55297bb72c6c9677b11","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ec57cac7_b0c50cb7","updated":"2024-02-02 19:43:46.000000000","message":"So the one problem here is that this \"nested\" router does not have a gateway port, i.e. external_gateway_info \u003d\u003d null in router show API call. That is why the functional tests are failing.\n\nSo I\u0027m not sure this is the correct fix, will have to get more input.","commit_id":"1166eb539fcfe1c3e578b12e6039b5c398ba3fb0"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"edda2f32bc8267a5e2bcb08a1b7b6846a8ae9ce6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d4cd0d9c_a45b317d","updated":"2024-02-06 10:15:23.000000000","message":"-1 for visibility","commit_id":"89cd3431b2c74f929cf96e8e957aecb93ec4b0a2"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"5ae777c36daacd544c35091566e57fc6937c238c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f0c868da_76c3712d","updated":"2024-02-06 10:15:15.000000000","message":"Please check [1] and [2]\n\n[1]https://bugs.launchpad.net/neutron/+bug/2035281\n[2]https://review.opendev.org/c/openstack/neutron/+/895260","commit_id":"89cd3431b2c74f929cf96e8e957aecb93ec4b0a2"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"6517930fc7b30a78a7f3de491b9b3db3059210cc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0264cc1c_e86a5e53","in_reply_to":"f0c868da_76c3712d","updated":"2024-02-06 14:37:34.000000000","message":"Hi Rodolfo, see my comments in the bug, but that change does not seem to fix this issue.","commit_id":"89cd3431b2c74f929cf96e8e957aecb93ec4b0a2"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b6bf991fca1901ce2e82f5b8a7501d0dc68c6e1e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"a61608d6_a8741b39","updated":"2024-02-10 18:47:46.000000000","message":"recheck unrelated functional test failure","commit_id":"6f2d97d55a64c0408c0133a4d05c145fd5881d5a"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"a880cc7eb25734dd1059c8dd4230ac14719def49","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"ccce558f_ae9bd93d","updated":"2024-03-13 10:58:51.000000000","message":"According to https://issues.redhat.com/browse/FDP-448, this functionality is not going to be implemented in core OVN.\n\nRegarding to this patch, I have some concerns about how is implemented. Instead of relying on the API calls (router creation), I would implement this feature attending to the OVN events, in particular to the LRP register events.\n* LRP creation:\n** If the LRP is a GW, do nothing.\n** If the LRP is not a GW port, check if there are other LRP in the same subnet. If that is the case:\n*** If the LRP router has a GW port, retrieve the other LRP router subnets and add them to the router NATs.\n*** If the LRP router has not a GW port, then it could be connected to other router with GW port. Check that and add the corresponding NAT rules to the GW router.\n* LRP deletion: do the same as in the \"creation\" event but deleting the NAT rules.","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"37eef8b3b15f2d000e6962ac0de34ae8092a6e06","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"8f15e555_fa9414d7","updated":"2024-02-14 08:12:30.000000000","message":"I\u0027m not in favor of this functionality. By looping across the nested routers, we are breaking the isolation provided by the router NATs. The upper router external network should not be aware of the down routers internal networks; the traffic from down routers should be NATed to the intermediate network (that connected down and upper routers) and then this traffic should be then NATed again to the external network.\n\nI\u0027m going to ask in the bug for a reproducer, including the two routers, subnets and routes added (in the router or in the VMs).","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"309c61b91ad97eb3062f25d46c9c5c6c7e590460","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"85acb670_01d15b1d","in_reply_to":"0f845267_1e40f4c0","updated":"2024-03-19 22:13:40.000000000","message":"I realize this discussion seems to be just between Rodolfo and myself, but anyone with an opinion is free to comment 😊\n\nAfter looking at this again, one of the reasons to keep it in the client is that it\u0027s the only place these rules are added today, via API calls. There is no event handling for any of these. Maybe in the future that will change but then we could do this for a lot of this code I would think.\n\nThanks","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"3a52760e1b746b72f35ac5ff545277f57cc46cf4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"7b1fb263_a3301f5b","in_reply_to":"379ad0eb_f6dbbe8e","updated":"2024-02-14 14:47:20.000000000","message":"You should be able to cascade as many routers as you like, and you can with ML2/OVS, assuming you setup static routes properly.\n\nI don\u0027t think it is required to have multiple nested SNATs in this case, as everything is routed properly between all these private nested networks, it\u0027s only when it needs to exit to the external network where we need the rule.\n\nI\u0027ll post my setup.","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e31aaaa84423eefaae8a7441d823de35ea841f5e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"7fe6bcb2_9e00c3bb","in_reply_to":"7b1fb263_a3301f5b","updated":"2024-02-14 19:47:45.000000000","message":"I added a lot of notes to the bug on how to reproduce this.","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"380024e11bbc5455aa6d3b552c11fc8cbddaba97","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"379ad0eb_f6dbbe8e","in_reply_to":"8f15e555_fa9414d7","updated":"2024-02-14 08:17:49.000000000","message":"What if a user wants to cascade three routers? That could be a \"fair request\", same as LP#2051935.","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"6a417400da429b36ca7a445c7f1387c3f190a9ad","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"0f845267_1e40f4c0","in_reply_to":"ccce558f_ae9bd93d","updated":"2024-03-15 20:05:20.000000000","message":"I could look into doing it differently, but in the end the result should be the same - the LRP router with a gateway port will have snat rules for these nested subnets, and remove them when they are deleted or disconnected. The one thing about this solution is I know it works 😊\n\nI also wanted to address this comment from the RH issue:\n\n\"But the suggested solution implies to loop over all nested routers to add the inner subnet SNAT rules (not related to the outer router) and won\u0027t work if there is another nesting level.\"\n\nThis is False, the code here will work for any nesting level up to something like 900 levels (python recursive limit), I have just only tested up to three.","commit_id":"036597f025190383dd3b6dad3897899dff2d4bb9"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"895a068457c9b2da373748090e201ef366fe669a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"099b6361_7a88be1f","updated":"2024-03-25 20:43:42.000000000","message":"I think in general the behavior is probably desired, but I\u0027d like to confirm that this won\u0027t expose us to some kind of leakage of traffic that would e.g. be blocked if it\u0027d carry snatted transit-network ip addresses when hitting the outer-router, but that would not be blocked because e.g. there\u0027s an enforcement rule (fwaas or something else?) that matches against the ip range that belongs to the transit-network (not the private-network.)\n\n-1 for recursion handling that I think could be improved. As to the above, I am on the fence and would like people more knowledgeable to consider the security angle.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"e907f3d36e4167238ae5be5b00b58a0ec3619aef","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"00bc0442_580fbdc5","updated":"2024-03-25 22:05:08.000000000","message":"It does seem weird for me for the proposed change to be the default behavior unless there is specific documentation stating that a gateway router should automatically NAT for all possible networks on routers that are attached. That isn\u0027t how any network equipment I\u0027m familiar with has operated. e.g. a cisco router you\u0027d generally set up interfaces for inside/outside nat and then have an acl specifically allowing certain subnets.\n\nIf someone comes to my office and sets up a router inside with another network behind it, I wouldn\u0027t expect the default behavior of the gateway router to be to NAT for that traffic (unless they set up NAT on their internal router, hiding that network from the GW router).","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"92453e15893a001f012d31477cd5696433d98544","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"29c37622_758afe9e","updated":"2024-03-25 20:45:01.000000000","message":"Liushy, could you please check if there are some unexpected implications for this patch in context of fwaas rules that could be defined in transit networks? Thanks!","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"78596f5603574d3818dc7e850108cc51da1f8dd8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a0772620_5253e3a4","in_reply_to":"00bc0442_580fbdc5","updated":"2024-03-26 01:00:43.000000000","message":"Is it a vote for a new API to allow to configure additional snat\u0027ed subnets for external gw?","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":28056,"name":"Liushy","email":"liuxie_11@163.com","username":"liuxie_11"},"change_message_id":"dc62cf5559f65bd4b160114003f32f0b73297200","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"8d2ff0ef_0cd79b82","in_reply_to":"29c37622_758afe9e","updated":"2024-03-26 06:18:43.000000000","message":"It seems OK for fwaas based on OVN if use this patch [1].\n\n[1]https://review.opendev.org/c/openstack/neutron-fwaas/+/845756","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"9610eb1959722fbc509a571e63aa0379c8523d9a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"fe994fbc_2965abce","in_reply_to":"a0772620_5253e3a4","updated":"2024-03-26 13:44:45.000000000","message":"Hey Terry. This has been the default behavior in Neutron since day one, and afaik similar to other clouds - the edge router will snat for anything that arrives at it\u0027s private interface, we actually program the routing rules that way. It\u0027s something customers have come to rely on.\n\nTaking your second example - the neutron router is that NAT gateway, it just happens to have a multi-tier set of networks behind it, and it\u0027s all owned by the same tenant, so they want it to work that way. The private IP addresses are confined to the internal network and never seen on the external network.\n\nI used to have to do this when I did QA, I was given one IP but had 50 machines to run tests on, so I added a second NIC and made one a jump host with a 10-net behind it. Enabling snat on it let them all pull new bits but they were otherwise invisible to the infrastructure.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"a6fd8fe5_a46d1db1","updated":"2024-04-19 16:13:06.000000000","message":"I haven\u0027t reviewed tests, yet.\n\nI think this patch has to be merged, with caveats as to how we should deal with other cases (dvr, ha); how api-ref should describe the expected behavior; etc.\n\nMore information about the background and the rationale of this change in commit message and comments is needed. A release note is needed too.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"7b902d9e2b6a92969b745aefa7e8c7ae87a84894","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"6ab47eb6_fe015bf4","updated":"2024-04-17 13:13:00.000000000","message":"Marking important since it\u0027s a regression in the OVN routing code wrt ML2/OVS.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"8dea15ad979eaa3f68b6ff751cc6b19e8448874a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"e67d1a48_9ee9fdbf","updated":"2024-04-17 13:08:49.000000000","message":"ping reviewers, updated based on comments","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d26b79c880657a17b0e4d5db42af22d97f2d37b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"b8e6dc18_d96e2035","updated":"2024-04-24 17:30:04.000000000","message":"I\u0027m ok with it. I\u0027d like us to clean up the recursion and redundant nested set. See follow-up: https://review.opendev.org/c/openstack/neutron/+/916939","commit_id":"392281ef0759487998242c69cf2ded850b0bf83a"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1a899a82dfed9f656448b4476f91ada3cd7ba138","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"d563d2ab_975b1057","updated":"2024-04-30 16:56:27.000000000","message":"Do we btw know that 0.0.0.0/0 wouldn\u0027t work, or it\u0027s just assumption? I understand there are some risks with 0.0.0.0/0 conflicting with something (what?) but the recursive collection of subnets - with god knows the number of recursive calls and the number of subnets total? - is also a risk. Do we have a good justification to not try 0.0.0.0/0?","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"047c1367147454318f2cc3e791a43716a9c6afd6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"517fdab1_5cf0dc71","updated":"2024-05-01 23:24:36.000000000","message":"I created this other change as a possible solution using 0.0.0.0/0:\n\nhttps://review.opendev.org/c/openstack/neutron/+/917904","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"8d23542db63946bc650008b8dc883787817d6fcc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"6450ecb3_4c141a4e","updated":"2024-05-08 21:13:46.000000000","message":"So my try using 0.0.0.0/0 failed miserably, we need to list all the cidrs. I\u0027ve not done very well with Ihar\u0027s follow-on to this, but can try one more time, just don\u0027t have a ton of cycles left this week.","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"72254f9e24a15db59a81006a5b8ca4c8bd3f89fe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"88e0a46e_4ac40a59","in_reply_to":"d563d2ab_975b1057","updated":"2024-04-30 18:12:23.000000000","message":"Yes, I believe 0.0.0.0/0 works, I just didn\u0027t know of any implications of using that instead of the cidrs we know about.","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c4e7d7837aebb5661d008e87d21ce19138e35d43","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"ddd7cc0e_12e30004","updated":"2024-05-11 15:39:30.000000000","message":"Squashed the follow-on change started by Ihar into this","commit_id":"1457777952c21e50e99e27e89c8c44551b521c92"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"26a0c84c_17639cb2","updated":"2024-05-15 21:57:23.000000000","message":"This is mostly good and I am ready to +2 after:\n\n- error / warning message gets a bit more informative\n- the way the \"recursion limit reached\" flag is returned doesn\u0027t involve mixing the flag into the nested subnets set but is separated into a separate boolean flag.\n\nThe rest of comments are just nits and style suggestions.","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"a47779b1cdf0defbcc1e2740c61ead0fc3e416b0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"cde335f6_78330207","updated":"2024-05-16 19:58:19.000000000","message":"In case you respin for some reason, a minor style comment.","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"5436cc62ae5aa8dc79daf782e3541acfd06ee999","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"1d9edc3f_4298d3f3","updated":"2024-05-16 12:37:21.000000000","message":"Thanks for bearing my feedback.","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"67ab908ae80d445ba287a6cd4f843845aca959ae","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9fbb04e1_493cd0ba","updated":"2024-05-16 21:50:44.000000000","message":"Yeah sadly the `limit` is not considered, so there are scenarios where the warning message is not logged, depending on the order of nested ports considered. This should be fixed by accumulating `limit` flag values (`limit |\u003d sublimit`).","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"5cc196f8d0861fd69df3baf2daa5222b8898fe45","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"4ef1a66a_905bff73","in_reply_to":"1d9edc3f_4298d3f3","updated":"2024-05-16 13:19:15.000000000","message":"Thanks for giving it, made this patch better. I will continue to see why I couldn\u0027t get 0.0.0.0/0 to work correctly, I\u0027m with our OVN team this week and they just created a 24.03 version for me to try to see if there was a bug fixed. If so that\u0027s something we could do in a later version when our minimum requirement is greater than that.","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e328d3808e6ba9063e0444d421b5e1523ea9eb1c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"f88cb4ee_74b1ee04","updated":"2024-05-22 21:48:02.000000000","message":"Sigh, failures related to changes since PS13","commit_id":"641d3b2090aabb2f143a11f04c9e33ef761d8b97"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"02df5be56c07a90a4fd835d4624b65179734e606","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":18,"id":"7b8a7317_99342d27","updated":"2024-06-05 20:10:17.000000000","message":"I am sorry Brian for the hassle with this patch. But the behavior changes through iterations, so I keep coming with different scenarios and concerns to consider. I hope you understand I don\u0027t do it for fun. (Well, maybe just a bit!..)","commit_id":"5713642867137f69eb108360ed07d67649136f1d"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c1b44cbc72d2b3f8febe5aae7bb9dcb7869b1a23","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"0589d056_f3c438d3","updated":"2024-06-12 02:30:55.000000000","message":"Forgot to update commit message and release note, will do that now.","commit_id":"5e4f656eea5cdee3892e7ef964ce4d38b3225565"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9667aa6a10f48a2e9fb2112856bfaa015886ca29","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"a1862433_0a78f0c1","updated":"2024-06-14 18:16:50.000000000","message":"My main concern is that this adds a lot of complexity (recursion, requires an arbitrary limit because of recursion, changes behavior, requires db_sync repair for existing routers, etc.), when it really seems like we should just try to fix OVN to handle 0.0.0.0/0 NAT w/o breaking FIPs.\n\nAdditionally, I am absolutely not an ml2/ovs expert, but from what I\u0027ve heard discussed, handling nested routers (added in https://review.opendev.org/c/openstack/neutron/+/131905) is not available in dvr/ha ml2/ovs. Is this ture? If so, the argument that this is a feature gap and that ml2/ovn needs to follow suit doesn\u0027t necessarily work for me. If they do support it, then maybe that\u0027s a different story. Also, maybe this behavior should be an option, but not necessarily a default one.\n\nI can probably be convinced that I\u0027m wrong--others have spent more time looking at this, but just my $0.02.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"db51b9345efa520191ab83cd4d80ca89f2f7eec1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"d27c8c3d_f8ba5f93","updated":"2024-06-27 14:54:32.000000000","message":"Thanks Rodolfo, will push an update to fix your relnote comment.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc63f69d03c17e8a517e439f8a86a78aa50b01a2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"632229c6_2aff034a","updated":"2024-06-27 07:38:15.000000000","message":"This new functionality deserves neutron-tempest-plugin tests, that\u0027s for sure.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"2ae8d275072c8216677082409efa79dcf18236c1","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"6f60c845_975906cb","in_reply_to":"473d3ed0_76fb5fbe","updated":"2024-06-26 20:00:43.000000000","message":"I\u0027m not sure if anyone else had a comment on this patch?\n\nI am willing to revert if we are able to get core OVN code to work with a 0.0.0.0/0 SNAT match (tested and failed in a different patch), but I don\u0027t have the cycles for that right now, and I\u0027m not sure if I found a bug we\u0027d be able to get it back into older releases. As it is, fixing this addresses a bug/gap compared to ML2/OVS and stops migration to OVN.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e38559229f2d74654a84185f1c2704fc5c78a5f6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"f2ad442f_fd324685","in_reply_to":"5a779d85_71ac4a4d","updated":"2024-06-27 15:14:21.000000000","message":"Someone internally tested it and said that HA routers did nested snat before. I wonder why the discrepancy.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"96c0b6fe4c17ff55a99101d97023ad76087865cf","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"b55d3088_77ed2760","in_reply_to":"5af80896_86fde64c","updated":"2024-06-17 05:49:37.000000000","message":"\u003e To me, that implies Neutron needs to configure the additional SNAT rules in order for this to work.\n\nTo me, it implies that \"there is no SNAT rule that includes both subnets *in the current setup*\". But a 0.0.0.0/0 rule *would* include both subnets if it existed. You tried that, and it failed for some reason. But nothing in FDP-448 sounded like it was trying to fix *that* issue.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"12470519037b1f42abab86c9ca93e792b1faea6c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"a60a9697_238a297c","in_reply_to":"6500258a_bc5896b8","updated":"2024-06-27 17:11:49.000000000","message":"OK, I was under impression only legacy is doing nested snat right now. (The release note talks about legacy routers only.)","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc63f69d03c17e8a517e439f8a86a78aa50b01a2","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"a77d981f_f5850b2e","in_reply_to":"6f60c845_975906cb","updated":"2024-06-27 07:38:15.000000000","message":"I commented that in this same LP bug: https://bugs.launchpad.net/ubuntu/+source/neutron/+bug/2051935/comments/20. ML2/OVS DVR and HA doesn\u0027t support nested routing. You can check it too.\n\nUntil we implement this feature in core OVN, we\u0027ll take this implementation.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"a631ee9531e457bb9ba7ff4e37426d67808d1bed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"eef26d5c_65789508","in_reply_to":"76c4a7c7_7a17ab1e","updated":"2024-06-14 21:47:06.000000000","message":"re: FDP-448, it doesn\u0027t look like that bug is about the problem with having a NAT rule of 0.0.0.0/0 and it negatively impacting FIP traffic. It looks like it is closed because there is no NAT rule that matches, and the reporter expected it to work.\n\nre: 1a2 I\u0027ve also seen a customer run into this, so, I get the investment in getting it fixed too. But looking at it from a customer perspective, customer\u0027s don\u0027t run master. So fixes for their issues need to be backportable (at least downstream). And since it\u0027s a pretty huge change in behavior for all other potential existing customers mid-stream, it seems like being able to make it a non-default configurable behavior would be a good option for customers. Only those that needed it would get the behavior change.\n\nThen we could argue about what the default value going forward should be separately. Thoughts?\n\nRE:2 So the majority of implementations don\u0027t support it, but we are deciding to make it the default behavior for everything. ;-)","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"a5bb2209145968c4416eee006b091b6ffae1d649","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"76c4a7c7_7a17ab1e","in_reply_to":"a1862433_0a78f0c1","updated":"2024-06-14 20:11:51.000000000","message":"Hi Terry,\n\nI\u0027ll try and address all your questions.\n\n1) We should try and fix OVN to handle 0.0.0.0/0 NAT.\n\n1a1) https://issues.redhat.com/browse/FDP-448 was closed as \"not a bug\", so trying to push a change to OVN seems like even a harder ask. Plus it would then need to be backported. Also, it is hard for Neutron to know if that fix is in place, from what I tested with my other related patch - https://review.opendev.org/c/openstack/neutron/+/917904 - using 0.0.0.0/0 with the current OVN code will break FIPs, and I don\u0027t want to do that. An earlier version went until it hit the python recursion limit, and I believe I had negative feedback, so I went with a 50 routers version, which is probably still way above what will ever happen.\n\n1a2) The reason I\u0027m so invested in fixing this is it is blocking a customer with ML2/OVS (no DVR) from upgrading to OVN. We\u0027re talking Yoga (LTS) code, so moving to a version of OVN/OVS with a potential fix might be impossible, but getting a neutron change there is possible.\n\n\n2) Handling nested routers is not available in DVR/HA ML2/OVS. Is this true? \n\n2a) It is also a bug - https://bugs.launchpad.net/neutron/+bug/2029722 - and there is a proposed patch, I will look at fixing that next, boiling one ocean at a time :)","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"3bcd6b4087f6ec73f5d5323edbcdac36d7d661d7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"3007c242_da34c822","in_reply_to":"a60a9697_238a297c","updated":"2024-06-27 17:21:37.000000000","message":"I can change it to ML2/OVS if that makes it clearer? I also noticed another place I should have used \u0027L3 router\u0027 so was going to update.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"db51b9345efa520191ab83cd4d80ca89f2f7eec1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"5a779d85_71ac4a4d","in_reply_to":"a77d981f_f5850b2e","updated":"2024-06-27 14:54:32.000000000","message":"Done","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"75dffe3bd2d17db9098a1504a23d0c9703f3eedf","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"473d3ed0_76fb5fbe","in_reply_to":"b55d3088_77ed2760","updated":"2024-06-18 17:49:08.000000000","message":"Just a couple of comments on this.\n\nWhile it would be easier to have a single SNAT rule for OVN, it still does require finding out where the code needs to change and getting it merged and backported. I am not optimistic that would happen quickly since I\u0027m not as familiar with that code and not sure I won\u0027t get told \"you have a workaround of configuring all the subnets\".\n\nThe existing Neutron code configures SNAT rules for all the subnets, so doing it similarly in the OVN code seems to mimic that, and it\u0027s what the initial patch for DVR looks like as well.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e39a083afdb961db51f16094eee5f9a78f5cc872","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"5af80896_86fde64c","in_reply_to":"eef26d5c_65789508","updated":"2024-06-16 00:35:20.000000000","message":"This was the first comment in the FDP-448 case:\n\n\"I might be missing some piece of information but for me it works as I would expect. The SNAT is not applied because the packet doesn\u0027t originate from the 10.10.0.0/24 subnet, but from 10.20.0.0/24. In order to get it working you would need to extend the SNAT rule to include both subnets (which works for the reporter). I don\u0027t think there is anything required to be changed from OVN perpective, it feels like Neutron should allow to configure broader SNAT in that case?\"\n\nTo me, that implies Neutron needs to configure the additional SNAT rules in order for this to work.\n\nI think this is backportable, at least to stable/yoga, if not I can handle whatever small delta\u0027s I need to to get it into our cloud archive.\n\nRegarding changing what the default should be, I have one thought: It has worked this way for years with ML2/OVS, so imo it is the default behavior. Being broken in OVN is a \"gap\" issue, and broken in DVR is a bug.\n\nSorry for being stubborn on this, but I don\u0027t see another easy way forward. Being explicit in the CIDRs we allow (and not using 0.0.0.0/0) seems to check all the boxes and fits within the confines of OVN.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c91321f2b871fde0f8c823bebccc61db49a9d8c7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"6500258a_bc5896b8","in_reply_to":"f2ad442f_fd324685","updated":"2024-06-27 16:50:32.000000000","message":"I would think \u0027regular\u0027 HA would work, just not anything with DVR based on the other bug.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"7ff7dab4dfa649a4b672c5646078dd96d45136f3","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"0c23ebda_6128dcb1","updated":"2024-06-27 15:43:45.000000000","message":"Ultimately, this is a change in behavior for ml2/ovn. This is master, so we can certainly do that if we decide to, but here is my concern with it being unconfigurable: ml2/ovn has *not* done this for nearly as long as ml2/ovs *has*.\n\nMaking this change will take existing ml2/ovn users who could have this setup and expect VMs behind a nested router to *not* have external Internet access and now all of the sudden they will--with no way to disable the behavior. This could be considered a security issue. At the very least, a customer needs to be able to disable the behavior. IMHO, it\u0027s safer for it to not be default behavior, regardless of what ml2/ovs did in some cases as ml2/ovn has a long history as well at this point.\n\nPrevious discussions have also said that the plan would be to backport this change. While I would only -1 this change for master due to possible security issue with people upgrading and getting an unexpected behavior, I\u0027d very likely -2 a backport w/o configurability and default\u003dFalse if I had the power to do so (but I don\u0027t think I do).\n\nUltimately, I think this should be a new API allowing people to specify allowed source addresses for NATting. I get that that would be a long involved project and customers doing migrations from ovs-\u003eovn need something before that though. Even though I\u0027d prefer to avoid the recursive solution, I also get that that might be the only way to fix this from within neutron (as opposed to fixing OVN\u0027s handling of 0.0.0.0/0). So I\u0027m open to fixing this here, but I\u0027d strongly recommend making this configurable (and would insist on it for backported versions) and my preference would be for it to be disabled by default, but I\u0027m flexible on that.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"3c1d89e1da8d7404d1d2a6ba9fe05fb91890e8c6","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"3fc1b215_d9657779","in_reply_to":"0104325e_6ead02a8","updated":"2024-06-27 17:46:18.000000000","message":"Whether it\u0027s a gap or not--we\u0027re kind of past that at this point. We can\u0027t just change 8 years of ml2/ovn behavior in stable branches. We can\u0027t just assume that the \"average\" user at this point is coming from ml2/ovs. There have been lots of people using ml2/ovn for many years. It\u0027s been the default for several years. Plenty of people have never even used ml2/ovs at this point--and in my opinion, changing their default behavior in stable branches seems irresponsible. \"Oh, btw, we\u0027ve opened up some of your VMs *to the Internet* in the install you\u0027ve been using for 3 years already. Enjoy!\" doesn\u0027t seem like the kind of thing that customers in general would like. And the last thing I want is to do that, then have people complaining that they relied on the previous behavior and then have to put out those fires too.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c91321f2b871fde0f8c823bebccc61db49a9d8c7","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"d31f99e7_be251bc2","in_reply_to":"0c23ebda_6128dcb1","updated":"2024-06-27 16:50:32.000000000","message":"Hi Terry,\n\nWhile I can agree this is a change in behavior for OVN, I feel it falls in the \u0027gap\u0027 category as the intent, since Kilo (https://review.opendev.org/c/openstack/neutron/+/131905), was to have default SNAT for indirectly-connected instances to work. One thing this bug has highlighted is that there are no tests for this in tempest, so we should have caught it sooner.\n\nThe problem I see with making it configurable is that, in my opinion, it should always be enabled. Plus, adding a flag will impact cloud install/management tools (e.g. charms) that then have to add support for that flag, and backport it...\n\nRegarding the backport comment - yes, I did intend to cherry-pick this back to Yoga, mostly to align with what I see (from Canonical\u0027s viewpoint) as an LTS release, i.e. 22.04, and that is where this issue was found by a customer. I could probably deal with just doing this downstream if necessary, but think 2024.1 should have it being a SLURP release.\n\nCan you expand a little on your security concern? As far as I can tell, this does not change ingress of packets, just allows all instances to egress via the default SNAT address.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"22a1b7db2299964a09fe57e15f79e58203da20dd","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"cc824c51_0c8ce8c2","in_reply_to":"3fc1b215_d9657779","updated":"2024-06-27 21:59:29.000000000","message":"I want to find a solution here we can all agree on, please don\u0027t feel I\u0027m trying push this patch unnecessarily.\n\nIn a perfect world a new API extension for subnets seems ideal, since it would allow very granular settings for each subnet, but it gets complicated since we are dealing with two drivers here, and what would the default be to not break the current ML2/OVS behavior while allowing the OVN one? It also would not be back-portable. Could be future work.\n\nAn OVN-specific flag does allow us to enable/disable this behavior while leaving ML2/OVS alone, but controls the setting for the entire cloud. I would be Ok doing this based on the feedback here since it both meets my needs (albeit with extra work), and addresses the concerns of not enabling it by default.\n\nI will send out a new version tomorrow.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"629f2f87017a8c02fc111a3b831f2b46f41f140c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"ed569436_6d14c513","in_reply_to":"cc824c51_0c8ce8c2","updated":"2024-06-28 13:43:49.000000000","message":"Definitely don\u0027t feel like you\u0027ve been pushing unnecessarily--you\u0027ve been exceptionally patient. 😊\n\nI would be good with including it the ability to toggle it for now, default in master whatever people think is best and my hope would be defaulted off in backports. There can\u0027t be *that* many people relying on this behavior, so it seems like since you need to restart neutron anyway for the fix, a customer who needs the fix could just set the config value upgrade neutron and restart it. And a release/upgrade note for people coming from some product version and using ml2/ovs and upgrading to a stable version that is defaulted off isn\u0027t too much work. Just seems like immediately finding \"This VM doesn\u0027t have access to the Internet\" and fixing it is much nicer than finding out months down the road that a VM *has* had access to the Internet and due to some exploit has made an outbound connection to a botnet control program or something.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"12470519037b1f42abab86c9ca93e792b1faea6c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":22,"id":"0104325e_6ead02a8","in_reply_to":"d31f99e7_be251bc2","updated":"2024-06-27 17:11:49.000000000","message":"\u003e it should always be enabled\n\nThis is debatable, isn\u0027t it? (As evidenced here.) We know we have users that want this behavior. But we also know that there are lots (all) current ml2/ovn users that are at least happy about current behavior and some of them would probably be unhappy / surprised now that nested networks can egress, and with no way to opt out of it. (I think it may be a security concern for some environments - they may want to keep nested resources cut off external network.)\n\nRodolfo also expressed this concern here in comments before, though now his vote flipped to +2, not sure why since the approach in this patch stays the same.\n\n---\n\nThe reason why a knob / api are not being brought up here much that much so far is because the choice - and perhaps a new api - will be more challenging to implement than what you have here. But in an ideal world with lots of dev capacity - I\u0027d prefer to not surprise current users by changes in behavior; and to have an api to control it; and to have the api more flexible than a boolean flag to enable/disable nested snat. That\u0027s a lot of work, and it\u0027s not clear who\u0027d would work on it...","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"d1bf0dd91189a6c7aac1d4cdfcae75831a0c0840","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":22,"id":"6b84c0fd_1200e922","in_reply_to":"ed569436_6d14c513","updated":"2024-07-02 17:09:55.000000000","message":"I left the default at False, if people besides me think it should be True in master I can do that and add an \u0027updates\u0027 release note section.","commit_id":"a4e4524526b20a7eb63b043d89a96619df864830"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"aa219d9cabd03b208009d7e2295e2937044d3762","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"3d8a9e19_cb975d0a","updated":"2024-07-01 21:41:49.000000000","message":"recheck unrelated test timeout","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"5344c74a1e9c732049a6702652ea2b068ba205fa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"04a5dbd7_fc81fcdb","updated":"2024-08-15 01:40:10.000000000","message":"A relevant OVN bug report that, if fixed, will allow us to simplify the implementation here to just creating a single snat catch-all 0.0.0.0/0 entry. (Per router?)","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"7ab679404746fd5d5c851fe08b0ed22f1d62969b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"060e274b_cea7bb39","updated":"2024-08-26 20:23:05.000000000","message":"Abandoning this patch because we are taking an alternative path with 0.0.0.0/0 snats: https://review.opendev.org/c/openstack/neutron/+/926495","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"211316987af3dba444cbdcc92744d5f0c92f8a19","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"93070123_4d0da377","updated":"2024-08-14 15:16:15.000000000","message":"FYI I discussed the matter with Brian and he confirmed that I can take it over. AFAIU the missing part right now is the cleanup of nat entries on disabling the settings in the config file. Brian also said that in his latest testing, he seemed to have noticed that when repair mode is off, create / update requests were not creating snat entries either. (I thought we had test coverage for these basic code paths, but maybe not - will confirm.)","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"77556d9baafb444fcf18745f727d52e60324a474","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"1aa48bca_bdb5aab4","updated":"2024-07-18 00:36:29.000000000","message":"Hey Brian. If you need help with updating the patch to cover the case Terry pointed out, please let us know. If you are busy, we may find someone to take it over and drive to the finish line. Thanks!","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"adf4b6a30a2d6f1936abe84b7855a58b89f97d04","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"c8099797_7be9aa4a","updated":"2024-08-14 23:19:35.000000000","message":"I\u0027m working on this patch.","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"5e95d1586e65e8b177eba97ebe8a5d631b583773","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"2e28b81d_dd404aa6","updated":"2024-07-03 23:49:50.000000000","message":"Thanks for the update! When testing locally, if I follow the reproducer instructions, I get the desired behavior when creating the nested router with the proper routes, etc. and when creating with the config option false, then enabling, restarting, and running the db_sync util.\n\nBut when I delete the routes from router-nested, remove the private_port, and even delete the subnet from the router and delete the router, the NAT entries for the nested subnet remain. So it looks like we don\u0027t handle the fun cases associated with deleting nested routers/un-nesting routers.","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"094c9569246ffe480f0e1efb007dd9161cf16100","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"cb90f0e5_1bcde935","updated":"2024-07-03 14:29:22.000000000","message":"recheck unrelated multinode failure","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"73035657dfc5d9aa79868126d3d0fcfd69269f8e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"d97af511_e262f2a2","updated":"2024-07-02 20:06:51.000000000","message":"recheck unrelated ovs test failure","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"4252b8513e9372653fa81eba1298f6adb387bb19","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"246798a9_435924d5","updated":"2024-07-04 00:35:34.000000000","message":"😞","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"6ce145644c824745f4a70f10f1c622abc27b10b0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"dc238da3_67ac1a77","in_reply_to":"04a5dbd7_fc81fcdb","updated":"2024-08-15 01:40:37.000000000","message":"https://issues.redhat.com/browse/FDP-744","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0934cca8bb34de24e12bdddcf54f3e8d50ca0fe8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"1c3f3f2b_da6baec6","in_reply_to":"1aa48bca_bdb5aab4","updated":"2024-07-18 19:55:53.000000000","message":"Just pasting from irc in case you miss that.\n\nIt\u0027s broken worse than I thought, it\u0027s really only good at fixing things when the sync tool is run or \u0027repair\u0027 mode is set in the conf file. I\u0027ve been playing locally but don\u0027t have a new PS yet, and I\u0027m out next week so won\u0027t get to it until after that","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"2dbcec1635c443f3dc1a4e2e4168602345c3cbdb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"f7f98b93_0ef083ab","in_reply_to":"1c3f3f2b_da6baec6","updated":"2024-08-07 21:23:18.000000000","message":"Hey Brian, I\u0027d love to see this going. Please let me know if you have a new PS. If not, I am ready to step in to get it to the finish line. Let me know. This patch became hot. ;)","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0697f7d5757358b662d34c944dae591d0920fde1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"6bc12e9d_22f8420d","in_reply_to":"dc238da3_67ac1a77","updated":"2024-08-15 16:59:32.000000000","message":"Just an FYI that I had pushed a patch using 0.0.0.0/0 [0] but abandoned since we need this to be backported. But if we can figure out the bug in OVN there\u0027s no reason we can\u0027t change the code to be much simpler, assuming we require that fixed version.\n\n[0] https://review.opendev.org/c/openstack/neutron/+/917904","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py":[{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"df7755db97b26ff226fbdf7619e6225cb21257de","unresolved":true,"context_lines":[{"line_number":1386,"context_line":"        ports \u003d ports or self._get_router_ports(context, router_id)"},{"line_number":1387,"context_line":"        for port in ports:"},{"line_number":1388,"context_line":"            # \u0027network\u0027 here is actually a cidr"},{"line_number":1389,"context_line":"            network, s_id \u003d self._get_v4_network_for_router_port(context, port)"},{"line_number":1390,"context_line":"            if network:"},{"line_number":1391,"context_line":"                networks.append(network)"},{"line_number":1392,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9ae8f278_b76f1f52","line":1389,"updated":"2024-02-09 21:32:45.000000000","message":"nit: remove the comment and name the variable `network_cidr`. Unless it will cause lot of renaming in unit tests(i have not looked)?","commit_id":"4700deb58f3e4cc831fd179da85abc189eb11622"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"703c78f37c4ffd744f597a90a7fbc70dd74870b2","unresolved":false,"context_lines":[{"line_number":1386,"context_line":"        ports \u003d ports or self._get_router_ports(context, router_id)"},{"line_number":1387,"context_line":"        for port in ports:"},{"line_number":1388,"context_line":"            # \u0027network\u0027 here is actually a cidr"},{"line_number":1389,"context_line":"            network, s_id \u003d self._get_v4_network_for_router_port(context, port)"},{"line_number":1390,"context_line":"            if network:"},{"line_number":1391,"context_line":"                networks.append(network)"},{"line_number":1392,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"37a8f4b4_9d8988a6","line":1389,"in_reply_to":"9ae8f278_b76f1f52","updated":"2024-02-10 16:14:13.000000000","message":"It wasn\u0027t so much the unit tests as it was all the code below that uses \u0027network\u0027 instead of \u0027cidr\u0027. I can at least change this code, but will leave the reset alone to aid in backports.","commit_id":"4700deb58f3e4cc831fd179da85abc189eb11622"},{"author":{"_account_id":34271,"name":"Miro Tomaska","display_name":"Miro Tomaska","email":"mtomaska@redhat.com","username":"mtomaska"},"change_message_id":"df7755db97b26ff226fbdf7619e6225cb21257de","unresolved":true,"context_lines":[{"line_number":1405,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1406,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1407,"context_line":""},{"line_number":1408,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1409,"context_line":"                                                subnet_id):"},{"line_number":1410,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1411,"context_line":"        networks \u003d []"}],"source_content_type":"text/x-python","patch_set":3,"id":"71081e86_9068cccc","line":1408,"range":{"start_line":1408,"start_character":7,"end_line":1408,"end_character":8},"updated":"2024-02-09 21:32:45.000000000","message":"Just thinking out loud and wondering if such network can exists:\n1. Can there be a scenario with a network loop such that it will throw this recurssion into an infinite loop?\n2. Should this method memoize such that we dont recompute on the same subnet more than once?","commit_id":"4700deb58f3e4cc831fd179da85abc189eb11622"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"703c78f37c4ffd744f597a90a7fbc70dd74870b2","unresolved":false,"context_lines":[{"line_number":1405,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1406,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1407,"context_line":""},{"line_number":1408,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1409,"context_line":"                                                subnet_id):"},{"line_number":1410,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1411,"context_line":"        networks \u003d []"}],"source_content_type":"text/x-python","patch_set":3,"id":"fccdec31_fe20ffb8","line":1408,"range":{"start_line":1408,"start_character":7,"end_line":1408,"end_character":8},"in_reply_to":"71081e86_9068cccc","updated":"2024-02-10 16:14:13.000000000","message":"I thought about that since I did hit the recursion limit while testing. I will catch that exception and log a message.","commit_id":"4700deb58f3e4cc831fd179da85abc189eb11622"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"c7ab5156ca6cd7808b11ca90712b7c49941b9b53","unresolved":false,"context_lines":[{"line_number":1397,"context_line":"                        context, port, s_id)"},{"line_number":1398,"context_line":"                except RecursionError:"},{"line_number":1399,"context_line":"                    LOG.error(\u0027Possible network loop for router %s, not \u0027"},{"line_number":1400,"context_line":"                              \u0027adding nested networks for SNAT\u0027, router_id)"},{"line_number":1401,"context_line":"                if nested:"},{"line_number":1402,"context_line":"                    n_cidrs.extend(nested)"},{"line_number":1403,"context_line":"                    cidrs.extend(nested)"}],"source_content_type":"text/x-python","patch_set":4,"id":"d8bdb416_ca3e0c2d","line":1400,"range":{"start_line":1400,"start_character":54,"end_line":1400,"end_character":62},"updated":"2024-02-12 18:56:57.000000000","message":"I\u0027m just going to update this string as it might not be for SNAT.","commit_id":"6f2d97d55a64c0408c0133a4d05c145fd5881d5a"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"895a068457c9b2da373748090e201ef366fe669a","unresolved":true,"context_lines":[{"line_number":1390,"context_line":""},{"line_number":1391,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1392,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1393,"context_line":"                nested \u003d []"},{"line_number":1394,"context_line":"                try:"},{"line_number":1395,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1396,"context_line":"                        context, port, s_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bb441f3d_a60a7ec1","line":1393,"updated":"2024-03-25 20:43:42.000000000","message":"nit:\n\n```\ntry:\n  nested \u003d\n  n_cidr.extend()\n  cidrs.extend()\nexcept:\n  LOG.error\n  nested \u003d []\n```","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"820a07cbf6c819c0074d7a23a0736a8b52a96908","unresolved":false,"context_lines":[{"line_number":1390,"context_line":""},{"line_number":1391,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1392,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1393,"context_line":"                nested \u003d []"},{"line_number":1394,"context_line":"                try:"},{"line_number":1395,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1396,"context_line":"                        context, port, s_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"d4e005cc_39a92912","line":1393,"in_reply_to":"bb441f3d_a60a7ec1","updated":"2024-04-01 21:46:19.000000000","message":"Done","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"895a068457c9b2da373748090e201ef366fe669a","unresolved":true,"context_lines":[{"line_number":1394,"context_line":"                try:"},{"line_number":1395,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1396,"context_line":"                        context, port, s_id)"},{"line_number":1397,"context_line":"                except RecursionError:"},{"line_number":1398,"context_line":"                    LOG.error(\u0027Possible network loop for router %s, not \u0027"},{"line_number":1399,"context_line":"                              \u0027returning nested networks\u0027, router_id)"},{"line_number":1400,"context_line":"                if nested:"}],"source_content_type":"text/x-python","patch_set":6,"id":"df2c9232_b541767d","line":1397,"updated":"2024-03-25 20:43:42.000000000","message":"I think we should enforce the depth ourselves and not rely on python implementation to cut us short. 1) because it may be cpython specific; 2) because recursion depth is programmatically controllable, so any library we pull can in theory change it - perhaps a malicious code can be side loaded somehow to do that?; 3) because maybe it\u0027s platform dependent?\n\nLet\u0027s control the depth ourselves and raise a custom error when overflowing.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"820a07cbf6c819c0074d7a23a0736a8b52a96908","unresolved":false,"context_lines":[{"line_number":1394,"context_line":"                try:"},{"line_number":1395,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1396,"context_line":"                        context, port, s_id)"},{"line_number":1397,"context_line":"                except RecursionError:"},{"line_number":1398,"context_line":"                    LOG.error(\u0027Possible network loop for router %s, not \u0027"},{"line_number":1399,"context_line":"                              \u0027returning nested networks\u0027, router_id)"},{"line_number":1400,"context_line":"                if nested:"}],"source_content_type":"text/x-python","patch_set":6,"id":"e5088b7d_fcf4e70a","line":1397,"in_reply_to":"df2c9232_b541767d","updated":"2024-04-01 21:46:19.000000000","message":"Ack, set a depth of 20 to start.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"07ad01c1ab9aad41adbd10e2dde64d5f9590d187","unresolved":true,"context_lines":[{"line_number":1401,"context_line":"                    n_cidrs.extend(nested)"},{"line_number":1402,"context_line":"                    cidrs.extend(nested)"},{"line_number":1403,"context_line":""},{"line_number":1404,"context_line":"        return {\u0027all\u0027: cidrs, \u0027nested\u0027: n_cidrs}"},{"line_number":1405,"context_line":""},{"line_number":1406,"context_line":"    def _get_subnet_router_ports(self, context, subnet_id):"},{"line_number":1407,"context_line":"        filters \u003d {\u0027fixed_ips\u0027: {\u0027subnet_id\u0027: [subnet_id]},"}],"source_content_type":"text/x-python","patch_set":6,"id":"cfd5f59e_1399af2b","line":1404,"updated":"2024-03-25 21:16:48.000000000","message":"you can just return two values here and unpack it from the caller... no need to construct a dict.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"820a07cbf6c819c0074d7a23a0736a8b52a96908","unresolved":false,"context_lines":[{"line_number":1401,"context_line":"                    n_cidrs.extend(nested)"},{"line_number":1402,"context_line":"                    cidrs.extend(nested)"},{"line_number":1403,"context_line":""},{"line_number":1404,"context_line":"        return {\u0027all\u0027: cidrs, \u0027nested\u0027: n_cidrs}"},{"line_number":1405,"context_line":""},{"line_number":1406,"context_line":"    def _get_subnet_router_ports(self, context, subnet_id):"},{"line_number":1407,"context_line":"        filters \u003d {\u0027fixed_ips\u0027: {\u0027subnet_id\u0027: [subnet_id]},"}],"source_content_type":"text/x-python","patch_set":6,"id":"5899337a_e1167e15","line":1404,"in_reply_to":"cfd5f59e_1399af2b","updated":"2024-04-01 21:46:19.000000000","message":"Done","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"895a068457c9b2da373748090e201ef366fe669a","unresolved":true,"context_lines":[{"line_number":1412,"context_line":"                                                subnet_id):"},{"line_number":1413,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1414,"context_line":"        cidrs \u003d []"},{"line_number":1415,"context_line":"        # Get all router ports on the subnet, then filter the one passed"},{"line_number":1416,"context_line":"        all_ports \u003d self._get_subnet_router_ports(context, subnet_id)"},{"line_number":1417,"context_line":"        new_ports \u003d [p for p in all_ports if p[\u0027id\u0027] !\u003d port[\u0027id\u0027]]"},{"line_number":1418,"context_line":"        for nport in new_ports:"}],"source_content_type":"text/x-python","patch_set":6,"id":"b807783f_eff641c4","line":1415,"updated":"2024-03-25 20:43:42.000000000","message":"I don\u0027t see where you carry the list of ports already visited. Does neutron api allow to build loops? (I believe so?) If so, we can go here in circles. To avoid it, maintain a \"seen\" list and pass it down as you call recursively.","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"820a07cbf6c819c0074d7a23a0736a8b52a96908","unresolved":false,"context_lines":[{"line_number":1412,"context_line":"                                                subnet_id):"},{"line_number":1413,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1414,"context_line":"        cidrs \u003d []"},{"line_number":1415,"context_line":"        # Get all router ports on the subnet, then filter the one passed"},{"line_number":1416,"context_line":"        all_ports \u003d self._get_subnet_router_ports(context, subnet_id)"},{"line_number":1417,"context_line":"        new_ports \u003d [p for p in all_ports if p[\u0027id\u0027] !\u003d port[\u0027id\u0027]]"},{"line_number":1418,"context_line":"        for nport in new_ports:"}],"source_content_type":"text/x-python","patch_set":6,"id":"81d257b8_a4eed53a","line":1415,"in_reply_to":"b807783f_eff641c4","updated":"2024-04-01 21:46:19.000000000","message":"Done","commit_id":"9a45b1b061ca54a0d10fbda64e260ac73b8db7fa"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1398,"context_line":"                                   const.DEVICE_OWNER_HA_REPLICATED_INT,"},{"line_number":1399,"context_line":"                                   const.DEVICE_OWNER_ROUTER_HA_INTF]]"},{"line_number":1400,"context_line":""},{"line_number":1401,"context_line":"    def _get_v4_network_for_router_port(self, context, port):"},{"line_number":1402,"context_line":"        # return both a cidr and the subnet id it belongs to"},{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a82bb7f_3545ab90","line":1401,"updated":"2024-04-19 16:13:06.000000000","message":"(No action required) This function is not specific to router ports and could be renamed to reflect its more abstract meaning (e.g. get_v4_network_for_port)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1398,"context_line":"                                   const.DEVICE_OWNER_HA_REPLICATED_INT,"},{"line_number":1399,"context_line":"                                   const.DEVICE_OWNER_ROUTER_HA_INTF]]"},{"line_number":1400,"context_line":""},{"line_number":1401,"context_line":"    def _get_v4_network_for_router_port(self, context, port):"},{"line_number":1402,"context_line":"        # return both a cidr and the subnet id it belongs to"},{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"}],"source_content_type":"text/x-python","patch_set":7,"id":"73f6737a_c4f06d58","line":1401,"in_reply_to":"5a82bb7f_3545ab90","updated":"2024-04-19 21:04:19.000000000","message":"Done","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"},{"line_number":1405,"context_line":"            subnet_id \u003d fixed_ip[\u0027subnet_id\u0027]"},{"line_number":1406,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":7,"id":"411ef67e_414df97b","line":1406,"updated":"2024-04-19 16:13:06.000000000","message":"(No action required) I think we could safely avoid fetching a ipv6 subnet if we\u0027d check if fixed_ip is ipv4 by other means (e.g. netaddr library)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"},{"line_number":1405,"context_line":"            subnet_id \u003d fixed_ip[\u0027subnet_id\u0027]"},{"line_number":1406,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":7,"id":"41605c6c_57dde8eb","line":1406,"in_reply_to":"411ef67e_414df97b","updated":"2024-04-19 21:04:19.000000000","message":"Done","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1406,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1410,"context_line":"        return cidr_sid"},{"line_number":1411,"context_line":""},{"line_number":1412,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id):"}],"source_content_type":"text/x-python","patch_set":7,"id":"8d944996_7fe0aad5","line":1409,"updated":"2024-04-19 16:13:06.000000000","message":"should it return here? Why do we continue to iterate over fixed_ips?","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1406,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1410,"context_line":"        return cidr_sid"},{"line_number":1411,"context_line":""},{"line_number":1412,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id):"}],"source_content_type":"text/x-python","patch_set":7,"id":"5443b8e9_5512fdce","line":1409,"in_reply_to":"8d944996_7fe0aad5","updated":"2024-04-19 21:04:19.000000000","message":"I think it should, will change it.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1410,"context_line":"        return cidr_sid"},{"line_number":1411,"context_line":""},{"line_number":1412,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1413,"context_line":"        cidrs \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"b284621a_71ce2ed3","line":1410,"updated":"2024-04-19 16:13:06.000000000","message":"(No action required) Can\u0027t there be multiple fixed_ips of ipv4 type on the same port? Why do we return only one of them? Is it correct? \"No action required\" is because evidently it was the case before this patch.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1407,"context_line":"            if subnet[\u0027ip_version\u0027] !\u003d const.IP_VERSION_4:"},{"line_number":1408,"context_line":"                continue"},{"line_number":1409,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1410,"context_line":"        return cidr_sid"},{"line_number":1411,"context_line":""},{"line_number":1412,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1413,"context_line":"        cidrs \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"175b538a_21f1d8a2","line":1410,"in_reply_to":"b284621a_71ce2ed3","updated":"2024-04-19 21:04:19.000000000","message":"Technically there can be more than one fixed IP, so there could be some edge case we\u0027re not accounting for, i.e. they\u0027re on two different subnets. I will leave this alone for now.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1420,"context_line":"            if cidr:"},{"line_number":1421,"context_line":"                cidrs.append(cidr)"},{"line_number":1422,"context_line":""},{"line_number":1423,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1424,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1425,"context_line":"                try:"},{"line_number":1426,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("}],"source_content_type":"text/x-python","patch_set":7,"id":"bb949455_6807cb71","line":1423,"updated":"2024-04-19 16:13:06.000000000","message":"why do we want it? I don\u0027t see where the second element of the returned tuple is used in code. Why do we maintain it and return here?","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d26b79c880657a17b0e4d5db42af22d97f2d37b5","unresolved":false,"context_lines":[{"line_number":1420,"context_line":"            if cidr:"},{"line_number":1421,"context_line":"                cidrs.append(cidr)"},{"line_number":1422,"context_line":""},{"line_number":1423,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1424,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1425,"context_line":"                try:"},{"line_number":1426,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("}],"source_content_type":"text/x-python","patch_set":7,"id":"7b88be6d_cc583bf0","line":1423,"in_reply_to":"8db3b842_46663a97","updated":"2024-04-24 17:30:04.000000000","message":"That doesn\u0027t seem like a good reason to keep production code more complex than needed?..","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1420,"context_line":"            if cidr:"},{"line_number":1421,"context_line":"                cidrs.append(cidr)"},{"line_number":1422,"context_line":""},{"line_number":1423,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1424,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1425,"context_line":"                try:"},{"line_number":1426,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("}],"source_content_type":"text/x-python","patch_set":7,"id":"8db3b842_46663a97","line":1423,"in_reply_to":"bb949455_6807cb71","updated":"2024-04-19 21:04:19.000000000","message":"It\u0027s used in the functional tests.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1424,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1425,"context_line":"                try:"},{"line_number":1426,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1427,"context_line":"                        context, port, s_id, visited, 0)"},{"line_number":1428,"context_line":"                    nested_cidrs.extend(nested)"},{"line_number":1429,"context_line":"                    cidrs.extend(nested)"},{"line_number":1430,"context_line":"                except ovn_exc.RecursionDepthReached:"}],"source_content_type":"text/x-python","patch_set":7,"id":"56c017ba_ff57eae1","line":1427,"updated":"2024-04-19 16:13:06.000000000","message":"instead of hardcoding OVN_RECURSION_DEPTH inside the called function, you could pass it here (and make the called function count backwards with `depth--` until it reaches `0`, at which point you can return - or raise an exception, if you believe it\u0027s what should happen after all.)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1424,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1425,"context_line":"                try:"},{"line_number":1426,"context_line":"                    nested \u003d self._get_v4_networks_of_nested_router_ports("},{"line_number":1427,"context_line":"                        context, port, s_id, visited, 0)"},{"line_number":1428,"context_line":"                    nested_cidrs.extend(nested)"},{"line_number":1429,"context_line":"                    cidrs.extend(nested)"},{"line_number":1430,"context_line":"                except ovn_exc.RecursionDepthReached:"}],"source_content_type":"text/x-python","patch_set":7,"id":"06e313af_26c2ac27","line":1427,"in_reply_to":"56c017ba_ff57eae1","updated":"2024-04-19 21:04:19.000000000","message":"Acknowledged","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1429,"context_line":"                    cidrs.extend(nested)"},{"line_number":1430,"context_line":"                except ovn_exc.RecursionDepthReached:"},{"line_number":1431,"context_line":"                    LOG.error(\u0027Possible network loop for router %s, not \u0027"},{"line_number":1432,"context_line":"                              \u0027returning nested networks\u0027, router_id)"},{"line_number":1433,"context_line":""},{"line_number":1434,"context_line":"        return cidrs, nested_cidrs"},{"line_number":1435,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"e6a6ab28_10cd1b7d","line":1432,"updated":"2024-04-19 16:13:06.000000000","message":"this message is not very clear. \"not returning\"? but you return it below. What would an admin do with this error?","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1429,"context_line":"                    cidrs.extend(nested)"},{"line_number":1430,"context_line":"                except ovn_exc.RecursionDepthReached:"},{"line_number":1431,"context_line":"                    LOG.error(\u0027Possible network loop for router %s, not \u0027"},{"line_number":1432,"context_line":"                              \u0027returning nested networks\u0027, router_id)"},{"line_number":1433,"context_line":""},{"line_number":1434,"context_line":"        return cidrs, nested_cidrs"},{"line_number":1435,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"94307450_ca19ed2a","line":1432,"in_reply_to":"e6a6ab28_10cd1b7d","updated":"2024-04-19 21:04:19.000000000","message":"Done","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"0c80e523_070f7da4","line":1441,"updated":"2024-04-19 16:13:06.000000000","message":"the need to have two separate functions - one as \"entry point\" - _get_v4_network_of_all_router_ports - and another as \"depth\u003d1+\" - this function - is not evident to me. Both functions seem to do the same thing:\n\n- pull router ports for a router\n- remember cidrs for each of its fixed_ips\n- repeat for each of the \"other\" routers on each of the subnets\n\nI\u0027d think they could be squashed in a single function, with a default depth argument initialized to the starting position. (0)","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"97706dc28c2d52d30360b66a65c1a5ec94c6c8b5","unresolved":false,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"03013fcf_19133bec","line":1441,"in_reply_to":"06daa793_d0a3f3aa","updated":"2024-04-26 16:20:41.000000000","message":"Let me rebase and push an update, but it might need more work","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"04b3509ef39582070cf7b3cffa82e7abc1b1fb42","unresolved":false,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"06daa793_d0a3f3aa","line":1441,"in_reply_to":"085d9e39_b90164b3","updated":"2024-04-26 16:04:14.000000000","message":"Do you plan to incorporate the follow-up here?","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":true,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"c54a01e7_4bbf841c","line":1441,"in_reply_to":"0c80e523_070f7da4","updated":"2024-04-19 21:04:19.000000000","message":"Understood, and I think I pulled my hair out trying to do this previously due to the slightly different way it is filtering ports on a subnet to find just the router ports. I\u0027ll see if I can get it to work.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"7f58e387e050046c08d83eb30ea9ebac517b2c42","unresolved":false,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"085d9e39_b90164b3","line":1441,"in_reply_to":"a2c51f63_e39ee250","updated":"2024-04-25 22:34:41.000000000","message":"Ack, I\u0027ll see if I can add that in.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d26b79c880657a17b0e4d5db42af22d97f2d37b5","unresolved":true,"context_lines":[{"line_number":1438,"context_line":"                   \u0027device_owner\u0027: [const.DEVICE_OWNER_ROUTER_INTF]}"},{"line_number":1439,"context_line":"        return self._plugin.get_ports(context, filters\u003dfilters)"},{"line_number":1440,"context_line":""},{"line_number":1441,"context_line":"    def _get_v4_networks_of_nested_router_ports(self, context, port,"},{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"}],"source_content_type":"text/x-python","patch_set":7,"id":"a2c51f63_e39ee250","line":1441,"in_reply_to":"c54a01e7_4bbf841c","updated":"2024-04-24 17:30:04.000000000","message":"This is what I was thinking about: https://review.opendev.org/c/openstack/neutron/+/916939","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"bf273c4233be22ba643e49ae2eb80ab48989b762","unresolved":true,"context_lines":[{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"},{"line_number":1445,"context_line":"            raise ovn_exc.RecursionDepthReached(depth\u003ddepth)"},{"line_number":1446,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1447,"context_line":"        cidrs \u003d []"},{"line_number":1448,"context_line":"        # Get all router ports on the subnet, then filter the one passed"}],"source_content_type":"text/x-python","patch_set":7,"id":"97c794e2_8c105bea","line":1445,"updated":"2024-04-19 16:13:06.000000000","message":"why do we have to raise an error and not just return here? maybe warn in a log, but otherwise - why would anyone want to not be able to create a router because the chain is long?","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"baf7b5cb072f04c22163aa0d446ac878a778affe","unresolved":true,"context_lines":[{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"},{"line_number":1445,"context_line":"            raise ovn_exc.RecursionDepthReached(depth\u003ddepth)"},{"line_number":1446,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1447,"context_line":"        cidrs \u003d []"},{"line_number":1448,"context_line":"        # Get all router ports on the subnet, then filter the one passed"}],"source_content_type":"text/x-python","patch_set":7,"id":"fb8990ac_f15d12fb","line":1445,"in_reply_to":"97c794e2_8c105bea","updated":"2024-04-19 16:17:08.000000000","message":"OK this argument doesn\u0027t apply directly, since your caller catches the exception; but if you squash the two functions as I suggest elsewhere, then this stands.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"793b826202c08dd08a809cd3d4befca70a0f6381","unresolved":false,"context_lines":[{"line_number":1442,"context_line":"                                                subnet_id, visited, depth):"},{"line_number":1443,"context_line":"        depth +\u003d 1"},{"line_number":1444,"context_line":"        if depth \u003e OVN_RECURSION_DEPTH:"},{"line_number":1445,"context_line":"            raise ovn_exc.RecursionDepthReached(depth\u003ddepth)"},{"line_number":1446,"context_line":"        # This needs to be recursive since it could be more than one level deep"},{"line_number":1447,"context_line":"        cidrs \u003d []"},{"line_number":1448,"context_line":"        # Get all router ports on the subnet, then filter the one passed"}],"source_content_type":"text/x-python","patch_set":7,"id":"97e11638_66e70675","line":1445,"in_reply_to":"fb8990ac_f15d12fb","updated":"2024-04-19 21:04:19.000000000","message":"I think one of the reasons I had this raise was so I could easily test it. I\u0027ll noodle on it a little.","commit_id":"39b526e61aac9b9531344e633197083fd9fb239e"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"72c856f42277f72a57919c40255f62f9a52bc47b","unresolved":true,"context_lines":[{"line_number":1418,"context_line":"        for port in ports:"},{"line_number":1419,"context_line":"            cidr, s_id \u003d self._get_v4_network_for_port(context, port)"},{"line_number":1420,"context_line":"            # a set of all ports we have visited, so we don\u0027t loop"},{"line_number":1421,"context_line":"            visited \u003d set(port[\u0027id\u0027])"},{"line_number":1422,"context_line":"            if cidr:"},{"line_number":1423,"context_line":"                cidrs.append(cidr)"},{"line_number":1424,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"7027502c_e33ad31e","line":1421,"updated":"2024-04-25 21:54:03.000000000","message":"I think there is a bug here, need to split this into two operations (create and add) otherwise we end up with this:\n\n{\u00276\u0027, \u0027a\u0027, \u00277\u0027, \u00270\u0027, \u0027d\u0027, \u00274\u0027, \u0027f\u0027, \u00271\u0027, \u00272\u0027, \u00279\u0027, \u00275\u0027, \u0027b51b0bbe-7aa0-4deb-98b2-10327d6d65ed\u0027, \u00273\u0027, \u0027e\u0027, \u0027b\u0027, \u0027-\u0027}","commit_id":"392281ef0759487998242c69cf2ded850b0bf83a"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"7f58e387e050046c08d83eb30ea9ebac517b2c42","unresolved":false,"context_lines":[{"line_number":1418,"context_line":"        for port in ports:"},{"line_number":1419,"context_line":"            cidr, s_id \u003d self._get_v4_network_for_port(context, port)"},{"line_number":1420,"context_line":"            # a set of all ports we have visited, so we don\u0027t loop"},{"line_number":1421,"context_line":"            visited \u003d set(port[\u0027id\u0027])"},{"line_number":1422,"context_line":"            if cidr:"},{"line_number":1423,"context_line":"                cidrs.append(cidr)"},{"line_number":1424,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"4d2e63aa_93289fba","line":1421,"in_reply_to":"7027502c_e33ad31e","updated":"2024-04-25 22:34:41.000000000","message":"Done","commit_id":"392281ef0759487998242c69cf2ded850b0bf83a"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1400,"context_line":""},{"line_number":1401,"context_line":"    def _get_v4_network_for_port(self, context, port):"},{"line_number":1402,"context_line":"        # return both a cidr and the subnet id it belongs to"},{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"},{"line_number":1405,"context_line":"            if (netaddr.IPAddress(fixed_ip[\u0027ip_address\u0027]).version \u003d\u003d"},{"line_number":1406,"context_line":"                    const.IP_VERSION_6):"}],"source_content_type":"text/x-python","patch_set":12,"id":"a48d3d62_63ea9a04","line":1403,"updated":"2024-05-15 21:57:23.000000000","message":"the variable is not really needed; just return (none, none) at the end.","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1400,"context_line":""},{"line_number":1401,"context_line":"    def _get_v4_network_for_port(self, context, port):"},{"line_number":1402,"context_line":"        # return both a cidr and the subnet id it belongs to"},{"line_number":1403,"context_line":"        cidr_sid \u003d (None, None)"},{"line_number":1404,"context_line":"        for fixed_ip in port[\u0027fixed_ips\u0027]:"},{"line_number":1405,"context_line":"            if (netaddr.IPAddress(fixed_ip[\u0027ip_address\u0027]).version \u003d\u003d"},{"line_number":1406,"context_line":"                    const.IP_VERSION_6):"}],"source_content_type":"text/x-python","patch_set":12,"id":"cd8c2e99_dbe15620","line":1403,"in_reply_to":"a48d3d62_63ea9a04","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1407,"context_line":"                continue"},{"line_number":1408,"context_line":"            subnet_id \u003d fixed_ip[\u0027subnet_id\u0027]"},{"line_number":1409,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1410,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1411,"context_line":"            break"},{"line_number":1412,"context_line":"        return cidr_sid"},{"line_number":1413,"context_line":""},{"line_number":1414,"context_line":"    def _get_subnet_router_ports(self, context, subnet_id):"}],"source_content_type":"text/x-python","patch_set":12,"id":"2ca38443_2d3070a8","line":1411,"range":{"start_line":1410,"start_character":12,"end_line":1411,"end_character":17},"updated":"2024-05-15 21:57:23.000000000","message":"this could just return the tuple without a variable","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1407,"context_line":"                continue"},{"line_number":1408,"context_line":"            subnet_id \u003d fixed_ip[\u0027subnet_id\u0027]"},{"line_number":1409,"context_line":"            subnet \u003d self._plugin.get_subnet(context, subnet_id)"},{"line_number":1410,"context_line":"            cidr_sid \u003d (subnet[\u0027cidr\u0027], subnet[\u0027id\u0027])"},{"line_number":1411,"context_line":"            break"},{"line_number":1412,"context_line":"        return cidr_sid"},{"line_number":1413,"context_line":""},{"line_number":1414,"context_line":"    def _get_subnet_router_ports(self, context, subnet_id):"}],"source_content_type":"text/x-python","patch_set":12,"id":"b4831874_7e4c5b29","line":1411,"range":{"start_line":1410,"start_character":12,"end_line":1411,"end_character":17},"in_reply_to":"2ca38443_2d3070a8","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1419,"context_line":"    def get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1420,"context_line":"        cidrs \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1421,"context_line":"            context, router_id, None, OVN_RECURSION_DEPTH)"},{"line_number":1422,"context_line":"        if OVN_RECURSION_REACHED in cidrs:"},{"line_number":1423,"context_line":"            cidrs.remove(OVN_RECURSION_REACHED)"},{"line_number":1424,"context_line":"            LOG.error(\u0027Possible network loop for router %s, \u0027"},{"line_number":1425,"context_line":"                      \u0027please check network topology.\u0027, router_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"fe3573f0_dfec46df","line":1422,"updated":"2024-05-15 21:57:23.000000000","message":"we should not mold the \"error flag\" into the data structure since python allows us to return multiple values in a packed tuple. Instead, we can return a tuple of (result, ok), similar to how golang pattern for error handling works.\n\nSo you would:\n\n```\ncidrs, ok \u003d self._get(...)\nif not ok:\n    LOG.error()\n...\n```","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"    def get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1420,"context_line":"        cidrs \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1421,"context_line":"            context, router_id, None, OVN_RECURSION_DEPTH)"},{"line_number":1422,"context_line":"        if OVN_RECURSION_REACHED in cidrs:"},{"line_number":1423,"context_line":"            cidrs.remove(OVN_RECURSION_REACHED)"},{"line_number":1424,"context_line":"            LOG.error(\u0027Possible network loop for router %s, \u0027"},{"line_number":1425,"context_line":"                      \u0027please check network topology.\u0027, router_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"51c8c24d_08a0caa9","line":1422,"in_reply_to":"fe3573f0_dfec46df","updated":"2024-05-16 10:26:37.000000000","message":"Boo, I liked this hack :(","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1421,"context_line":"            context, router_id, None, OVN_RECURSION_DEPTH)"},{"line_number":1422,"context_line":"        if OVN_RECURSION_REACHED in cidrs:"},{"line_number":1423,"context_line":"            cidrs.remove(OVN_RECURSION_REACHED)"},{"line_number":1424,"context_line":"            LOG.error(\u0027Possible network loop for router %s, \u0027"},{"line_number":1425,"context_line":"                      \u0027please check network topology.\u0027, router_id)"},{"line_number":1426,"context_line":"        return cidrs"},{"line_number":1427,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"c7a90a70_c8f56fa1","line":1424,"updated":"2024-05-15 21:57:23.000000000","message":"is it an error or a warning?\n\nShould the message explain the implication of the situation in some more details? (mention that nested NAT may not work for deeply embedded networks.)","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1421,"context_line":"            context, router_id, None, OVN_RECURSION_DEPTH)"},{"line_number":1422,"context_line":"        if OVN_RECURSION_REACHED in cidrs:"},{"line_number":1423,"context_line":"            cidrs.remove(OVN_RECURSION_REACHED)"},{"line_number":1424,"context_line":"            LOG.error(\u0027Possible network loop for router %s, \u0027"},{"line_number":1425,"context_line":"                      \u0027please check network topology.\u0027, router_id)"},{"line_number":1426,"context_line":"        return cidrs"},{"line_number":1427,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"6f0c4576_241496a7","line":1424,"in_reply_to":"c7a90a70_c8f56fa1","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1441,"context_line":"            # Only check if not a top-level router."},{"line_number":1442,"context_line":"            gw_ports \u003d self._get_router_gw_ports(context, router_id)"},{"line_number":1443,"context_line":"            if gw_ports:"},{"line_number":1444,"context_line":"                gw_ext \u003d any(gw_port"},{"line_number":1445,"context_line":"                             for gw_port in gw_ports"},{"line_number":1446,"context_line":"                             for gw_info in self._get_gw_info(context, gw_port)"},{"line_number":1447,"context_line":"                             if gw_info.ip_version \u003d\u003d const.IP_VERSION_4)"}],"source_content_type":"text/x-python","patch_set":12,"id":"930adb1a_e5d6cca4","line":1444,"updated":"2024-05-15 21:57:23.000000000","message":"nit: could avoid the variable (just do `if any`), esp. since the name of the variable is not very clear - was it supposed to be a flag of existence or the port itself?.. (I know i\u0027s a boolean flag, but the name doesn\u0027t imply it.)","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1441,"context_line":"            # Only check if not a top-level router."},{"line_number":1442,"context_line":"            gw_ports \u003d self._get_router_gw_ports(context, router_id)"},{"line_number":1443,"context_line":"            if gw_ports:"},{"line_number":1444,"context_line":"                gw_ext \u003d any(gw_port"},{"line_number":1445,"context_line":"                             for gw_port in gw_ports"},{"line_number":1446,"context_line":"                             for gw_info in self._get_gw_info(context, gw_port)"},{"line_number":1447,"context_line":"                             if gw_info.ip_version \u003d\u003d const.IP_VERSION_4)"}],"source_content_type":"text/x-python","patch_set":12,"id":"6a3e2d48_99d5a9bf","line":1444,"in_reply_to":"930adb1a_e5d6cca4","updated":"2024-05-16 10:26:37.000000000","message":"Was just supposed to be a flag of existence, will fix.","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1456,"context_line":"                cidrs.add(cidr)"},{"line_number":1457,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1458,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1459,"context_line":"                for rport in self._get_subnet_router_ports(context, s_id):"},{"line_number":1460,"context_line":"                    # add to the set of ports we have visited, so we don\u0027t loop"},{"line_number":1461,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1462,"context_line":"                    # From here on down we want to collect all cidrs as"}],"source_content_type":"text/x-python","patch_set":12,"id":"f7a42709_daf97812","line":1459,"updated":"2024-05-15 21:57:23.000000000","message":"should we iterate further if `depth` is already `1` here? we may short-circuit here without executing this `for` block.","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1456,"context_line":"                cidrs.add(cidr)"},{"line_number":1457,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1458,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1459,"context_line":"                for rport in self._get_subnet_router_ports(context, s_id):"},{"line_number":1460,"context_line":"                    # add to the set of ports we have visited, so we don\u0027t loop"},{"line_number":1461,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1462,"context_line":"                    # From here on down we want to collect all cidrs as"}],"source_content_type":"text/x-python","patch_set":12,"id":"6b68ff45_0f3dca24","line":1459,"in_reply_to":"f7a42709_daf97812","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":1463,"context_line":"                    # nested cidrs."},{"line_number":1464,"context_line":"                    nested \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1465,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1466,"context_line":"                    if nested:"},{"line_number":1467,"context_line":"                        cidrs.update(nested)"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"        return cidrs"}],"source_content_type":"text/x-python","patch_set":12,"id":"ffe2bf0e_956da952","line":1466,"updated":"2024-05-15 21:57:23.000000000","message":"nit: `if` check here is probably redundant: `update()` on an empty set will do nothing.","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":1463,"context_line":"                    # nested cidrs."},{"line_number":1464,"context_line":"                    nested \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1465,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1466,"context_line":"                    if nested:"},{"line_number":1467,"context_line":"                        cidrs.update(nested)"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"        return cidrs"}],"source_content_type":"text/x-python","patch_set":12,"id":"74dfd53f_988c4363","line":1466,"in_reply_to":"ffe2bf0e_956da952","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"a47779b1cdf0defbcc1e2740c61ead0fc3e416b0","unresolved":true,"context_lines":[{"line_number":1457,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1458,"context_line":"                    # From here on down we want to collect all cidrs as"},{"line_number":1459,"context_line":"                    # nested cidrs."},{"line_number":1460,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1461,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1462,"context_line":"                    cidrs.update(nested)"},{"line_number":1463,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"8fffa0b3_06488dfb","line":1460,"updated":"2024-05-16 19:58:19.000000000","message":"btw this is never used (and is never True because we short-circuit in line 1452.)\n\nYou could probably replace `limit` with `_` here, remove the variable, and return `False` in line 1464.","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"92e6ee497657977109001a47bc2005f78fa508ab","unresolved":false,"context_lines":[{"line_number":1457,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1458,"context_line":"                    # From here on down we want to collect all cidrs as"},{"line_number":1459,"context_line":"                    # nested cidrs."},{"line_number":1460,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1461,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1462,"context_line":"                    cidrs.update(nested)"},{"line_number":1463,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3cde465a_64221802","line":1460,"in_reply_to":"3dc2208f_034061fe","updated":"2024-05-20 22:33:58.000000000","message":"Done","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"829629488e2ae9398c7025a936077c29dc254546","unresolved":true,"context_lines":[{"line_number":1457,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1458,"context_line":"                    # From here on down we want to collect all cidrs as"},{"line_number":1459,"context_line":"                    # nested cidrs."},{"line_number":1460,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1461,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1462,"context_line":"                    cidrs.update(nested)"},{"line_number":1463,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"e6e55b67_9ed7dfe0","line":1460,"in_reply_to":"8fffa0b3_06488dfb","updated":"2024-05-16 20:01:21.000000000","message":"Disregard. It is used. But if so, isn\u0027t there a bug somewhere here since you always override `limit`, instead of accumulating it? Shouldn\u0027t you do:\n\n```\nnested, limit_ \u003d ...\nlimit |\u003d limit_\n```\n\n?","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"dfbe992ac29e9d6045ede7c3bd8c3d0545abad84","unresolved":true,"context_lines":[{"line_number":1457,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1458,"context_line":"                    # From here on down we want to collect all cidrs as"},{"line_number":1459,"context_line":"                    # nested cidrs."},{"line_number":1460,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1461,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1462,"context_line":"                    cidrs.update(nested)"},{"line_number":1463,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3dc2208f_034061fe","line":1460,"in_reply_to":"ddb1adc6_71dfbf33","updated":"2024-05-20 19:32:48.000000000","message":"Yes it\u0027s not tested; I don\u0027t think reverting to the previous version is a good idea since the flag and the set of networks are two different types of data and should be treated separately.","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"891816afb2ae9bb6eb33aa1c0c1afb93ca995c3a","unresolved":true,"context_lines":[{"line_number":1457,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1458,"context_line":"                    # From here on down we want to collect all cidrs as"},{"line_number":1459,"context_line":"                    # nested cidrs."},{"line_number":1460,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1461,"context_line":"                        context, rport[\u0027device_id\u0027], visited, depth - 1)"},{"line_number":1462,"context_line":"                    cidrs.update(nested)"},{"line_number":1463,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ddb1adc6_71dfbf33","line":1460,"in_reply_to":"e6e55b67_9ed7dfe0","updated":"2024-05-20 18:19:47.000000000","message":"True, if there were multiple routers and only one hit the depth limit, we could still return False depending on the order. I can update, but I guess the unit tests aren\u0027t testing that case either.\n\nMakes my previous version putting a special key in the set seem simpler 😐","commit_id":"1dacb75b63fdfe17c87d6665f2711e672ae2fd41"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"92e6ee497657977109001a47bc2005f78fa508ab","unresolved":true,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"OVN_RECURSION_LIMIT \u003d 20"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"OvnPortInfo \u003d collections.namedtuple("}],"source_content_type":"text/x-python","patch_set":14,"id":"a4d13dbd_d96e05f1","line":68,"updated":"2024-05-20 22:33:58.000000000","message":"I just wanted to point out this change, reflected in the code below. Under stress testing where I just kept creating entire subnets filled with router ports nested in each other, things got very slow and eventually hit a length limit for a set (I think):\n\n    ValueError: Length too long: 6836941\n\nFor that reason it just counts cidrs now instead of depth.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"4aa4127e30fd3a627a5666523ed1ca779385272b","unresolved":true,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"OVN_RECURSION_LIMIT \u003d 20"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"OvnPortInfo \u003d collections.namedtuple("}],"source_content_type":"text/x-python","patch_set":14,"id":"f7b3c80e_d5a816af","line":68,"in_reply_to":"a4d13dbd_d96e05f1","updated":"2024-05-20 22:36:17.000000000","message":"Hm. I wonder if maybe `.copy()` the set on each iteration that I introduced is not a great idea. It may add unnecessary overhead.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"87f3bb18dc9a839fd1781c0858013db825fbe584","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"OVN_RECURSION_LIMIT \u003d 20"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"OvnPortInfo \u003d collections.namedtuple("}],"source_content_type":"text/x-python","patch_set":14,"id":"bb054bab_d78c6891","line":68,"in_reply_to":"be2c273e_302531a1","updated":"2024-06-06 20:48:41.000000000","message":"Done","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"bd61ed331bdbe566bd9dac4ca5626941fa162f4d","unresolved":true,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"OVN_RECURSION_LIMIT \u003d 20"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"OvnPortInfo \u003d collections.namedtuple("}],"source_content_type":"text/x-python","patch_set":14,"id":"be2c273e_302531a1","line":68,"in_reply_to":"f7b3c80e_d5a816af","updated":"2024-05-22 20:47:41.000000000","message":"I can remove the .copy() but don\u0027t know if I\u0027ll be able to recreate the test code that triggered the failure as easily, it was an intermediate state.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"a7697f7f363359a0fa26198982b044cd323c4944","unresolved":true,"context_lines":[{"line_number":1465,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1466,"context_line":"                        context, rport[\u0027device_id\u0027], cidrs, visited)"},{"line_number":1467,"context_line":"                    cidrs.update(nested)"},{"line_number":1468,"context_line":"                    if limit is True:"},{"line_number":1469,"context_line":"                        return (cidrs, True)"},{"line_number":1470,"context_line":""},{"line_number":1471,"context_line":"        return (cidrs, limit)"}],"source_content_type":"text/x-python","patch_set":14,"id":"ec96cba4_9196a196","line":1468,"updated":"2024-05-20 22:34:42.000000000","message":"hm. is it how we would want to handle this though? (returning immediately?)\n\nAFAIU this makes the behavior depend on the order of router port iteration (in line 1460) since it will seize from iterating on the first limit reached; leaving the other ports not configured for snat at all, even they maybe don\u0027t reach the limit.\n\nI was thinking that we would accumulate `limit` values, (set `limit |\u003d limit_`) but let the rest of router ports to be configured; then return the final accumulated `limit` (already done in line 1471).\n\nIn this way, we are not leaving some routing paths not configured for SNAT just because some OTHER path is too deep. What do you think?\n\n---\n\nthis is probably irrelevant based on the above, but also nit:\n\n```\nif limit:\n  ...\n```","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"02df5be56c07a90a4fd835d4624b65179734e606","unresolved":true,"context_lines":[{"line_number":1465,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1466,"context_line":"                        context, rport[\u0027device_id\u0027], cidrs, visited)"},{"line_number":1467,"context_line":"                    cidrs.update(nested)"},{"line_number":1468,"context_line":"                    if limit is True:"},{"line_number":1469,"context_line":"                        return (cidrs, True)"},{"line_number":1470,"context_line":""},{"line_number":1471,"context_line":"        return (cidrs, limit)"}],"source_content_type":"text/x-python","patch_set":14,"id":"e6b0554e_e0a6dd36","line":1468,"in_reply_to":"91ec7bbb_d4cb15d1","updated":"2024-06-05 20:10:17.000000000","message":"(Sorry for boiling the water, I\u0027m just scared of the whole recursion with limits thing...)\n\nI\u0027d like the behavior to not depend on the order of resources iteration. Whatever the behavior is, I\u0027d like it to be consistent.\n\nThe scenario you are concerned about (proceeding iterating in depth after the limit is hit SOMEWHERE) is a problem, and that\u0027s why I was always feeling uneasy about a deep recursive walk. You are probably right that cutting at a particular number of discovered subnets is safer - O(1) vs O(n^2)? -or is it even r^n potentially?\n\nConsidering all this, I would 1) search for up to N subnets; if N+1 are discovered, bail out + warn + don\u0027t configure any nested snats at all.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b3c758e14184223e0153d101987f54b52762c04a","unresolved":false,"context_lines":[{"line_number":1465,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1466,"context_line":"                        context, rport[\u0027device_id\u0027], cidrs, visited)"},{"line_number":1467,"context_line":"                    cidrs.update(nested)"},{"line_number":1468,"context_line":"                    if limit is True:"},{"line_number":1469,"context_line":"                        return (cidrs, True)"},{"line_number":1470,"context_line":""},{"line_number":1471,"context_line":"        return (cidrs, limit)"}],"source_content_type":"text/x-python","patch_set":14,"id":"76bab8c8_821b7a7f","line":1468,"in_reply_to":"e6b0554e_e0a6dd36","updated":"2024-06-06 02:20:19.000000000","message":"That is my plan for the next patch, I just didn\u0027t send since the gate was wonky. But basically succeed if 50 subnets, fail if 51+ and LOG.error() - I chose that over warning but aren\u0027t tied to it.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"bd61ed331bdbe566bd9dac4ca5626941fa162f4d","unresolved":true,"context_lines":[{"line_number":1465,"context_line":"                    nested, limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1466,"context_line":"                        context, rport[\u0027device_id\u0027], cidrs, visited)"},{"line_number":1467,"context_line":"                    cidrs.update(nested)"},{"line_number":1468,"context_line":"                    if limit is True:"},{"line_number":1469,"context_line":"                        return (cidrs, True)"},{"line_number":1470,"context_line":""},{"line_number":1471,"context_line":"        return (cidrs, limit)"}],"source_content_type":"text/x-python","patch_set":14,"id":"91ec7bbb_d4cb15d1","line":1468,"in_reply_to":"ec96cba4_9196a196","updated":"2024-05-22 20:47:41.000000000","message":"I don\u0027t know what the best thing to do is here.\n\nIf we, for example, don\u0027t return here but OR the values, we don\u0027t know how many loops we\u0027ll do - a 10/8 has 16 million ports. Of course the quota limit will probably be reached well before then, it\u0027s just a worst-case scenario.\n\nThe default quota for routers is 10 I believe, and I used 20. I could use 50? There\u0027s only so much of the ocean I can boil.","commit_id":"3bed312ac1631b9607f0224bae6cadc5aaf6c8e8"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"02df5be56c07a90a4fd835d4624b65179734e606","unresolved":true,"context_lines":[{"line_number":1456,"context_line":"                cidrs.add(cidr)"},{"line_number":1457,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1458,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1459,"context_line":"                for rport in self._get_subnet_router_ports(context, s_id):"},{"line_number":1460,"context_line":"                    # add to the set of ports we have visited, so we don\u0027t loop"},{"line_number":1461,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1462,"context_line":"                    hit_limit \u003d self._get_v4_network_of_all_router_ports("}],"source_content_type":"text/x-python","patch_set":18,"id":"94b7f679_7cb5c96d","line":1459,"updated":"2024-06-05 20:10:17.000000000","message":"the behavior here depends on the (random) order in which ports are returned; when we hit the limit, some subnets will be configured for snat, some won\u0027t, with no way to predict the actual behavior. This is concerning. I think we should either configure everything (perhaps up to a depth - as was implemented before) or just not configure any nested snat at all. But not what this revision of the patch is doing. (Letting the chance throw a dice on which snats will be set.)","commit_id":"5713642867137f69eb108360ed07d67649136f1d"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b3c758e14184223e0153d101987f54b52762c04a","unresolved":false,"context_lines":[{"line_number":1456,"context_line":"                cidrs.add(cidr)"},{"line_number":1457,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."},{"line_number":1458,"context_line":"                # routers on this network with an interface on another network."},{"line_number":1459,"context_line":"                for rport in self._get_subnet_router_ports(context, s_id):"},{"line_number":1460,"context_line":"                    # add to the set of ports we have visited, so we don\u0027t loop"},{"line_number":1461,"context_line":"                    visited.add(rport[\u0027id\u0027])"},{"line_number":1462,"context_line":"                    hit_limit \u003d self._get_v4_network_of_all_router_ports("}],"source_content_type":"text/x-python","patch_set":18,"id":"b0a8ce3c_585b5a91","line":1459,"in_reply_to":"94b7f679_7cb5c96d","updated":"2024-06-06 02:20:19.000000000","message":"See my other comment :)","commit_id":"5713642867137f69eb108360ed07d67649136f1d"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d53aa9a9824127c6c9bab1b728597d2711327654","unresolved":true,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"# We used 50 as it is 5x the default router quota value of 10"},{"line_number":69,"context_line":"OVN_ROUTER_RECURSION_LIMIT \u003d 50"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"96df9194_31335a0f","line":68,"updated":"2024-06-11 21:30:08.000000000","message":"then should we multiply DEFAULT_QUOTA_ROUTER by 5 here?","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"91c825a085e72045bd3c49b88555d344eb453b50","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"# The number of router port cidrs we will return on a call to"},{"line_number":67,"context_line":"# get_v4_network_of_all_router_ports(), in case there are nested networks"},{"line_number":68,"context_line":"# We used 50 as it is 5x the default router quota value of 10"},{"line_number":69,"context_line":"OVN_ROUTER_RECURSION_LIMIT \u003d 50"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"99e67c89_f94bfd4c","line":68,"in_reply_to":"96df9194_31335a0f","updated":"2024-06-12 01:36:21.000000000","message":"Done","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d53aa9a9824127c6c9bab1b728597d2711327654","unresolved":true,"context_lines":[{"line_number":1426,"context_line":"                        \u0027for router %s, SNAT will not be enabled for subnets. \u0027"},{"line_number":1427,"context_line":"                        \u0027Please check network topology.\u0027,"},{"line_number":1428,"context_line":"                        router_id)"},{"line_number":1429,"context_line":"            return set()"},{"line_number":1430,"context_line":"        return cidrs"},{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id,"}],"source_content_type":"text/x-python","patch_set":19,"id":"7a6bb39c_e392e8b5","line":1429,"updated":"2024-06-11 21:30:08.000000000","message":"wonder if fallback should be \"configure snat for first level\" (as prior this patch), not disabling snat for everything. Otherwise, a user with nested networks hitting this corner case will lose its connectivity without any action on their part.","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"91c825a085e72045bd3c49b88555d344eb453b50","unresolved":false,"context_lines":[{"line_number":1426,"context_line":"                        \u0027for router %s, SNAT will not be enabled for subnets. \u0027"},{"line_number":1427,"context_line":"                        \u0027Please check network topology.\u0027,"},{"line_number":1428,"context_line":"                        router_id)"},{"line_number":1429,"context_line":"            return set()"},{"line_number":1430,"context_line":"        return cidrs"},{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    def _get_v4_network_of_all_router_ports(self, context, router_id,"}],"source_content_type":"text/x-python","patch_set":19,"id":"595611e1_27a914f2","line":1429,"in_reply_to":"7a6bb39c_e392e8b5","updated":"2024-06-12 01:36:21.000000000","message":"Done","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d53aa9a9824127c6c9bab1b728597d2711327654","unresolved":true,"context_lines":[{"line_number":1435,"context_line":"        # have been found, else False, adding to \u0027cidrs\u0027 and \u0027visited\u0027 as it"},{"line_number":1436,"context_line":"        # recurses through the networks."},{"line_number":1437,"context_line":""},{"line_number":1438,"context_line":"        if len(cidrs) \u003e 0:"},{"line_number":1439,"context_line":"            # If this router has an external gateway port it should"},{"line_number":1440,"context_line":"            # not be included in the list for additional SNAT entries."},{"line_number":1441,"context_line":"            # Only check if not a top-level router."}],"source_content_type":"text/x-python","patch_set":19,"id":"fb1aacaf_80030478","line":1438,"updated":"2024-06-11 21:30:08.000000000","message":"nit: `if cidrs:`","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"91c825a085e72045bd3c49b88555d344eb453b50","unresolved":false,"context_lines":[{"line_number":1435,"context_line":"        # have been found, else False, adding to \u0027cidrs\u0027 and \u0027visited\u0027 as it"},{"line_number":1436,"context_line":"        # recurses through the networks."},{"line_number":1437,"context_line":""},{"line_number":1438,"context_line":"        if len(cidrs) \u003e 0:"},{"line_number":1439,"context_line":"            # If this router has an external gateway port it should"},{"line_number":1440,"context_line":"            # not be included in the list for additional SNAT entries."},{"line_number":1441,"context_line":"            # Only check if not a top-level router."}],"source_content_type":"text/x-python","patch_set":19,"id":"c209e46a_2457e8e9","line":1438,"in_reply_to":"fb1aacaf_80030478","updated":"2024-06-12 01:36:21.000000000","message":"Done","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d53aa9a9824127c6c9bab1b728597d2711327654","unresolved":true,"context_lines":[{"line_number":1452,"context_line":"            cidr, s_id \u003d self._get_v4_network_for_port(context, port)"},{"line_number":1453,"context_line":"            if cidr:"},{"line_number":1454,"context_line":"                # Do not add this cidr, we are at the limit"},{"line_number":1455,"context_line":"                if len(cidrs) \u003d\u003d OVN_ROUTER_RECURSION_LIMIT:"},{"line_number":1456,"context_line":"                    return True"},{"line_number":1457,"context_line":"                cidrs.add(cidr)"},{"line_number":1458,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."}],"source_content_type":"text/x-python","patch_set":19,"id":"e2c995a3_022118cd","line":1455,"updated":"2024-06-11 21:30:08.000000000","message":"nit: you could probably run this check a bit earlier, in line 1450. Saves one `_get_v4_network_for_port` call.","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"91c825a085e72045bd3c49b88555d344eb453b50","unresolved":false,"context_lines":[{"line_number":1452,"context_line":"            cidr, s_id \u003d self._get_v4_network_for_port(context, port)"},{"line_number":1453,"context_line":"            if cidr:"},{"line_number":1454,"context_line":"                # Do not add this cidr, we are at the limit"},{"line_number":1455,"context_line":"                if len(cidrs) \u003d\u003d OVN_ROUTER_RECURSION_LIMIT:"},{"line_number":1456,"context_line":"                    return True"},{"line_number":1457,"context_line":"                cidrs.add(cidr)"},{"line_number":1458,"context_line":"                # We also want to get network cidrs of \"nested\" routers, i.e."}],"source_content_type":"text/x-python","patch_set":19,"id":"d30005e5_110b24e5","line":1455,"in_reply_to":"e2c995a3_022118cd","updated":"2024-06-12 01:36:21.000000000","message":"The reason I put this here is that we don\u0027t know if _get_v4_network_for_port() will return any cidrs, so have to wait until we know we\u0027ll cross the limit.","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"d53aa9a9824127c6c9bab1b728597d2711327654","unresolved":true,"context_lines":[{"line_number":1465,"context_line":"                    if hit_limit:"},{"line_number":1466,"context_line":"                        return True"},{"line_number":1467,"context_line":""},{"line_number":1468,"context_line":"        return hit_limit"},{"line_number":1469,"context_line":""},{"line_number":1470,"context_line":"    def _gen_router_ext_ids(self, router):"},{"line_number":1471,"context_line":"        return {"}],"source_content_type":"text/x-python","patch_set":19,"id":"f00aa252_30da548a","line":1468,"updated":"2024-06-11 21:30:08.000000000","message":"(no action required) nit: I think this will always be `False`, so you could just `return False` and not initialize the variable in line 1448 either.","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"91c825a085e72045bd3c49b88555d344eb453b50","unresolved":false,"context_lines":[{"line_number":1465,"context_line":"                    if hit_limit:"},{"line_number":1466,"context_line":"                        return True"},{"line_number":1467,"context_line":""},{"line_number":1468,"context_line":"        return hit_limit"},{"line_number":1469,"context_line":""},{"line_number":1470,"context_line":"    def _gen_router_ext_ids(self, router):"},{"line_number":1471,"context_line":"        return {"}],"source_content_type":"text/x-python","patch_set":19,"id":"6448134f_bd80e058","line":1468,"in_reply_to":"f00aa252_30da548a","updated":"2024-06-12 01:36:21.000000000","message":"Done","commit_id":"b1ee966bbdaaf004cdd40a12f1337c0a2bca66e1"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"be66c69015bfa7c5189064a80fec5983b4955b75","unresolved":true,"context_lines":[{"line_number":1420,"context_line":"    def get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1421,"context_line":"        cidrs \u003d set()"},{"line_number":1422,"context_line":"        visited \u003d set()"},{"line_number":1423,"context_line":"        recurse \u003d False"},{"line_number":1424,"context_line":"        if ovn_conf.is_ovn_router_indirect_snat_enabled():"},{"line_number":1425,"context_line":"            recurse \u003d True"},{"line_number":1426,"context_line":"        hit_limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1427,"context_line":"            context, router_id, cidrs, visited, recurse)"},{"line_number":1428,"context_line":"        if hit_limit and recurse:"}],"source_content_type":"text/x-python","patch_set":24,"id":"0fb7818b_2f761fec","line":1425,"range":{"start_line":1423,"start_character":8,"end_line":1425,"end_character":26},"updated":"2024-07-02 15:53:21.000000000","message":"nit\n\n```\nrecurse \u003d ovn_conf.is_ovn_router_indirect_snat_enabled()\n```","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"d1bf0dd91189a6c7aac1d4cdfcae75831a0c0840","unresolved":false,"context_lines":[{"line_number":1420,"context_line":"    def get_v4_network_of_all_router_ports(self, context, router_id):"},{"line_number":1421,"context_line":"        cidrs \u003d set()"},{"line_number":1422,"context_line":"        visited \u003d set()"},{"line_number":1423,"context_line":"        recurse \u003d False"},{"line_number":1424,"context_line":"        if ovn_conf.is_ovn_router_indirect_snat_enabled():"},{"line_number":1425,"context_line":"            recurse \u003d True"},{"line_number":1426,"context_line":"        hit_limit \u003d self._get_v4_network_of_all_router_ports("},{"line_number":1427,"context_line":"            context, router_id, cidrs, visited, recurse)"},{"line_number":1428,"context_line":"        if hit_limit and recurse:"}],"source_content_type":"text/x-python","patch_set":24,"id":"331ae0f1_8c9a260b","line":1425,"range":{"start_line":1423,"start_character":8,"end_line":1425,"end_character":26},"in_reply_to":"0fb7818b_2f761fec","updated":"2024-07-02 17:09:55.000000000","message":"Done","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"91f2576bb63dfa62ecfe8207ad70c009eb9aa63d","unresolved":true,"context_lines":[{"line_number":1495,"context_line":"                \u0027,\u0027.join(common_utils.get_az_hints(router)),"},{"line_number":1496,"context_line":"        }"},{"line_number":1497,"context_line":""},{"line_number":1498,"context_line":"    def create_router(self, context, router, add_external_gateway\u003dTrue):"},{"line_number":1499,"context_line":"        \"\"\"Create a logical router.\"\"\""},{"line_number":1500,"context_line":"        external_ids \u003d self._gen_router_ext_ids(router)"},{"line_number":1501,"context_line":"        enabled \u003d router.get(\u0027admin_state_up\u0027)"}],"source_content_type":"text/x-python","patch_set":25,"id":"bff392ec_ac2bac62","line":1498,"updated":"2024-08-14 23:19:17.000000000","message":"the reason why this patch doesn\u0027t work unless repair is enabled is that only create_router and update_router are plugged to this nested network walk function, so when a new port is attached, snat are not recalculated.\n\nI think we will need to list all possible ways the new snat list can be affected (through router port attachments) and make sure snat records are updated then. I think create_router_port and update_router_port and delete_router_port should be covered.\n\nThe general idea is to look at all inputs (incl. db reads) of the recursive walk function and update snat entries every time any of the inputs is updated. This includes both the presence (create and delete) as well as substance (fixed_ips) of router ports.","commit_id":"77cab6c48ef39f9b7187cb7fbce7f9d1a935d386"}],"neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e7974e821efeb5e2a41becae5a6de5af5e379ada","unresolved":true,"context_lines":[{"line_number":1307,"context_line":"                    for nat in nats]"},{"line_number":1308,"context_line":"                for net in r_nested:"},{"line_number":1309,"context_line":"                    if net not in plugin_nats:"},{"line_number":1310,"context_line":"                        plugin_nats.extend(r_nested)"},{"line_number":1311,"context_line":"            except idlutils.RowNotFound:"},{"line_number":1312,"context_line":"                plugin_lrouter_port_ids \u003d []"},{"line_number":1313,"context_line":"                plugin_routes \u003d []"}],"source_content_type":"text/x-python","patch_set":10,"id":"698de365_fe50010d","line":1310,"range":{"start_line":1310,"start_character":36,"end_line":1310,"end_character":52},"updated":"2024-05-09 13:23:46.000000000","message":"s/append(net)","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"1c01c7506f16f2821e476b4a89ff65d0930f136b","unresolved":false,"context_lines":[{"line_number":1307,"context_line":"                    for nat in nats]"},{"line_number":1308,"context_line":"                for net in r_nested:"},{"line_number":1309,"context_line":"                    if net not in plugin_nats:"},{"line_number":1310,"context_line":"                        plugin_nats.extend(r_nested)"},{"line_number":1311,"context_line":"            except idlutils.RowNotFound:"},{"line_number":1312,"context_line":"                plugin_lrouter_port_ids \u003d []"},{"line_number":1313,"context_line":"                plugin_routes \u003d []"}],"source_content_type":"text/x-python","patch_set":10,"id":"d5ba52fc_a324303c","line":1310,"range":{"start_line":1310,"start_character":36,"end_line":1310,"end_character":52},"in_reply_to":"698de365_fe50010d","updated":"2024-05-13 10:06:34.000000000","message":"Done","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e7974e821efeb5e2a41becae5a6de5af5e379ada","unresolved":true,"context_lines":[{"line_number":1337,"context_line":"                    for nat in nats]"},{"line_number":1338,"context_line":"                for net in r_nested:"},{"line_number":1339,"context_line":"                    if net not in monitor_nats:"},{"line_number":1340,"context_line":"                        monitor_nats.extend(r_nested)"},{"line_number":1341,"context_line":"            except idlutils.RowNotFound:"},{"line_number":1342,"context_line":"                monitor_lrouter_port_ids \u003d []"},{"line_number":1343,"context_line":"                monitor_routes \u003d []"}],"source_content_type":"text/x-python","patch_set":10,"id":"6b9da96d_8430acfb","line":1340,"range":{"start_line":1340,"start_character":37,"end_line":1340,"end_character":53},"updated":"2024-05-09 13:23:46.000000000","message":"s/append(net)","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"1c01c7506f16f2821e476b4a89ff65d0930f136b","unresolved":false,"context_lines":[{"line_number":1337,"context_line":"                    for nat in nats]"},{"line_number":1338,"context_line":"                for net in r_nested:"},{"line_number":1339,"context_line":"                    if net not in monitor_nats:"},{"line_number":1340,"context_line":"                        monitor_nats.extend(r_nested)"},{"line_number":1341,"context_line":"            except idlutils.RowNotFound:"},{"line_number":1342,"context_line":"                monitor_lrouter_port_ids \u003d []"},{"line_number":1343,"context_line":"                monitor_routes \u003d []"}],"source_content_type":"text/x-python","patch_set":10,"id":"83101bc9_f8d401af","line":1340,"range":{"start_line":1340,"start_character":37,"end_line":1340,"end_character":53},"in_reply_to":"6b9da96d_8430acfb","updated":"2024-05-13 10:06:34.000000000","message":"Done","commit_id":"78915bd1bedd0d8f9c945aa63dad6f9603e00cdb"}],"neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"be66c69015bfa7c5189064a80fec5983b4955b75","unresolved":true,"context_lines":[{"line_number":346,"context_line":"        # number of router ports"},{"line_number":347,"context_line":"        def mock__get_router_ports(*args, **kwargs):"},{"line_number":348,"context_line":"            self.grp_calls +\u003d 1"},{"line_number":349,"context_line":"            if self.grp_calls \u003e limit:"},{"line_number":350,"context_line":"                return []"},{"line_number":351,"context_line":"            return [{\u0027id\u0027: str(uuid.uuid4())}]"},{"line_number":352,"context_line":""}],"source_content_type":"text/x-python","patch_set":24,"id":"b08d64b9_375b79d2","line":349,"updated":"2024-07-02 15:53:21.000000000","message":"Shouldn\u0027t you keep the mock identical for both scenarios (indirect\u003dTrue/False) while only making sure that len(cidrs) \u003d\u003d 1 when indirect\u003dFalse and \u003d\u003d 5 when it\u0027s True? (in line 358). Otherwise I think you don\u0027t give a chance to your function under test to misbehave (by continuing the recursive walk despite that indirect\u003dFalse).","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"d1bf0dd91189a6c7aac1d4cdfcae75831a0c0840","unresolved":false,"context_lines":[{"line_number":346,"context_line":"        # number of router ports"},{"line_number":347,"context_line":"        def mock__get_router_ports(*args, **kwargs):"},{"line_number":348,"context_line":"            self.grp_calls +\u003d 1"},{"line_number":349,"context_line":"            if self.grp_calls \u003e limit:"},{"line_number":350,"context_line":"                return []"},{"line_number":351,"context_line":"            return [{\u0027id\u0027: str(uuid.uuid4())}]"},{"line_number":352,"context_line":""}],"source_content_type":"text/x-python","patch_set":24,"id":"4ac17f32_44d79866","line":349,"in_reply_to":"b08d64b9_375b79d2","updated":"2024-07-02 17:09:55.000000000","message":"Done","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"be66c69015bfa7c5189064a80fec5983b4955b75","unresolved":true,"context_lines":[{"line_number":374,"context_line":"        # number of router ports"},{"line_number":375,"context_line":"        def mock__get_router_ports(*args, **kwargs):"},{"line_number":376,"context_line":"            self.grp_calls +\u003d 1"},{"line_number":377,"context_line":"            if self.grp_calls \u003e limit:"},{"line_number":378,"context_line":"                return []"},{"line_number":379,"context_line":"            return [{\u0027id\u0027: str(uuid.uuid4())}]"},{"line_number":380,"context_line":""}],"source_content_type":"text/x-python","patch_set":24,"id":"a435e48a_f3359990","line":377,"updated":"2024-07-02 15:53:21.000000000","message":"same","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"d1bf0dd91189a6c7aac1d4cdfcae75831a0c0840","unresolved":false,"context_lines":[{"line_number":374,"context_line":"        # number of router ports"},{"line_number":375,"context_line":"        def mock__get_router_ports(*args, **kwargs):"},{"line_number":376,"context_line":"            self.grp_calls +\u003d 1"},{"line_number":377,"context_line":"            if self.grp_calls \u003e limit:"},{"line_number":378,"context_line":"                return []"},{"line_number":379,"context_line":"            return [{\u0027id\u0027: str(uuid.uuid4())}]"},{"line_number":380,"context_line":""}],"source_content_type":"text/x-python","patch_set":24,"id":"fb134930_6e9c1c66","line":377,"in_reply_to":"a435e48a_f3359990","updated":"2024-07-02 17:09:55.000000000","message":"Done","commit_id":"d9102d262eb2f253efc333d433f53b20371c2d1f"}],"neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":254,"context_line":"                                  \u0027ip_address\u0027: \u0027100.0.0.2\u0027}]}},"},{"line_number":255,"context_line":"                        {\u0027id\u0027: \u0027r4\u0027, \u0027routes\u0027: []},"},{"line_number":256,"context_line":"                        # Router r5 is \"nested\" on the same subnet as Router"},{"line_number":257,"context_line":"                        # r1, 192.168.1.0/24, so it\u0027s default route is the"},{"line_number":258,"context_line":"                        # internal interface of Router r1. It has no external"},{"line_number":259,"context_line":"                        # gateway, but relies on that of Router r1.."},{"line_number":260,"context_line":"                        {\u0027id\u0027: \u0027r5\u0027, \u0027routes\u0027: [{\u0027nexthop\u0027: \u0027192.168.1.1\u0027,"}],"source_content_type":"text/x-python","patch_set":12,"id":"f67f6a72_b6173487","line":257,"range":{"start_line":257,"start_character":49,"end_line":257,"end_character":53},"updated":"2024-05-15 21:57:23.000000000","message":"its","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":254,"context_line":"                                  \u0027ip_address\u0027: \u0027100.0.0.2\u0027}]}},"},{"line_number":255,"context_line":"                        {\u0027id\u0027: \u0027r4\u0027, \u0027routes\u0027: []},"},{"line_number":256,"context_line":"                        # Router r5 is \"nested\" on the same subnet as Router"},{"line_number":257,"context_line":"                        # r1, 192.168.1.0/24, so it\u0027s default route is the"},{"line_number":258,"context_line":"                        # internal interface of Router r1. It has no external"},{"line_number":259,"context_line":"                        # gateway, but relies on that of Router r1.."},{"line_number":260,"context_line":"                        {\u0027id\u0027: \u0027r5\u0027, \u0027routes\u0027: [{\u0027nexthop\u0027: \u0027192.168.1.1\u0027,"}],"source_content_type":"text/x-python","patch_set":12,"id":"53fcc74d_f0a78e9f","line":257,"range":{"start_line":257,"start_character":49,"end_line":257,"end_character":53},"in_reply_to":"f67f6a72_b6173487","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e40fb344e63bd26d8b439634295e7fe879b660b8","unresolved":true,"context_lines":[{"line_number":256,"context_line":"                        # Router r5 is \"nested\" on the same subnet as Router"},{"line_number":257,"context_line":"                        # r1, 192.168.1.0/24, so it\u0027s default route is the"},{"line_number":258,"context_line":"                        # internal interface of Router r1. It has no external"},{"line_number":259,"context_line":"                        # gateway, but relies on that of Router r1.."},{"line_number":260,"context_line":"                        {\u0027id\u0027: \u0027r5\u0027, \u0027routes\u0027: [{\u0027nexthop\u0027: \u0027192.168.1.1\u0027,"},{"line_number":261,"context_line":"                         \u0027destination\u0027: \u00270.0.0.0/0\u0027,"},{"line_number":262,"context_line":"                         \u0027external_ids\u0027: {}}],"}],"source_content_type":"text/x-python","patch_set":12,"id":"40c39134_1efc8648","line":259,"range":{"start_line":259,"start_character":67,"end_line":259,"end_character":68},"updated":"2024-05-15 21:57:23.000000000","message":"duplicate dot","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"ce4fcd4b3f6723d9e86a51a1d6239e7e10ed05cd","unresolved":false,"context_lines":[{"line_number":256,"context_line":"                        # Router r5 is \"nested\" on the same subnet as Router"},{"line_number":257,"context_line":"                        # r1, 192.168.1.0/24, so it\u0027s default route is the"},{"line_number":258,"context_line":"                        # internal interface of Router r1. It has no external"},{"line_number":259,"context_line":"                        # gateway, but relies on that of Router r1.."},{"line_number":260,"context_line":"                        {\u0027id\u0027: \u0027r5\u0027, \u0027routes\u0027: [{\u0027nexthop\u0027: \u0027192.168.1.1\u0027,"},{"line_number":261,"context_line":"                         \u0027destination\u0027: \u00270.0.0.0/0\u0027,"},{"line_number":262,"context_line":"                         \u0027external_ids\u0027: {}}],"}],"source_content_type":"text/x-python","patch_set":12,"id":"1995b8e2_12101c76","line":259,"range":{"start_line":259,"start_character":67,"end_line":259,"end_character":68},"in_reply_to":"40c39134_1efc8648","updated":"2024-05-16 10:26:37.000000000","message":"Done","commit_id":"3538ff9e95d918dd314ba0cc0850a78d5acfe240"}],"releasenotes/notes/ovn-router-nested-networks-e57978cf71760703.yaml":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc63f69d03c17e8a517e439f8a86a78aa50b01a2","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixes an issue where an OVN router behaved differently from a legacy"},{"line_number":5,"context_line":"    router with respect to having multiple nested private networks"},{"line_number":6,"context_line":"    configured. With a legacy router, all private subnets, regardless"},{"line_number":7,"context_line":"    of network, are allowed to use the single router external gateway IP"}],"source_content_type":"text/x-yaml","patch_set":21,"id":"df60a06c_a831db28","line":4,"range":{"start_line":4,"start_character":66,"end_line":4,"end_character":72},"updated":"2024-06-27 07:38:15.000000000","message":"``legacy L3 router`` (as opposed to OVN router)","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"db51b9345efa520191ab83cd4d80ca89f2f7eec1","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixes an issue where an OVN router behaved differently from a legacy"},{"line_number":5,"context_line":"    router with respect to having multiple nested private networks"},{"line_number":6,"context_line":"    configured. With a legacy router, all private subnets, regardless"},{"line_number":7,"context_line":"    of network, are allowed to use the single router external gateway IP"}],"source_content_type":"text/x-yaml","patch_set":21,"id":"f9077d6e_68dedc38","line":4,"range":{"start_line":4,"start_character":66,"end_line":4,"end_character":72},"in_reply_to":"df60a06c_a831db28","updated":"2024-06-27 14:54:32.000000000","message":"Done","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc63f69d03c17e8a517e439f8a86a78aa50b01a2","unresolved":false,"context_lines":[{"line_number":17,"context_line":"    to existing routers. For more information,"},{"line_number":18,"context_line":"    see bug `2051935 \u003chttps://bugs.launchpad.net/neutron/+bug/2051935\u003e`_."},{"line_number":19,"context_line":"    For more information on the rationale for this behavior in Neutron,"},{"line_number":20,"context_line":"    see bug `1386041 \u003chttps://bugs.launchpad.net/neutron/+bug/1386041\u003e`_."}],"source_content_type":"text/x-yaml","patch_set":21,"id":"d890984c_9b296905","line":20,"range":{"start_line":20,"start_character":22,"end_line":20,"end_character":69},"updated":"2024-06-27 07:38:15.000000000","message":"+1, thanks for this reference.","commit_id":"ac95497ee48e703e9e3fb4a7786bad4c38d9a2f1"}]}