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

Add ostreecontainer support #5277

Closed
wants to merge 14 commits into from
Closed

Conversation

kwozyman
Copy link

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
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
@pep8speaks
Copy link

pep8speaks commented Oct 24, 2023

Hello @kwozyman! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 501:9: E265 block comment should start with '# '

Line 74:100: E501 line too long (102 > 99 characters)

Line 26:100: E501 line too long (100 > 99 characters)
Line 76:100: E501 line too long (103 > 99 characters)
Line 86:100: E501 line too long (108 > 99 characters)

Line 23:100: E501 line too long (119 > 99 characters)

Line 100:100: E501 line too long (159 > 99 characters)
Line 104:100: E501 line too long (159 > 99 characters)
Line 124:100: E501 line too long (106 > 99 characters)
Line 128:100: E501 line too long (106 > 99 characters)

Comment last updated at 2024-01-15 11:45:18 UTC

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Thanks! There's a lot of code, so lots of comments :)

Apart from the points below, there is an empty commit "Add new OSTree container source test (#2125655)" - what's the plan there?

Comment on lines 1 to 2
:Type: Kickstart
:Summary: Add support for OSTree native containers (#2125655)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add this release note file, on RHEL we track release notes in the mandatory bugs/bzs/jira items.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed this from the current PR

Comment on lines 60 to 65
from pykickstart.commands.ostreecontainer import F38_OSTreeContainer as OSTreeContainer
from pykickstart.commands.ostreesetup import F38_OSTreeSetup as OSTreeSetup
from pykickstart.commands.ostreecontainer import F38_OSTreeContainer as OSTreeContainer
from pykickstart.commands.ostreesetup import F21_OSTreeSetup as OSTreeSetup
from pykickstart.commands.partition import F34_Partition as Partition
from pykickstart.commands.raid import F29_Raid as Raid
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep this in sync with the pykickstart PR - I vaguely recall Brian asked for versioning something new as RHEL9_*, I think?

Copy link
Author

Choose a reason for hiding this comment

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

Correct, I've updated this area to only reference RHEL9_* to match PyKickstart

@@ -429,11 +429,15 @@ class DisplayModes(Enum):
# Types of the payload source.
SOURCE_TYPE_LIVE_OS_IMAGE = "LIVE_OS_IMAGE"
SOURCE_TYPE_LIVE_IMAGE = "LIVE_IMAGE"
SOURCE_TYPE_LIVE_TAR = "LIVE_TAR"
Copy link
Contributor

@VladimirSlavik VladimirSlavik Nov 13, 2023

Choose a reason for hiding this comment

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

I'm not sure, but this does not seem like a related change?

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right, I've removed all the references to SOURCE_TYPE_LIVE_TAR

@@ -42,6 +42,7 @@ Source0: %{name}-%{version}.tar.bz2
%define simplelinever 1.8.3-1
%define subscriptionmanagerver 1.29.31
%define utillinuxver 2.15.1
%define rpmostreever 2023.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct for RHEL?

Makefile.am Outdated
CONTAINER_BUILD_ARGS ?= --no-cache
CONTAINER_BUILD_ARGS ?= --no-cache --network=host
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unrelated change, ideally I'd see it as a separate PR from this.

Copy link
Author

@kwozyman kwozyman Jan 15, 2024

Choose a reason for hiding this comment

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

I've removed this from the current PR

Comment on lines 512 to 426
safe_exec_with_redirect(
"ostree",
["admin",
"--sysroot=" + self._sysroot,
"os-init",
stateroot]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The commit says "Remove duplicate call to ostree". But I can't see any other place where os-init would be called?

Comment on lines +515 to +520
args = ["admin", "os-init",
"--sysroot=" + self._physroot]
if self._data.stateroot:
args.append(self._data.stateroot)
safe_exec_with_redirect("ostree", args)
Copy link
Contributor

@VladimirSlavik VladimirSlavik Nov 13, 2023

Choose a reason for hiding this comment

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

Now here you add os-init again, a few commits later, but it's only for the container code path, so for the regular ostree it is still removed?

@rvykydal rvykydal self-assigned this Jan 10, 2024
@rvykydal rvykydal mentioned this pull request Jan 12, 2024
2 tasks
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
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
@rvykydal
Copy link
Contributor

I should have mentioned it here explicitly (I noted it only in the RHEL jira issue) I am working on alternative PR #5399.

@jkonecny12
Copy link
Member

Closing this PR on behalf of #5399 which is based on this PR. Thanks a lot for your work @kwozyman.
The #5399 is ready for next RHEL-9 release.

@jkonecny12 jkonecny12 closed this Jan 23, 2024
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.

6 participants