Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require podman #5285

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Require podman #5285

merged 1 commit into from
Mar 12, 2024

Conversation

cgwalters
Copy link
Contributor

This is prep for supporting bootc:
#5197

A big part of the idea with bootc is that with bootc install, a container image can install itself:
https://github.com/containers/bootc/blob/main/docs/install.md

This will longer term replace the ostreecontainer verb.

However even beyond that, having podman on the (smaller netinst, as well as bigger DVD) installer ISO will just generally be useful for a variety of things. For example,
one can now do e.g.:

%post --no-chroot
podman run ...

Which unblocks a lot of things! (Some of which admittedly will be hacks, but that's what a lot of %post is...)

Having a container runtime in the Live ISO is a very key feature of the Fedora CoreOS (and RHEL CoreOS) Live ISOs, and this brings Anaconda closer to parity too.

@jkonecny12
Copy link
Member

Hi @cgwalters looks good to me. However, don't we want to add the new dependency together with the new code?

Just to be clear, I'm not opposed to merge this if it will improve your life even like this.

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

/kickstart-test --testtype ostree

@jkonecny12
Copy link
Member

I guess this is related change to Accepted Fedora 40 change.

@jkonecny12
Copy link
Member

@KKoukiou do you know why the icon is broken on pixel tests? I don't think it's happening because of these change.

@cgwalters
Copy link
Contributor Author

Hi @cgwalters looks good to me. However, don't we want to add the new dependency together with the new code?

Yep, up to you! Though as I said in the commit message I think it'd likely be very generally useful to be able to run podman as part of the installation environment today; right?

@jkonecny12
Copy link
Member

We did not had a request so far but can't say it will not be :D

@M4rtinK
Copy link
Contributor

M4rtinK commented Nov 9, 2023

Hi @cgwalters looks good to me. However, don't we want to add the new dependency together with the new code?

Yep, up to you! Though as I said in the commit message I think it'd likely be very generally useful to be able to run podman as part of the installation environment today; right?

I'm thinking we might have to bump the advertised RAM requirements a bit or add a disclaimer there as I would guess that running a container could result in more resource usage. Like, of course it was already possible to hit OOM with regular scriptlets if you are crazy enough, but running a container will likely move the baseline.

@jkonecny12
Copy link
Member

Adding podman itself will not run a containers so until we do that (bootc support) we probably don't want to raise the requirements.

@jkonecny12
Copy link
Member

/build-image --boot.iso

Copy link

Images built based on commit 036646c:

  • boot.iso: success

Download the images from the bottom of the job status page.

@jkonecny12
Copy link
Member

jkonecny12 commented Nov 20, 2023

I did some testing and this will make the ISO bigger by 20MB.
From 763MB to 783MB size of the ISO.

@M4rtinK, @KKoukiou, @bcl I wonder if this is the issue or not

@cgwalters
Copy link
Contributor Author

Currently the ostree-container stack depends on skopeo, which is 27M. We've talked in containers/bootc#81 about supporting using podman for this too (it'd be an obvious step) in which case it's likely skopeo could drop out - that'd likely make this a much smaller total size increase.

@jkonecny12
Copy link
Member

@AdamWill I wonder how we should proceed to find out if the size change is an issue. Do you remember what is the process?

@poncovka
Copy link
Contributor

Currently the ostree-container stack depends on skopeo, which is 27M.

@jkonecny12 Btw. our dependency on skopeo is defined in the anaconda-install-img-deps meta package. This pull request adds podman to the anaconda-core package. This means that the impact is going to be broader (not just a boot.iso) unless this is unintentional.

@AdamWill
Copy link
Contributor

I can do a scratch build and run it through openQA to see the size change for an x86_64 netinst image.

@AdamWill
Copy link
Contributor

although, building your own netinst image is pretty easy (much easier than live image) - basically you do setenforce Permissive then lorax -p Fedora -v Rawhide -r Rawhide --repo=/etc/yum.repos.d/fedora-rawhide.repo --repo=/etc/yum.repos.d/happyassassin-side.repo --rootfs-size=3 --squashfs-only ./results , where the second repo file is a repo definition pointing to the repo where you've stashed whatever different package(s) you want to be in the build.

@cgwalters
Copy link
Contributor Author

our dependency on skopeo is defined in the anaconda-install-img-deps meta package. This pull request adds podman to the anaconda-core package. This means that the impact is going to be broader (not just a boot.iso) unless this is unintentional.

AIUI a (the?) difference is that dependencies of anaconda.rpm will also show up on e.g. the Fedora Workstation live ISO? In that case, podman is already there, so I don't think that's a concern right?

@jkonecny12
Copy link
Member

@cgwalters if we do it this way, then it's dependency of anaconda-core package. That means that everywhere you would install Anaconda (or something dragging in Anaconda) would also install podman. In other words, it will impact every deliverable with Anaconda or Inital Setup (for example ARM board deployment).

For Anaconda I think this is correct, however, I'm not that sure about Initial Setup case. That could raise more troubles.

@jkonecny12
Copy link
Member

@AdamWill I already did the comparation by our automation and local build from the latest Anaconda sources:
https://github.com/rhinstaller/anaconda/blob/master/dockerfile/anaconda-iso-creator/lorax-build#L43

However, yeah, maybe we should also try to do this without our modifications to be sure.

@jkonecny12
Copy link
Member

@AdamWill verified in the latest fedora Rawhide container:
Without podman:

[root@e226d6855e3c build]# ls -lh just-rawhide/results/images/boot.iso 
-rw-r--r--. 1 root root 744M Nov 22 08:51 just-rawhide/results/images/boot.iso

With podman:

[root@e226d6855e3c build]# ls -lh with-podman/results/images/boot.iso 
-rw-r--r--. 1 root root 764M Nov 22 09:31 with-podman/results/images/boot.iso

So the size increase is 20 MB as mentioned above. Now I wonder who to ask or where to verify that such a size increase is fine.

Also we might need to move this to anaconda-install-img-deps instead of core because of the Inital Setup use-case.

@poncovka
Copy link
Contributor

@jkonecny12 Maybe I am missing something, but I don't understand the reasoning behind having a hard dependency on podman via anaconda-core. The installer is able to run without it. I think it should be required by one of our meta packages: anaconda-install-env-deps or anaconda-install-img-deps.

@jkonecny12
Copy link
Member

jkonecny12 commented Nov 22, 2023

Yes, I agree it should be part of other package. However, the main question here is if the 20MB of image size is an issue or no, that will not change with the move of the package.

@AdamWill
Copy link
Contributor

Well, there isn't really a simple answer to that, it's a judgment call. 20M is a big chunk, relatively speaking, so the functionality bonus should be quite large to justify it. This will make it very difficult to keep netinst images under the 700M size limit we still have for x86_64 at present (though possibly removing yelp will help counterbalance that for now).

Is there any way we could do a podman-core package which strips it (and its dependencies) down to a really minimal set for what the anaconda use case needs, or anything like that?

@cgwalters
Copy link
Contributor Author

In the long term at least, having podman and the bootc kickstart verb will let us move closer to dropping rpm-ostree out of the set of dependencies.

Is there any way we could do a podman-core package which strips it (and its dependencies) down to a really minimal set for what the anaconda use case needs, or anything like that?

That already happens when Recommends is turned off right?

@jkonecny12
Copy link
Member

@AdamWill I wonder if I should create a FESCO ticket to get the final decision about this?

@cgwalters IIRC lorax is not installing Recommends at all so it should be stripped down already.

@AdamWill
Copy link
Contributor

Hum, maybe. Oh, in your result above - was that with or without the yelp removal PR?

@github-actions github-actions bot added the stale label Feb 13, 2024
@AdamWill
Copy link
Contributor

Oh, right, the size limit increase went through, so that is no longer an immediate concern. Of course, it's still always worthwhile to consider whether the value gained by increasing the image size is worth it.

@AdamWill AdamWill removed the stale label Feb 13, 2024
@AdamWill
Copy link
Contributor

@cgwalters can you rebase this?

@cgwalters
Copy link
Contributor Author

@cgwalters can you rebase this?

Done, though there appeared to be no conflicts?

@AdamWill
Copy link
Contributor

I was just hoping it might make the tests pass. :D But then today was branching, so maybe not the best time...

@poncovka
Copy link
Contributor

/kickstart-test --testtype smoke

@KKoukiou KKoukiou added f41 and removed f40 labels Feb 29, 2024
@rhatdan
Copy link

rhatdan commented Mar 5, 2024

Any movement on this?

@jkonecny12
Copy link
Member

Oh, right, the size limit increase went through, so that is no longer an immediate concern. Of course, it's still always worthwhile to consider whether the value gained by increasing the image size is worth it.

I think it is worth it and it is also inevitable if we want to embrace bootc in the future (what we want to do).

@jkonecny12
Copy link
Member

@cgwalters would it be fine to move this to F41 or you would rather want F40?

@jkonecny12
Copy link
Member

/packit build

@jkonecny12
Copy link
Member

Also @cgwalters could you please move this dependency to install-img-deps sub-package? Or if you want I can do that, feel free to say :)

This is prep for supporting bootc:
rhinstaller#5197

A big part of the idea with bootc is that with `bootc install`,
a container image can *install itself*:
https://github.com/containers/bootc/blob/main/docs/install.md

This will longer term replace the `ostreecontainer` verb.

However even beyond that, having podman on the (smaller netinst,
as well as bigger DVD) installer ISO will just generally be
useful for a variety of things.  For example,
one can now do e.g.:

```
%post --no-chroot
podman run ...
```

Which unblocks a lot of things!  (Some of which admittedly will
be hacks, but that's what a lot of `%post` is...)

Having a container runtime in the Live ISO is a very key
feature of the Fedora CoreOS (and RHEL CoreOS) Live ISOs,
and this brings Anaconda closer to parity too.
@cgwalters
Copy link
Contributor Author

Thanks, done and rebased 🏄

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me. Thanks!

@jkonecny12
Copy link
Member

/build-image --boot.iso

Copy link

Images built based on commit ccb3475:

  • boot.iso: success

Download the images from the bottom of the job status page.

@jkonecny12
Copy link
Member

@cgwalters so I tested this change and I found out that podman is correctly in place. Unfortunately, it is not working when you try to run the container.

$ podman run -it fedora:rawhide
Error: netavark: code: 3, msg: iptables v1.8.10 (legacy): can't initialize iptables table `nat': Table does not exist (do you need to insmod?)
Perhaps iptables or your kernel needs to be upgraded.

I'm inclined to merge this anyway because this seems to me like something we need to resolve in Lorax or special way to execute podman. Do you agree @cgwalters?

@cgwalters
Copy link
Contributor Author

It should work with podman run --net=host. The default podman network stack is not installed without Recommends in place. IMO, the use cases for running podman in anaconda do not strictly need the default podman (and docker) setup of a bridge.

So we could either:

  • Document this (for the bootc use case, as well as generic "run a container in anaconda", it's generally going to be fine to use --net=host)
  • Override the recommends and install the network stack too (probably saner longer term)

@jkonecny12
Copy link
Member

I also wonder if this shouldn't be a fallback? I mean not fail on podman but print warning and fallback to --net=host if this happens?

@jkonecny12
Copy link
Member

Anyway, tested with --net=host and it works correctly.

Merging.

P.S.: I just realized that this is great tool for debugging. Basically it will add dnf environment to the installation image when needed so anything could be installed to the boot.iso for debugging purposes. Sounds like a great improvement!

@jkonecny12 jkonecny12 merged commit 34aa993 into rhinstaller:master Mar 12, 2024
16 checks passed
@cgwalters
Copy link
Contributor Author

I also wonder if this shouldn't be a fallback? I mean not fail on podman but print warning and fallback to --net=host if this happens?

That'd be insecure in the general case. But, it would clearly make sense to have a more useful error message.

P.S.: I just realized that this is great tool for debugging. Basically it will add dnf environment to the installation image when needed so anything could be installed to the boot.iso for debugging purposes.

Right.

Probably the immediately most interesting will be documenting use of

%pre
podman run --net=host ...

And similar for %post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

8 participants