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 podman static build #5566

Merged
merged 1 commit into from
May 11, 2020

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Mar 20, 2020

We’re now able to build a static podman binary based on a custom nix
derivation. This is integrated in cirrus as well, whereas a later target
would be to provide a self-contained static binary bundle which can be
installed on any Linux x64-bit system.

Fixes: #1399

Test it out yourself:

> make build-static
…
> ldd bin/podman-static
        not a dynamic executable

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-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2020
@saschagrunert
Copy link
Member Author

Just have to make the CI happy :)

@vrothberg
Copy link
Member

vrothberg commented Mar 20, 2020

This is integrated in cirrus as well, whereas a later target
would be to provide a self-contained static binary bundle which can be
installed on any Linux x64-bit system.

I feel rather objective toward that idea. There's a whole load of issues with static binaries working around NSS (breaking LDAP etc.). We don't provide static builds of Skopeo for that reason. If we allow for static builds, we need to make clear that this is not supported. I think we need some way in podman info to see if it's a static binary; issues should only be filed for dynamic ones.

Makefile Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Member Author

This is integrated in cirrus as well, whereas a later target
would be to provide a self-contained static binary bundle which can be
installed on any Linux x64-bit system.

I feel rather objective toward that idea. There's a whole load of issues with static binaries working around NSS (breaking LDAP etc.). We don't provide static builds of Skopeo for that reason. If we allow for static builds, we need to make clear that this is not supported. I think we need some way in podman info to see if it's a static binary; issues should only be filed for dynamic ones.

Do you recap which kind of issue that was? I'm not aware of any issues for example in conjunction with the CRI-O static binary bundle.

@rhatdan
Copy link
Member

rhatdan commented Mar 20, 2020

Discussion here
containers/skopeo#670

@saschagrunert
Copy link
Member Author

Discussion here
containers/skopeo#670

Thanks. To me it looks like an usual bug in golang which has been fixed long time ago, right? I would not say that this falls into the category of: load of issues with static binaries. WDYT?

@vrothberg
Copy link
Member

Discussion here
containers/skopeo#670

Thanks. To me it looks like an usual bug in golang which has been fixed long time ago, right? I would not say that this falls into the category of: load of issues with static binaries. WDYT?

I don't see that bypassing NSS has been resolved. @mtrmac will know.

@rhatdan
Copy link
Member

rhatdan commented Mar 20, 2020

This is an issue of support. It is likely that a static version of podman would not work well on a system that used nsswitch to modify the way host resolution worked. Or how UID/GID lookups worked.
It would just fall back to reading /etc/hosts and /etc/resolv.conf, /etc/passwd, /etc/shadow.

This is definitely not something we would want to 'SUPPORT', But I have no problem providing it for people to play with.

I agree that podman info should display that it was built staticly to prevent confusion.

@saschagrunert
Copy link
Member Author

I agree that podman info should display that it was built staticly to prevent confusion.

Sounds good, I'll follow up with that on Monday if there are no further concerns.

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 20, 2020

AFAIK the NSS situation remains the same (either dynamically link, or use a Go-only approximation of the default behavior, losing all NSS plugins).

Separately, statically-building device-mapper and/or LVM, if nothing else, seems pretty scary. Is it safe to mix&match versions of libraries dealing with filesystems and user data on one system?


My first, fairly strong, instinct is to say ”no, get tested packages from your distribution” (and that includes Nix). Admittedly I’ve been living in the distribution world for ages, so I’m not at all neutral.

And we do see that whether one makes container builds or static builds difficult, one or the other tends to happen anyway just because users feel they need one of those. So it might be, after all, worthwhile to produce one of the two just to make sure most people use a known build instead of home-grown differently-patched one. Perhaps. I have no idea how prevalent are the uses of the static/container builds, just that at least one person probably makes one.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5571) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2020
@saschagrunert saschagrunert force-pushed the static-binary branch 2 times, most recently from 7d19775 to 0557f48 Compare March 23, 2020 11:22
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2020
@saschagrunert
Copy link
Member Author

I added an additional PodmanLinkmode field to the podman info output.

@saschagrunert saschagrunert force-pushed the static-binary branch 2 times, most recently from dc9c6d6 to 74d2843 Compare March 23, 2020 12:10
@TomSweeneyRedHat
Copy link
Member

Code LGTM, I'll leave it to others for the should/should not debate. Personally I think it's fine as long as it's documented.

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 still have very mixed feelings about it and I frankly don't have time (or desire) to get into Nix, but as long as I don't have to maintain it, I am cool with it 🤣

What I would love to have is some visible statement that the nix stuff is not supported. Which further makes me wonder why to offer something that is not supported but I don't want to be the party pooper and really want some more opinions.

@mheon @baude PTAL

@saschagrunert
Copy link
Member Author

I still have very mixed feelings about it and I frankly don't have time (or desire) to get into Nix, but as long as I don't have to maintain it, I am cool with it

What I would love to have is some visible statement that the nix stuff is not supported. Which further makes me wonder why to offer something that is not supported but I don't want to be the party pooper and really want some more opinions.

@mheon @baude PTAL

🤷‍♂️ I think if the majority does not want to have it then feel free to close it.

@rhatdan
Copy link
Member

rhatdan commented Mar 24, 2020

Since this is an upstream project, support is best effort. We can document the short comings of this version and state what does and does not work. But if community wants it and is willing to deal with issues, then I have no problem doing it.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5507) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@jwhonce
Copy link
Member

jwhonce commented Apr 23, 2020

@saschagrunert

I have a request #5765 points out info is already a bit slow. Could you change podmanLinkmode() to PodmanLinkMode() and use a build tag to just report which mode? You would need two files linkmode_dynamic.go and linkmode_static.go which are tagged to be mutually exclusive. That way info can call and pay a minimal price.

@rhatdan
Copy link
Member

rhatdan commented May 9, 2020

I just added podman-remote-static, which was needed for CRC, @saschagrunert is this still something you are working on?

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2020
@saschagrunert
Copy link
Member Author

I just added podman-remote-static, which was needed for CRC, @saschagrunert is this still something you are working on?

Yes I updated the PR and also the suggestion from @jwhonce. Should I state somewhere explicitly that the static binary is not supported? I mean if we not ship it and podman info returns the "static" linkmode we should be probably fine already, right?

@vrothberg
Copy link
Member

Should I state somewhere explicitly that the static binary is not supported?

Yes, please add a note in the README. If we don't make it explicit, users will not know.

We’re now able to build a static podman binary based on a custom nix
derivation. This is integrated in cirrus as well, whereas a later target
would be to provide a self-contained static binary bundle which can be
installed on any Linux x64-bit system.

Fixes: containers#1399

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
Comment on lines +184 to +188
## Static Binary Builds
The Cirrus CI integration within this repository contains a `static_build` job
which produces a static Podman binary for testing purposes. Please note that
this binary is not officially supported with respect to feature-completeness
and functionality and should be only used for testing.
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the requested note here. ☝️

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks a lot!

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.

LGTM

@baude @jwhonce @mheon PTAL

Comment on lines +184 to +188
## Static Binary Builds
The Cirrus CI integration within this repository contains a `static_build` job
which produces a static Podman binary for testing purposes. Please note that
this binary is not officially supported with respect to feature-completeness
and functionality and should be only used for testing.
Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks a lot!

@rhatdan
Copy link
Member

rhatdan commented May 11, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2020
@openshift-merge-robot openshift-merge-robot merged commit d473e6e into containers:master May 11, 2020
@saschagrunert saschagrunert deleted the static-binary branch May 11, 2020 13:42
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
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. 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
Development

Successfully merging this pull request may close these issues.

Static podman
10 participants