From 20b8afd035e6f8a98390490d74676c150d746bcc Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Fri, 1 Sep 2023 00:15:33 +0200 Subject: [PATCH] Add functional tests for grub lib (#104) Switch to use update_config instead of save_config in Config.update function to ensure charm configs are updated and not overwritten. Fix wiring lines in _save_config function. Raise ApplyError if grub-mkconfig command failed, since it is failing also if there is wrong validation. Since the grub lib required to run integration tests on machine or VM, we are switch to running integration tests directly on GitHub runner. It was not possible to run it on VMs, since the GitHub runner do not supports nested virtualization. --- .github/workflows/build-and-test.yaml | 20 +- lib/charms/operator_libs_linux/v0/grub.py | 24 +- tests/integration/test_apt.py | 2 +- tests/integration/test_grub.py | 201 ++++++++++++ tests/unit/test_grub.py | 360 +++++++++++++++------- tox.ini | 20 +- 6 files changed, 485 insertions(+), 142 deletions(-) create mode 100644 tests/integration/test_grub.py diff --git a/.github/workflows/build-and-test.yaml b/.github/workflows/build-and-test.yaml index a0582282..ebdf9dc2 100644 --- a/.github/workflows/build-and-test.yaml +++ b/.github/workflows/build-and-test.yaml @@ -31,19 +31,23 @@ jobs: run: python -m pip install tox - name: Run tests run: tox -e unit - integration-test: - name: Integration tests + integration-test-ubuntu: + name: Ubuntu integration tests runs-on: ubuntu-22.04 steps: - name: Checkout uses: actions/checkout@v3 - - uses: canonical/setup-lxd@v0.1.1 - with: - channel: 5.0/stable - name: Install dependencies - run: python -m pip install tox - - name: Run integration tests - run: tox -e integration + run: sudo python -m pip install tox + - name: Run integration tests for Ubuntu + # tests must be run as root because they configure the system + run: sudo tox -e integration-ubuntu + integration-test-juju-systemd-notices: + name: Juju systemd notices integration tests + runs-on: ubuntu-22.04 + steps: + - name: Checkout + uses: actions/checkout@v3 - name: Setup operator environment uses: charmed-kubernetes/actions-operator@main with: diff --git a/lib/charms/operator_libs_linux/v0/grub.py b/lib/charms/operator_libs_linux/v0/grub.py index ce44050c..d6277e84 100644 --- a/lib/charms/operator_libs_linux/v0/grub.py +++ b/lib/charms/operator_libs_linux/v0/grub.py @@ -54,7 +54,6 @@ def _on_remove(self, _): import filecmp import io import logging -import os import shlex import subprocess from pathlib import Path @@ -70,7 +69,7 @@ def _on_remove(self, _): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 2 +LIBPATCH = 3 GRUB_DIRECTORY = Path("/etc/default/grub.d/") CHARM_CONFIG_PREFIX = "90-juju" @@ -161,13 +160,24 @@ def _save_config(path: Path, config: Dict[str, str], header: str = CONFIG_HEADER if path.exists(): logger.debug("GRUB config %s already exist and it will overwritten", path) - context = [f"{key}={shlex.quote(value)}" for key, value in config.items()] with open(path, "w", encoding="UTF-8") as file: - file.writelines([header, *context]) + file.write(header) + for key, value in config.items(): + file.write(f"{key}={shlex.quote(value)}\n") logger.info("GRUB config file %s was saved", path) +def _update_config(path: Path, config: Dict[str, str], header: str = CONFIG_HEADER) -> None: + """Update existing GRUB config file.""" + if path.exists(): + original_config = _load_config(path) + original_config.update(config) + config = original_config.copy() + + _save_config(path, config, header) + + def check_update_grub() -> bool: """Report whether an update to /boot/grub/grub.cfg is available.""" main_grub_cfg = Path("/boot/grub/grub.cfg") @@ -178,7 +188,7 @@ def check_update_grub() -> bool: ) except subprocess.CalledProcessError as error: logger.exception(error) - raise + raise ApplyError from error return not filecmp.cmp(main_grub_cfg, tmp_path) @@ -240,7 +250,7 @@ def _save_grub_configuration(self) -> None: """Save current GRUB configuration.""" logger.info("saving new GRUB config to %s", GRUB_CONFIG) applied_configs = {self.path, *self.applied_configs} # using set to drop duplicity - registered_configs = os.linesep.join( + registered_configs = "\n".join( FILE_LINE_IN_DESCRIPTION.format(path=path) for path in applied_configs ) header = CONFIG_HEADER + CONFIG_DESCRIPTION.format(configs=registered_configs) @@ -378,5 +388,5 @@ def update(self, config: Dict[str, str], apply: bool = True) -> Set[str]: raise logger.debug("[%s] saving copy of charm config to %s", self.charm_name, GRUB_DIRECTORY) - _save_config(self.path, config) + _update_config(self.path, config) return changed_keys diff --git a/tests/integration/test_apt.py b/tests/integration/test_apt.py index 287f994c..ecbd4962 100644 --- a/tests/integration/test_apt.py +++ b/tests/integration/test_apt.py @@ -55,7 +55,7 @@ def test_install_package_external_repository(): apt.update() apt.add_package("terraform") - assert get_command_path("terraform") == "/usr/bin/terraform" + assert get_command_path("terraform") == "/usr/local/bin/terraform" def test_list_file_generation_external_repository(): diff --git a/tests/integration/test_grub.py b/tests/integration/test_grub.py new file mode 100644 index 00000000..2bc57a26 --- /dev/null +++ b/tests/integration/test_grub.py @@ -0,0 +1,201 @@ +#!/usr/bin/env python3 +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +import logging + +import pytest +from charms.operator_libs_linux.v0 import grub + +logger = logging.getLogger(__name__) + + +@pytest.fixture(autouse=True) +def clean_configs(): + """Clean main and charms configs after each test.""" + yield # run test + grub.GRUB_CONFIG.unlink(missing_ok=True) + for charm_config in grub.GRUB_DIRECTORY.glob(f"{grub.CHARM_CONFIG_PREFIX}-*"): + charm_config.unlink(missing_ok=True) + + +@pytest.mark.parametrize( + "config", + [ + {"GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"}, + { + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepages=64 hugepagesz=1G", + "GRUB_DEFAULT": "0", + }, + {"GRUB_TIMEOUT": "0"}, + ], +) +def test_single_charm_valid_update(config): + """Test single charm update GRUB configuration.""" + grub_conf = grub.Config("test-charm") + grub_conf.update(config) + # check that config was set for charm config file + assert config == grub_conf + assert config == grub._load_config(grub_conf.path) + # check the main config + assert config == grub._load_config(grub.GRUB_CONFIG) + + +@pytest.mark.parametrize("config", [{"TEST_WRONG_KEY:test": "1"}]) +def test_single_charm_update_apply_failure(config): + """Test single charm update GRUB configuration with ApplyError.""" + # create empty grub config + grub.GRUB_CONFIG.touch() + grub_conf = grub.Config("test-charm") + + with pytest.raises(grub.ApplyError): + grub_conf.update(config) + + # check that charm file was not configured + assert not grub_conf.path.exists() + # check the main config + main_config = grub._load_config(grub.GRUB_CONFIG) + for key in config: + assert key not in main_config + + +def test_single_charm_multiple_update(): + """Test that charm can do multiple updates and update it's own configuration.""" + # charms using this config to make update + configs = [ + {"GRUB_TIMEOUT": "0"}, + { + "GRUB_TIMEOUT": "0", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + {"GRUB_TIMEOUT": "1"}, + ] + # charms configs in time + exp_charms_configs = [ + {"GRUB_TIMEOUT": "0"}, + { + "GRUB_TIMEOUT": "0", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + { + "GRUB_TIMEOUT": "1", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + ] + exp_main_config = { + "GRUB_TIMEOUT": "1", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + } + grub_conf = grub.Config("test-charm") + + for config, exp_conf in zip(configs, exp_charms_configs): + grub_conf.update(config) + assert exp_conf == grub_conf + assert exp_conf == grub._load_config(grub_conf.path) + + # check the main config + assert exp_main_config == grub._load_config(grub.GRUB_CONFIG) + + +@pytest.mark.parametrize( + "config_1, config_2", + [ + ({"GRUB_TIMEOUT": "0"}, {"GRUB_TIMEOUT": "0"}), + ( + {"GRUB_TIMEOUT": "0"}, + {"GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"}, + ), + ( + {"GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"}, + {"GRUB_TIMEOUT": "0"}, + ), + ], +) +def test_two_charms_no_conflict(config_1, config_2): + """Test two charms update GRUB configuration without any conflict.""" + for name, config in [("test-charm-1", config_1), ("test-charm-2", config_2)]: + grub_conf = grub.Config(name) + grub_conf.update(config) + assert config == grub._load_config(grub_conf.path) + + # check the main config + assert {**config_1, **config_2} == grub._load_config(grub.GRUB_CONFIG) + + +@pytest.mark.parametrize( + "config_1, config_2", + [ + ({"GRUB_TIMEOUT": "0"}, {"GRUB_TIMEOUT": "1"}), + ( + {"GRUB_TIMEOUT": "0"}, + { + "GRUB_TIMEOUT": "1", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + ), + ], +) +def test_two_charms_with_conflict(config_1, config_2): + """Test two charms update GRUB configuration with conflict.""" + # configure charm 1 + grub_conf_1 = grub.Config("test-charm-1") + grub_conf_1.update(config_1) + assert config_1 == grub._load_config(grub_conf_1.path) + + # configure charm 2 + grub_conf_2 = grub.Config("test-charm-2") + with pytest.raises(grub.ValidationError): + grub_conf_2.update(config_2) + + assert not grub_conf_2.path.exists() + # check the main config + assert config_1 == grub._load_config(grub.GRUB_CONFIG) + + +def test_charm_remove_configuration(): + """Test removing charm configuration.""" + config = {"GRUB_TIMEOUT": "0"} + grub_conf = grub.Config("test-charm") + grub_conf.update(config) + + assert grub_conf.path.exists(), "Config file is missing, check test_single_charm_valid_update" + assert config == grub._load_config(grub_conf.path) + assert config == grub._load_config(grub.GRUB_CONFIG) + + grub_conf.remove() + assert not grub_conf.path.exists() + assert {} == grub._load_config(grub.GRUB_CONFIG) + + +@pytest.mark.parametrize( + "config_1, config_2", + [ + ( + {"GRUB_TIMEOUT": "0"}, + { + "GRUB_TIMEOUT": "0", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + ), + ( + { + "GRUB_TIMEOUT": "0", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + {"GRUB_TIMEOUT": "0"}, + ), + ], +) +def test_charm_remove_configuration_without_changing_others(config_1, config_2): + """Test removing charm configuration and do not touch other.""" + grub_conf_1 = grub.Config("test-charm-1") + grub_conf_1.update(config_1) + grub_conf_2 = grub.Config("test-charm-2") + grub_conf_2.update(config_2) + + assert grub_conf_1.path.exists() + assert grub_conf_2.path.exists() + + grub_conf_1.remove() + assert not grub_conf_1.path.exists() + assert config_2 == grub._load_config(grub.GRUB_CONFIG) diff --git a/tests/unit/test_grub.py b/tests/unit/test_grub.py index d134cdbc..a0ab1c74 100644 --- a/tests/unit/test_grub.py +++ b/tests/unit/test_grub.py @@ -59,7 +59,7 @@ def test_is_container(output, exp_result): ) -class BaseTestGrubLib(unittest.TestCase): +class BaseTest(unittest.TestCase): def setUp(self) -> None: tmp_dir = tempfile.TemporaryDirectory() self.tmp_dir = Path(tmp_dir.name) @@ -67,14 +67,31 @@ def setUp(self) -> None: # configured paths grub.GRUB_DIRECTORY = self.tmp_dir + grub.GRUB_CONFIG = self.tmp_dir / "95-juju-charm.cfg" + with open(grub.GRUB_CONFIG, "w") as file: + file.write(GRUB_CONFIG_EXAMPLE) + + # is_container + mocker_is_container = mock.patch.object(grub, "is_container") + self.is_container = mocker_is_container.start() + self.is_container.return_value = False + self.addCleanup(mocker_is_container.stop) + + # subprocess + mocker_check_call = mock.patch.object(grub.subprocess, "check_call") + self.check_call = mocker_check_call.start() + self.addCleanup(mocker_check_call.stop) + + +class TestUtils(BaseTest): + def setUp(self) -> None: + super().setUp() # change logger mocked_logger = mock.patch.object(grub, "logger") self.logger = mocked_logger.start() self.addCleanup(mocked_logger.stop) - -class TestGrubUtils(BaseTestGrubLib): def test_split_config_line(self): """Test splitting single line.""" key, value = grub._split_config_line('test="1234"') @@ -138,7 +155,12 @@ def test_save_config(self): grub._save_config(path, {"test": '"1234"'}) mock_open.assert_called_once_with(path, "w", encoding="UTF-8") - mock_open.return_value.writelines.assert_called_once_with([mock.ANY, "test='\"1234\"'"]) + mock_open.return_value.write.assert_has_calls( + [ + mock.call(grub.CONFIG_HEADER), + mock.call("test='\"1234\"'\n"), + ] + ) def test_save_config_overwrite(self): """Test overwriting if GRUB config already exist.""" @@ -152,31 +174,55 @@ def test_save_config_overwrite(self): "GRUB config %s already exist and it will overwritten", path ) - @mock.patch("subprocess.check_call") + @mock.patch.object(grub, "_load_config") + @mock.patch.object(grub, "_save_config") + def test_update_config(self, mock_save, mock_load): + """Test update existing config file.""" + mock_load.return_value = {"test1": "1", "test2": "2"} + path = self.tmp_dir / "test-config" + path.touch() + + grub._update_config(path, {"test2": "22", "test3": "3"}) + + mock_load.assert_called_once_with(path) + mock_save.assert_called_once_with( + path, {"test1": "1", "test2": "22", "test3": "3"}, grub.CONFIG_HEADER + ) + + @mock.patch.object(grub, "_load_config") + @mock.patch.object(grub, "_save_config") + def test_update_not_existing_config(self, mock_save, mock_load): + """Test update not existing config file.""" + path = self.tmp_dir / "test-config" + + grub._update_config(path, {"test2": "22", "test3": "3"}) + + mock_load.assert_not_called() + mock_save.assert_called_once_with(path, {"test2": "22", "test3": "3"}, grub.CONFIG_HEADER) + @mock.patch("filecmp.cmp") - def test_check_update_grub(self, mock_filecmp, mock_check_call): + def test_check_update_grub(self, mock_filecmp): """Test check update function.""" grub.check_update_grub() - mock_check_call.assert_called_once_with( + self.check_call.assert_called_once_with( ["/usr/sbin/grub-mkconfig", "-o", "/tmp/tmp_grub.cfg"], stderr=subprocess.STDOUT ) mock_filecmp.assert_called_once_with( Path("/boot/grub/grub.cfg"), Path("/tmp/tmp_grub.cfg") ) - @mock.patch("subprocess.check_call") @mock.patch("filecmp.cmp") - def test_check_update_grub_failure(self, mock_filecmp, mock_check_call): + def test_check_update_grub_failure(self, mock_filecmp): """Test check update function.""" - mock_check_call.side_effect = subprocess.CalledProcessError(1, []) + self.check_call.side_effect = subprocess.CalledProcessError(1, []) - with self.assertRaises(subprocess.CalledProcessError): + with self.assertRaises(grub.ApplyError): grub.check_update_grub() mock_filecmp.assert_not_called() -class TestGrubConfig(BaseTestGrubLib): +class TestSmokeConfig(BaseTest): def setUp(self) -> None: super().setUp() # load config @@ -187,23 +233,15 @@ def setUp(self) -> None: mocker_save_config = mock.patch.object(grub, "_save_config") self.save_config = mocker_save_config.start() self.addCleanup(mocker_save_config.stop) - # is_container - mocker_is_container = mock.patch.object(grub, "is_container") - self.is_container = mocker_is_container.start() - self.is_container.return_value = False - self.addCleanup(mocker_is_container.stop) # check_update_grub mocker_check_update_grub = mock.patch.object(grub, "check_update_grub") self.check_update_grub = mocker_check_update_grub.start() + self.check_update_grub.return_value = True self.addCleanup(mocker_check_update_grub.stop) self.name = "charm-a" - self.path = self.tmp_dir / f"90-juju-{self.name}" - with open(self.path, "w") as file: - # create example of charm-a config - file.write(GRUB_CONFIG_EXAMPLE_BODY) self.config = grub.Config(self.name) - self.config._lazy_data = EXP_GRUB_CONFIG.copy() + self.load_config.return_value = EXP_GRUB_CONFIG.copy() def test_lazy_data_not_loaded(self): """Test data not loaded.""" @@ -291,7 +329,7 @@ def test_update_data(self): def test_applied_configs(self): """Test applied_configs property.""" exp_configs = { - self.path: self.load_config.return_value, + self.config.path: self.load_config.return_value, self.tmp_dir / "90-juju-charm-b": self.load_config.return_value, self.tmp_dir / "90-juju-charm-c": self.load_config.return_value, } @@ -302,7 +340,7 @@ def test_applied_configs(self): def test_blocked_keys(self): """Test blocked_keys property.""" exp_configs = { - self.path: {"A": "1", "B": "2"}, + self.config.path: {"A": "1", "B": "2"}, self.tmp_dir / "90-juju-charm-b": {"B": "3", "C": "2"}, self.tmp_dir / "90-juju-charm-c": {"D": "4"}, } @@ -318,36 +356,33 @@ def test_path(self): """Test path property.""" self.assertEqual(self.config.path, self.tmp_dir / f"90-juju-{self.name}") - @mock.patch("subprocess.check_call") - def test_apply_without_changes(self, mock_call): + def test_apply_without_changes(self): """Test applying GRUB config without any changes.""" self.check_update_grub.return_value = False self.config.apply() - self.check_update_grub.assert_called_once() - mock_call.assert_not_called() - - @mock.patch("subprocess.check_call") - def test_apply_with_new_changes(self, mock_call): - """Test applying GRUB config.""" - self.check_update_grub.return_value = True - self.config.apply() - - self.check_update_grub.assert_called_once() - mock_call.assert_called_once_with(["/usr/sbin/update-grub"], stderr=subprocess.STDOUT) + self.check_update_grub.assert_called_once_with() + self.check_call.assert_not_called() - @mock.patch("subprocess.check_call") - def test_apply_failure(self, mock_call): + def test_apply_failure(self): """Test applying GRUB config failure.""" - mock_call.side_effect = subprocess.CalledProcessError(1, []) + self.check_call.side_effect = subprocess.CalledProcessError(1, []) with self.assertRaises(grub.ApplyError): self.config.apply() + def test_apply(self): + """Test applying GRUB config failure.""" + self.config.apply() + self.check_update_grub.assert_called_once_with() + self.check_call.assert_called_once_with( + ["/usr/sbin/update-grub"], stderr=subprocess.STDOUT + ) + @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") def test_remove_no_config(self, mock_save, mock_apply): """Test removing when there is no charm config.""" - self.config.path.unlink() # remove charm config file + self.config.path.unlink(missing_ok=True) # remove charm config file changed_keys = self.config.remove() self.assertSetEqual(changed_keys, set()) @@ -358,144 +393,233 @@ def test_remove_no_config(self, mock_save, mock_apply): @mock.patch.object(grub.Config, "_save_grub_configuration") def test_remove_no_apply(self, mock_save, mock_apply): """Test removing without applying.""" + self.config.path.touch() # created charm file changed_keys = self.config.remove(apply=False) + self.assertFalse(self.config.path.exists()) self.assertSetEqual(changed_keys, set(EXP_GRUB_CONFIG.keys())) - mock_save.assert_called_once() + mock_save.assert_called_once_with() mock_apply.assert_not_called() @mock.patch.object(grub.Config, "apply") + @mock.patch.object(grub.Config, "applied_configs", call=mock.PropertyMock) @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_remove(self, mock_save, mock_apply): - """Test removing config for current charm.""" - exp_changed_keys = {"GRUB_RECORDFAIL_TIMEOUT"} - exp_configs = { - self.name: EXP_GRUB_CONFIG, - "charm-b": {"GRUB_TIMEOUT": "0"}, - "charm-c": { - "GRUB_TERMINAL": "console", - "GRUB_CMDLINE_LINUX_DEFAULT": '"$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"', - }, - } - self.load_config.side_effect = [exp_configs["charm-b"], exp_configs["charm-c"]] - (self.tmp_dir / "90-juju-charm-b").touch() - (self.tmp_dir / "90-juju-charm-c").touch() + def test_remove(self, mock_save, mock_applied_configs, mock_apply): + """Test removing with applying.""" + self.config.path.touch() # created charm file + mock_applied_configs.values.return_value = [ + {key: value for key, value in EXP_GRUB_CONFIG.items() if key != "GRUB_TIMEOUT"} + ] changed_keys = self.config.remove() - self.assertFalse((self.tmp_dir / self.name).exists()) - self.load_config.assert_has_calls( - [ - mock.call(self.tmp_dir / f"90-juju-{charm}") - for charm in exp_configs - if charm != self.name - ] - ) - self.assertSetEqual(changed_keys, exp_changed_keys) - self.assertDictEqual( - self.config._data, - { - "GRUB_TIMEOUT": "0", - "GRUB_TERMINAL": "console", - "GRUB_CMDLINE_LINUX_DEFAULT": '"$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"', - }, - ) - mock_save.assert_called_once() - mock_apply.assert_called_once() + self.assertFalse(self.config.path.exists()) + self.assertSetEqual(changed_keys, {"GRUB_TIMEOUT"}) + mock_save.assert_called_once_with() + mock_apply.assert_called_once_with() @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update_on_container(self, mock_save, mock_apply): + @mock.patch.object(grub, "_update_config") + def test_update_on_container(self, mock_update, mock_save, mock_apply): """Test update current GRUB config on container.""" self.is_container.return_value = True with self.assertRaises(grub.IsContainerError): self.config.update({"GRUB_TIMEOUT": "0"}) + self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG, "config was changed") mock_save.assert_not_called() mock_apply.assert_not_called() - self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG, "config was changed") + mock_update.assert_not_called() @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - @mock.patch.object( - grub.Config, "_set_value", side_effect=grub.ValidationError("test", "test_message") - ) - def test_update_validation_failure(self, _, mock_save, mock_apply): + @mock.patch.object(grub.Config, "_update") + @mock.patch.object(grub, "_update_config") + def test_update_validation_failure( + self, mock_update_config, mock_update, mock_save, mock_apply + ): """Test update current GRUB config with validation failure.""" + mock_update.side_effect = grub.ValidationError("GRUB_TIMEOUT", "failed") + with self.assertRaises(grub.ValidationError): # trying to set already existing key with different value -> ValidationError self.config.update({"GRUB_TIMEOUT": "1"}) + self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG, "snapshot was not restored") mock_save.assert_not_called() mock_apply.assert_not_called() - self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG, "config was changed") + mock_update_config.assert_not_called() @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update_apply_failure(self, mock_save, mock_apply): + @mock.patch.object(grub, "_update_config") + def test_update_apply_failure(self, mock_update, mock_save, mock_apply): """Test update current GRUB config with applied failure.""" - mock_apply.side_effect = grub.ApplyError("failed to apply") + mock_apply.side_effect = grub.ApplyError() with self.assertRaises(grub.ApplyError): self.config.update({"GRUB_NEW_KEY": "1"}) - mock_save.assert_has_calls( - [mock.call()] * 2, - "it should be called once before apply and one after snapshot restore", - ) - mock_apply.assert_called_once() self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG, "snapshot was not restored") + mock_save.assert_has_calls([mock.call(), mock.call()]) + mock_apply.assert_called_once_with() + mock_update.assert_not_called() @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update_without_changes(self, mock_save, mock_apply): + @mock.patch.object(grub, "_update_config") + def test_update_without_changes(self, mock_update, mock_save, mock_apply): """Test update current GRUB config without any changes.""" - changed_keys = self.config.update({"GRUB_TIMEOUT": "0"}) + # running with same key and value from example above + config = {"GRUB_TIMEOUT": "0"} + changed_keys = self.config.update(config) self.assertSetEqual(changed_keys, set()) mock_save.assert_not_called() mock_apply.assert_not_called() - self.save_config.assert_called_once_with(self.path, mock.ANY) - self.assertDictEqual(self.config._data, EXP_GRUB_CONFIG) + mock_update.assert_called_once_with(self.config.path, config) @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update(self, mock_save, mock_apply): - """Test update current GRUB config without applying it.""" - new_config = {"GRUB_NEW_KEY": "1"} - exp_config = {**EXP_GRUB_CONFIG, **new_config} + @mock.patch.object(grub, "_update_config") + def test_update_current_configuration(self, mock_update, mock_save, mock_apply): + """Test update current GRUB config with different values. - self.config.update(new_config) + This test is simulating the scenario, when same charm want to change it's own + values. + """ + # running with same key, but different value from example above + config = {"GRUB_TIMEOUT": "1"} + changed_keys = self.config.update(config) - mock_save.assert_called_once() - mock_apply.assert_called_once() - self.save_config.assert_called_once_with(self.path, new_config) - self.assertDictEqual(self.config._data, exp_config) + self.assertSetEqual(changed_keys, set(config.keys())) + mock_save.assert_called_once_with() + mock_apply.assert_called_once_with() + mock_update.assert_called_once_with(self.config.path, config) @mock.patch.object(grub.Config, "apply") @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update_same_charm(self, *_): - """Test update current GRUB config twice with different values. + @mock.patch.object(grub, "_update_config") + def test_update_without_apply(self, mock_update, mock_save, mock_apply): + """Test update current GRUB config with different values. This test is simulating the scenario, when same charm want to change it's own values. """ - first_config = {"GRUB_NEW_KEY": "0"} - new_config = {"GRUB_NEW_KEY": "1"} - exp_config = {**EXP_GRUB_CONFIG, **new_config} + # running with same key, but different value from example above + config = {"GRUB_TIMEOUT": "1"} + changed_keys = self.config.update(config, apply=False) - self.config.update(first_config) - self.config.update(new_config) + self.assertSetEqual(changed_keys, set(config.keys())) + mock_save.assert_called_once_with() + mock_apply.assert_not_called() + mock_update.assert_called_once_with(self.config.path, config) - self.assertDictEqual(self.config._data, exp_config) - @mock.patch.object(grub.Config, "apply") - @mock.patch.object(grub.Config, "_save_grub_configuration") - def test_update_without_apply(self, mock_save, mock_apply): - """Test update current GRUB config without applying it.""" - self.config.update({"GRUB_NEW_KEY": "1"}, apply=False) +class TestFullConfig(BaseTest): + """The tests contains minimal mocks to test more details.""" - mock_save.assert_called_once() - mock_apply.assert_not_called() - self.save_config.assert_called_once_with(self.path, mock.ANY) + def setUp(self) -> None: + super().setUp() + + # filecmp + mocker_filecmp = mock.patch.object(grub, "filecmp") + self.filecmp = mocker_filecmp.start() + self.filecmp.cmp.return_value = False # check_update_grub -> True + self.addCleanup(mocker_filecmp.stop) + + # define and create test charm configs + self.configs = { + "charm-1": { + "GRUB_TIMEOUT": EXP_GRUB_CONFIG["GRUB_TIMEOUT"], + "GRUB_RECORDFAIL_TIMEOUT": EXP_GRUB_CONFIG["GRUB_RECORDFAIL_TIMEOUT"], + "GRUB_CMDLINE_LINUX_DEFAULT": EXP_GRUB_CONFIG["GRUB_CMDLINE_LINUX_DEFAULT"], + }, + "charm-2": {"GRUB_TIMEOUT": EXP_GRUB_CONFIG["GRUB_TIMEOUT"]}, + "charm-3": { + "GRUB_TERMINAL": EXP_GRUB_CONFIG["GRUB_TERMINAL"], + "GRUB_CMDLINE_LINUX_DEFAULT": EXP_GRUB_CONFIG["GRUB_CMDLINE_LINUX_DEFAULT"], + }, + } + for name, conf in self.configs.items(): + grub._save_config(self.tmp_dir / f"{grub.CHARM_CONFIG_PREFIX}-{name}", conf) + + def test_remove(self): + """Test removing config for current charm.""" + name = "charm-1" + exp_changed_keys = {"GRUB_RECORDFAIL_TIMEOUT"} + config = grub.Config(name) + self.assertTrue(config.path.exists(), "missing required file for test") + + changed_keys = config.remove() + + self.assertFalse(config.path.exists()) + self.assertSetEqual(changed_keys, exp_changed_keys) + self.assertDictEqual( + config._data, + { + "GRUB_TIMEOUT": "0", + "GRUB_TERMINAL": "console", + "GRUB_CMDLINE_LINUX_DEFAULT": "$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G", + }, + ) + self.filecmp.cmp.assert_called_once_with( + Path("/boot/grub/grub.cfg"), Path("/tmp/tmp_grub.cfg") + ) + self.check_call.assert_has_calls( + [ + mock.call( + ["/usr/sbin/grub-mkconfig", "-o", "/tmp/tmp_grub.cfg"], + stderr=subprocess.STDOUT, + ), + mock.call(["/usr/sbin/update-grub"], stderr=subprocess.STDOUT), + ] + ) + + def test_update_validation_error(self): + """Test update raising ValidationError.""" + name = "charm-1" + new_config = {"GRUB_TIMEOUT": "1"} + exp_config = EXP_GRUB_CONFIG + exp_charm_config = self.configs[name] + + config = grub.Config(name) + + with pytest.raises(grub.ValidationError): + config.update(new_config) + + self.assertDictEqual(config._data, exp_config) + self.assertDictEqual(grub._load_config(grub.GRUB_CONFIG), exp_config) + self.assertDictEqual(grub._load_config(config.path), exp_charm_config) + self.filecmp.cmp.assert_not_called() + self.check_call.assert_not_called() + + def test_update(self): + """Test successful update.""" + name = "charm-1" + new_config = {"GRUB_RECORDFAIL_TIMEOUT": "1"} + exp_config = {**EXP_GRUB_CONFIG, **new_config} + exp_charm_config = {**self.configs[name], **new_config} + + config = grub.Config(name) + + changed_keys = config.update(new_config) + + self.assertSetEqual(changed_keys, set(new_config.keys())) + self.assertDictEqual(config._data, exp_config) + self.assertDictEqual(grub._load_config(grub.GRUB_CONFIG), exp_config) + self.assertDictEqual(grub._load_config(config.path), exp_charm_config) + self.filecmp.cmp.assert_called_once_with( + Path("/boot/grub/grub.cfg"), Path("/tmp/tmp_grub.cfg") + ) + self.check_call.assert_has_calls( + [ + mock.call( + ["/usr/sbin/grub-mkconfig", "-o", "/tmp/tmp_grub.cfg"], + stderr=subprocess.STDOUT, + ), + mock.call(["/usr/sbin/update-grub"], stderr=subprocess.STDOUT), + ] + ) diff --git a/tox.ini b/tox.ini index 04aad1b8..b87321b5 100644 --- a/tox.ini +++ b/tox.ini @@ -14,6 +14,7 @@ lib_dir = {toxinidir}/lib/charms/operator_libs_linux/ all_dir = {[vars]src_dir} {[vars]tst_dir} {[vars]lib_dir} lxd_ubuntu = ops-libs-test-ubuntu lxd_centos = ops-libs-test-centos +wait = 5 [testenv] setenv = @@ -69,10 +70,13 @@ allowlist_externals = bash commands = # Create a LXD containers for Ubuntu and CentOS with necessary packages installed. - bash -c 'lxc launch -qe ubuntu:focal {[vars]lxd_ubuntu} -c=user.user-data="$(<{[vars]itst_dir}/test_setup.yaml)"' - bash -c 'lxc launch -qe images:centos/9-Stream/cloud {[vars]lxd_centos} -c=user.user-data="$(<{[vars]itst_dir}/test_setup.yaml)"' + # Note (rgildein): running integration tests on a VM because the grub library could not be used on containers + bash -c 'lxc launch --vm -qe ubuntu:focal {[vars]lxd_ubuntu} -c=user.user-data="$(<{[vars]itst_dir}/test_setup.yaml)"' + bash -c 'lxc launch --vm -qe images:centos/9-Stream/cloud {[vars]lxd_centos} -c=user.user-data="$(<{[vars]itst_dir}/test_setup.yaml)"' + bash -c 'while !(lxc exec {[vars]lxd_ubuntu} -- bash -c "echo ready"); do sleep {[vars]wait}; done' + bash -c 'while !(lxc exec {[vars]lxd_centos} -- bash -c "echo ready"); do sleep {[vars]wait}; done' - # Wait for the cloud-init process to finish in both Ubuntu and CentOS image. + # Wait for the cloud-init process to finish in both Ubuntu and CentOS image lxc exec {[vars]lxd_ubuntu} -- bash -c "cloud-init status -w >/dev/null 2>&1" lxc exec {[vars]lxd_centos} -- bash -c "cloud-init status -w >/dev/null 2>&1" @@ -132,8 +136,8 @@ deps = -r {toxinidir}/requirements.txt commands = pytest -v \ - -s \ - --tb native \ - --log-cli-level=INFO \ - {[vars]tst_dir}integration/juju_systemd_notices - {posargs} + -s \ + --tb native \ + --log-cli-level=INFO \ + {[vars]tst_dir}integration/juju_systemd_notices + {posargs}