diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index 31affc98bb..82b3ec6a44 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -98,6 +98,18 @@ def turn_down_nm_connections(): continue uuid = value["uuid"] print("Turn down connection", name) + # Re-check this specific UUID's state right before calling down + # to handle the race condition where a connection becomes inactive + # between the initial query and this point (e.g. another connection + # was brought down first, causing NM to deactivate this one too). + check_cmd = "nmcli -t -f GENERAL.STATE c show {}".format(uuid) + print_cmd(check_cmd) + current_state = sp.check_output( + shlex.split(check_cmd), universal_newlines=True + ).strip() + if current_state != "GENERAL.STATE:activated": + print("WARN: {} is no longer active, skipping".format(name)) + continue cmd = "nmcli c down {}".format(uuid) print_cmd(cmd) sp.check_call(shlex.split(cmd)) diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index 26683277c0..fd5a23d1db 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -140,15 +140,21 @@ def test_connection_activation_fails_due_to_exception( class TestTurnDownNmConnections(unittest.TestCase): - @patch("wifi_nmcli_test.sp.call") + @patch("wifi_nmcli_test.sp.check_output") + @patch("wifi_nmcli_test.sp.check_call") @patch("wifi_nmcli_test._get_nm_wireless_connections", return_value={}) def test_no_connections_to_turn_down( - self, get_connections_mock, sp_call_mock + self, get_connections_mock, sp_check_call_mock, sp_check_output_mock ): turn_down_nm_connections() self.assertEqual(get_connections_mock.call_count, 1) - sp_call_mock.assert_not_called() + sp_check_output_mock.assert_not_called() + sp_check_call_mock.assert_not_called() + @patch( + "wifi_nmcli_test.sp.check_output", + return_value="GENERAL.STATE:activated", + ) @patch("wifi_nmcli_test.sp.check_call") @patch( "wifi_nmcli_test._get_nm_wireless_connections", @@ -158,30 +164,60 @@ def test_no_connections_to_turn_down( }, ) def test_turn_down_single_connection( - self, get_connections_mock, sp_check_call_mock + self, get_connections_mock, sp_check_call_mock, sp_check_output_mock ): turn_down_nm_connections() - self.assertEqual(get_connections_mock.call_count, 1) + sp_check_output_mock.assert_called_once_with( + "nmcli -t -f GENERAL.STATE c show uuid1".split(), + universal_newlines=True, + ) sp_check_call_mock.assert_called_once_with( "nmcli c down uuid1".split() ) + @patch( + "wifi_nmcli_test.sp.check_output", + return_value="GENERAL.STATE:deactivated", + ) + @patch("wifi_nmcli_test.sp.check_call") + @patch( + "wifi_nmcli_test._get_nm_wireless_connections", + return_value={"Wireless1": {"uuid": "uuid1", "state": "activated"}}, + ) + def test_turn_down_connection_already_inactive( + self, get_connections_mock, sp_check_call_mock, sp_check_output_mock + ): + # UUID state re-check shows connection already inactive — skip down + turn_down_nm_connections() + sp_check_output_mock.assert_called_once_with( + "nmcli -t -f GENERAL.STATE c show uuid1".split(), + universal_newlines=True, + ) + sp_check_call_mock.assert_not_called() + + @patch( + "wifi_nmcli_test.sp.check_output", + return_value="GENERAL.STATE:activated", + ) @patch("wifi_nmcli_test.sp.check_call") @patch( "wifi_nmcli_test._get_nm_wireless_connections", return_value={"Wireless1": {"uuid": "uuid1", "state": "activated"}}, ) def test_turn_down_single_connection_with_exception( - self, get_connections_mock, sp_check_call_mock + self, get_connections_mock, sp_check_call_mock, sp_check_output_mock ): - sp_check_call_mock.side_effect = subprocess.CalledProcessError("", 1) + sp_check_call_mock.side_effect = subprocess.CalledProcessError(1, "") with self.assertRaises(subprocess.CalledProcessError): turn_down_nm_connections() - self.assertEqual(get_connections_mock.call_count, 1) sp_check_call_mock.assert_called_once_with( "nmcli c down uuid1".split() ) + @patch( + "wifi_nmcli_test.sp.check_output", + return_value="GENERAL.STATE:activated", + ) @patch("wifi_nmcli_test.sp.check_call") @patch( "wifi_nmcli_test._get_nm_wireless_connections", @@ -191,10 +227,11 @@ def test_turn_down_single_connection_with_exception( }, ) def test_turn_down_multiple_connections( - self, get_connections_mock, sp_check_call_mock + self, get_connections_mock, sp_check_call_mock, sp_check_output_mock ): turn_down_nm_connections() - self.assertEqual(get_connections_mock.call_count, 1) + # one UUID state check per activated connection + self.assertEqual(sp_check_output_mock.call_count, 2) calls = [ call("nmcli c down uuid1".split()), call("nmcli c down uuid2".split()),