Skip to content

Freebsd linux containers #5480

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
Closed
gizahNL wants to merge 7 commits into from
Closed

Freebsd linux containers #5480

gizahNL wants to merge 7 commits into from

Conversation

@gizahNL
Copy link
Contributor

@gizahNL gizahNL commented May 11, 2021

This allows running Linux containers on FreeBSD and modifies the mounts so that they represent the linux emulated filesystems, as per: https://wiki.freebsd.org/LinuxJails
Depends on PR: #5472

@k8s-ci-robot
Copy link

k8s-ci-robot commented May 11, 2021

Hi @gizahNL. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 11, 2021

Build succeeded.

@AkihiroSuda AkihiroSuda added this to the 1.6 milestone May 11, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented May 11, 2021

Build succeeded.

@gizahNL gizahNL mentioned this pull request May 11, 2021
Closed
AkihiroSuda
AkihiroSuda reviewed May 11, 2021
View reviewed changes
@theopenlab-ci
Copy link

theopenlab-ci bot commented May 11, 2021

Build succeeded.

@gizahNL
Copy link
Contributor Author

gizahNL commented May 11, 2021

Build succeeded.

* [containerd-build-arm64 ](https://logs.openlabtesting.org/logs/80/5480/56574357a0fcac46e3a639c32e401818523b6fc4/check/containerd-build-arm64/8f864eb/) : SUCCESS in 4m 49s (non-voting)

* [containerd-test-arm64 ](https://logs.openlabtesting.org/logs/80/5480/56574357a0fcac46e3a639c32e401818523b6fc4/check/containerd-test-arm64/0ab977d/) : SUCCESS in 5m 27s (non-voting)

* [containerd-integration-test-arm64 ](https://logs.openlabtesting.org/logs/80/5480/56574357a0fcac46e3a639c32e401818523b6fc4/check/containerd-integration-test-arm64/550b8ef/) : FAILURE in 27m 52s (non-voting)

I think the integration test failing is not related to my commits. (I see a timeout)

@cpuguy83 cpuguy83 requested a review from samuelkarp May 11, 2021 17:11
mxpv
mxpv reviewed May 11, 2021
View reviewed changes
@theopenlab-ci
Copy link

theopenlab-ci bot commented May 12, 2021

Build succeeded.

samuelkarp
samuelkarp reviewed May 13, 2021
View reviewed changes
oci/spec_opts.go Outdated
Copy link
Member

@samuelkarp samuelkarp May 13, 2021

Choose a reason for hiding this comment

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

The other maintainers might feel differently about this so I'm not asking you to make a change yet, but the way I probably would have done this is to define a function like appendOSMounts in each of spec_opts_freebsd.go/spec_opts_linux.go/spec_opts_windows.go with whatever content is appropriate there. The Linux one would likely be empty, but I imagine there could be some use for this on Windows with the LCOW/WCOW work.

In addition to providing a possibly-reusable abstraction for FreeBSD and Windows to achieve similar goals, an advantage is keeping FreeBSD-specific filesystems like "linprocfs" and "linsysfs" out of the common file that is compiled for all operating systems.

The other advantage of this is that, when compiled for FreeBSD, we could also add a WithLinuxEmulationMounts functional option that clients of containerd can use without having to use WithImageConfig/WithImageConfigArgs or even having an image with a declared OS of "linux".

Copy link
Contributor Author

@gizahNL gizahNL May 13, 2021

Choose a reason for hiding this comment

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

Yes, that seems like a better approach. I'll wait for feedback from the other maintainers.

Copy link
Contributor Author

@gizahNL gizahNL Jun 7, 2021

Choose a reason for hiding this comment

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

Does anybody else want to way in on this? I see no issues in rewriting as to Samuel's suggestion.

Copy link

@emaste emaste Jun 7, 2021

Choose a reason for hiding this comment

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

Samuel's suggestion seems reasonable to me.

Copy link
Member

@samuelkarp samuelkarp Jun 11, 2021

Choose a reason for hiding this comment

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

Since no one else has said anything: I'd like to see the change that I suggested. 😄

@samuelkarp
Copy link
Member

samuelkarp commented May 13, 2021

/ok-to-test

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 13, 2021

Build succeeded.

alakesh
alakesh reviewed May 19, 2021
View reviewed changes
Copy link
Contributor

@alakesh alakesh left a comment

Choose a reason for hiding this comment

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

Wanted to understand why the freebsd defaults for mount points /proc and /dev are missing some of the defaults that we use for linux.

@alakesh
Copy link
Contributor

alakesh commented May 20, 2021

Thanks for explaining. Looks good to me otherwise.

@samuelkarp samuelkarp mentioned this pull request May 22, 2021
Open
akhramov
akhramov reviewed May 22, 2021
View reviewed changes
gizahNL added 4 commits June 7, 2021 20:47
@gizahNL
Allow Linux on FreeBSD
19e96b3 
Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL
Modify FreeBSD mounts when running a Linux image
7c32453 
Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL
gofmt'ed
6bf12b0 
Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL
Cleanup left-over debug print
40a7c84 
Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL gizahNL force-pushed the freebsd_linux_emulation branch from b1171b2 to 1903d87 Compare June 7, 2021 18:59
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 7, 2021

Build succeeded.

@gizahNL
Add comments to conditional and function.
073e1f3 
Signed-off-by: Gijs Peskens <gijs@peskens.net>
gizahNL and others added 2 commits June 7, 2021 21:02
@gizahNL @samuelkarp
Mirror the DefaultSpec definition.
b4b8eb1 
Co-authored-by: Samuel Karp <samuelkarp@users.noreply.github.com>
Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL
Add mount options & make sure all fs are present.
13e69ae 
nosuid, noexec options added to /proc and /sys.
ro added to /sys

Modified code to always add linprocfs & /dev/fd even
if not present in existing Mounts slice.

Signed-off-by: Gijs Peskens <gijs@peskens.net>
@gizahNL gizahNL force-pushed the freebsd_linux_emulation branch from 1903d87 to 13e69ae Compare June 7, 2021 19:02
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 7, 2021

Build succeeded.

kzys
kzys reviewed Jun 10, 2021
View reviewed changes
Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

The change looks good. It would be great if you can tidy up your commit history a bit. For example, the fact you didn't gofmt files in the beginning is not worth to keep.

@kzys kzys added the status/needs-update Awaiting contributor update label Jun 15, 2021
@akhramov
Copy link
Contributor

akhramov commented Sep 15, 2021

Hey @gizahNL. I'm sorry to bug you, but are you still working on this?

@gizahNL
Copy link
Contributor Author

gizahNL commented Sep 21, 2021

Hey @gizahNL. I'm sorry to bug you, but are you still working on this?

Haven't had time recently, I'll try to cleanup and rebase this week :)

Thanks for the reminder!

@dmcgowan
Copy link
Member

dmcgowan commented Dec 9, 2021

If we want to get this into 1.6 then can we get a rebase on this PR

@dmcgowan dmcgowan modified the milestones: 1.6, 1.7 Jan 12, 2022
@akhramov akhramov mentioned this pull request May 29, 2022
Merged
@akhramov
Copy link
Contributor

akhramov commented May 29, 2022

Superseded by #7000

@samuelkarp
Copy link
Member

samuelkarp commented Jun 9, 2022

Closing since we've now merged #7000. Thank you for working on this!

@samuelkarp samuelkarp closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kzys kzys kzys left review comments

@samuelkarp samuelkarp samuelkarp left review comments

@mxpv mxpv mxpv left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

+3 more reviewers

@emaste emaste emaste left review comments

@alakesh alakesh alakesh left review comments

@akhramov akhramov akhramov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/feature ok-to-test platform/freebsd FreeBSD status/needs-update Awaiting contributor update

Projects

None yet

Milestone

1.7

Development

Successfully merging this pull request may close these issues.

10 participants

@gizahNL @k8s-ci-robot @samuelkarp @alakesh @akhramov @dmcgowan @kzys @mxpv @emaste @AkihiroSuda