Merge charm-sysconfig:bug/1923171 into charm-sysconfig:master

[Peter Sabaini]

Fix missing linux-modules-extra

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:a7fe8945f54340b6afb2852b271cb7ea8f8c6edc
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/1/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/56/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/143/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/1//rebuild

review: Needs Fixing (continuous-integration)

[Paul Goins (vultaire)]

I'm +1 for this change, but note that CI is failing:

./lib/lib_sysconfig.py:360:5: C901 'SysConfigHelper.update_grub_file' is too complex (11)

If possible to fix things for CI, please do so.

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:5e66cf6e2e726ba46eddfc1602ee32e74808b1ed
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/2/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/85/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/227/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/2//rebuild

review: Needs Fixing (continuous-integration)

[Xav Paice (xavpaice)]

CI test fail seems to be a genuine failure here, we'll need that fixed before merging.

[Celia Wang (ziyiwang)]

unittests passed on charmlab: https://pastebin.canonical.com/p/3DN7Bk8BqN/
func tests also passed: https://pastebin.canonical.com/p/726YpJBShc/

CI failed with unit tests:
tests/unit/test_lib.py::TestLib::test_update_grub_file FAILED [ 37%]
tests/unit/test_lib.py::TestLib::test_grub_legacy_reservation FAILED [ 40%]
tests/unit/test_lib.py::TestLib::test_legacy_grub_config_flags FAILED [ 42%]

[Drew Freiberger (afreiberger)]

I also get successful unit testing on my Hirsuite laptop.

I think that with the unit tests, we need to mock the lib_sysconfig.host.is_container() with return_value = False to make this unit test pass.

It may be that the jenkins worker is triggering is_container() to be True, and we're just not mocking that during unit testing and have been lucky with running this on VMs and laptops.

Issue is not caused by this commit, but I'm going to add in the mock to see if it resolves this CI issue.

[Drew Freiberger (afreiberger)]

When testing this patch, I did set is_container = True and got the same failure as jenkins. setting to = False resolves. Let's pray for CI.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:0695c4b340f03ec457590eb6478e2fbfb458c69a
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/4/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/157/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/408/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/4//rebuild

review: Approve (continuous-integration)

[Drew Freiberger (afreiberger)]

lgtm and passes CI now.

review: Approve

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

Change successfully merged at revision 505ab8823cb78953449391976a81fb6dc9abec3b

diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index d2c4baa..be5de9d 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -357,11 +357,7 @@ class SysConfigHelper:
 
         return valid
 
-    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.
-        """
+    def _assemble_context(self):
         context = {}
 
         # The isolcpus boot option can be used to isolate one or more CPUs at
@@ -372,14 +368,11 @@ class SysConfigHelper:
 
         # This is to keep the old method of specifying the isolcpus
         # by specifying the reservation and the cpu_range
-        if self.isolcpus:
-            context["isolcpus"] = self.isolcpus
-        if self.hugepages:
-            context["hugepages"] = self.hugepages
-        if self.hugepagesz:
-            context["hugepagesz"] = self.hugepagesz
-        if self.default_hugepagesz:
-            context["default_hugepagesz"] = self.default_hugepagesz
+        for attr in ["isolcpus", "hugepages", "hugepagesz", "default_hugepagesz"]:
+            val = getattr(self, attr, None)
+            if val:
+                context[attr] = val
+
         if self.raid_autodetection:
             context["raid"] = self.raid_autodetection
         if not self.enable_pti:
@@ -401,6 +394,14 @@ class SysConfigHelper:
         if self.kernel_version and not self._is_kernel_already_running():
             context["grub_default"] = GRUB_DEFAULT.format(self.kernel_version)
 
+        return context
+
+    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.
+        """
+        context = self._assemble_context()
         self._render_boot_resource(GRUB_CONF_TMPL, GRUB_CONF, context)
         hookenv.log("grub configuration updated")
         self._update_grub()
@@ -441,18 +442,15 @@ class SysConfigHelper:
 
         Will install kernel and matching modules-extra package
         """
-        if not self.kernel_version or self._is_kernel_already_running():
-            hookenv.log("Kernel is already running the required version", hookenv.DEBUG)
-            return
-
         configured = self.kernel_version
-        pkgs = [
-            tmpl.format(configured)
-            for tmpl in ["linux-image-{}", "linux-modules-extra-{}"]
-        ]
+        if not configured:
+            return
         apt_update()
-        apt_install(pkgs)
-        hookenv.log("installing: {}".format(pkgs))
+        apt_install("linux-modules-extra-{}".format(configured))
+        if self._is_kernel_already_running():
+            hookenv.log("Kernel is already running the required version", hookenv.DEBUG)
+            return
+        apt_install("linux-image-{}".format(configured))
         self.boot_resources.set_resource(KERNEL)
 
     def update_cpufreq(self):
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index d23feb1..ad9a140 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -190,15 +190,17 @@ class TestLib:
         restart.assert_not_called()
         check_call.assert_not_called()
 
+    @mock.patch("lib_sysconfig.host.is_container")
     @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_update_grub_file(self, render, log, config, check_call):
+    def test_update_grub_file(self, render, log, config, check_call, is_container):
         """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub true.
 
         Expect file is rendered with correct config and updated-grub is called.
         """
+        is_container.return_value = False
         config.return_value = {
             "reservation": "off",
             "isolcpus": "0-10",
@@ -240,15 +242,19 @@ class TestLib:
         )
         check_call.assert_called()
 
+    @mock.patch("lib_sysconfig.host.is_container")
     @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_grub_legacy_reservation(self, render, log, config, check_call):
+    def test_grub_legacy_reservation(
+        self, render, log, config, check_call, is_container
+    ):
         """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub true.
 
         Expect file is rendered with correct config and updated-grub is called.
         """
+        is_container.return_value = False
         config.return_value = {
             "reservation": "isolcpus",
             "cpu-range": "0-10",
@@ -290,15 +296,19 @@ class TestLib:
         )
         check_call.assert_called()
 
+    @mock.patch("lib_sysconfig.host.is_container")
     @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):
+    def test_legacy_grub_config_flags(
+        self, render, log, config, check_call, is_container
+    ):
         """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub true.
 
         Expect file is rendered with correct config and updated-grub is called.
         """
+        is_container.return_value = False
         config.return_value = {
             "reservation": "off",
             "isolcpus": "",
@@ -485,7 +495,7 @@ class TestLib:
     ):
         """Set config kernel=4.15.0-38-generic and running kernel is different.
 
-        Expect apt install is called.
+        Expect apt install is called twice.
         """
         config.return_value = {"kernel-version": "4.15.0-38-generic"}
 
@@ -493,12 +503,10 @@ class TestLib:
         sysh = lib_sysconfig.SysConfigHelper()
         sysh.install_configured_kernel()
 
-        apt_install.assert_called_with(
-            [
-                "linux-image-{}".format("4.15.0-38-generic"),
-                "linux-modules-extra-{}".format("4.15.0-38-generic"),
-            ]
+        apt_install.assert_any_call(
+            "linux-modules-extra-{}".format("4.15.0-38-generic")
         )
+        apt_install.assert_any_call("linux-image-{}".format("4.15.0-38-generic"))
 
     @mock.patch("lib_sysconfig.apt_install")
     @mock.patch("lib_sysconfig.apt_update")
@@ -510,7 +518,7 @@ class TestLib:
     ):
         """Set config kernel=4.15.0-38-generic and running kernel is the same.
 
-        Expect apt install is not called.
+        Expect apt install is called once for linux-modules-extra.
         """
         kernel_version = "4.15.0-38-generic"
         config.return_value = {"kernel-version": kernel_version}
@@ -519,7 +527,9 @@ class TestLib:
         sysh = lib_sysconfig.SysConfigHelper()
         sysh.install_configured_kernel()
 
-        apt_install.assert_not_called()
+        apt_install.assert_called_with(
+            "linux-modules-extra-{}".format("4.15.0-38-generic")
+        )
 
     @mock.patch("lib_sysconfig.apt_install")
     @mock.patch("lib_sysconfig.apt_update")