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

[Chi Wai CHAN]

Add an extra check to see if update-grub is required or not. This addresses bug #1895189: "reboot required" juju status message disappears even if update-grub was not run.

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

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

[Robert Gildein (rgildein)]

It seems good to me, but we need to at least add unit tests for these changes.
Also I would prefer at least a basic functional test.

review: Needs Fixing

[Chi Wai CHAN (raychan96)]

Added an action to check if any updates to grub is available and add a functional test for that.

[Robert Gildein (rgildein)]

See comments below.

review: Needs Fixing

[Chi Wai CHAN (raychan96)]

Updated the patch based on your comments. For lines 121-124[0] and lines 133-138[1], they are similar but served for different purpose. [0] tests that if update-grub will be reported as "required" whenever a grub config has changed. [1] tests the if check-update-grub actions can complete without error, the outcome is unimportant, that's why it only assert status == "completed".

[Eric Chen (eric-chen)]

I will write the code as following to reduce the level of if/else code structure. That is a choice, how do you and Robert think?

def main():
    if not config_valid():
        message = "configurtion is invalid"
    elif check_update_grub():
        message = "update-grub required."
    else:
        message = "nothing to be updated."
    hookenv.action_set({"message": message})

There are also some other comment inline

review: Needs Fixing

[Chi Wai CHAN (raychan96)]

I think there's no simple way to check if grub config is valid or not, until you reboot. Though, it does complain about the syntax being invalid. Currently, there are only two options: update-grub can be applied (syntax correct and something is different) or no update-grub can be applied (syntax incorrect *or* syntax is correct and nothing has changed). If you prefer, I can split the "syntax incorrect" and "syntax is correct and nothing has changed".

[Eric Chen (eric-chen)]

> I think there's no simple way to check if grub config is valid or not, until
> you reboot. Though, it does complain about the syntax being invalid.
> Currently, there are only two options: update-grub can be applied (syntax
> correct and something is different) or no update-grub can be applied (syntax
> incorrect *or* syntax is correct and nothing has changed). If you prefer, I
> can split the "syntax incorrect" and "syntax is correct and nothing has
> changed".

Yes, I prefer to separte these case and give clear prompt to user.

[Robert Gildein (rgildein)]

> I will write the code as following to reduce the level of if/else code structure. That is a choice, how do you and Robert think?

I like it, look simple and easy to read.

[Chi Wai CHAN (raychan96)]

Update the patch based on the comments.

Unittests: https://pastebin.ubuntu.com/p/2pk5Q8kC2S/
Functional tests: https://pastebin.ubuntu.com/p/rwnq8G3fxj/

Functional tests: 1. Skip xenial because of operator framework bug. 2. Pin juju < 3.0.0 because libjuju regression.

[Eric Chen (eric-chen)]

much much better this time. It's simple and clean.

My last comment for the unit test. In idea case:

Rule: Make each test case method test Just One Thing
https://pylonsproject.org/community-unit-testing-guidelines.html

The other unit test is written by Alvaro which is a very good example for you.

review: Needs Fixing

[Chi Wai CHAN (raychan96)]

Thanks, I've splitted the unittest.

[Eric Chen (eric-chen)]

review: Approve

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

Merge proposal is approved, but source revision has changed, setting status to needs review.

[Chi Wai CHAN (raychan96)]

lint and black:

https://pastebin.ubuntu.com/p/tTDxYv9Z6x/

Note: pyproject-flake8 is currently broken because of API changes in flake8 >= 5. pyproject-flake8 is working on fixing it. Current work around is to pin flake8 < 5.0.0

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

Change successfully merged at revision 62059a0dd7be66a3f0bebd48712eb41f9d7a8026

diff --git a/src/actions.yaml b/src/actions.yaml
index c496add..1dcaedb 100644
--- a/src/actions.yaml
+++ b/src/actions.yaml
@@ -7,3 +7,6 @@ clear-notification:
 update-grub:
   description: |
     This action will run 'update-grub' to the generate a GRUB configuration file.
+check-update-grub:
+  description: |
+    This action will test if there is any GRUB configuration changes available.
diff --git a/src/actions/check-update-grub b/src/actions/check-update-grub
new file mode 100755
index 0000000..46693aa
--- /dev/null
+++ b/src/actions/check-update-grub
@@ -0,0 +1,12 @@
+#!/usr/local/sbin/charm-env python3
+
+from charmhelpers.core import hookenv
+from lib_sysconfig import check_update_grub
+
+
+def main():
+    _, message = check_update_grub()
+    hookenv.action_set({"message": message})
+
+if __name__ == "__main__":
+    main()
diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index c7b59b2..19931df 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -2,6 +2,7 @@
 
 Manage grub, systemd, coufrequtils and kernel version configuration.
 """
+import filecmp
 import hashlib
 import os
 import subprocess
@@ -67,6 +68,39 @@ def boot_time():
         return boot_time
 
 
+def check_update_grub(tmp_output="/tmp/tmp_grub.cfg"):
+    """Check if an update to /boot/grub/grub.cfg is available."""
+    # Some sensible default values
+    update_available = False
+    message = "No available grub updates found."
+
+    # Check for grub config update.
+    hookenv.log(
+        "Checking if an update to /boot/grub/grub.cfg is available.", hookenv.DEBUG
+    )
+    try:
+        subprocess.check_output(
+            "grub-mkconfig -o {}".format(tmp_output),
+            stderr=subprocess.STDOUT,
+            shell=True,
+        )
+    except subprocess.CalledProcessError as err:
+        update_available = False
+        message = "Unable to check update-grub: {}".format(err)
+    else:
+        if not filecmp.cmp("/boot/grub/grub.cfg", tmp_output):
+            update_available = True
+            message = (
+                "Found available grub updates. You can run "
+                "`juju run-action <sysconfig-unit> update-grub` to update grub."
+            )
+        else:
+            update_available = False
+            message = "No available grub updates found."
+    hookenv.log(message, hookenv.DEBUG)
+    return update_available, message
+
+
 class BootResourceState:
     """A class to track resources changed since last reboot."""
 
diff --git a/src/reactive/sysconfig.py b/src/reactive/sysconfig.py
index 1137304..d1983fd 100644
--- a/src/reactive/sysconfig.py
+++ b/src/reactive/sysconfig.py
@@ -35,6 +35,7 @@ from lib_sysconfig import (
     SYSTEMD_RESOLVED,
     SYSTEMD_SYSTEM,
     SysConfigHelper,
+    check_update_grub,
 )
 
 
@@ -173,12 +174,24 @@ def update_status():
         resources
     )
 
-    if boot_changes:
-        hookenv.status_set(
-            "blocked", "reboot required. Changes in: {}".format(", ".join(boot_changes))
-        )
+    config = hookenv.config()
+    if not config["update-grub"]:
+        grub_update_available, _ = check_update_grub()
     else:
-        hookenv.status_set("active", "ready")
+        # if update-grub is set to true, then no need to check for grub update
+        # since it will be applied automatically.
+        grub_update_available = False
+
+    status = "active"
+    message = "ready"
+    if boot_changes:
+        status = "blocked"
+        message = "reboot required. Changes in: {}".format(", ".join(boot_changes))
+        # if update-grub is set to false and there is an update to grub config,
+        # then add more info to the status message. (addressing #1895189)
+        if grub_update_available:
+            message = "update-grub and " + message
+    hookenv.status_set(status, message)
 
 
 @when("config.changed.enable-container")
diff --git a/src/tests/functional/conftest.py b/src/tests/functional/conftest.py
index aee0555..672e1c3 100644
--- a/src/tests/functional/conftest.py
+++ b/src/tests/functional/conftest.py
@@ -13,7 +13,6 @@ import os
 import subprocess
 import uuid
 
-import pytest
 import pytest_asyncio
 from juju.controller import Controller
 from juju_tools import JujuTools
diff --git a/src/tests/functional/test_deploy.py b/src/tests/functional/test_deploy.py
index bec17ea..aa1f371 100644
--- a/src/tests/functional/test_deploy.py
+++ b/src/tests/functional/test_deploy.py
@@ -127,7 +127,7 @@ async def test_default_config(app, jujutools):
     assert "GRUB_DEFAULT" not in grub_content
     assert "default_hugepagesz" not in grub_content
 
-    sysctl_path = '/etc/sysctl.d/90-charm-sysconfig.conf'
+    sysctl_path = "/etc/sysctl.d/90-charm-sysconfig.conf"
     sysctl_exists = await jujutools.file_exists(sysctl_path, unit)
     assert sysctl_exists
 
@@ -241,8 +241,18 @@ async def test_config_changed(app, model, jujutools):
     irqbalance_content = await jujutools.file_contents(irqbalance_path, unit)
     assert "IRQBALANCE_BANNED_CPUS=3000030000300003" in irqbalance_content
 
-    # test update-status show that reboot is required
-    assert "reboot required." in unit.workload_status_message
+    # test update-status show that update-grub and reboot is required, since
+    # "grub-config-flags" is changed and "update-grub" is set to false by
+    # default.
+    assert "update-grub and reboot required." in unit.workload_status_message
+
+
+async def test_check_update_grub(app):
+    """Tests that check-update-grub action complete."""
+    unit = app.units[0]
+    action = await unit.run_action("check-update-grub")
+    action = await action.wait()
+    assert action.status == "completed"
 
 
 async def test_clear_notification(app):
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index 73b9b3e..93dac0a 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -9,6 +9,42 @@ import lib_sysconfig
 import pytest
 
 
[e-mail]("filecmp.cmp")
[e-mail]("lib_sysconfig.subprocess.check_output")
+def test_check_update_grub_error(check_output, cmp_file):
+    """Test check_update_grub function when error occurs."""
+    tmp_output = "/tmp/tmp_grub.cfg"
+
+    check_output.side_effect = subprocess.CalledProcessError(1, "grub-mkconfig")
+    update_available, message = lib_sysconfig.check_update_grub(tmp_output)
+    assert update_available is False
+    assert "Unable to check update-grub" in message
+
+
[e-mail]("filecmp.cmp")
[e-mail]("lib_sysconfig.subprocess.check_output")
+def test_check_update_grub_available(check_output, cmp_file):
+    """Test check_update_grub function when grub update available."""
+    tmp_output = "/tmp/tmp_grub.cfg"
+
+    cmp_file.return_value = False
+    update_available, message = lib_sysconfig.check_update_grub(tmp_output)
+    assert update_available is True
+    assert "Found available grub updates." in message
+
+
[e-mail]("filecmp.cmp")
[e-mail]("lib_sysconfig.subprocess.check_output")
+def test_check_update_grub_unavailable(check_output, cmp_file):
+    """Test check_update_grub function when grub update unavailable."""
+    tmp_output = "/tmp/tmp_grub.cfg"
+
+    cmp_file.return_value = True
+    update_available, message = lib_sysconfig.check_update_grub(tmp_output)
+    assert update_available is False
+    assert "No available grub updates found." in message
+
+
 class TestBootResourceState:
     """Test BootResourceState class."""