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

@@ -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/[email protected]
-        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:

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

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():

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)