Merge ~dashmage/charm-sysconfig:systemd-reboot-notification into charm-sysconfig:master

[Ashley James]

Check for any actual changes before updating systemd conf file.

Currently when the install hook handler is executed, the systemd conf file rendered by the charm is installed on the machine. This change causes a false positive reboot notification to appear even where there are no changes performed by the charm during initial deployment.

In order to fix this, the systemd conf file is only updated during the install hook and config-changed hook after checking whether there's a logical change to the file. This comparison is done through the use of the configparser module.

[Ashley James (dashmage)]

One problem that pops up while testing these changes is when providing the config options directly during deploy time.

For example, running
```
$ juju deploy ./sysconfig-new.charm sysconfig-new --config cpu-affinity-range="0-5"
$ juju relate ubuntu sysconfig-new
```
will not trigger the reboot notification even though running the config command separately (as shown below) will trigger the notification.

```
$ juju deploy ./sysconfig-new.charm sysconfig-new
$ juju relate ubuntu sysconfig-new
$ juju config sysconfig-new cpu-affinity-range="0-5"
```

This is because the if condition code in the config-changed handler checking whether the config has changed is not executed. This check is done through the `hookenv.config.changed` method documented here [1].

[1]: https://github.com/juju/charm-helpers/blob/77bb1c88e26b12ba7eddcfb6d597db1568a9e22b/charmhelpers/core/hookenv.py#L386

[Ashley James (dashmage)]

```
$ juju deploy ./sysconfig-new.charm sysconfig-new
$ juju config sysconfig-new cpu-affinity-range="0-5"
$ juju relate ubuntu sysconfig-new
```

Relating the subordinate sysconfig to the ubuntu principal *after* setting the config doesn't trigger the reboot notification. Only once the relation is joined and then config changed, would the if condition logic in the config-changed hook be executed and the reboot notification be displayed.

[Andrea Ieri (aieri)]

A few comments:

* setting config options at deploy time or subsequently *must* yield the same behavior, both in terms of messages and on-disk configuration

* the config-changed hook is run right after the install one (see[0]), although whether sysconfig correctly fires the config_changed() function when the config-changed hook is run is a separate issue

* the way I see it, being careful about when we do or we do not run the system.conf update function is very fragile. We should rather make update_systemd_system_file() able to be run at any point, safely, without triggering unnecessary 'reboot required'. Since systemd conf files are INI-like, they can easily be parsed with configparser. What I think you should do is have the charm:
  1. render the template, preferably in memory, or in a tempfile
  2. load the rendered template in a configparser object
  3. load system.conf in another configparser object
  4. compare the two configparser objects; if they are different, mv the tempfile into place and trigger the reboot-required notification

I have done a very quick check with two semantically identical systemd.conf files (only the order of the keys changed) and yes, something like this does work:

```
c_old = configparser.ConfigParser()
c_new = configparser.ConfigParser()
c_old.read('/etc/systemd/system.conf')
c_new.read('/some/temp/file')

c_old == c_new # returns True if the configs are semantically equal
```

[0] https://juju.is/docs/sdk/events#heading--lifecycle-event-triggers

review: Needs Fixing

[Ashley James (dashmage)]

Thanks for your suggestion @aieri.

> We should rather make update_systemd_system_file() able to be run at any point, safely, without triggering unnecessary 'reboot required'. Since systemd conf files are INI-like, they can easily be parsed with configparser.

I think this is a good idea. I've implemented a new method that performs this check through configparser.

The only problem I see is the earlier issue I raised with setting the config during the time of deployment. After some testing, it looks like the config-changed event handler code is not being executed at all when set with "juju deploy --config x=y". So the reboot notification is not raised in this scenario.

[Andrea Ieri (aieri)]

Here is a preliminary quick review. Please see the comments in-line. Additionally, yes, having the charm be able to apply config options during deployment is mandatory. I have no idea why the config_changed function isn't being called. You could run debug hooks on a stable revision and compare to your build.

review: Needs Fixing

[Eric Chen (eric-chen)]

The naming between grub and systemd seems very different. It's really confusing and hard to understand the logic. Can we align the naming and logic?

in src/lib/lib_sysconfig.py

Grub:
- _assemble_grub_context: generate the context
- update_grub_file: generate the temporary grub-file
- check_update_grub: compare final grub-file with the temporary grub-file
- update-grub: generate the final grub file
- check_grub_reboot: check if we need to reboot the server

Sysconfig
- _assemble_systemd_context: generate the context
- systemd_conf_changed (?) : compare final systemd file with the temporary systemd context
- update_systemd_system_file (?): generate the final systemd file
- clear_systemd_notification (?)

review: Needs Fixing

[Ashley James (dashmage)]

> The naming between grub and systemd seems very different. It's really
> confusing and hard to understand the logic. Can we align the naming and logic?
>
> in src/lib/lib_sysconfig.py
>
> Grub:
> - _assemble_grub_context: generate the context
> - update_grub_file: generate the temporary grub-file
> - check_update_grub: compare final grub-file with the temporary grub-file
> - update-grub: generate the final grub file
> - check_grub_reboot: check if we need to reboot the server
>
> Sysconfig
> - _assemble_systemd_context: generate the context
> - systemd_conf_changed (?) : compare final systemd file with the temporary
> systemd context
> - update_systemd_system_file (?): generate the final systemd file
> - clear_systemd_notification (?)

We can't strictly align the naming/logic between grub and systemd. This is because for grub, the charm doesn't update the final config file (/boot/grub/grub.cfg) but only the intermediate one in /etc/grub.d which is used to generate the final config file (using grub-mkconfig). Whereas for systemd, the charm updates the final conf file (/etc/systemd/system.conf) itself. We set a flag in the config-changed handler for systemd since we have to perform this comparison check before the final conf file is updated.

Although I do agree with your point that the systemd method naming could be better to make it more understandable. Here is what I propose:

Systemd
- _assemble_systemd_context: generate the context

- update_systemd_system_file: generate the final systemd file with updated configs

- check_update_systemd : compare system.conf file (before config changes have been updated) with the temp file (where config changes are updated). I've made the name similar to the grub one.

- check_systemd_clear_notification: This method name would make it clearer that we are checking whether the systemd reboot notification should be cleared. This logic is slightly different with grub because of the same reasons mentioned earlier.

I've also added a comment to clarify the if condition check in the update_status handler.

Let me know if these changes look good to you @eric-chen.

[Eric Chen (eric-chen)]

Other than the naming, an inline comment for the logic.

review: Needs Fixing

[🤖 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)]

FAILED: Continuous integration, rev:8404b07fadee319ad0c8ccd63075d8b954eec1b0
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/52/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1748/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/497/

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

review: Needs Fixing (continuous-integration)

[Eric Chen (eric-chen)]

nitpick about the comment, other than that, LGTM

review: Needs Fixing

[Eric Chen (eric-chen)]

One more thing, we should add functional test

1. In a clean server, no reboot notification after simple deployment `juju deploy charm-sysconfig`
2. trigger reboot notificaiton for `juju deploy charm-sysconfig --config x=y`
3. trigger reboot notification after config changed `juju config xxxx`

review: Needs Fixing

[Ashley James (dashmage)]

Updated functional tests. Couple of points to note:

* trigger reboot notification after config changed `juju config xxxx`: This case was already present in the existing functional tests.
* I've modified some of the older code pertaining to the deployment method - changed using "juju deploy" directly to using libjuju to perform the deployment. The reasoning for doing this earlier was that libjuju fails with deployment with JAAS according to https://github.com/juju/python-libjuju/issues/221 which has expired. I'm not really sure if we do any testing on JAAS now because otherwise, it's much better practice to do the deployment with libjuju. In case the JAAS based deployment is still necessary, I can revert that part of my code.

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

PASSED: Continuous integration, rev:5055be3957928577cf6ad3a5b9964f463f925a29
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/53/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1792/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/505/

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

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

LGTM

review: Approve

[Andrea Ieri (aieri)]

Overall lgtm, I have a few optional suggestions and a question about the test.

review: Needs Information

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

FAILED: Continuous integration, rev:bc456f2da21fc21434e09da313efacd444bf9ef4
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/54/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1801/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/514/

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:b0fd90864a6bb83f4d1e2b602c72b0f4fd98e6fe
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/55/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1802/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/515/

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

review: Needs Fixing (continuous-integration)

[Ashley James (dashmage)]

@aieri: Addressed your comments. Only one interesting behaviour about your nitpick with the ConfigParser initialization which I've responded inline.

In short, these 2 code snippets are not functionally the same which I didn't know before either.

```
config = ConfigParser()
config.read('config.txt')
```

is not equivalent to

```
config = ConfigParser().read('config.txt')
```

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

PASSED: Continuous integration, rev:058376cf6f00d13ed5d7a3fa258ff63b3554c51e
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/56/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1803/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/516/

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

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

> @aieri: Addressed your comments. Only one interesting behaviour about your
> nitpick with the ConfigParser initialization which I've responded inline.
>
> In short, these 2 code snippets are not functionally the same which I didn't
> know before either.
>
> ```
> config = ConfigParser()
> config.read('config.txt')
> ```
>
> is not equivalent to
>
> ```
> config = ConfigParser().read('config.txt')
> ```

I'm struggling to see your inline reply... where is it? It's neither attached to commit 5055be3 nor 058376c

[Ashley James (dashmage)]

So I can see my inline responses attached to commit 5055be3. Not quite sure why it's not visible to others. In any case, let me paste those responses on this comment.

> [very nit-picky] since you're not dealing with long lines, you could skip lines 59 and 60 with
```
existing_config = ConfigParser().read(SYSTEMD_SYSTEM)
new_config = ConfigParser().read_string(render_output)
```

I had initially made this change and couldn't figure out why the code wasn't working anymore. Turns out that

```
config = ConfigParser()
config.read('config.txt')
```

is not equivalent to

```
config = ConfigParser().read('config.txt')
```

The first code snippet correctly returns a ConfigParser object whereas the second one (where we directly call the read method of a newly created ConfigParser object) returns a list object with the name of the file (['config.txt']).

Even the python docs (https://docs.python.org/3/library/configparser.html) provide examples where the instance is defined first and then used to call the `read` method.

Similarly for the `read_string` method, if we directly call it like so `ConfigParser().read_string(my_string)`, it returns None.

So we do need to declare the ConfigParser object explicitly before any usage. I'm surprised this caveat isn't mentioned anywhere in the docs.

-----

> there are only two places where you call update_systemd_system_file(),
(...)

This makes a lot of sense and the code is much cleaner. Thanks for the suggestion!

-----

> I am confused... where is the assertion? TBH I'm confused by the old test too, how did it work?
(...)

The old test didn't seem like it tests anything but just gets sysconfig deployed and ensures that the unit has the right status at the end.

My intention with the second sysconfig app was to test for the deployment scenario where we perform "juju deploy xyz --config x=y" and config options were defined at deployment time itself and not after that through an explicit "juju config" command. I added the second sysconfig deployment in the same test in order to have them both deploy parallelly and not have execution be blocked (with the model.block_until call) waiting for one sysconfig app to finish deploying before the other one starts.

I've now added a try-except block checking if the timeout has reached in case the app status is not what's expected.

-----

> nit: do you need to serialize adding the relation and setting the enable-container config? If you don't, you could let both tasks run asynchronously: just create them and gather both at the same time.

Good idea, let me do that.

[Andrea Ieri (aieri)]

TIL! I had run some small tests on my side but did not catch that `config = ConfigParser().read('config.txt')` is not equivalent to instantiating and reading separately.

Regarding the functional tests:
* perhaps the idea was that the block_until would eventually time out if the app doesn't deploy correctly, and thus fail the test. But it's a weird way to write a test, using an explicit assertion would be better
* I understand your desire to start building two apps in parallel to save time, but tests should really have a narrow scope and only test one thing at a time. What you could perhaps do is create the apps as fixtures (potentially in parallel), and run the checks you need in the test code.
* I also have the suspicion that the tests that come after test_sysconfig_deploy would not run without it. This is not good design (all tests should be independent) and makes me really think the deployment work should move to conftest.py

review: Needs Fixing

[Ashley James (dashmage)]

Thanks for the latest round of suggestions!

I've pushed a new commit with changes to move the deployment work over to conftest.py and also enable running each of the tests independently. The deployments happen parallelly by setting the deployment fixtures to "autouse" so they both deploy at the start of the test. Both the deployments have separate tests which check whether the timeout exception has been raised while blocking the model and checking for a particular status.

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

FAILED: Continuous integration, rev:658ea4d68fad5566e4031640888f293ece298517
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/57/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1856/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/530/

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:8f715537bb7c9c9925be6aefb2dc9e725be82fe1
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/58/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1858/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/532/

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

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

thank you, this is a lot cleaner!

review: Approve

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

Change successfully merged at revision ec090fc3fd17648d455b838b57e284b11e28ec9f

diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index 83b5dd6..63ebec9 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -7,6 +7,7 @@ import hashlib
 import os
 import re
 import subprocess
+from configparser import ConfigParser
 from datetime import datetime, timedelta, timezone
 
 import charmhelpers.core.sysctl as sysctl
@@ -469,7 +470,7 @@ class SysConfigHelper:
 
         return valid
 
-    def _assemble_context(self):
+    def _assemble_grub_context(self):
         context = {}
 
         # The isolcpus boot option can be used to isolate one or more CPUs at
@@ -519,14 +520,14 @@ class SysConfigHelper:
 
         Will call update-grub if update-grub config is set to True.
         """
-        context = self._assemble_context()
+        context = self._assemble_grub_context()
         self._render_boot_resource(GRUB_CONF_TMPL, GRUB_CONF, context)
         hookenv.log("grub configuration updated")
         self._update_grub()
 
-    def update_systemd_system_file(self):
-        """Update /etc/systemd/system.conf according to charm configuration."""
+    def _assemble_systemd_context(self):
         context = {}
+
         if self.cpu_affinity_range:
             context["cpu_affinity_range"] = self.cpu_affinity_range
 
@@ -538,9 +539,45 @@ class SysConfigHelper:
             context["systemd_config_flags"] = parse_config_flags(
                 self.config_flags.get("systemd", "")
             )
+        return context
 
-        self._render_boot_resource(SYSTEMD_SYSTEM_TMPL, SYSTEMD_SYSTEM, context)
-        hookenv.log("systemd configuration updated")
+    def _systemd_update_available(self, context):
+        """Compare the systemd system.conf file with the one rendered by the charm.
+
+        This method renders the systemd conf file in memory along with the configured
+        values (if any) and then compares with the existing system.conf file.
+
+        Returns True in case there are changes present between the files.
+        """
+        hookenv.log(
+            "Checking for changes to /etc/systemd/system.conf.",
+            hookenv.DEBUG,
+        )
+
+        new_config = ConfigParser()
+        existing_config = ConfigParser()
+
+        render_output = render(
+            source=SYSTEMD_SYSTEM_TMPL,
+            templates_dir="templates",
+            target=None,
+            context=context,
+        )
+
+        existing_config.read(SYSTEMD_SYSTEM)
+        new_config.read_string(render_output)
+
+        return existing_config != new_config
+
+    def update_systemd_system_file(self):
+        """Update /etc/systemd/system.conf according to charm configuration."""
+        context = self._assemble_systemd_context()
+        # update systemd config only if there's an actual change
+        if self._systemd_update_available(context):
+            self._render_boot_resource(SYSTEMD_SYSTEM_TMPL, SYSTEMD_SYSTEM, context)
+            hookenv.log("systemd configuration updated")
+        else:
+            hookenv.log("no systemd configuration changes, not rendering file")
 
     def update_systemd_resolved(self):
         """Update /etc/systemd/resolved.conf according to charm configuration."""
diff --git a/src/tests/functional/conftest.py b/src/tests/functional/conftest.py
index 672e1c3..317b398 100644
--- a/src/tests/functional/conftest.py
+++ b/src/tests/functional/conftest.py
@@ -17,6 +17,13 @@ import pytest_asyncio
 from juju.controller import Controller
 from juju_tools import JujuTools
 
+charm_location = os.getenv("CHARM_LOCATION", "..").rstrip("/")
+charm_name = os.getenv("CHARM_NAME", "sysconfig")
+series = ["jammy", "focal", "bionic"]
+sources = [("local", "{}/{}.charm".format(charm_location, charm_name))]
+
+PRINCIPAL_APP_NAME = "ubuntu-{}"
+
 
 @pytest_asyncio.fixture(scope="module")
 def event_loop():
@@ -62,6 +69,100 @@ async def model(controller):
             await asyncio.sleep(1)
 
 
+@pytest_asyncio.fixture(scope="module", params=series)
+def series(request):
+    """Return ubuntu version (i.e. xenial) in use in the test."""
+    return request.param
+
+
+@pytest_asyncio.fixture(scope="module", params=sources, ids=[s[0] for s in sources])
+def source(request):
+    """Return source of the charm under test (i.e. local, cs)."""
+    return request.param
+
+
+@pytest_asyncio.fixture(scope="module", autouse=True)
+async def app(model, series, source, request):
+    """Deploy sysconfig app along with a principal ubuntu unit."""
+    channel = "stable"
+    sysconfig_app_name = "sysconfig-{}-{}".format(series, source[0])
+    principal_app_name = PRINCIPAL_APP_NAME.format(series)
+
+    # uncomment if app is already deployed while re-testing on same model
+    # sysconfig_app = model.applications.get(sysconfig_app_name)
+    # if sysconfig_app:
+    #     return sysconfig_app
+
+    await model.deploy(
+        "ubuntu", application_name=principal_app_name, series=series, channel=channel
+    )
+
+    # If series is 'xfail' force install to allow testing against versions not in
+    # metadata.yaml
+    force = True if request.node.get_closest_marker("xfail") else False
+
+    sysconfig_app = await model.deploy(
+        source[1],
+        application_name=sysconfig_app_name,
+        series=series,
+        force=force,
+        num_units=0,
+    )
+    await asyncio.gather(
+        sysconfig_app.add_relation(
+            "juju-info", "{}:juju-info".format(principal_app_name)
+        ),
+        sysconfig_app.set_config({"enable-container": "true"}),
+    )
+
+    return sysconfig_app
+
+
+@pytest_asyncio.fixture(scope="module", autouse=True)
+async def app_with_config(model, series, source):
+    """Deploy sysconfig app + config along with a principal ubuntu unit."""
+    channel = "stable"
+    sysconfig_app_with_config_name = "sysconfig-{}-{}-with-config".format(
+        series, source[0]
+    )
+    principal_app_name = PRINCIPAL_APP_NAME.format(series)
+    principal_app_with_config_name = principal_app_name + "-with-config"
+
+    # uncomment if app is already deployed while re-testing on same model
+    # sysconfig_app_with_config = model.applications.get(sysconfig_app_with_config_name)
+    # if sysconfig_app_with_config:
+    #     return sysconfig_app_with_config
+
+    await model.deploy(
+        "ubuntu",
+        application_name=principal_app_with_config_name,
+        series=series,
+        channel=channel,
+    )
+
+    config = {
+        "isolcpus": "1,2,3,4",
+        "enable-pti": "on",
+        "systemd-config-flags": "LogLevel=warning,DumpCore=no",
+        "governor": "powersave",
+    }
+    sysconfig_app_with_config = await model.deploy(
+        source[1],
+        application_name=sysconfig_app_with_config_name,
+        series=series,
+        num_units=0,
+        config=config,
+    )
+    await asyncio.gather(
+        sysconfig_app_with_config.add_relation(
+            "juju-info", "{}:juju-info".format(principal_app_with_config_name)
+        ),
+        sysconfig_app_with_config.set_config({"enable-container": "true"}),
+    )
+
+    return sysconfig_app_with_config
+
+
 @pytest_asyncio.fixture(scope="module")
 async def jujutools(controller, model):
     """Return JujuTools instance."""
diff --git a/src/tests/functional/test_deploy.py b/src/tests/functional/test_deploy.py
index 01b5f78..46e1dcc 100644
--- a/src/tests/functional/test_deploy.py
+++ b/src/tests/functional/test_deploy.py
@@ -1,25 +1,15 @@
 """Functional tests for sysconfig charm."""
 
 import asyncio
-import os
 import re
-import subprocess
 
 import pytest
-import pytest_asyncio
 import tenacity
 import websockets
 
 # Treat all tests as coroutines
 pytestmark = pytest.mark.asyncio
 
-charm_location = os.getenv("CHARM_LOCATION", "..").rstrip("/")
-charm_name = os.getenv("CHARM_NAME", "sysconfig")
-
-series = ["jammy", "focal", "bionic"]
-
-sources = [("local", "{}/{}.charm".format(charm_location, charm_name))]
-
 TIMEOUT = 600
 MODEL_ACTIVE_TIMEOUT = 10
 GRUB_DEFAULT = "Advanced options for Ubuntu>Ubuntu, with Linux {}"
@@ -30,83 +20,33 @@ RETRY = tenacity.retry(
     stop=tenacity.stop_after_attempt(4),
 )
 
+
 # Uncomment for re-using the current model, useful for debugging functional tests
-# @pytest.fixture(scope='module')
+# @pytest_asyncio.fixture(scope="module")
 # async def model():
 #     from juju.model import Model
 #     model = Model()
-#     await model.connect_current()
+#     await model.connect()
 #     yield model
 #     await model.disconnect()
 
 
-# Custom fixtures
-@pytest_asyncio.fixture(params=series)
-def series(request):
-    """Return ubuntu version (i.e. xenial) in use in the test."""
-    return request.param
-
-
-@pytest_asyncio.fixture(params=sources, ids=[s[0] for s in sources])
-def source(request):
-    """Return source of the charm under test (i.e. local, cs)."""
-    return request.param
-
-
-@pytest_asyncio.fixture
-async def app(model, series, source):
-    """Return application of the charm under test."""
-    app_name = "sysconfig-{}-{}".format(series, source[0])
-    return await model._wait_for_new("application", app_name)
-
-
-# Tests
-
-
-async def test_sysconfig_deploy(model, series, source, request):
-    """Deploys the sysconfig charm as a subordinate of ubuntu."""
-    channel = "stable"
-    sysconfig_app_name = "sysconfig-{}-{}".format(series, source[0])
-    principal_app_name = PRINCIPAL_APP_NAME.format(series)
-
-    ubuntu_app = await model.deploy(
-        "ubuntu", application_name=principal_app_name, series=series, channel=channel
-    )
+async def test_app_deploy(model, app):
+    """Verify if the sysconfig app has been successfully deployed."""
+    try:
+        await model.block_until(lambda: app.status == "active", timeout=TIMEOUT)
+    except asyncio.exceptions.TimeoutError:
+        assert False, "Sysconfig app should have active status."
 
-    await model.block_until(lambda: ubuntu_app.status == "active", timeout=TIMEOUT)
-
-    # Using subprocess b/c libjuju fails with JAAS
-    # https://github.com/juju/python-libjuju/issues/221
-    cmd = [
-        "juju",
-        "deploy",
-        source[1],
-        "-m",
-        model.info.name,
-        "--series",
-        series,
-        sysconfig_app_name,
-    ]
 
-    if request.node.get_closest_marker("xfail"):
-        # If series is 'xfail' force install to allow testing against versions not in
-        # metadata.yaml
-        cmd.append("--force")
-    subprocess.check_call(cmd)
-
-    # This is pretty horrible, but we can't deploy via libjuju
-    while True:
-        try:
-            sysconfig_app = model.applications[sysconfig_app_name]
-            break
-        except KeyError:
-            await asyncio.sleep(5)
-
-    await sysconfig_app.add_relation(
-        "juju-info", "{}:juju-info".format(principal_app_name)
-    )
-    await sysconfig_app.set_config({"enable-container": "true"})
-    await model.block_until(lambda: sysconfig_app.status == "blocked", timeout=TIMEOUT)
+async def test_app_with_config_deploy(model, app_with_config):
+    """Check if status of sysconfig app is "blocked" if deployed along with config."""
+    try:
+        await model.block_until(
+            lambda: app_with_config.status == "blocked", timeout=TIMEOUT
+        )
+    except asyncio.exceptions.TimeoutError:
+        assert False, "Sysconfig app with config should have blocked status."
 
 
 async def test_cpufrequtils_intalled(app, jujutools):
@@ -420,7 +360,7 @@ async def test_set_sysctl(app, model, jujutools, sysctl):
 
 
 async def test_uninstall(app, model, jujutools, series):
-    """Tests unistall the unit removing the subordinate relation."""
+    """Test uninstall of the unit  by removing the subordinate relation."""
     # Apply systemd and cpufrequtils configuration to test that is deleted
     # after removing the relation with ubuntu
     await app.set_config(
diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py
index b2b5354..1fdf204 100644
--- a/src/tests/unit/test_lib.py
+++ b/src/tests/unit/test_lib.py
@@ -4,6 +4,7 @@ import subprocess
 import unittest.mock as mock
 from datetime import datetime, timedelta, timezone
 from tempfile import NamedTemporaryFile
+from unittest.mock import Mock
 
 import lib_sysconfig
 import pytest
@@ -540,7 +541,7 @@ class TestLib:
     @mock.patch("lib_sysconfig.hookenv.log")
     @mock.patch("lib_sysconfig.render")
     def test_update_systemd_system_file(self, render, log, config):
-        """Update /etc/default/grub.d/90-sysconfig.cfg and update-grub false.
+        """Update config and see whether /etc/systemd/system.conf is rendered.
 
         Expect file is rendered with correct config and updated-grub is not called.
         """
@@ -559,6 +560,10 @@ class TestLib:
         }
 
         sysh = lib_sysconfig.SysConfigHelper()
+        update_available = Mock()
+        update_available.return_value = True
+        sysh._systemd_update_available = update_available
+
         sysh.update_systemd_system_file()
         render.assert_called_with(
             source=lib_sysconfig.SYSTEMD_SYSTEM_TMPL,
@@ -566,6 +571,27 @@ class TestLib:
             templates_dir="templates",
             context=expected,
         )
+        log.assert_called_with("systemd configuration updated")
+
+    @mock.patch("lib_sysconfig.hookenv.config")
+    @mock.patch("lib_sysconfig.hookenv.log")
+    @mock.patch("lib_sysconfig.render")
+    def test_update_systemd_system_file_no_change(self, render, log, config):
+        """Test whether /etc/systemd/system.conf is rendered if there are no changes."""
+        config.return_value = {
+            "cpu-affinity-range": "",
+            "reservation": "off",
+            "systemd-config-flags": "",
+        }
+
+        sysh = lib_sysconfig.SysConfigHelper()
+        update_available = Mock()
+        update_available.return_value = False
+        sysh._systemd_update_available = update_available
+
+        sysh.update_systemd_system_file()
+        render.assert_not_called()
+        log.assert_called_with("no systemd configuration changes, not rendering file")
 
     @mock.patch("lib_sysconfig.hookenv.config")
     @mock.patch("lib_sysconfig.hookenv.log")
@@ -591,6 +617,10 @@ class TestLib:
         }
 
         sysh = lib_sysconfig.SysConfigHelper()
+        update_available = Mock()
+        update_available.return_value = True
+        sysh._systemd_update_available = update_available
+
         sysh.update_systemd_system_file()
         render.assert_called_with(
             source=lib_sysconfig.SYSTEMD_SYSTEM_TMPL,
@@ -623,6 +653,10 @@ class TestLib:
         }
 
         sysh = lib_sysconfig.SysConfigHelper()
+        update_available = Mock()
+        update_available.return_value = True
+        sysh._systemd_update_available = update_available
+
         sysh.update_systemd_system_file()
         render.assert_called_with(
             source=lib_sysconfig.SYSTEMD_SYSTEM_TMPL,
@@ -655,6 +689,10 @@ class TestLib:
         }
 
         sysh = lib_sysconfig.SysConfigHelper()
+        update_available = Mock()
+        update_available.return_value = True
+        sysh._systemd_update_available = update_available
+
         sysh.update_systemd_system_file()
         render.assert_called_with(
             source=lib_sysconfig.SYSTEMD_SYSTEM_TMPL,