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

manifests: Split container engines and deps into sub manifests #2877

Draft
wants to merge 1 commit into
base: testing-devel
Choose a base branch
from

Conversation

travier
Copy link
Member

@travier travier commented Feb 26, 2024

manifests: Split container engines into sub-manifests

  • Split podman, moby-engine and related packages into distinct
    sub-manifests
  • Include both container engines by default
  • Ensure that they are excluded if specified as such

manifest.yaml Outdated
@@ -1,6 +1,7 @@
variables:
stream: testing-devel
prod: false
podman-machine-os: false
Copy link
Member

Choose a reason for hiding this comment

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

I imagine we'll use a new stream name for the new stream. My proposal is that we make our conditional logic based on that and not a new var here.

Copy link
Member

Choose a reason for hiding this comment

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

This has the added benefit of not requiring us to change all of our manifest.yaml files on every branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is likely going to be multiple streams, one per version of podman

Copy link
Member Author

Choose a reason for hiding this comment

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

But if we define the streams exhaustively that could work. Or we can probably set a "default" value somewhere that is overridden in more top level manifest?

Copy link
Member

Choose a reason for hiding this comment

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

so add multiple conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a variable set with a default value that can be overridden by a top level manifest.

@@ -0,0 +1,3 @@
packages:
- podman
- crun
Copy link
Member

Choose a reason for hiding this comment

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

any openshift/os changes we need to make because we're breaking this out into a separate manifest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Likely, will look at this next.

@travier travier marked this pull request as draft February 26, 2024 17:53
@travier travier changed the title manifests: Split podman & moby-engine into sub manifests manifests: Split container engines and deps into sub manifests Feb 26, 2024
@travier
Copy link
Member Author

travier commented Feb 26, 2024

Things to clarify:

  • Should we include/exclude runc/crun?
  • Should we include/exclude skopeo (pulls-in container-commons)?

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Things to clarify:

* Should we include/exclude runc/crun?

What you have right now looks reasonable to me. I would expect anyone using container_engines: false would want to make that choice themselves in their derived layers. (But in practice, likely they'd just let the container engine of choice pull in its dep.)

* Should we include/exclude skopeo (pulls-in container-commons)?

This is used by rpm-ostree for the container update flow.

Actually, it's a runtime dep so a minimal compose would break anyway if it were excluded.

- |
#!/usr/bin/env bash
set -xeuo pipefail
setsebool -P -N container_use_cephfs on # RHBZ#1692369
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this seems more general than a podman thing. Let's keep that bit in fedora-coreos-base.yaml?

Copy link
Member Author

Choose a reason for hiding this comment

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

This requires the containers selinux policy module otherwise it fails.

- Split podman, moby-engine and related packages into distinct
  sub-manifests
- Include both container engines by default
@travier travier marked this pull request as ready for review February 27, 2024 13:16
@travier
Copy link
Member Author

travier commented Feb 27, 2024

This is working for me with https://github.com/travier/podman-machine-config

@dustymabe dustymabe marked this pull request as draft February 27, 2024 15:19
@dustymabe
Copy link
Member

Converted back to draft. Let's hold this while we discuss with the podman folks more.

@dustymabe
Copy link
Member

Should we close this?

@travier
Copy link
Member Author

travier commented Mar 25, 2024

I think it's still worth doing, even if it's not urgent anymore.

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

Successfully merging this pull request may close these issues.

None yet

3 participants