Fixes for system containers generate RPM

[giuseppe]

Add some tests for the generate RPM feature of system containers and fix some issues that I've found while using these tests.

[giuseppe]

bot, retest this please

[rhatdan]

Great work. LGTM

[giuseppe]

I am adding some more cleanups @TomasTomecek would like to use the same feature for Docker containers, so I am moving the RPM generator in its own file.

[giuseppe]

@TomasTomecek I have added some more patches to move the code to a new file.

It is not the nicest API, but it is a starting point from what I refactored out from syscontainers.py. We can clean it up incrementally with your feedbacks.

The entrypoint is generate_rpm. For the Docker part, you won't probably need the arguments that have a default value (values, installed_files, installed_files_template, rename_files).

Beside that, it makes sense to share the same structure /exports and /exports/hostfs so to keep the possibility of having the same image for Docker and for system containers.

[TomasTomecek]

@giuseppe Thank you!

Beside that, it makes sense to share the same structure /exports and /exports/hostfs so to keep the possibility of having the same image for Docker and for system containers.

Absolutely. I want to reuse as much code as possible.

Unfortunately there is some further refactoring required: if I want to use functionality from mount.py inside rpm_host_install.py, the code halts on import loop:

Traceback (most recent call last):
  File "./atomic", line 33, in <module>
    import Atomic
  File "Atomic/__init__.py", line 3, in <module>
    from .atomic import Atomic
  File "Atomic/atomic.py", line 6, in <module>
    from .syscontainers import SystemContainers
  File "Atomic/syscontainers.py", line 17, in <module>
    from .rpm_host_install import RPMHostInstall, RPM_NAME_PREFIX
  File "Atomic/rpm_host_install.py", line 5, in <module>
    from . import mount
  File "Atomic/mount.py", line 23, in <module>
    from . import Atomic
ImportError: cannot import name Atomic

I'm not sure how to refactor it -- likely syscontainers import from atomic.py needs to be taken out.

[giuseppe]

@TomasTomecek should the Docker code be added somewhere else? I think rpm_host_install.py should not be an user of mount but that we add the should be used only for the part in common between OSTree and Docker.
As I see it, we need to add the --system-package option for Docker (which differently from OSTree, I think it should default to no, as it is expensive to mount the image each time a container is installed and probably only few containers will have /exports, or even better we add a LABEL used both by Docker and system containers so users don't need to be aware of these details), then from the Docker backend we mount the image and once it is mounted the functions from rpm_host_install.py are used.

Do you think it makes sense?

[giuseppe]

@TomasTomecek I pushed a small fixup patch that enables to do this:

diff --git a/Atomic/install.py b/Atomic/install.py
index 855f137..8527b51 100644
--- a/Atomic/install.py
+++ b/Atomic/install.py
@@ -4,6 +4,11 @@ from . import util
 from .util import add_opt
 from .syscontainers import OSTREE_PRESENT
 from Atomic.backendutils import BackendUtils
+import os
+import shutil
+from .mount import DockerMount
+from .rpm_host_install import RPMHostInstall, RPM_NAME_PREFIX
+import tempfile
 
 try:
     from . import Atomic
@@ -121,6 +126,23 @@ class Install(Atomic):
                 return self.syscontainers.install(self.image, self.name)
             be.pull_image(self.args.image, remote_image_obj, debug=self.args.debug)
             img_obj = be.has_image(self.image)
+
+
+        if self.args.system_package in ['build', 'yes']:
+            temp_dir = tempfile.mkdtemp()
+            os.makedirs(os.path.join(temp_dir, "root"))
+            m = DockerMount(os.path.join(temp_dir, "root"))
+            m.mount(self.image)
+            try:
+                origin_name, path = RPMHostInstall.generate_rpm("test", self.image, img_obj.labels, os.path.join(temp_dir, "root/rootfs/exports"), temp_dir)
+                shutil.move(path, origin_name)
+            finally:
+                m.unmount()
+                shutil.rmtree(temp_dir)
+            sys.exit(0)
+
         install_args = img_obj.get_label('INSTALL')
         if not install_args:
             return 0

Of course, this is just a proof of concept and cannot be used as it is (also should not probably stay here in install.py). Just wanted to show what we can do with the new code in common.

[giuseppe]

There are a few fixes that are nice to have as soon as possible and would be nice to have before #952. Is this ready to 🚢?

[TomasTomecek]

@giuseppe Thank you, that diff was very helpful! I opened #955, feel free to take a look once you have time.

[giuseppe]

@TomasTomecek thanks for the feedback. I saw the /etc related fix, that is indeed a good thing to have. Though, let's keep working on the new PR you have opened, and if there are no blockers let's merge this PR. It will also help @ashcrow and me for the oneshot feature in system containers

[rh-atomic-bot]

☔ The latest upstream changes (presumably 1b216a4) made this pull request unmergeable. Please resolve the merge conflicts.

[baude]

looks like we need a rebase and to pass tests still ?

[giuseppe]

I rebased and now I get some weird errors, taking a look...

[giuseppe]

@baude tests are passing again

[baude]

lgtm

[baude]

@rh-atomic-bot r+ a22fda4

[rh-atomic-bot]

⌛ Testing commit a22fda4 with merge 2981a0e...

[rh-atomic-bot]

☀️ Test successful - status-redhatci
Approved by: baude
Pushing 2981a0e to master...