diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 15f4eff63..8dcf0306e 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -1,5 +1,4 @@ import collections -import logging import django import jsonschema @@ -32,8 +31,6 @@ from ..signals import is_working_changed from ..tasks import auto_add_credentials_to_devices, launch_command -logger = logging.getLogger(__name__) - class ConnectorMixin(object): _connector_field = "connector" @@ -367,9 +364,7 @@ def update_config(self): if self.is_working: try: self.connector_instance.update_config() - except Exception as e: - logger.exception(e) - else: + finally: self.disconnect() def save(self, *args, **kwargs): diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 56e6a7bb1..a277aed47 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -61,7 +61,14 @@ def update_config(self, device_id): return else: logger.info(f"Updating {device} (pk: {device_id})") - device_conn.update_config() + try: + device_conn.update_config() + except Exception as e: + logger.exception( + f"Failed to push configuration to {device} (pk: {device_id})" + ) + if hasattr(device, "config"): + device.config.set_status_error(reason=str(e)) # task timeout is SSH_COMMAND_TIMEOUT plus a 20% margin diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 4766a3f7e..d3d483b80 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -900,6 +900,21 @@ def test_command_multiple_connections(self, connect_mocked): command.refresh_from_db() self.assertIn(command.connection, [dc1, dc2]) + def test_update_config_disconnect_always_called(self): + """ + disconnect() must be called even when connector_instance.update_config() + raises, so that the SSH session is never leaked. + """ + dc = self._create_device_connection() + connector_mock = mock.Mock() + connector_mock.update_config.side_effect = Exception("SSH channel closed") + dc.connector_instance = connector_mock + + with self.assertRaises(Exception): + dc.update_config() + + connector_mock.disconnect.assert_called_once() + class TestModelsTransaction(BaseTestModels, TransactionTestCase): def _prepare_conf_object(self, organization=None): @@ -969,7 +984,7 @@ def _assert_applying_conf_test_command(mocked_exec): self.assertEqual(mocked_exec_command.call_count, 1) _assert_version_check_command(mocked_exec_command) conf.refresh_from_db() - self.assertEqual(conf.status, "modified") + self.assertEqual(conf.status, "error") with self.subTest("openwisp_config >= 0.6.0a"): conf.config = '{"dns_servers": []}' @@ -1020,8 +1035,8 @@ def _assert_applying_conf_test_command(mocked_exec): args, _ = mocked_exec_command.call_args_list[2] self.assertEqual(args[0], "/etc/init.d/openwisp_config restart") conf.refresh_from_db() - # exit code 1 considers the update not successful - self.assertEqual(conf.status, "modified") + # failed connector command sets config status to error + self.assertEqual(conf.status, "error") @mock.patch("time.sleep") @mock.patch.object(DeviceConnection, "update_config") @@ -1107,6 +1122,27 @@ def test_device_update_config_not_in_progress( mocked_get_working_connection.assert_called_once() mocked_update_config.assert_called_once() + @capture_any_output() + @mock.patch(_connect_path) + @mock.patch("time.sleep") + def test_update_config_task_sets_status_error_on_failure( + self, mocked_sleep, mocked_connect + ): + """ + When the SSH connector raises during a config push, the update_config + task must set the config status to 'error' so the failure is visible + in the admin dashboard rather than silently staying 'modified'. + """ + conf = self._prepare_conf_object() + conf.config = {"general": {"timezone": "UTC"}} + conf.full_clean() + with mock.patch(_exec_command_path) as mocked_exec_command: + mocked_exec_command.side_effect = Exception("SSH channel closed") + conf.save() + conf.refresh_from_db() + self.assertEqual(conf.status, "error") + self.assertIn("SSH channel closed", conf.error_reason) + @mock.patch(_connect_path) def test_schedule_command_called(self, connect_mocked): dc = self._create_device_connection()