)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"9ccf0025c7a0c7f02cdd12a90d809070f323d0c1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c356471e_7d2e6386","updated":"2023-02-28 15:16:56.000000000","message":"While running into some issues with inconsistencies in IPtables rules or status of ipsec connections, I dove through the code and tried to improve on the reconciliation.\n\nPlease kindly take a look.","commit_id":"3ecf59dbeb7fdd0e247649583d8a4e0b749b6429"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a56a04a07e6883c4aab4187b51b8092860fc6b2d","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"2a482897_88f8611d","updated":"2023-09-12 07:04:29.000000000","message":"Hello Bodo, sorry for dragging you in as a reviewer this bluntly. I unfortunately did not pursue this patch for a while. But since you did the OVN patch for VPNaaS (https://review.opendev.org/c/openstack/neutron-vpnaas/+/765353), and that is \"almost\" merged, I\u0027d kindly like to ask you to take a peek at my ideas an changes here and maybe give some feedback for things I should change.\n\nIf the approach looks good and valuable to you, I\u0027d really like to finish this (tests failing) to make VPNaaS services more stable.","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"},{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"5ee4d2e2b8c4bbe57e96d57c56b5173b9efb3ee3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f3c66170_296db31c","updated":"2023-09-13 11:21:28.000000000","message":"I started to look at it. Maybe you can change the `sync` method back to take a router dict.","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"e3ae0b002548d09a78eef3333f1248dfa79c1ebd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b6dc1aad_7e47e4a8","updated":"2023-09-26 07:36:29.000000000","message":"Thanks a bunch for your time and feedback Bodo!\nSorry for my late response. I shall take another slash at this one soon.","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"810a3fea8b8fb9865b20d8c870fe28c856815f16","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"d11351c2_6be4b05d","updated":"2024-02-12 16:58:50.000000000","message":"Sorry guys for the delay. We now revisited this patch again and rebased it to the current master while (hopefully) fixing all of the tests. Especially them tests were a little challenging, as \"Router\" and \"RouterInfo\" objects are seemingly used with little relation to their variables names. The lack of type annotations also contributed.\n\nIn any case, we tried to give this a good shot again and would appreciate your kind review time and input.","commit_id":"b89ecaee2b7f3634cf54dee4d67df99a4a1b2593"},{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"6939beaa6265d1ea08837e99ae31d0a00cd0bfed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"5475536d_586d9b8a","updated":"2024-03-04 18:22:57.000000000","message":"recheck functional-sswan","commit_id":"01ee07618be72c50cca89989abd1b0f486665657"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0b27a5b366c6fc3d8c7bf692b231002ecb9f4aed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"cb940a70_f4fcbd8a","updated":"2024-09-05 21:59:51.000000000","message":"I only had nits, which don\u0027t affect the code","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"97636c5f684924bfb854a8fb76d8b9612d32f975","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"898d308c_1cd0b558","updated":"2024-09-09 07:23:25.000000000","message":"Thanks for for working on this, as I see we are safe to merge this in this cycle to fix the mentioned issues","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"}],"neutron_vpnaas/services/vpn/agent.py":[{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"5ee4d2e2b8c4bbe57e96d57c56b5173b9efb3ee3","unresolved":true,"context_lines":[{"line_number":74,"context_line":"        ri \u003d self.agent_api.get_router_info(data[\u0027id\u0027])"},{"line_number":75,"context_line":"        if ri is not None:"},{"line_number":76,"context_line":"            for device_driver in self.device_drivers:"},{"line_number":77,"context_line":"                device_driver.sync(context, [ri])"},{"line_number":78,"context_line":"        else:"},{"line_number":79,"context_line":"            LOG.debug(\"Router %s was concurrently deleted while \""},{"line_number":80,"context_line":"                      \"updating VPN for it\", data[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"06310f9f_1f3d0b6b","line":77,"updated":"2023-09-13 11:21:28.000000000","message":"Instead of passing the router info (ri), keep the old way with a router dict that at least contains the \u0027id\u0027. There are other code paths that call sync still in the old way. And some tests fail because of that.\nTo update the device_driver\u0027s router cache with the updated info, I\u0027d suggest to add a device_driver method `update_router(ri)`.\nI\u0027m also thinking about how a change would impact my vpnaas-for-ovn patch and there I don\u0027t really need the router info","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"56513d0c91252bf05a53bd78e0a6996830925986","unresolved":true,"context_lines":[{"line_number":74,"context_line":"        ri \u003d self.agent_api.get_router_info(data[\u0027id\u0027])"},{"line_number":75,"context_line":"        if ri is not None:"},{"line_number":76,"context_line":"            for device_driver in self.device_drivers:"},{"line_number":77,"context_line":"                device_driver.sync(context, [ri])"},{"line_number":78,"context_line":"        else:"},{"line_number":79,"context_line":"            LOG.debug(\"Router %s was concurrently deleted while \""},{"line_number":80,"context_line":"                      \"updating VPN for it\", data[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"e8249abe_a7938aae","line":77,"in_reply_to":"06310f9f_1f3d0b6b","updated":"2024-02-12 17:02:43.000000000","message":"The add_router method was, as far as I can see always storing the router_info, while the sync method was not involved in updating the self.routers dict.\n\nWith this change here, I added updating the stored / cached routers (which are actually router info objects if I am not mistaken).\n\nIn the end I simply want to align the method signatures a little more and work on the type types of objects everywhere. \n\nIf you have the time, please check out my recent patchset. I truly intented to increase my responsiveness, in comments and code.","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"}],"neutron_vpnaas/services/vpn/device_drivers/ipsec.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0b27a5b366c6fc3d8c7bf692b231002ecb9f4aed","unresolved":true,"context_lines":[{"line_number":908,"context_line":"                    continue"},{"line_number":909,"context_line":""},{"line_number":910,"context_line":"                for peer_cidr in ipsec_site_connection[\u0027peer_cidrs\u0027]:"},{"line_number":911,"context_line":"                    LOG.debug(\"Adding an ipsec policy NAT rule\""},{"line_number":912,"context_line":"                              \"%s \u003c-\u003e %s to router id %s\","},{"line_number":913,"context_line":"                              peer_cidr, local_cidr, vpnservice[\u0027router_id\u0027])"},{"line_number":914,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"4ca78749_6eb92391","line":911,"range":{"start_line":911,"start_character":58,"end_line":911,"end_character":62},"updated":"2024-09-05 21:59:51.000000000","message":"nit: missing trailing space here","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0b27a5b366c6fc3d8c7bf692b231002ecb9f4aed","unresolved":true,"context_lines":[{"line_number":910,"context_line":"                for peer_cidr in ipsec_site_connection[\u0027peer_cidrs\u0027]:"},{"line_number":911,"context_line":"                    LOG.debug(\"Adding an ipsec policy NAT rule\""},{"line_number":912,"context_line":"                              \"%s \u003c-\u003e %s to router id %s\","},{"line_number":913,"context_line":"                              peer_cidr, local_cidr, vpnservice[\u0027router_id\u0027])"},{"line_number":914,"context_line":""},{"line_number":915,"context_line":"                    iptables_manager.ipv4[\u0027nat\u0027].add_rule("},{"line_number":916,"context_line":"                        \u0027POSTROUTING\u0027,"}],"source_content_type":"text/x-python","patch_set":20,"id":"74300472_146bc046","line":913,"range":{"start_line":913,"start_character":53,"end_line":913,"end_character":76},"updated":"2024-09-05 21:59:51.000000000","message":"nit: should use ri.router_id to be consistent with L901 usage","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0b27a5b366c6fc3d8c7bf692b231002ecb9f4aed","unresolved":true,"context_lines":[{"line_number":920,"context_line":"                        top\u003dTrue, tag\u003d\u0027vpnaas\u0027)"},{"line_number":921,"context_line":""},{"line_number":922,"context_line":"        LOG.debug(\"Applying iptables for router id %s\","},{"line_number":923,"context_line":"                  vpnservice[\u0027router_id\u0027])"},{"line_number":924,"context_line":"        iptables_manager.apply()"},{"line_number":925,"context_line":""},{"line_number":926,"context_line":"    @log_helpers.log_method_call"}],"source_content_type":"text/x-python","patch_set":20,"id":"c79ef595_616b020b","line":923,"range":{"start_line":923,"start_character":18,"end_line":923,"end_character":41},"updated":"2024-09-05 21:59:51.000000000","message":"same","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"0b27a5b366c6fc3d8c7bf692b231002ecb9f4aed","unresolved":true,"context_lines":[{"line_number":929,"context_line":""},{"line_number":930,"context_line":"        :param router_id: router_id"},{"line_number":931,"context_line":"        \"\"\""},{"line_number":932,"context_line":"        router \u003d self.routers.get(router_id)"},{"line_number":933,"context_line":"        if not router:"},{"line_number":934,"context_line":"            return"},{"line_number":935,"context_line":"        iptables_manager \u003d self.get_router_based_iptables_manager(router)"}],"source_content_type":"text/x-python","patch_set":20,"id":"51598a17_1e0330d3","line":932,"range":{"start_line":932,"start_character":8,"end_line":932,"end_character":14},"updated":"2024-09-05 21:59:51.000000000","message":"nit: s/router_info to be consistent with L859 change","commit_id":"7c2018b6fb8d859792fd10d5251abb33d1dfbbde"}],"neutron_vpnaas/services/vpn/device_drivers/strongswan_ipsec.py":[{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"ab619d10cf50c716d18ad2467b2e0151da42f696","unresolved":true,"context_lines":[{"line_number":193,"context_line":"        self._execute([self.binary, \u0027start\u0027])"},{"line_number":194,"context_line":"        # initiate ipsec connection"},{"line_number":195,"context_line":"        for ipsec_site_conn in self.vpnservice[\u0027ipsec_site_connections\u0027]:"},{"line_number":196,"context_line":"            stdout, stderr \u003d self._execute([self.binary, \u0027stroke\u0027, \u0027up-nb\u0027,"},{"line_number":197,"context_line":"                           ipsec_site_conn[\u0027id\u0027]])"},{"line_number":198,"context_line":"            LOG.debug(\"stroke up-nb for %s returned stdout: %s, stderr: %s \","},{"line_number":199,"context_line":"                      ipsec_site_conn[\u0027id\u0027], stdout, stderr)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5db8de64_9082edff","line":196,"updated":"2023-09-13 15:01:10.000000000","message":"`self._execute` only returns stdout, so I assume you will get a `ValueError: too many values to unpack (expected 2)` here. Internally `execute` in neutron.agent.linux.utils is called without setting `return_stderr\u003dTrue`.","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"56513d0c91252bf05a53bd78e0a6996830925986","unresolved":false,"context_lines":[{"line_number":193,"context_line":"        self._execute([self.binary, \u0027start\u0027])"},{"line_number":194,"context_line":"        # initiate ipsec connection"},{"line_number":195,"context_line":"        for ipsec_site_conn in self.vpnservice[\u0027ipsec_site_connections\u0027]:"},{"line_number":196,"context_line":"            stdout, stderr \u003d self._execute([self.binary, \u0027stroke\u0027, \u0027up-nb\u0027,"},{"line_number":197,"context_line":"                           ipsec_site_conn[\u0027id\u0027]])"},{"line_number":198,"context_line":"            LOG.debug(\"stroke up-nb for %s returned stdout: %s, stderr: %s \","},{"line_number":199,"context_line":"                      ipsec_site_conn[\u0027id\u0027], stdout, stderr)"}],"source_content_type":"text/x-python","patch_set":4,"id":"27d158d5_91389a06","line":196,"in_reply_to":"5db8de64_9082edff","updated":"2024-02-12 17:02:43.000000000","message":"Done","commit_id":"f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9"}]}