Add base logic for grub lib #98

rgildein · 2023-07-07T13:15:41Z

add load function for grub config (assumption no duplicate keys)
add save function for grub config
add is_container function to determine if running machine is container
add GrubConfig object, which load config in lazy mode and provide function to make update (validate, merge and apply changes)
- apply function with usage grub-mkconfig and update-grub
- update function to wrap all functionality to update grub config
- remove function if the charm is removed to remove charm configuration
add docstring with example

I tested this with simple ubuntu charm, where I tried to configure GRUB_CMDLINE_LINUX_DEFAULT. What I tested:

install method with ValidationError (by manually changing /etc/default/grub.d/95-juju-charm.cfg
update-status, where I check if current value is what charm expected
remove method, when I removed the charm

#!/usr/bin/env python3

import logging
from pathlib import Path
from subprocess import check_call, check_output
import charms.operator_libs_linux.v0.grub as grub

from ops.charm import CharmBase
from ops.main import main
from ops.model import ActiveStatus, BlockedStatus


log = logging.getLogger(__name__)


class UbuntuCharm(CharmBase):
    def __init__(self, *args):
        super().__init__(*args)
        ...
        ### testing of grub library
        self.framework.observe(self.on.install, self._on_install)
        self.framework.observe(self.on.update_status, self._on_update_status)
        self.framework.observe(self.on.remove, self._on_remove)
        self.grub = grub.GrubConfig(self.meta.name)
        log.info("found keys %s in grub config file", self.grub.keys())

   ...

    def _on_install(self, _):
        """Test configuring grub during install hook."""
        try:
            self.grub.update({"GRUB_CMDLINE_LINUX_DEFAULT": '"$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"'})
        except grub.ValidationError as error:
            self.unit.status = BlockedStatus(f"[{error.key}] {error.message}")

    def _on_update_status(self, _):
        """Test checking grub configuration during update-status hook."""
        if self.grub["GRUB_CMDLINE_LINUX_DEFAULT"] != '"$GRUB_CMDLINE_LINUX_DEFAULT hugepagesz=1G"':
            self.unit.status = BlockedStatus("wrong grub configuration")

    def _on_remove(self, _):
        """Test cleaning grub configuration during remove hook."""
        self.grub.remove()


if __name__ == "__main__":
    main(UbuntuCharm)

- add load function for grub config (assumption no duplicate keys) - add save function for grub config - add GrubConfig object, which load config in lazy mode and provide function to make update (validate, merge and apply changes)

- add update function - add remove function

dashmage · 2023-07-10T07:48:56Z

lib/charms/operator_libs_linux/v0/grub.py


-Complete documentation about creating and documenting libraries can be found
-in the SDK docs at https://juju.is/docs/sdk/libraries.
+NOTE: This library is not capable to read grub config file containes


Is something missing at the end of the note?

NOTE: This library is not capable of reading grub config files containing...

This was part of not finished docstring. I update it, so you can check it now. Thanks

dashmage · 2023-07-10T07:50:43Z

lib/charms/operator_libs_linux/v0/grub.py

+def _parse_config(data: str) -> Dict[str, str]:
+    """Parse config file lines.
+
+    This function is capable to update single key value if it's used with $ symbol.


Second line of docstring might sound better like this:

This function is capable of updating single key values using the "$" symbol

Can you check it again, I add full docstring with example.

dashmage · 2023-07-10T07:51:21Z

lib/charms/operator_libs_linux/v0/grub.py

+            raise ApplyError("New config check failed.") from error
+
+    def _save_grub_configuration(self) -> None:
+        """Save current gru configuration."""


typo: gru should be grub

esunar

We should add:

Note that this only handles simple configurations that starts with GRUB_
add GRUB_.* check for configuration keys

benhoyt

Looks like good code. Added some comments to consider.

benhoyt · 2023-07-11T01:40:10Z

lib/charms/operator_libs_linux/v0/grub.py

-This is a placeholder docstring for this charm library. Docstrings are
-presented on Charmhub and updated whenever you push a new version of the
-library.
+"""Simple library for managing Linux kernel configuration via grub.


Nit: it looks like the official spelling of the project is "GRUB". See https://www.gnu.org/software/grub/ -- so we should probably use that in docstrings where applicable.

You mean that I should use GRUB instead of grub?

Yeah, GRUB, but only when we're referring to the project in prose (per that doc, it's an acronym for GRand Unified Bootloader). But of course the module name will still be grub. And I guess a class name like GrubConfig would be GRUBConfig, like HTTPServer.

benhoyt · 2023-07-11T01:40:52Z

lib/charms/operator_libs_linux/v0/grub.py

@@ -29,4 +37,259 @@
 # to 0 if you are raising the major API version
 LIBPATCH = 1

-# TODO: add your code here! Happy coding!
+LIB_CONFIG_DIRECTORY = Path("/var/lib/charm-grub/")
+GRUB_CONFIG = Path("/etc/default/grub.d/") / "95-juju-charm.cfg"


Nit: Why use the / operator here instead of just specifying the full path inside the Path() call?

I was using directory / path, than I removed the directory and now it's back as GRUB_DIRECTORY.

benhoyt · 2023-07-11T01:49:42Z

lib/charms/operator_libs_linux/v0/grub.py

+
+    def __init__(self, key: str, message: str) -> None:
+        self._key = key
+        self._message = message


Normally you'd simplify and remove the property boilerplate just by having these as self.key = key and so on. It doesn't really matter that they're writeable.

Also, you might want to customize the __str__ so it's a bit more readable, maybe just def __str__(self): return self.message?

Thanks that is good point, I'll do it.

benhoyt · 2023-07-11T01:58:21Z

lib/charms/operator_libs_linux/v0/grub.py

+        if key in config:
+            logger.warning("key %s is duplicated in config", key)
+
+        config[key] = value


It seems like we're not handling quoting/escaping here. Won't that break things? Perhaps we should use shlex?

benhoyt · 2023-07-11T01:58:41Z

lib/charms/operator_libs_linux/v0/grub.py

+    if path.exists():
+        logger.debug("grub config %s already exist and it will overwritten", path)
+
+    context = [f"{key}={value}" for key, value in config.items()]


Similar here. Maybe we need to use shlex.quote?

benhoyt · 2023-07-11T01:59:34Z

lib/charms/operator_libs_linux/v0/grub.py

+
+    with open(path, "r", encoding="UTF-8") as file:
+        data = file.read()
+        config = _parse_config(data)


Seeing we've got the file object anyway, why not avoid reading the whole thing into memory, and use the fact that for line in file iterates over lines? (make _parse_config take a file / iterable-of-lines). I realize GRUB configs shouldn't be large, but it's good practice and no harder that way. Similar for writing it below.

Good point I will do it. It happens if you start with _parse_config function and than add _load_config 🔢

benhoyt · 2023-07-11T02:10:35Z

lib/charms/operator_libs_linux/v0/grub.py

+    def charm_name(self) -> str:
+        """Get charm name or use value obtained from JUJU_UNIT_NAME env."""
+        if self._charm_name is None:
+            self._charm_name, *_ = os.getenv("JUJU_UNIT_NAME", "unknown").split("/")


To me it seems odd that this is so closely tied to the charm name, particularly the use of environment variables. What's the need for having the charm name in this class? And if needed, I think it'd be preferable to require it to be passed in, rather than parsing environment variables in a library.

To be able to store only charm config file at /etc/default/grub.d/90-juju-<charm-name>. Using the env variable is only there so the charm developer does not have to think about it, we do not want to use ops here. Do you think that it's not needed? I can remove it.

Yeah, I would prefer charm libs don't read environment variables directly, but that the charm name is passed in explicitly to the initialiser.

Ok, I'm going to remove it.

benhoyt · 2023-07-11T02:12:15Z

lib/charms/operator_libs_linux/v0/grub.py

+    This object will load current configuration option for grub and provide option
+    to update it with simple validation.
+
+    There is only one public function `update`, which should handle everything. It will


There's also remove, by the looks. Might be better not to mention the public methods here to avoid this getting out of date. Perhaps define the public methods at the top (after __init__) so they're more discoverable?

You are ring, I forgot that I add remove and apply. I'll update docstring.
I'm used to define all public on bottom (my order magic objects (init, eq, ...), private properties, private functions, public properties and public method), but I'm ok to move it to top if that required.

benhoyt · 2023-07-11T02:14:31Z

lib/charms/operator_libs_linux/v0/grub.py

+        """Initialize the grub config."""
+        self._charm_name = charm_name
+
+    def __contains__(self, key: str) -> bool:


Would it be simpler to just have GrubConfig subclass dict directly?

That said, I often find these "smart objects" cause more confusion than they bring value. It's kind of a dict, but not quite. Worth considering a dumber/simpler API that deals in plain dicts, perhaps top-level functions like read_config() -> dict and update_config(dict)?

It's not subclass of dict, only due to usage of lazy loading. If you check example in lib docstring on in this PR.
Basically the idea is to define self.grub = GrubConfig() in charm __init__, so it can be access in any function. Some charm may want to use apply in action instead of install, some maybe will do some configuration via config_changed.

benhoyt · 2023-07-11T02:26:24Z

lib/charms/operator_libs_linux/v0/grub.py

+    return False
+
+
+class GrubConfig(Mapping[str, str]):


I think we should avoid repeating the name of the module/lib in the class name. Especially for short import names, it's usually better to encourage module.Name use. So this would be just Config, and code would use it as import grub ... grub.Config(). See also https://benhoyt.com/writings/python-api-file-structure/

- implement the is_container funcion - add docstring - test the lib with custom charm

rgildein · 2023-07-11T12:09:30Z

We should add:

1. Note that this only handles simple configurations that starts with GRUB_

2. add GRUB_.* check for configuration keys

I do not think that this is necessary, since this lib will be used by charm developer and not end user. If charm developer want, he/she can open the config file and do anything, that's why I do not see any benefits to add such validation.

Also without such validation we can do

{"TEST" : "1", "GRUB_A": "$TEST", "GRUB_B": "$TEST"}

benhoyt · 2023-07-12T01:08:00Z

lib/charms/operator_libs_linux/v0/grub.py

+#   info -f grub -n 'Simple configuration'
+"""
+
+# TODO: use shlex


IMO this should block merge, otherwise it'll be very easy to get config files with invalid format.

You are right, I forgot to implement and I add only TODO for it. Thanks

benhoyt · 2023-07-12T01:09:18Z

lib/charms/operator_libs_linux/v0/grub.py

+    def __init__(self, key: str, message: str) -> None:
+        self.key = key
+        self.message = message
+        super().__init__(message)


Normally you'd call super first (eg: in case it sets .message and you want to overwrite it). Doesn't matter in this specific case, but it's not good practice.

benhoyt · 2023-07-12T01:11:15Z

lib/charms/operator_libs_linux/v0/grub.py

+
+
+def check_update_grub() -> bool:
+    """Check if an update to /boot/grub/grub.cfg is available."""


For boolean-returning functions, I like the word "Report whether an update..." as that indicates what the return value is. Similar for is_container below, "Report whether the local machine is a container."""

benhoyt · 2023-07-12T01:12:20Z

lib/charms/operator_libs_linux/v0/grub.py

+
+
+class Config(Mapping[str, str]):
+    """Grub config object.


Suggested change

"""Grub config object.

"""Manages GRUB configuration.

benhoyt · 2023-07-17T05:31:10Z

lib/charms/operator_libs_linux/v0/grub.py


-Markdown is supported, following the CommonMark specification.
+```python


Should this have an ending ``` before the end of the docstring?

benhoyt · 2023-07-17T05:34:09Z

lib/charms/operator_libs_linux/v0/grub.py

+        if key in config:
+            logger.warning("key %s is duplicated in config", key)
+
+        config[key] = shlex.quote(value)


Should _parse_config be unquoting the value? Perhaps using shlex.split?

benhoyt · 2023-07-17T05:37:50Z

tests/unit/test_grub.py

+            "key %s is duplicated in config", "GRUB_CMDLINE_LINUX_DEFAULT"
+        )
+        self.assertEqual(
+            result["GRUB_CMDLINE_LINUX_DEFAULT"], "'\"$GRUB_CMDLINE_LINUX_DEFAULT pti=on\"'"


Yeah, per the comment above about quoting -- it's double-quoting things, when it should be unquoting the value. I would expect this:

self.assertEqual( result["GRUB_CMDLINE_LINUX_DEFAULT"], "$GRUB_CMDLINE_LINUX_DEFAULT pti=on", )

You are right, I did not though about it at all. Thanks for pointing this out.

tests/unit/test_grub.py

benhoyt

Left a couple of minor comments, but it looks reasonably good to me. I'm not an expert in GRUB, so might pay to get someone to review that is, but maybe one of the other reviewers that's already looked at it has done that?

lib/charms/operator_libs_linux/v0/grub.py

benhoyt

LGTM (one nit comment). Do you think you're ready to merge this?

benhoyt · 2023-07-21T00:15:02Z

tests/unit/test_grub.py

+
+class TestGrubUtils(BaseTestGrubLib):
+    def test_split_config_line(self):
+        """Tets splitting single line."""


Nit: Tets -> Test (there's also similar instance below).

rgildein · 2023-07-24T12:51:52Z

LGTM (one nit comment). Do you think you're ready to merge this?

I think it's ready. Is there anyone else who should do review?

rgildein added 2 commits July 7, 2023 15:12

Add load and save logic for grub lirary

bfe9d5f

- add load function for grub config (assumption no duplicate keys) - add save function for grub config - add GrubConfig object, which load config in lazy mode and provide function to make update (validate, merge and apply changes)

apply suggestion + add update and remove

b15fcad

- add update function - add remove function

rgildein force-pushed the grub/config branch from 2fea075 to b15fcad Compare July 9, 2023 22:19

dashmage reviewed Jul 10, 2023

View reviewed changes

esunar suggested changes Jul 10, 2023

View reviewed changes

benhoyt changed the title ~~Add load and save logic for grub lirary~~ Add load and save logic for grub library Jul 10, 2023

benhoyt reviewed Jul 11, 2023

View reviewed changes

rgildein changed the title ~~Add load and save logic for grub library~~ Add base logic for grub lib Jul 11, 2023

rgildein marked this pull request as ready for review July 11, 2023 12:02

Finished the grub lib

4a50221

- implement the is_container funcion - add docstring - test the lib with custom charm

rgildein force-pushed the grub/config branch from e81bbbf to 4a50221 Compare July 11, 2023 12:04

apply suggestions, add update logic for overriding

b6d7b26

benhoyt reviewed Jul 12, 2023

View reviewed changes

rgildein added 2 commits July 12, 2023 09:51

fix logic for set_value and add more tests

adf94c3

implement shlex + rephrasing

1e1e4ac

rgildein requested review from benhoyt and esunar July 12, 2023 08:29

zmraul mentioned this pull request Jul 14, 2023

Sysctl library #99

Merged

improve logging + small fix in unittests

9bcc910

rgildein force-pushed the grub/config branch from 765f5ef to 9bcc910 Compare July 16, 2023 22:09

benhoyt requested changes Jul 17, 2023

View reviewed changes

apply suggestions

2520e22

rgildein requested a review from benhoyt July 17, 2023 16:05

benhoyt approved these changes Jul 18, 2023

View reviewed changes

lib/charms/operator_libs_linux/v0/grub.py Outdated Show resolved Hide resolved

lib/charms/operator_libs_linux/v0/grub.py Outdated Show resolved Hide resolved

improve spliting line of GRUB config

d8242de

rgildein requested a review from benhoyt July 19, 2023 07:45

benhoyt approved these changes Jul 21, 2023

View reviewed changes

fix typos in doc-string

a90404c

rgildein mentioned this pull request Jul 24, 2023

Add functional test for grub lib #100

Open

benhoyt merged commit b8efcff into canonical:main Jul 24, 2023
4 checks passed



		def check_update_grub() -> bool:
		"""Check if an update to /boot/grub/grub.cfg is available."""


		Markdown is supported, following the CommonMark specification.
		```python

Add base logic for grub lib #98

Add base logic for grub lib #98

Conversation

rgildein commented Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esunar left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgildein commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgildein commented Jul 24, 2023

rgildein commented Jul 7, 2023 •

edited

Loading

benhoyt Jul 11, 2023 •

edited

Loading

benhoyt Jul 11, 2023 •

edited

Loading