-
Notifications
You must be signed in to change notification settings - Fork 359
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
Master enable ostree containers #4561
Master enable ostree containers #4561
Conversation
def containers_needs_network(url): | ||
"""Detect if the container registry URL needs network. | ||
|
||
:return: True or False | ||
""" | ||
if not url: | ||
return False | ||
# Only docker:// and registry:// (could be omitted) should be used for remote installations | ||
# however, you can use 'localhost' in these situations to connect local resources. | ||
if url.startswith("docker://localhost") or \ | ||
url.startswith("registry://localhost") or \ | ||
url.startswith("localhost"): | ||
return False | ||
|
||
# Not connecting localhost (above check) anything else in docker:// or registry:// should | ||
# require network. | ||
if url.startswith("docker://") or \ | ||
url.startswith("registry://") or \ | ||
"://" not in url: | ||
return True | ||
|
||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgwalters What do you think about this solution for network detection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, seems fine to start. It'd be better probably to have this be a shared library function somewhere, we could even do something like ostree container url-requires-network <imgref>
or perhaps skopeo image-requires-network <imgref>
.
But I'm not really concerned about this to start, I think the 99% cases are:
- netinst ISO pulling from a remote registry
- "Full" ISO pulling from embedded content i.e.
oci:/path/to/embedded/image
as is done for desktop installers today, and is also how the Fedora CoreOS and derivative Live ISO works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea about the shared version. Please ping us in case it's implemented.
I think you are correct but I rather play safe in Anaconda about these.
1bfbeae
to
6047d4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks so much for working on this!
def containers_needs_network(url): | ||
"""Detect if the container registry URL needs network. | ||
|
||
:return: True or False | ||
""" | ||
if not url: | ||
return False | ||
# Only docker:// and registry:// (could be omitted) should be used for remote installations | ||
# however, you can use 'localhost' in these situations to connect local resources. | ||
if url.startswith("docker://localhost") or \ | ||
url.startswith("registry://localhost") or \ | ||
url.startswith("localhost"): | ||
return False | ||
|
||
# Not connecting localhost (above check) anything else in docker:// or registry:// should | ||
# require network. | ||
if url.startswith("docker://") or \ | ||
url.startswith("registry://") or \ | ||
"://" not in url: | ||
return True | ||
|
||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, seems fine to start. It'd be better probably to have this be a shared library function somewhere, we could even do something like ostree container url-requires-network <imgref>
or perhaps skopeo image-requires-network <imgref>
.
But I'm not really concerned about this to start, I think the 99% cases are:
- netinst ISO pulling from a remote registry
- "Full" ISO pulling from embedded content i.e.
oci:/path/to/embedded/image
as is done for desktop installers today, and is also how the Fedora CoreOS and derivative Live ISO works.
data.osname = "fedora-coreos" | ||
data.remote = "fcos-28" | ||
data.url = "quay.io/fedora/coreos:stable" | ||
data._signature_verification_enabled = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the FCOS image actually does have ostree-based GPG signatures, and verifying those does work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just test values, we don't run any ostree command here.
"""Test the configuration property.""" | ||
data = RPMOSTreeContainerConfigurationData() | ||
data.osname = "fedora-coreos" | ||
data.remote = "fcos-28" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fedora
most likely; we're just using the ostree remote name as a way to find the GPG verification setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value is not important in tests :).
@@ -580,7 +599,11 @@ def run(self): | |||
# We use the UNTRUSTED flag as a way to help verification for potentially | |||
# corrupted ISOs - this way ostree will validate the checksum of objects as | |||
# it writes data. | |||
pull_opts = {'refs': Variant('as', [ref]), 'flags': Variant('i', OSTree.RepoPullFlags.UNTRUSTED)} | |||
if self._is_container: | |||
pull_opts = {'flags': Variant('i', OSTree.RepoPullFlags.UNTRUSTED)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is going to work. It needs to instead fork off ostree container image pull
if that's the goal (to separate pulling from deployment).
(Which i think may complicate things as that isn't part of the C library today exposed via pygobject, so progress reporting is more complex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to know more about this. I honestly not sure why we are pulling the remote then remove remote and do the deployment. What is the difference between deployment and pull?
Honestly, I don't think I care if it's done by one command or two. Less is better for us probably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason to distinguish between the pulling and deployment is that pulling from the network is something for which one commonly wants e.g. a progress bar in a GUI environment.
The deployment phase is usually fast enough that it doesn't need it.
But indeed, today in Anaconda we could just skip this separate pull phase and rely on the deploy code automatically doing a pull if the image isn't present.
This whole discussion is really exactly the semantics of e.g.:
$ podman run busybox echo hello
If you don't have the busybox
image present, podman will pull it by default. But it also means that if the image pull fails (due to network flakes) the command will fail. And so in many environments there's going to be a separation between pulling (and retries) versus the run/deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the deployment is enough for this task we can probably just read command output to get progress reporting? Or for first version don't complicate things and instead just show "deploying". Could be improved later.
Should we leave the pull for ostree repository and bypass that just for container or there is no benefit in doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, worst case anaconda in the GUI could display a terminal with the status.
(That said, what will be glaring in this model is the fact that the ostree stack today is not translated, while Anaconda last I looked is...)
Found another blocker for this feature. We don't have skopeo in the installation environment so skopeo needs to be added to Lorax. |
6047d4b
to
59d526b
Compare
UPDATED:
Test coverage should be fine after these changes. Now I need to make it working in real world. |
59d526b
to
e80c0aa
Compare
Rebased. |
Add missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, it looks good to me. Thanks!
anaconda.spec.in
Outdated
@@ -102,6 +102,10 @@ Requires: python3-systemd | |||
Requires: python3-productmd | |||
Requires: python3-dasbus >= %{dasbusver} | |||
Requires: flatpak-libs | |||
# depndencies for rpm-ostree payload module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo.
:Summary: Add support for OSTree native containers (#2125655) | ||
|
||
:Description: | ||
Fedora is adding a new enhance the (rpm-)ostree stack to natively support OCI/Docker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a new enhancement for
?
if not self._data.signature_verification_enabled: | ||
remote_options['gpg-verify'] = Variant('b', False) | ||
else: | ||
if not self._data.gpg_verification_enabled: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hide these details in new methods of this task. This logic can be moved to a new method called something like _is_gpg_verify_enabled
that will return True or False. Then you can set up the remote options based on one check.
if self._data.is_container(): | ||
ref = self._data.url | ||
else: | ||
ref = RpmOstree.varsubst_basearch(self._data.ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. The ref object can be returned by a new method.
if source: | ||
return source | ||
|
||
return self._get_source(SourceType.RPM_OSTREE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be simplified:
return self._get_source(SourceType.RPM_OSTREE_CONTAINER) or self._get_source(SourceType.RPM_OSTREE)
a0d9c6f
to
9f44e70
Compare
UPDATED:
@cgwalters I've build ISO with the skope new requirement. However, I'm not able to make that work because I'm still getting message: Unfortunately I'm not sure what happens and I'm not able to find more verbose output. When I use To test this, run the ISO and add |
Otherwise, minimal environments like Anaconda might omit this. xref rhinstaller/anaconda#4561 (comment)
Hi, I think this is because the Anaconda environment is missing Filed coreos/rpm-ostree#4317 |
# FIXME: this is not useful because the URL seems to request reference-scheme | ||
# https://github.com/ostreedev/ostree-rs-ext/blob/b93946a1c512661ed4026914ff48c7c4ef25d18a/lib/src/container/mod.rs#L167 | ||
# self.module.configuration.url = "localhost/my/path" | ||
# assert self.module.network_required is False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it won't work, please drop it from the final version.
Would it be possible to also add dependency for |
Just testing the I'll make PR also to Lorax if this will work correctly. |
Ah, yes. The space savings from filtering packages like util-linux really can't be worth the maintenance overhead of the bugs (like this) that it introduces; at least for any Anaconda image which contains a GUI. |
Agree and we don't plan to keep that with Image Builder. I was able to move a bit. By using KS command:
and changing the lorax. I was able to get to this error where Anaconda complains about missing |
anaconda.spec.in
Outdated
@@ -102,6 +102,10 @@ Requires: python3-systemd | |||
Requires: python3-productmd | |||
Requires: python3-dasbus >= %{dasbusver} | |||
Requires: flatpak-libs | |||
# dependencies for rpm-ostree payload module | |||
Requires: rpm-ostree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where rpm-ostree
and skopeo
are used. Is it transparently called by another command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, today the command ostree container
is actually forwarded to rpm-ostree. (This is confusing I know, but it's to avoid duplicating the Rust code for containers...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ostree container image deploy
calls skopeo
and I think also rpm-ostree
is called by ostree
. I took that dependency from Lorax. Might be worth checking why it's not ostree
...
@cgwalters could you please help me with the grub error above? I would like to have a working version. Anaconda tries to write to grub.cfg file and we are getting this:
|
9f44e70
to
3fc7480
Compare
UPDATED:
|
Yes. This is also related to e.g. https://pagure.io/workstation-ostree-config/pull-request/344 What I'd actually like to accomplish here is to have anaconda switch to using (I know this is a bigger change because it hence needs to take over more of what anaconda is doing today with the bootloader...so for now, it does make sense to continue to work to enable the pure-ostree-container logic) Specifically instead of trying with the fedora-coreos image, well, I was hoping to get official desktop container images pushed (see e.g. https://pagure.io/releng/issue/11047 and it looks like that's super close and needs one more fix) but anyways you can try for now the images built here https://github.com/cgwalters/sync-fedora-ostree-containers specifically e.g. |
Also if you need to make changes to that image to test stuff, you can now use any container buildsystem (e.g. a Dockerfile) to derive a custom image and inject directories or grub configs as needed. |
77de815
to
7939f76
Compare
Fix the broken indentation. |
/kickstart-test --testtype smoke |
/kickstart-test --testtype payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
HMC installation failed on timetout -> shouldn't be related |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the code really wrongly formed. A lot of the if is_container()
spaghetti could be avoided, starting with better data class field naming and common ancestors, and this problem then flows all the way from the data/config to the task's run()
.
I won't nack this because it's something that people ask for and is running late. BUT, I won't ack this either; I do not want to maintain this, the right time to do it right is now, by the author. It will work, but it feels write-only.
def _get_stateroot(data): | ||
"""Get stateroot. | ||
|
||
The OSTree renamed old osname to stateroot for containers. | ||
|
||
:param data: OSTree source structure | ||
:return str: stateroot or osname value based on source | ||
""" | ||
if data.is_container(): | ||
# osname was renamed to stateroot so let's use the new name | ||
if data.stateroot: | ||
return data.stateroot | ||
else: | ||
# The stateroot doesn't have to be defined | ||
# https://github.com/ostreedev/ostree-rs-ext/pull/462/files | ||
# However, it's working just for a subset of calls now. | ||
# TODO: Remove this when all ostree commands undestarstands this | ||
return "default" | ||
else: | ||
return data.osname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this code simply does not have to exist. I think you should have renamed one of these fields so that they match. Especially when the first thing "stateroot" docstring says is that it's the osname...
if data.is_container(): | ||
return data.signature_verification_enabled | ||
else: | ||
return data.gpg_verification_enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here. If it's the same thing, just call it the same. The docstrings also say that it's just gpg now, so why did we have to invent a new name?
# Variable substitute the ref: https://pagure.io/atomic-wg/issue/299 | ||
if data.is_container(): | ||
# we don't have ref with container; there are not multiple references in one container | ||
return data.url | ||
else: | ||
return RpmOstree.varsubst_basearch(data.ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is above both calls, so perhaps you should substitute also for the url?
) | ||
|
||
log.info("ostree admin deploy starting") | ||
if self._data.is_container(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part and below feel as if you forgot that polymorphism exists?
data = self._get_data() | ||
data = _make_config_data() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to remove the actual function after it's not called here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
@staticmethod | ||
def is_container(): | ||
"""Is this native container source?""" | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You extended this class so it's the same as the new class... but then made it different anyway?
7939f76
to
021a5b0
Compare
Fixed typo in RN and leftover testing method mentioned by @VladimirSlavik . For other point we agreed to do it after merging this PR. |
/kickstart-test --testtype smoke |
In the future though, I think we can remove the non-container ostree backend. |
021a5b0
to
251dc7a
Compare
New ISO with new pykickstart (reason for failures here) on Cockpit side is prepared cockpit-project/bots#4530. However, it has an issue blocking for merge. Hopefully, we will be able to resolve that on Monday. |
/kickstart-test --testtype smoke |
1 similar comment
/kickstart-test --testtype smoke |
Probably you want to rebase and drop the pykickstart bump, it's at next version already. |
RPM OSTree is getting new functionality to use containers as the base image. Thanks to that it's possible to use standard container repositories. This source enables to use these repositories. Related: rhbz#2125655
We have a new RPMOStreeContainer source now which enables us to use containers as installation source. In this commit we adapt the current rpm_ostree to be able to consume this new container source. Add a lot of rpm ostree tasks tests for the container source. Resolves: rhbz#2125655
Related: rhbz#2125655
Related: rhbz#2125655
There is already existing top level function to create the same, so let's re-use it and remove duplication.
Our intention is to move dependencies specific for Anaconda execution from Lorax into Anaconda. Let's do that for rpm-ostree because we need to also add `skopeo` project to be able to download container images. Require rpm-ostree version which contains: - ostreedev/ostree-rs-ext#464 (simplified syntax) - ostreedev/ostree-rs-ext#462 (stateroot is not mandatory) Related: rhbz#2125655
Based on my discussion and recommendation on the bug, the 'registry' transport should be the only one which needs network to work. Other ways of transport should be used for local management. Related: rhbz#2125655
251dc7a
to
377f54a
Compare
/kickstart-test --testtype smoke |
RPM OSTree is getting new functionality to use containers as the base
image. Thanks to that it's possible to use standard container
repositories.
This source enables to use these repositories.
Depends on
rpm-ostree
version 2023.2Related: rhbz#2125655
Please check that your PR follows these rules:
Code conventions. tl;dr: Follow PEP8, wrap at 100 chars, and provide docstrings.
Commit message conventions. tl;dr: Heading, empty line, longer explanations, all wrapped manually. If in doubt, write a longer commit message with more details.
Release notes and docs if the PR adds something major or changes existing behavior.
If you don't know how, ask us for help!
Add better coverage. The rpm-ostree payload needs tests to cover new functionality.
Change osname to stateroot. If the changes are accepted here Enable container support for ostreesetup command pykickstart/pykickstart#434 .
Add skopeo. The ostree container deployment is using skopeo tool to download image. However, it's not part of Lorax. Will add this as Anaconda dependency.
Add kickstart test. We already have kickstart test for
ostreesetup
would be great to add one also for the newostreecontainer
command.