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.

Sign up for GitHub

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

Jump to bottom

remove need to download pause image #11956

Merged
merged 4 commits into from
Oct 27, 2021
Merged

remove need to download pause image #11956

merged 4 commits into from
Oct 27, 2021

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Oct 13, 2021 •
edited
Loading

So far, the infra containers of pods required pulling down an image
rendering pods not usable in disconnected environments. Instead, create
infra containers with an overlay root FS on /usr/libexec/pause.

Signed-off-by: Valentin Rothberg [email protected]

Please refer to the individual commits for further details.

@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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 13, 2021
@vrothberg vrothberg changed the title WIP - remove need to download pause image remove need to download pause image Oct 15, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2021
@vrothberg
Copy link
Member Author

@containers/podman-maintainers PTAL

Luap99
Luap99 reviewed Oct 15, 2021
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Why do we have to build this image? I would expect that we can just use the hosts rootfs with this binary instead of adding a new image?

pause/pause.c
Comment on lines +52 to +54
if (getpid() != 1)
/* Not an error because pause sees use outside of infra containers. */
fprintf(stderr, "Warning: pause should be the first process\n");
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this check? It fails when I run a pod with --pid=host.
It also fails when we want to use this for the podman pause process.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the exact copy from K8s. Seems like we need tests for --pid=host.

@vrothberg
Copy link
Member Author

Why do we have to build this image? I would expect that we can just use the hosts rootfs with this binary instead of adding a new image?

We could do that as well. In that case, the pause binary needs a dedicated directory. Currently it's in .../libexec/pause, so we could turn it into .../libexec/pause/pause and run the infra container with --rootfs .../libexec/pause.

@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2021

Sounds good to me.

@vrothberg
Copy link
Member Author 

podman (pause) $ ./bin/podman run --rootfs /usr/libexec/podman/ /pause                      
Error: error creating mtab directory: mkdir /usr/libexec/podman/etc: permission denied

Looks like --rootfs is not the way to go.

@Luap99
Copy link
Member

Luap99 commented Oct 15, 2021

I tried something like this for my rootless cni work, if I remember correctly you have to create an empty writeable directory (ideally on tmpfs) and then bind mount the pause binary into this. This should allow you to use this dir as rootfs.

@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2021

Try:

./bin/podman run --rootfs /usr/libexec/podman:O /pause

@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2021

You can even play around with

podman run -ti --rootfs /:O sh

cdoern
cdoern reviewed Oct 15, 2021
View reviewed changes
Copy link
Contributor

@cdoern cdoern left a comment

Choose a reason for hiding this comment

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

I really like the changes you made to how/when specgen is filled out, I had been trying to think of a succinct way to move everything around! Only point of discussion is, we could use this as an opportunity (breaking change incoming) to marshal into the infra spec when using the API, could remove a lot of repetitive code and streamline the logic.

cmd/podman/containers/create.go Show resolved Hide resolved
cmd/podman/pods/create.go Outdated Show resolved Hide resolved
pkg/specgen/generate/pod_create.go Outdated Show resolved Hide resolved
cdoern
cdoern reviewed Oct 15, 2021
View reviewed changes
cmd/podman/pods/create.go Outdated Show resolved Hide resolved
@vrothberg
Copy link
Member Author

I really like the changes you made to how/when specgen is filled out, I had been trying to think of a succinct way to move everything around! Only point of discussion is, we could use this as an opportunity (breaking change incoming) to marshal into the infra spec when using the API, could remove a lot of repetitive code and streamline the logic.

I would absolutely love that. This PR attempts to tackle parts of the code scattering but if we'd allow for the infra spec being marshaled, we could get rid of quite some additional/redundant code.

@mheon, WDYT?

@rhatdan
Copy link
Member

rhatdan commented Oct 16, 2021

@vrothberg did my suggestion on an overlay mount on /usr/libexec/pause/pause work?

@vrothberg
Copy link
Member Author

@vrothberg did my suggestion on an overlay mount on /usr/libexec/pause/pause work?

Yes, thanks. The overlay feature doesn't seem to be documented for --rootfs though. I played with it prior to this PR but I refrained from doing it after the error.

@flouthoc
Copy link
Collaborator

@vrothberg did my suggestion on an overlay mount on /usr/libexec/pause/pause work?

Yes, thanks. The overlay feature doesn't seem to be documented for --rootfs though. I played with it prior to this PR but I refrained from doing it after the error.

@vrothberg Oh it seems I only added it with run manual https://docs.podman.io/en/latest/markdown/podman-run.1.html . Please let me know if its needs to be documented at any other place as well. I'll do it 😃

@vrothberg vrothberg mentioned this pull request Oct 18, 2021
Merged
@vrothberg
Copy link
Member Author

@vrothberg Oh it seems I only added it with run manual https://docs.podman.io/en/latest/markdown/podman-run.1.html . Please let me know if its needs to be documented at any other place as well. I'll do it

Thanks, @flouthoc! I have the 3.4 man pages installed, so I didn't see it :^)

@vrothberg
Copy link
Member Author

I think this is needed to set the userns

I tried that already but I keep getting the same errors.

@vrothberg
Copy link
Member Author

Back to building an image. I also refrained from consolidating the infra-spec gen code any further since it's super fragile.

@vrothberg
overlay root fs: create mount on runtime dir
2e3611d 
Make sure to create the mounts for containers with an overlay root FS in
the runtime dir (e.g., /run/user/1000/...) to guarantee that we can
actually overlay mount on the specific path which is not the case for
the graph root.

[NO NEW TESTS NEEDED] since it is not a user-facing change.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
cirrus: containers: mount directory in /var/tmp to /tmp
e86549b 
Mount a directory from /var/tmp to /tmp to make sure that /tmp is not on
an overlay mount.  This should make overlay mounts possible in the
containerized tests which we're currently skipping.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
add kubernetes pause
9d2b8d2 
Add the k8s pause binary to `pause/pause.c` and do the plumbing in the
Makefile to install it in $libexec/podman/pause/pause.  It is intended to
replace the k8s pause image and hence the need for network connectivity
when creating pods.

[NO NEW TESTS NEEDED] since it will be tested in a following commit.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
pod create: remove need for pause image
75f478c 
So far, the infra containers of pods required pulling down an image
rendering pods not usable in disconnected environments.  Instead, build
an image locally which uses local pause binary.

Fixes: containers#10354
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

vrothberg commented Oct 26, 2021 •
edited
Loading

Hallelujah 🙏

It's green. Once @giuseppe's openSUSE/catatonit#17 is merged (and released) we can update the code, remove pause entirely and use catatonit instead.

giuseppe
giuseppe approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

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

@rhatdan
Copy link
Member

rhatdan commented Oct 27, 2021

/lgtm
I would like to make the --rootfs pause/:/:O so that the pause image is not viewable to the user. But for now, this is a big improvement.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2021
@openshift-merge-robot openshift-merge-robot merged commit 979b631 into containers:main Oct 27, 2021
@vrothberg vrothberg deleted the pause branch October 27, 2021 10:24
@vrothberg
Copy link
Member Author

vrothberg commented Oct 27, 2021 •
edited
Loading

/lgtm I would like to make the --rootfs pause/:/:O so that the pause image is not viewable to the user. But for now, this is a big improvement.

Yes, that would be ideal. Also, I loved @giuseppe's idea to implement the notion of a scratch image. Similar to buildah from scratch we could do a podman run -v pause:/pause scratch.

@rhatdan
Copy link
Member

rhatdan commented Oct 27, 2021

Yes that is a neat idea. Might be useful for lots of static use cases.

@edsantiago edsantiago mentioned this pull request Oct 27, 2021
Closed
@edsantiago edsantiago mentioned this pull request Nov 17, 2021
Closed
@edsantiago edsantiago mentioned this pull request Mar 21, 2022
Merged
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@cdoern cdoern cdoern left review comments

@giuseppe giuseppe giuseppe approved these changes

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@vrothberg @rhatdan @Luap99 @flouthoc @TomSweeneyRedHat @mheon @cevich @cdoern @giuseppe @openshift-merge-robot