canonical / operator-libs-linux

Linux helper libraries for the operator framework
Apache License 2.0
11 stars 37 forks source link

Add base logic for grub lib #98

Closed rgildein closed 1 year ago

rgildein commented 1 year ago

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

#!/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)
rgildein commented 1 year ago

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"}
rgildein commented 1 year ago

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?