Add the bootc method for installing packages

[ukulekek]

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• adjust plugin docstring
• modify the json schema
• mention the version
• include a release note
  • When tmt works with the image mode images, it now uses native package installation method instead of using rpm-ostree: tmt creates a Containerfile based on the booted image, adds needed packages there, builds the image and reboot a system to use the fresh image with needed packages.

[ukulekek]

Hi, @happz. I wanted to ask what the relationship is between Install* classes and corresponding package managers? Respectively, what logic should I move to the package manager out of the InstallBootc class?

[happz]

Hi, @happz. I wanted to ask what the relationship is between Install* classes and corresponding package managers? Respectively, what logic should I move to the package manager out of the InstallBootc class?

Ideally, all of it. Package managers govern how packages are installed, providing the primitives. Constructing commands in install_from_repository would lead to duplication.

The issue is, they do run their commands immediately. How about we add a middleman, an "adapter" class, which would take command from a package manager, and do something with it? The default one, the current behavior, would be to take that command and run it on the guest, but your InstallBootc can have its own custom adapter which would just collect the commands rather than running them.

TL;DR: we want those commands, we just don't want them to run on the guest now. We need to tell package managers not to run them, instead we need to collect them and add them to the containerfile. Eventually, your InstallBootc should be pretty much the same as any other installation plugin, i.e. its interaction with a package manager in install_from_repository shouldn't be much more beyond the following:

        self.guest.package_manager.install(
            *self.list_installables("package", *self.packages),
            options=Options(
                excluded_packages=self.exclude,
                skip_missing=self.skip_missing,
            ),
        )

And the output would not be CommandOutput, but list[ShellScript], and you would take them and put them into containerfile.

I was thinking about something like this but I didn't have a chance to do it properly (or everywhere), and there are some typing issues that need to be sorted out, plus actions that run more than one command (if there are any, and there might be, in apt or apk). Also, the adapter may need more than one type variable, the one represents output of "perform the command", but there is also check_presence returning a mapping of present packages - instead, we want it to return the command, so that's one to parameterize as well. Let me know what you think, and I can give it a few runs over the weekend.

diff --git a/tmt/package_managers/__init__.py b/tmt/package_managers/__init__.py
index 829e33aa..f17ca6d8 100644
--- a/tmt/package_managers/__init__.py
+++ b/tmt/package_managers/__init__.py
@@ -1,13 +1,13 @@
 import shlex
 from collections.abc import Iterator
-from typing import TYPE_CHECKING, Any, Callable, Optional, Union
+from typing import TYPE_CHECKING, Any, Callable, Optional, Union, TypeVar, Generic

 import tmt
 import tmt.log
 import tmt.plugins
 import tmt.utils
 from tmt.container import container, simple_field
-from tmt.utils import Command, CommandOutput, Path
+from tmt.utils import Command, CommandOutput, Path, ShellScript

 if TYPE_CHECKING:
     from tmt._compat.typing import TypeAlias
@@ -16,6 +16,8 @@ if TYPE_CHECKING:
     #: A type of package manager names.
     GuestPackageManager: TypeAlias = str

+T = TypeVar('T')
+

 #
 # Installable objects
@@ -148,6 +150,29 @@ class Options:
     allow_untrusted: bool = False


+class CommandAdapter(Generic[T]):
+    def on_command(self, script: ShellScript) -> T:
+        raise NotImplementedError
+
+
+class RunImmediatelyAdapter(CommandAdapter[CommandOutput]):
+    def __init__(self, guest: 'Guest', logger: tmt.log.Logger) -> None:
+        self.logger = logger
+        self.guest = guest
+
+    def on_command(self, script):
+        return self.guest.execute(script)
+
+
+class CollectAdapter(CommandAdapter[ShellScript]):
+    def __init__(self, guest: 'Guest', logger: tmt.log.Logger) -> None:
+        self.logger = logger
+        self.guest = guest
+
+    def on_command(self, script):
+        return script
+
+
 class PackageManager(tmt.utils.Common):
     """
     A base class for package manager plugins
@@ -169,12 +194,16 @@ class PackageManager(tmt.utils.Common):
     command: Command
     options: Command

-    def __init__(self, *, guest: 'Guest', logger: tmt.log.Logger) -> None:
+    adapter: CommandAdapter[T]
+
+    def __init__(self, *, guest: 'Guest', adapter: CommandAdapter[T], logger: tmt.log.Logger) -> None:
         super().__init__(logger=logger)

         self.guest = guest
         self.command, self.options = self.prepare_command()

+        self.adapter = adapter or RunImmediatelyAdapter(guest, logger)
+
     def prepare_command(self) -> tuple[Command, Command]:
         """
         Prepare installation command and subcommand options
@@ -193,22 +222,22 @@ class PackageManager(tmt.utils.Common):
         self,
         *installables: Installable,
         options: Optional[Options] = None,
-    ) -> CommandOutput:
+    ) -> T:
         raise NotImplementedError

     def reinstall(
         self,
         *installables: Installable,
         options: Optional[Options] = None,
-    ) -> CommandOutput:
+    ) -> T:
         raise NotImplementedError

     def install_debuginfo(
         self,
         *installables: Installable,
         options: Optional[Options] = None,
-    ) -> CommandOutput:
+    ) -> T:
         raise NotImplementedError

-    def refresh_metadata(self) -> CommandOutput:
+    def refresh_metadata(self) -> T:
         raise NotImplementedError
diff --git a/tmt/package_managers/dnf.py b/tmt/package_managers/dnf.py
index cb7612f2..bc113985 100644
--- a/tmt/package_managers/dnf.py
+++ b/tmt/package_managers/dnf.py
@@ -160,14 +160,14 @@ class Dnf(tmt.package_managers.PackageManager):
             f'{self.command.to_script()} makecache {self.options.to_script()} --refresh'
         )

-        return self.guest.execute(script)
+        return self.adapter.on_command(script)

     def install(
         self,
         *installables: Installable,
         options: Optional[Options] = None,
     ) -> CommandOutput:
-        return self.guest.execute(self._construct_install_script(*installables, options=options))
+        return self.adapter.on_command(self._construct_install_script(*installables, options=options))

     def reinstall(
         self,
diff --git a/tmt/steps/provision/__init__.py b/tmt/steps/provision/__init__.py
index 6bc6e60a..692335f6 100644
--- a/tmt/steps/provision/__init__.py
+++ b/tmt/steps/provision/__init__.py
@@ -1095,7 +1095,7 @@ class Guest(tmt.utils.Common):
             )

         return tmt.package_managers.find_package_manager(self.facts.package_manager)(
-            guest=self, logger=self._logger
+            guest=self, logger=self._logger, adapter=RunImmediatelyAdapter(guest, logger)
         )

     @functools.cached_property

[ukulekek]

@happz, the idea with adapters sounds great to me as I was struggling to come up with a solution on how not to execute commands immediately. I'll try to move all logic to the package manager next week, thank you!

[happz]

@happz, the idea with adapters sounds great to me as I was struggling to come up with a solution on how not to execute commands immediately.

Sounds good, but it didn't work. I'm not smart enough to appease all type-related issues I was hitting. So, I took different approach: #3587 - not tested, just locally...

Package manager we have are now split into two parts: "engine" which does what PM does today, prepares commands to install stuff, but it does not run anything - instead, it returns these commands. The rest of code then works with "package manager" part, that is just a "call the engine and run what it gives you" frontend for its engine. tmt still uses this interface, calls guest.package_manager.install(...) which turns out to be self.guest.execute(self.engine.install(...)).

And for your use case, I imagined your code could take the engine itself, e.g. tmt.package_manager.dnf.DnfEngine, and then use its methods, e.g. install(...). It has the very same input as package manager's install, but it returns a shell script instead of running it. I imagine you would use the engine and its methods in the same way Install* does, except you would take their return values - scripts like dnf -y install foo bar baz - and inject them into Containerfiles you are building.

Something like this:

    # Let's say we collect instructions for the final Containerfile in self._containerfile_directives, and that we have self.engine which is the DnfEngine

    def install_from_repository(self) -> None:
        script = self.engine.install(
            *self.list_installables("package", *self.packages),
            options=Options(
                excluded_packages=self.exclude,
                skip_missing=self.skip_missing,
            ),
        )

        self._containerfile_directives.append(f'RUN {script}')

Feel free to play with it, I didn't experiment with your patch yet.

Also, all those \ns throughout the code are very hard to keep up with. Check out what install() and _install() do, they call sequence of methods of the selected installer - if I would working on this, I would take advantage of this: have a list of strings to collect Containerfile commands, populate it with some FROM ... and ENV ... directives as necessary, and then methods like install_local() or install_debuginfo() would talk to the engine (can it always be dnf? or do we need to detect it somehow?), and slap RUN ... before all scripts returned by the engine. Eventually, you'd have a list of strings, it should be enough to do some containerfile = '\n'.join(self._containerfile_directives), save it into a file and let podman build do its job.

Plus, you will not need an extra flag for tracking whether there are any commands to include - if self._containerfile_directives has just one item, it's your initial FROM ... and there is nothing else to do.

Let me know about your progress or any idea you get about the solution.

[thrix]

I was able to run systemd tests:

   .... (snip) ....
   prepare
        queued push task #1: push to default-0
        
        push task #1: push to default-0
    
        queued prepare task #1: essential-requires on default-0
        queued prepare task #2: requires on default-0
        queued prepare task #3: recommends on default-0
        
        prepare task #1: essential-requires on default-0
        how: install
        summary: Install essential required packages
        name: essential-requires
        where: default-0
        package: audit, policycoreutils, beakerlib and 1 more
        package: building container image with dependencies
        package: switching to new image localhost/tmt/bootc/6dde5b01-8c03-424f-897d-07f8a616ca76
        package: rebooting to apply new image
        
        prepare task #2: requires on default-0
        how: install
        summary: Install required packages
        name: requires
        where: default-0
        package: policycoreutils, rpm, netlabel_tools and 6 more
        package: building container image with dependencies
        package: switching to new image localhost/tmt/bootc/184033dd-ef2b-451d-9261-787d8c0ed555
        package: rebooting to apply new image
        
        prepare task #3: recommends on default-0
        how: install
        summary: Install recommended packages
        name: recommends
        where: default-0
        package: libvirt-client, glibc-langpack-sk, cups and 53 more
        package: building container image with dependencies
        package: switching to new image localhost/tmt/bootc/c6dd803b-9a27-4b5e-9d81-6f1b340dcc5c
        package: rebooting to apply new image
    
        queued pull task #1: pull from default-0
        
        pull task #1: pull from default-0
    
        summary: 3 preparations applied
    execute
        queued execute task #1: default-0 on default-0
        
        execute task #1: default-0 on default-0
        how: tmt
        progress: /Regression/leaking-scope-units [15/59]                                    
    .... (snip) ....

[thrix]

LGTM

[thrix]

@happz pls do a last check on the code, otherwise it has review

[thrix]

all tests passed, except provision which we saw already passing, but openstack today does not play well, will be doing a final run on #3693