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

[WIP] Add podman machine Containerfile #21251

Closed
wants to merge 1 commit into from
Closed

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 13, 2024

Move some of the functionality in ignition into the building of a bootc based podman-machine.

Does this PR introduce a user-facing change?

none

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Jan 13, 2024
Copy link
Contributor

openshift-ci bot commented Jan 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2024
@rhatdan
Copy link
Member Author

rhatdan commented Jan 13, 2024

@baude @ashley-cui @cgwalters PTAL

Once we have the design here approved, I can go and remove a lot of code from ignition.go that sets these up.

@cgwalters I do question whether I should setup the config files in /usr/etc or /etc?

RUN systemctl enable podman.service
RUN printf "\nconfdir /etc/chrony.d\n" >> /etc/chrony.conf

COPY 10-inotify-instances.conf /usr/etc/sysctl.d/10-inotify-instances.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

The preference here is is actually to use /usr/lib/sysctl.d. This configuration is part of the OS.

Otherwise, use /etc. Don't use /usr/etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

COPY subuid /usr/etc/subgid
COPY subuid /usr/etc/subuid
RUN groupadd -g 501 core; useradd -u 501 -g 501 core
RUN --mount=type=tmpfs,destination=/var ostree container commit
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be dropped currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped

COPY linger-example.service /home/core/.config/systemd/user/linger-example.service
COPY containers-home.conf /home/core/.config/containers/containers.conf

RUN mkdir -p /home/core/.config/systemd/user/default.target.wants; \
Copy link
Contributor

Choose a reason for hiding this comment

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

What I'd recommend instead of this is to write these files into /etc/skel - and have the user created on firstboot via systemd-sysusers (per above).

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 no support for /etc/subuid and /etc/subgid there though? Or just create those files and let systemd-sysusers create it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now using /etc/skel

COPY podman-docker.conf /usr/etc/tmpfiles.d/podman-docker.conf
COPY subuid /usr/etc/subgid
COPY subuid /usr/etc/subuid
RUN groupadd -g 501 core; useradd -u 501 -g 501 core
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is tricky...I know we're adding it for compatibility, but going forward I think it'd probably make sense to allocate a podman user for example.

But even more importantly it will work better if we change things to allocate the user on firstboot today.

More on this in https://centos.github.io/centos-bootc/builds/#injecting-users-at-build-time (that could be expanded too)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why 501 versus the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just going by the entries in ignition.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the default user on a MAC is UID=501


RUN mkdir -p /home/core/.config/systemd/user/default.target.wants; \
ln -s /home/core/.config/systemd/user/linger-example.service /home/core/.config/systemd/user/default.target.wants/linger-example.service
RUN --mount=type=tmpfs,destination=/var ostree container commit
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 a no-op and unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

COPY containers-etc.conf /usr/etc/containers/containers.conf
COPY delegate.conf /usr/etc/systemd/system/[email protected]/delegate.conf
COPY docker-host.sh /usr/etc/profile.d/docker-host.sh
COPY podman-docker.conf /usr/etc/tmpfiles.d/podman-docker.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

And this should be /usr/lib/tmpfiles.d etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved.

@@ -0,0 +1 @@
core:100000:1000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there really no way to tweak this on the useradd commandline? That'd work better...

Copy link
Member Author

Choose a reason for hiding this comment

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

None that I found.

RUN printf "\nconfdir /etc/chrony.d\n" >> /etc/chrony.conf

COPY 10-inotify-instances.conf /usr/etc/sysctl.d/10-inotify-instances.conf
COPY 50-podman-makestep.conf /usr/etc/chrony.d/50-podman-makestep.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Also what IMO is way nicer to read and maintain is just to have:

COPY rootfs/etc etc
COPY rootfs/usr usr

And then just drop your files in those directories in git.

There's one example of this here

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 just used usr and etc, but this was a good idea.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

I will start nasty and request to have comments in all configs and dockerfiles.

Since we can replace some parts that ignition would take at provisiong time to build time, we need comments to keep it maintianable and avoid collective amnesia why something is done the way it is.


RUN dnf -y update; \
rpm --setcaps shadow-utils 2>/dev/null; \
dnf -y install podman subscription-manager crun crun-wasm crun-krun chrony \
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need skopeo and buildah?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't but I think it is useful for debugging and difficult to install if they don't exist.

Copy link
Member

Choose a reason for hiding this comment

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

They add 60M to the disk / container so I feel rather hesitant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I can remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I've quite often wanted the ability to inspect a remote image without fetching it...and that's just in skopeo right? (It would make sense to me to have something like this in podman too...podman image remote-inspect? Dunno)

As far as buildah, I think it's basically because it's been promoted as a peer to podman build, and so it goes where podman goes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The funning thing, my story of skopeo being created is exactly this. we wanted docker inspect --remote. So maybe it is time to add podman image inspect --remote One issue there is inspecting images versus manifest.

--raw versus default. Also --remote might be misinterpreted by CLI.

Copy link
Member

@vrothberg vrothberg Jan 15, 2024

Choose a reason for hiding this comment

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

I think this should be a separate issue. Note that skopeo runs on Win and Mac as well. So I don't think this must be part of the podman machine where IMO the focus should be on what Podman needs rather than what some developers may find useful. Ideally, users shouldn't need to ssh into the machine.

ADD usr usr
ADD etc etc

RUN groupadd -g 501 core; useradd -u 501 -g 501 core
Copy link
Member

Choose a reason for hiding this comment

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

the user cannot be made in containerfile due to podman machine init --username

Copy link
Member

Choose a reason for hiding this comment

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

also, if you want to push forward on this, should be user 1000

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 would be fine with removing this, but do we want to set the default user for the case where these is no specified --username and then allow the user to modify it. Might be a case where want to use sysusers.


RUN dnf -y update; \
rpm --setcaps shadow-utils 2>/dev/null; \
dnf -y install podman subscription-manager crun crun-wasm crun-krun chrony \
Copy link
Member

Choose a reason for hiding this comment

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

long term, i dont think this will work as we will need versioned content ... i.e. podman 5.0, podman 5.1 (or even 5.1.1). I think fedora-coreos-config does this reasonably nicely with YAML that gets read and converted to what you want above. perhaps there is some variable magic we can do here.

@baude
Copy link
Member

baude commented Jan 15, 2024

No code in podman is changed here ... we could make the claim this is all for hacking ... though I would like to see the claim added to the commit and the containerfile. I have a series of long term questions that remain unanswered here but i'll just note them the best i can (given I had intended to do this work but you beat me to it).

  1. I'm not confident this is the right place for this ... but i see the dilemma too. You are are trying to move "stuff" from code to containerfiles. Clearly we want that living in podman or podman-machine. However, you are also laying groundwork for a long-term goal of podman-machine operating systems. And that, I'm confident should not live here; however, this approach now mixes the proverbial peanut butter and chocolate. I'd be generally in favor of supporting dev work here ... but no code removal yet.

  2. as i said earlier, i think fcos and related has set a nice model for using YAML to control packaging ... it makes it clear, diffs in git are nicer, etc.

  3. we need to force a long-term conversation on what the podman-machine os looks like in the future. I will own that. The answer to this question is critical for Podman 5.

@rhatdan
Copy link
Member Author

rhatdan commented Jan 15, 2024

I think we want to make it easy for people to find an easy example of building a podman machine. We need to figure out the best way of selecting the podman machine perhaps using build-args. Perhaps something like:

ARG PODMAN_VERSION
RUN dnf -y install podman${PODMAN_VERSION}

Then build with:
$ podman build --buildarg PODMAN_VERSION="5.1.*" --tag quay.io/podman/podman-machine:5.1 .

This Containerfile would be just be installing
quay.io/podman/podman-machine:latest
or should it be
quay.io/podman/machine:latest

To eliminate the stutter.

@vrothberg
Copy link
Member

vrothberg commented Jan 15, 2024

Then build with:
$ podman build --buildarg PODMAN_VERSION="5.1.*" --tag quay.io/podman/podman-machine:5.1 .

An alternative may be to build podman directly inside the Dockerfile. A multistage build with injecting the source. The benefit is that it can be built directly in the upstream release process without any downstream delay/lag. This certainly comes at a cost of it not being a distro build.

Just to further sketch out what I mean:

FROM $base-image as podman-build
COPY ./ /src/podman
WORKDIR /src/podman
RUN dnf install -y $build-dependencies
RUN make podman

FROM $base-image
COPY --from=podman-build /src/podman/bin/podman /usr/bin/podman
# All the rest

Move some of the functionality in ignition into the building of a
bootc based podman-machine.

Signed-off-by: Daniel J Walsh <[email protected]>
@cgwalters
Copy link
Contributor

cgwalters commented Feb 8, 2024

OK so to enable Ignition in derived images, just off the top of my head there are probably two major parts:

@rhatdan
Copy link
Member Author

rhatdan commented Feb 29, 2024

@baude can we update this Containerfile to match the current one you are using?

@rhatdan rhatdan closed this May 16, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 15, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants