Merge ~vultaire/charm-sysconfig:lp1841809 into charm-sysconfig:master

[Paul Goins]

Adding resolved-cache-mode config option

* Added the feature
* Added tests for the feature
* Removed 'cosmic' option from test_deploy.py since it adds significant test overhead time (10 minutes on certain tests), due to there not being a cosmic charm available to deploy with.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Alvaro Uria (aluria)]

* typo: Default value on config.yaml
* nitpick: code style
* Behavior around updating the resolved.conf file (systemd-resolved service should be restarted instead of notifying that the unit needs a reboot)

review: Needs Fixing

[Paul Goins (vultaire)]

Noting my responses. Will make changes.

[Paul Goins (vultaire)]

@aluria: Care to give this another look? I think I've addressed everything.

[Alvaro Uria (aluria)]

Hey Paul. I've added a comment inline, but I will repeat it here.
* checksum verification can be achieved via any_file_changed [1], helper from the reactive framework, A list of file(s) needs to be passed.
* "render" already checks if new content is going to be written, or it will skip the write
* As a suggestion of a diff approach, set_flag("systemd-resolved.restart") could always be set after the update and remove functions are called (maybe even merging the action into a single method?) and any_file_changed could be evaluated in the decorated function (within the reactive script), to see if a service restart is needed or not (+ clear the flag).

And think the above will make more sense in the comment inline.

Other than this, the change (and tests) look great.

1. https://github.com/juju-solutions/charms.reactive/blob/master/charms/reactive/helpers.py#L85

review: Needs Fixing

[Paul Goins (vultaire)]

Most comments addressed. Did not use set_flag(...) since cpufrequtils also handles a service restart in the same way (in the lib module rather than via a flag-based reactive hook); if we change it for resolved, I think we should change it for cpufrequtils as well - and that starts to work its way out of scope in my opinion.

[Alvaro Uria (aluria)]

Hey Paul, Thank you! I've added a minor suggestion for readability purpose, but I won't block it for merging :)

review: Approve

[Alvaro Uria (aluria)]

Hey Paul. I suggested the use of a list for [ONE_FILE], but failed to check the change. Good catch. I also failed to mention that "import hashlib" may not be needed anymore. Other than that, all looks good to me.

[Jeremy Lounder (jldev)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision cb5240074d70796e3c06b257756b00013a0ffda4

diff --git a/src/config.yaml b/src/config.yaml
index bd60238..4b93f3c 100644
--- a/src/config.yaml
+++ b/src/config.yaml
@@ -122,3 +122,9 @@ options:
                         Recommended option when Bios control power is set to the OS.
         - 'performance'
         - 'powersave'
+  resolved-cache-mode:
+    type: string
+    default: ""
+    description: |
+        Sets the "Cache" configuration value in /etc/systemd/resolved.conf.  Valid
+        values are '' (system default), 'yes', 'no', and 'no-negative'.
diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index 51bf049..73e1311 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -2,7 +2,6 @@
 
 Manage grub, systemd, coufrequtils and kernel version configuration.
 """
-
 import os
 import subprocess
 from datetime import datetime, timedelta, timezone
@@ -11,15 +10,18 @@ from charmhelpers.contrib.openstack.utils import config_flags_parser
 from charmhelpers.core import hookenv, host, unitdata
 from charmhelpers.core.templating import render
 from charmhelpers.fetch import apt_install, apt_update
+from charms.reactive.helpers import any_file_changed
 
 GRUB_DEFAULT = 'Advanced options for Ubuntu>Ubuntu, with Linux {}'
 CPUFREQUTILS_TMPL = 'cpufrequtils.j2'
 GRUB_CONF_TMPL = 'grub.j2'
 SYSTEMD_SYSTEM_TMPL = 'etc.systemd.system.conf.j2'
+SYSTEMD_RESOLVED_TMPL = 'etc.systemd.resolved.conf.j2'
 
 CPUFREQUTILS = '/etc/default/cpufrequtils'
 GRUB_CONF = '/etc/default/grub.d/90-sysconfig.cfg'
 SYSTEMD_SYSTEM = '/etc/systemd/system.conf'
+SYSTEMD_RESOLVED = '/etc/systemd/resolved.conf'
 KERNEL = 'kernel'
 
 
@@ -155,25 +157,30 @@ class SysConfigHelper:
 
     @property
     def systemd_config_flags(self):
-        """Return grub-config-flags config option."""
+        """Return systemd-config-flags config option."""
         return parse_config_flags(self.charm_config['systemd-config-flags'])
 
     @property
     def kernel_version(self):
-        """Return grub-config-flags config option."""
+        """Return kernel-version config option."""
         return self.charm_config['kernel-version']
 
     @property
     def update_grub(self):
-        """Return grub-config-flags config option."""
+        """Return update-grub config option."""
         return self.charm_config['update-grub']
 
     @property
     def governor(self):
-        """Return grub-config-flags config option."""
+        """Return governor config option."""
         return self.charm_config['governor']
 
     @property
+    def resolved_cache_mode(self):
+        """Return resolved-cache-mode config option."""
+        return self.charm_config['resolved-cache-mode']
+
+    @property
     def config_flags(self):
         """Return parsed config-flags into dict.
 
@@ -187,9 +194,14 @@ class SysConfigHelper:
 
     def _render_boot_resource(self, source, target, context):
         """Render the template and set the resource as changed."""
-        render(source=source, templates_dir='templates', target=target, context=context)
+        self._render_resource(source, target, context)
         self.boot_resources.set_resource(target)
 
+    @staticmethod
+    def _render_resource(source, target, context):
+        """Render the template."""
+        render(source=source, templates_dir='templates', target=target, context=context)
+
     def _is_kernel_already_running(self):
         """Check if the kernel version required by charm config is equal to kernel running."""
         configured = self.kernel_version
@@ -208,18 +220,15 @@ class SysConfigHelper:
         """Validate config parameters."""
         valid = True
 
-        if self.reservation not in ['off', 'isolcpus', 'affinity']:
-            hookenv.log('reservation not valid. Possible values: ["off", "isolcpus", "affinity"]', hookenv.DEBUG)
-            valid = False
-
-        if self.raid_autodetection not in ['', 'noautodetect', 'partitionable']:
-            hookenv.log('raid-autodetection not valid. '
-                        'Possible values: ["off", "noautodetect", "partitionable"]', hookenv.DEBUG)
-            valid = False
-
-        if self.governor not in ['', 'powersave', 'performance']:
-            hookenv.log('governor not valid. Possible values: ["", "powersave", "performance"]', hookenv.DEBUG)
-            valid = False
+        for config_key, value, valid_values in (
+                ('reservation', self.reservation, ['off', 'isolcpus', 'affinity']),
+                ('raid-autodetection', self.raid_autodetection, ['', 'noautodetect', 'partitionable']),
+                ('governor', self.governor, ['', 'powersave', 'performance']),
+                ('resolved-cache-mode', self.resolved_cache_mode, ['', 'yes', 'no', 'no-negative']),
+        ):
+            if value not in valid_values:
+                hookenv.log('{} not valid. Possible values: {}'.format(config_key, repr(valid_values)), hookenv.DEBUG)
+                valid = False
 
         return valid
 
@@ -272,6 +281,14 @@ class SysConfigHelper:
         self._render_boot_resource(SYSTEMD_SYSTEM_TMPL, SYSTEMD_SYSTEM, context)
         hookenv.log('systemd configuration updated')
 
+    def update_systemd_resolved(self):
+        """Update /etc/systemd/resolved.conf according to charm configuration."""
+        context = {}
+        if self.resolved_cache_mode:
+            context['cache'] = self.resolved_cache_mode
+        self._update_systemd_resolved(context)
+        hookenv.log('systemd-resolved configuration updated')
+
     def install_configured_kernel(self):
         """Install kernel as given by the kernel-version config option.
 
@@ -341,6 +358,19 @@ class SysConfigHelper:
             hookenv.DEBUG
         )
 
+    def remove_resolved_configuration(self):
+        """Remove systemd's resolved configuration.
+
+        Will render resolved config with defaults.
+        """
+        self._update_systemd_resolved({})
+        hookenv.log('deleted resolved configuration at '.format(SYSTEMD_RESOLVED), hookenv.DEBUG)
+
+    def _update_systemd_resolved(self, context):
+        self._render_resource(SYSTEMD_RESOLVED_TMPL, SYSTEMD_RESOLVED, context)
+        if any_file_changed([SYSTEMD_RESOLVED]):
+            host.service_restart('systemd-resolved')
+
     def remove_cpufreq_configuration(self):
         """Remove cpufrequtils configuration.
 
diff --git a/src/reactive/sysconfig.py b/src/reactive/sysconfig.py
index eba0916..6c70fba 100644
--- a/src/reactive/sysconfig.py
+++ b/src/reactive/sysconfig.py
@@ -28,7 +28,7 @@ from charms.reactive import (
     when_not,
 )
 
-from lib_sysconfig import CPUFREQUTILS, GRUB_CONF, KERNEL, SYSTEMD_SYSTEM, SysConfigHelper
+from lib_sysconfig import CPUFREQUTILS, GRUB_CONF, KERNEL, SYSTEMD_SYSTEM, SYSTEMD_RESOLVED, SysConfigHelper
 
 
 @when_none('sysconfig.installed', 'sysconfig.unsupported')
@@ -54,6 +54,7 @@ def install_sysconfig():
     syshelper.update_cpufreq()
     syshelper.update_grub_file()
     syshelper.update_systemd_system_file()
+    syshelper.update_systemd_resolved()
     set_flag('sysconfig.installed')
     update_status()
 
@@ -103,6 +104,12 @@ def config_changed():
     )) or helpers.any_file_changed([SYSTEMD_SYSTEM]):
         syshelper.update_systemd_system_file()
 
+    # systemd resolved
+    if any(syshelper.charm_config.changed(flag) for flag in (
+            'resolved-cache-mode',
+    )) or helpers.any_file_changed([SYSTEMD_RESOLVED]):
+        syshelper.update_systemd_resolved()
+
     update_status()
 
 
@@ -142,5 +149,6 @@ def remove_configuration():
     syshelper.remove_cpufreq_configuration()
     syshelper.remove_grub_configuration()
     syshelper.remove_systemd_configuration()
+    syshelper.remove_resolved_configuration()
     clear_flag('sysconfig.installed')
     clear_flag('sysconfig.unsupported')
diff --git a/src/templates/etc.systemd.resolved.conf.j2 b/src/templates/etc.systemd.resolved.conf.j2
 new file mode 100644
index 0000000..ab8832b
--- /dev/null
+++ b/src/templates/etc.systemd.resolved.conf.j2
@@ -0,0 +1,27 @@
+#  This file is part of systemd.
+#
+#  systemd is free software; you can redistribute it and/or modify it
+#  under the terms of the GNU Lesser General Public License as published by
+#  the Free Software Foundation; either version 2.1 of the License, or
+#  (at your option) any later version.
+#
+# Entries in this file show the compile time defaults.
+# You can change settings by editing this file.
+# Defaults can be restored by simply deleting this file.
+#
+# See resolved.conf(5) for details
+
+[Resolve]
+#DNS=
+#FallbackDNS=
+#Domains=
+#LLMNR=no
+#MulticastDNS=no
+#DNSSEC=no
+#DNSOverTLS=no
+#Cache=yes
+{% if cache -%}
+Cache={{ cache }}
+{% endif -%}
+#DNSStubListener=yes
+#ReadEtcHosts=yes
diff --git a/src/tests/functional/test_deploy.py b/src/tests/functional/test_deploy.py
index 968288b..2900e06 100644
--- a/src/tests/functional/test_deploy.py
+++ b/src/tests/functional/test_deploy.py
@@ -2,9 +2,11 @@
 
 import asyncio
 import os
+import re
 import subprocess
 
 import pytest
+import websockets
 
 
 # Treat all tests as coroutines
@@ -14,12 +16,12 @@ charm_build_dir = os.getenv('CHARM_BUILD_DIR', '..').rstrip('/')
 
 series = ['xenial',
           'bionic',
-          pytest.param('cosmic', marks=pytest.mark.xfail(reason='canary')),
           ]
 
 sources = [('local', '{}/builds/sysconfig'.format(charm_build_dir))]
 
 TIMEOUT = 600
+MODEL_ACTIVE_TIMEOUT = 10
 GRUB_DEFAULT = 'Advanced options for Ubuntu>Ubuntu, with Linux {}'
 PRINCIPAL_APP_NAME = 'ubuntu-{}'
 
@@ -269,14 +271,19 @@ async def test_wrong_reservation(app, model):
     )
 
 
-async def test_wrong_raid_autodetection(app, model):
-    """Tests wrong raid-autodetection value is used.
[e-mail]('key,bad_value,good_value', [
+    ('raid-autodetection', 'changeme', ''),
+    ('governor', 'changeme', ''),
+    ('resolved-cache-mode', 'changeme', ''),
+])
+async def test_invalid_configuration_parameters(app, model, key, bad_value, good_value):
+    """Tests wrong config value is used.
 
     Expect application is blocked until correct value is set.
     """
     await app.set_config(
         {
-            'raid-autodetection': 'changeme'
+            key: bad_value
         }
     )
     await model.block_until(
@@ -289,7 +296,7 @@ async def test_wrong_raid_autodetection(app, model):
 
     await app.set_config(
         {
-            'raid-autodetection': ''
+            key: good_value
         }
     )
     await model.block_until(
@@ -298,33 +305,32 @@ async def test_wrong_raid_autodetection(app, model):
     )
 
 
-async def test_wrong_governor(app, model):
-    """Tests wrong governor value is used.
[e-mail]('cache_setting', [
+    'yes',
+    'no',
+    'no-negative',
+])
+async def test_set_resolved_cache(app, model, jujutools, cache_setting):
+    """Tests resolved cache settings."""
+    def is_model_settled():
+        return app.units[0].workload_status == 'active' and app.units[0].agent_status == 'idle'
 
-    Expect application is blocked until correct value is set.
-    """
-    await app.set_config(
-        {
-            'governor': 'changeme'
-        }
-    )
-    await model.block_until(
-        lambda: app.status == 'blocked',
-        timeout=TIMEOUT
-    )
-    assert app.status == 'blocked'
-    unit = app.units[0]
-    assert 'configuration parameters not valid.' in unit.workload_status_message
+    await model.block_until(is_model_settled, timeout=TIMEOUT)
 
-    await app.set_config(
-        {
-            'governor': ''
-        }
-    )
-    await model.block_until(
-        lambda: app.status == 'active',
-        timeout=TIMEOUT
-    )
+    await app.set_config({'resolved-cache-mode': cache_setting})
+    # NOTE: app.set_config() doesn't seem to wait for the model to go to a
+    # non-active/idle state.
+    try:
+        await model.block_until(lambda: not is_model_settled(), timeout=MODEL_ACTIVE_TIMEOUT)
+    except websockets.ConnectionClosed:
+        # It's possible (although unlikely) that we missed the charm transitioning from
+        # idle to active and back.
+        pass
+
+    await model.block_until(is_model_settled, timeout=TIMEOUT)
+
+    resolved_conf_content = await jujutools.file_contents('/etc/systemd/resolved.conf', app.units[0])
+    assert re.search('^Cache={}$'.format(cache_setting), resolved_conf_content, re.MULTILINE)
 
 
 async def test_uninstall(app, model, jujutools, series):
@@ -365,6 +371,10 @@ async def test_uninstall(app, model, jujutools, series):
     systemd_content = await jujutools.file_contents(systemd_path, unit)
     assert 'CPUAffinity=1,2,3,4' not in systemd_content
 
+    resolved_path = '/etc/systemd/resolved.conf'
+    resolved_content = await jujutools.file_contents(resolved_path, unit)
+    assert not re.search('^Cache=', resolved_content, re.MULTILINE)
+
     cpufreq_path = '/etc/default/cpufrequtils'
     cpufreq_content = await jujutools.file_contents(cpufreq_path, unit)
     assert 'GOVERNOR' not in cpufreq_content
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index b7ac651..be25eb4 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -1,13 +1,14 @@
 #!/usr/bin/python3
 """Unit tests for SysConfigHelper and BootResourceState classes."""
-
-
+import os
 import subprocess
+import tempfile
 from datetime import datetime, timezone
 
 import lib_sysconfig
 
 import mock
+import pytest
 
 
 class TestBootResourceState:
@@ -479,45 +480,22 @@ class TestLib:
         )
         restart.assert_called()
 
+    @pytest.mark.parametrize("invalid_config_key", [
+        "reservation", "raid-autodetection", "governor", "resolved-cache-mode"])
     @mock.patch("lib_sysconfig.hookenv.config")
-    def test_wrong_reservation(self, config):
-        """Test wrong reservation value.
-
-        Expect that is_config_valid() return false
-        """
-        config.return_value = {
-            "reservation": "wrong",
-            "raid-autodetection": "",
-            "governor": ""
-        }
-        sysh = lib_sysconfig.SysConfigHelper()
-        assert not sysh.is_config_valid()
-
-    @mock.patch("lib_sysconfig.hookenv.config")
-    def test_wrong_raid(self, config):
-        """Test wrong raid autodetection value.
-
-        Expect that is_config_valid() return false
-        """
-        config.return_value = {
-            "reservation": "off",
-            "raid-autodetection": "wrong",
-            "governor": ""
-        }
-        sysh = lib_sysconfig.SysConfigHelper()
-        assert not sysh.is_config_valid()
-
-    @mock.patch("lib_sysconfig.hookenv.config")
-    def test_wrong_governor(self, config):
-        """Test wrong governor value.
+    def test_wrong_config(self, config, invalid_config_key):
+        """Test wrong configuration value.
 
         Expect that is_config_valid() return false
         """
-        config.return_value = {
+        return_value = {
             "reservation": "off",
             "raid-autodetection": "",
-            "governor": "wrong"
+            "governor": "",
+            "resolved-cache-mode": "",
+            invalid_config_key: 'wrong',  # Will override the selected key with an invalid value
         }
+        config.return_value = return_value
         sysh = lib_sysconfig.SysConfigHelper()
         assert not sysh.is_config_valid()
 
@@ -530,3 +508,32 @@ class TestLib:
         sysh = lib_sysconfig.SysConfigHelper()
 
         assert sysh.enable_container
+
+    @mock.patch("lib_sysconfig.any_file_changed")
+    @mock.patch("lib_sysconfig.hookenv.config")
+    @mock.patch("lib_sysconfig.host.service_restart")
+    @mock.patch("lib_sysconfig.render")
+    def test_update_resolved_file_unchanged(self, render, restart, config, file_changed):
+        file_changed.return_value = True
+        self._test_update_resolved_common(render, config)
+        restart.assert_not_called()
+
+    @mock.patch("lib_sysconfig.any_file_changed")
+    @mock.patch("lib_sysconfig.hookenv.config")
+    @mock.patch("lib_sysconfig.host.service_restart")
+    @mock.patch("lib_sysconfig.render")
+    def test_update_resolved_file_changed(self, render, restart, config, file_changed):
+        file_changed.return_value = True
+        self._test_update_resolved_common(render, config)
+        restart.assert_called()
+
+    def _test_update_resolved_common(self, render, config):
+        config.return_value = {"resolved-cache-mode": "no-negative"}
+        sysh = lib_sysconfig.SysConfigHelper()
+        sysh.update_systemd_resolved()
+        render.assert_called_with(
+            source=lib_sysconfig.SYSTEMD_RESOLVED_TMPL,
+            target=lib_sysconfig.SYSTEMD_RESOLVED,
+            templates_dir="templates",
+            context={"cache": "no-negative"},
+        )