Merge ~afreiberger/charm-sysconfig:lp1854135 into charm-sysconfig:master

[Drew Freiberger]

Verify grub/systemd parameters and affinity

    * Verify that grub parameters are set before running update_grub_file
    function. If it is not verified, this function will run and set the
    default options through the grub template which is undesirable.
    * Verify that systemd parameters are set before running
    update_systemd_system_file function.

11b15a8... by Drew Freiberger on 2020-01-14

Verify grub/systemd parameters and affinity

* Verify that grub parameters are set before running update_grub_file
function. If it is not verified, this function will run and set the
default options through the grub template which is undesirable.
* Verify that systemd parameters are set before running
update_systemd_system_file function.

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Peter Sabaini (peter-sabaini)]

LGTM, cheers

review: Approve

[Alvaro Uria (aluria)]

I think this change will conflict with Paul's, which uses pytest for the unit testing.

[Alvaro Uria (aluria)]

This change needs to be rebased. OTOH, I have added a couple of comments about the verifications added. If the issue is that checksums don't exist, shouldn't we call any_file_changed (without any action) in the entry function (install_sysconfig) so checksums are initialized? By doing so, we would only need to check that a file has changed (instead of validating every possible config parameter that affects the config file(s)).

review: Needs Fixing

[Drew Freiberger (afreiberger)]

I think you may be onto something with using any_file_changed. While the intention is not to render a grub config if no change was detected from the charm, we could accomplish that with a data_changed query. The reason for this would be that kernel upgrades would change contents of grub files between config-changed runs, so we'd need to know if there were changes effected since last install/upgrade-charm/config-changed that would affect a change in config files before re-running a grub config.

I see your point about the if statements not catching all exceptions and need to update the logic there. Maybe incorporating a dict of those values and assess the impact via data_changed could make this a bit more readable and consistent. Likely evaluating all the configs except for reservation, and then evaluating the setting/changing of reservation with the exclusion of off/isolcpus.

If I get time, I'll add this as a stretch goal for addressing in 20.01 release.

Unmerged commits

11b15a8... by Drew Freiberger on 2020-01-14

Verify grub/systemd parameters and affinity

* Verify that grub parameters are set before running update_grub_file
function. If it is not verified, this function will run and set the
default options through the grub template which is undesirable.
* Verify that systemd parameters are set before running
update_systemd_system_file function.

diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index 51bf049..b938f35 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -223,11 +223,30 @@ class SysConfigHelper:
 
         return valid
 
+    def is_update_grub_needed(self):
+        if any(self.charm_config[flag] for flag in (
+            'reservation',
+            'hugepages',
+            'hugepagesz',
+            'raid-autodetection',
+            'enable-pti',
+            'enable-iommu',
+            'grub-config-flags',
+            'kernel-version',
+            'update-grub',
+            'config-flags',
+            'cpu-range',
+        )) and self.reservation not in ('off', 'affinity'):
+            return True
+        return False
+
     def update_grub_file(self):
         """Update /etc/default/grub.d/90-sysconfig.cfg according to charm configuration.
 
         Will call update-grub if update-grub config is set to True.
         """
+        if not self.is_update_grub_needed():
+            return
         context = {}
         if self.reservation == 'isolcpus':
             context['cpu_range'] = self.cpu_range
@@ -256,8 +275,20 @@ class SysConfigHelper:
         hookenv.log('grub configuration updated')
         self._update_grub()
 
+    def is_update_systemd_needed(self):
+        if any(self.charm_config[flag] for flag in (
+            'reservation',
+            'systemd-config-flags',
+            'cpu-range',
+            'config-flags',
+        )) and self.reservation not in ('off', 'isolcpus'):
+            return True
+        return False
+
     def update_systemd_system_file(self):
         """Update /etc/systemd/system.conf according to charm configuration."""
+        if not self.is_update_systemd_needed():
+            return
         context = {}
         if self.reservation == 'affinity':
             context['cpu_range'] = self.cpu_range
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index b7ac651..3534ca9 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -198,6 +198,33 @@ class TestLib:
     @mock.patch("lib_sysconfig.hookenv.config")
     @mock.patch("lib_sysconfig.hookenv.log")
     @mock.patch("lib_sysconfig.render")
+    def test_update_grub_file_false(self, render, log, config, check_call):
+        """Do not update /etc/default/grub.d/90-sysconfig.cfg and update-grub false.
+
+        Expect file is not rendered and updated-grub is not called.
+        """
+        config.return_value = {
+            "reservation": "off",
+            "cpu-range": "",
+            "hugepages": "",
+            "hugepagesz": "",
+            "raid-autodetection": "",
+            "enable-pti": False,
+            "enable-iommu": False,
+            "grub-config-flags": "",
+            "kernel-version": "",
+            "update-grub": False,
+        }
+
+        sysh = lib_sysconfig.SysConfigHelper()
+        sysh.update_grub_file()
+        render.assert_not_called()
+        check_call.assert_not_called()
+
+    @mock.patch("lib_sysconfig.subprocess.check_call")
+    @mock.patch("lib_sysconfig.hookenv.config")
+    @mock.patch("lib_sysconfig.hookenv.log")
+    @mock.patch("lib_sysconfig.render")
     def test_legacy_grub_config_flags(self, render, log, config, check_call):
         """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub true.
 
@@ -307,6 +334,25 @@ class TestLib:
     @mock.patch("lib_sysconfig.hookenv.config")
     @mock.patch("lib_sysconfig.hookenv.log")
     @mock.patch("lib_sysconfig.render")
+    def test_update_systemd_system_file_false(self, render, log, config):
+        """Do not update /etc/default/grub.d/90-sysconfig.cfg
+
+        Expect file is not rendered given blank configs for file
+        """
+        config.return_value = {
+            "reservation": "off",
+            "cpu-range": "",
+            "systemd-config-flags": "",
+            "config-flags": "",
+        }
+
+        sysh = lib_sysconfig.SysConfigHelper()
+        sysh.update_systemd_system_file()
+        render.assert_not_called()
+
+    @mock.patch("lib_sysconfig.hookenv.config")
+    @mock.patch("lib_sysconfig.hookenv.log")
+    @mock.patch("lib_sysconfig.render")
     def test_legacy_systemd_config_flags(self, render, log, config):
         """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub false.