)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"7f583a88db32ac14216cdf445f9396645d18af8e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"03d08d0a_3aeb0ad1","updated":"2022-12-02 21:41:00.000000000","message":"Thanks for the fix Sebastian! The only thing I think it would be good to have is a unit test for this condition - wait timeout triggers and it tries SIGKILL. I think it should be easy enough to add another test using a decorator similar to the l3-ha test test_destroy_state_change_monitor_force(). Let us know if that is possible. Thanks.","commit_id":"3b08c74e17658fdcd33e9ad69b79a540ce095cb9"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"2f120d68033e34a62cf72001ed8cb016646365d8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1b6b6d5f_7ec21da2","updated":"2022-12-05 12:00:18.000000000","message":"We have done this before [1] so I\u0027m ok with this patch.\n\nHowever I agree with Brian about the need of at least UTs.\n\n[1]https://review.opendev.org/q/I2e784ea7e00c145135288bf309bb34ce311ac15c","commit_id":"3b08c74e17658fdcd33e9ad69b79a540ce095cb9"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"30a93ed92f859878e66400bffed12b8ff226ceee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c6384d70_e27c50ad","in_reply_to":"03d08d0a_3aeb0ad1","updated":"2022-12-05 15:20:51.000000000","message":"Definitely possible! I implemented the test before looking into Rodolfo\u0027s link to KeepalivedManagerTestCase.test_destroy_force, so my patch is a little bit different from the linked test. Please let me know if that works or if it needs adjustments.","commit_id":"3b08c74e17658fdcd33e9ad69b79a540ce095cb9"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"1d0192253b51c9d8bcf6a1689c4edeaf9449df14","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"55f00d1e_2546f8e0","updated":"2022-12-06 13:29:03.000000000","message":"Thanks","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"f72d3bfcebbff5947782f53b0e9a80677de74b59","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"5501ec84_6ea4ddd1","updated":"2022-12-12 17:41:20.000000000","message":"recheck timeout","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"aac88a33909853e8f0575fe1a87026add8cde25a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"9518d2cf_2887230d","updated":"2022-12-12 09:58:20.000000000","message":"recheck timeout","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"ca6bae475b58cd7ae3843ea0dd25231d3760d4c1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"c97f6a20_770edb65","updated":"2022-12-08 14:02:08.000000000","message":"recheck timeout","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6a8367a90a74ec8cf7d2ce673f9ed6748f3c1d6b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"bb2c12f0_ae5cfb0b","updated":"2022-12-16 15:24:32.000000000","message":"recheck timeout (just in case)","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"d156dbcd7426b89a216bb9cc499673492c629394","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"2e7ac123_dec63d10","updated":"2022-12-09 15:29:33.000000000","message":"recheck timeout / 867065 merged","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cbe21150d0590cafba28588ea4d0913abc740e62","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"86464a48_f95a7877","updated":"2022-12-21 17:08:18.000000000","message":"recheck unrelated FT error (bug created)","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":13861,"name":"yatin","email":"ykarel@redhat.com","username":"yatinkarel"},"change_message_id":"e551732e5a48233ef7b275b1d07e4ad4a97836db","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"742539eb_41ff80c4","updated":"2022-12-19 07:32:45.000000000","message":"recheck vexxhost timeouts","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"}],"neutron/agent/linux/dhcp.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"1d0192253b51c9d8bcf6a1689c4edeaf9449df14","unresolved":true,"context_lines":[{"line_number":46,"context_line":"from neutron.privileged.agent.linux import dhcp as priv_dhcp"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":49,"context_line":"SIGTERM_TIMEOUT \u003d 5"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"DNS_PORT \u003d 53"},{"line_number":52,"context_line":"WIN2k3_STATIC_DNS \u003d 249"}],"source_content_type":"text/x-python","patch_set":5,"id":"21a68657_f58a03c5","line":49,"updated":"2022-12-06 13:29:03.000000000","message":"Is that enough 5s, should we except the process to finish before that in normal condition?","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"c1399ccd540ed26d5e0c5f85a0940d3d59a8b4a1","unresolved":true,"context_lines":[{"line_number":46,"context_line":"from neutron.privileged.agent.linux import dhcp as priv_dhcp"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":49,"context_line":"SIGTERM_TIMEOUT \u003d 5"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"DNS_PORT \u003d 53"},{"line_number":52,"context_line":"WIN2k3_STATIC_DNS \u003d 249"}],"source_content_type":"text/x-python","patch_set":5,"id":"314682d7_4b7ffb35","line":49,"range":{"start_line":49,"start_character":0,"end_line":49,"end_character":15},"updated":"2022-12-07 14:33:24.000000000","message":"there should be even a cfg option for this (or a general one that can be used for dnsmasq, keepalived etc...)","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"5c458434297dc110695b1d71a99822054a62bb98","unresolved":true,"context_lines":[{"line_number":46,"context_line":"from neutron.privileged.agent.linux import dhcp as priv_dhcp"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":49,"context_line":"SIGTERM_TIMEOUT \u003d 5"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"DNS_PORT \u003d 53"},{"line_number":52,"context_line":"WIN2k3_STATIC_DNS \u003d 249"}],"source_content_type":"text/x-python","patch_set":5,"id":"9afff789_d5ad3d69","line":49,"in_reply_to":"21a68657_f58a03c5","updated":"2022-12-06 13:49:27.000000000","message":"In my experience (for which I have NOT done a proper measurement) dnsmasq shuts down pretty fast, generally in under a second (I don\u0027t notice it hanging). If it\u0027s not done after 5 seconds I\u0027d say dnsmasq had it\u0027s chance and should get killed.\n\nFor reference, this is what dnsmasq does when it gets a SIGTERM: https://github.com/imp/dnsmasq/blob/master/src/dnsmasq.c#L1586-L1628","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"cd167607e0c86c1296439fde233e57cdc0408694","unresolved":true,"context_lines":[{"line_number":46,"context_line":"from neutron.privileged.agent.linux import dhcp as priv_dhcp"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":49,"context_line":"SIGTERM_TIMEOUT \u003d 5"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"DNS_PORT \u003d 53"},{"line_number":52,"context_line":"WIN2k3_STATIC_DNS \u003d 249"}],"source_content_type":"text/x-python","patch_set":5,"id":"761a5abf_aac2143a","line":49,"range":{"start_line":49,"start_character":0,"end_line":49,"end_character":15},"in_reply_to":"314682d7_4b7ffb35","updated":"2022-12-07 15:14:14.000000000","message":"Hardcoding this seems to be established practice in Neutron, so I went for this instead of adding an option.\n\n$ grep -n -e ^SIGTERM_WAITTIME -e ^SIGTERM_TIMEOUT . -riI\n./agent/metadata/driver.py:42:SIGTERM_TIMEOUT \u003d 5\n./agent/l3/ha_router.py:38:SIGTERM_TIMEOUT \u003d 10\n./agent/linux/keepalived.py:45:SIGTERM_TIMEOUT \u003d 5\n./cmd/netns_cleanup.py:46:SIGTERM_WAITTIME \u003d 10\n\nI don\u0027t think this needs to be configurable, but I also speak from the perspective of my own OpenStack infra experiences, not for everyone.\n\nShall I add a config options to configure the SIGTERM_TIMEOUT for this case?","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"fc5f551b2cd3649e74606663d6c4de176b927dc7","unresolved":false,"context_lines":[{"line_number":46,"context_line":"from neutron.privileged.agent.linux import dhcp as priv_dhcp"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":49,"context_line":"SIGTERM_TIMEOUT \u003d 5"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"DNS_PORT \u003d 53"},{"line_number":52,"context_line":"WIN2k3_STATIC_DNS \u003d 249"}],"source_content_type":"text/x-python","patch_set":5,"id":"9ada6446_3199340f","line":49,"in_reply_to":"9afff789_d5ad3d69","updated":"2022-12-06 16:29:38.000000000","message":"ACK thank you for the sharing.","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"c1399ccd540ed26d5e0c5f85a0940d3d59a8b4a1","unresolved":true,"context_lines":[{"line_number":361,"context_line":"                LOG.warning(\u0027dnsmasq process %s did not finish after SIGTERM \u0027"},{"line_number":362,"context_line":"                            \u0027signal in %s seconds, sending SIGKILL signal\u0027,"},{"line_number":363,"context_line":"                            pm.pid, SIGTERM_TIMEOUT)"},{"line_number":364,"context_line":"                pm.disable(sig\u003dstr(int(signal.SIGKILL)))"},{"line_number":365,"context_line":"                common_utils.wait_until_true(lambda: not self.active)"},{"line_number":366,"context_line":"        self._del_running_interface(self.interface_name)"},{"line_number":367,"context_line":"        if not retain_port:"}],"source_content_type":"text/x-python","patch_set":5,"id":"c98b6235_0d265e17","line":364,"range":{"start_line":364,"start_character":39,"end_line":364,"end_character":53},"updated":"2022-12-07 14:33:24.000000000","message":"SIGKILL is the default:\nhttps://opendev.org/openstack/neutron/src/branch/master/neutron/agent/linux/external_process.py#L101","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"cd167607e0c86c1296439fde233e57cdc0408694","unresolved":true,"context_lines":[{"line_number":361,"context_line":"                LOG.warning(\u0027dnsmasq process %s did not finish after SIGTERM \u0027"},{"line_number":362,"context_line":"                            \u0027signal in %s seconds, sending SIGKILL signal\u0027,"},{"line_number":363,"context_line":"                            pm.pid, SIGTERM_TIMEOUT)"},{"line_number":364,"context_line":"                pm.disable(sig\u003dstr(int(signal.SIGKILL)))"},{"line_number":365,"context_line":"                common_utils.wait_until_true(lambda: not self.active)"},{"line_number":366,"context_line":"        self._del_running_interface(self.interface_name)"},{"line_number":367,"context_line":"        if not retain_port:"}],"source_content_type":"text/x-python","patch_set":5,"id":"d8ec9938_da4ef998","line":364,"range":{"start_line":364,"start_character":39,"end_line":364,"end_character":53},"in_reply_to":"c98b6235_0d265e17","updated":"2022-12-07 15:14:14.000000000","message":"I explicitly added it here to make it easier to understand what we\u0027re doing (by showing the SIGTERM -\u003e SIGKILL progression), though I agree with you in general that explicitly specifying default arguments isn\u0027t necessarily the best choice. As this was already done in three other places in the code base I thought it to be okay.\n\n $ grep disable\\( . -rI|grep KILL\n./agent/metadata/driver.py:            pm.disable(sig\u003dstr(int(signal.SIGKILL)))\n./agent/l3/ha_router.py:            pm.disable(sig\u003dstr(int(signal.SIGKILL)))\n./agent/linux/keepalived.py:            pm.disable(sig\u003dstr(int(signal.SIGKILL)))\n\nShall I drop the default argument, regardless?","commit_id":"74224e79e031636018b970fac9c2aa72516eb12d"}],"neutron/tests/unit/agent/linux/test_dhcp.py":[{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"efebbb45b33b03f987d112fa51361e2bc16e5567","unresolved":true,"context_lines":[{"line_number":1289,"context_line":"        self.assertEqual(2, mock_wait_until.call_count)"},{"line_number":1290,"context_line":"        mock_pm.disable.assert_has_calls(["},{"line_number":1291,"context_line":"            mock.call(sig\u003dstr(int(signal.SIGTERM))),"},{"line_number":1292,"context_line":"            mock.call(sig\u003dstr(int(signal.SIGKILL)))])"},{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"    def test_get_interface_name(self):"},{"line_number":1295,"context_line":"        net \u003d FakeDualNetwork()"}],"source_content_type":"text/x-python","patch_set":3,"id":"c435596b_79eaab20","line":1292,"updated":"2022-12-05 21:55:36.000000000","message":"I think this should be fine. Since you\u0027ve done this can you also add one for the \"normal\" case where the SIGTERM works?\n\n+    @mock.patch.object(common_utils, \u0027wait_until_true\u0027)\n+    def test_disable_blocking(self, mock_wait_until):\n+        lp \u003d LocalChild(self.conf, FakeDualNetwork())\n+        mock_pm \u003d mock.Mock()\n+        with mock.patch(\u0027neutron.agent.linux.ip_lib.\u0027\n+                        \u0027delete_network_namespace\u0027), \\\n+                mock.patch.object(dhcp.DhcpLocalProcess,\n+                                  \u0027_get_process_manager\u0027,\n+                                  return_value\u003dmock_pm):\n+            lp.disable(block\u003dTrue)\n+        self.assertEqual(1, mock_wait_until.call_count)\n+        mock_pm.disable.assert_called_once_with(\n+            sig\u003dstr(int(signal.SIGTERM)))","commit_id":"55fb39a1f7c8ff77db1c68132854be27cd99c3c0"},{"author":{"_account_id":30314,"name":"Sebastian Lohff","email":"sebastian.lohff@sap.com","username":"seba"},"change_message_id":"375739555a1d9d9a47488fce3ef3d1ab5add9d8b","unresolved":false,"context_lines":[{"line_number":1289,"context_line":"        self.assertEqual(2, mock_wait_until.call_count)"},{"line_number":1290,"context_line":"        mock_pm.disable.assert_has_calls(["},{"line_number":1291,"context_line":"            mock.call(sig\u003dstr(int(signal.SIGTERM))),"},{"line_number":1292,"context_line":"            mock.call(sig\u003dstr(int(signal.SIGKILL)))])"},{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"    def test_get_interface_name(self):"},{"line_number":1295,"context_line":"        net \u003d FakeDualNetwork()"}],"source_content_type":"text/x-python","patch_set":3,"id":"10475ff5_7598b238","line":1292,"in_reply_to":"c435596b_79eaab20","updated":"2022-12-06 12:56:04.000000000","message":"Sure thing.","commit_id":"55fb39a1f7c8ff77db1c68132854be27cd99c3c0"}]}