https://github.com/containerd/containerd/pull/7000 Skip to content Sign up * Product + Features + Mobile + Actions + Codespaces + Packages + Security + Code review + Issues + Integrations + GitHub Sponsors + Customer stories * Team * Enterprise * Explore + Explore GitHub + Learn and contribute + Topics + Collections + Trending + Skills + GitHub Sponsors + Open source guides + Connect with others + The ReadME Project + Events + Community forum + GitHub Education + GitHub Stars program * Marketplace * Pricing + Plans + Compare plans + Contact Sales + Education [ ] * # In this repository All GitHub | Jump to | * No suggested jump to results * # In this repository All GitHub | Jump to | * # In this organization All GitHub | Jump to | * # In this repository All GitHub | Jump to | Sign in Sign up {{ message }} containerd / containerd Public * Notifications * Fork 2.2k * Star 11.3k * Code * Issues 295 * Pull requests 102 * Discussions * Actions * Projects 0 * Security * Insights More * Code * Issues * Pull requests * Discussions * Actions * Projects * Security * Insights 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. Pick a username [ ] Email Address [ ] Password [ ] [ ] 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 Linux containers on FreeBSD #7000 Merged samuelkarp merged 1 commit into containerd:main from akhramov: freebsd_linux_containers Jun 9, 2022 Merged Linux containers on FreeBSD #7000 samuelkarp merged 1 commit into containerd:main from akhramov: freebsd_linux_containers Jun 9, 2022 +124 -2 Conversation 29 Commits 1 Checks 34 Files changed 7 Conversation This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters Show hidden characters akhramov Copy link Contributor @akhramov akhramov commented May 29, 2022 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 Supersedes #5480 per convo with @gizahNL in OCI Slack chat Sorry, something went wrong. 3 sftim, chrisafraser, and wdzeng reacted with hooray emoji [?] 5 gizahNL, dungdm93, trasz, nindanaoto, and wdzeng reacted with heart emoji All reactions * 3 reactions * [?] 5 reactions @k8s-ci-robot Copy link Collaborator @k8s-ci-robot k8s-ci-robot commented May 29, 2022 Hi @akhramov. 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. 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. All reactions Sorry, something went wrong. @k8s-ci-robot k8s-ci-robot added the needs-ok-to-test label May 29, 2022 @akhramov akhramov mentioned this pull request May 29, 2022 Freebsd linux containers #5480 Closed @akhramov akhramov force-pushed the freebsd_linux_containers branch from 0e57bf3 to 6726a95 Compare May 29, 2022 samuelkarp samuelkarp reviewed May 29, 2022 View changes oci/spec_opts.go Outdated //FreeBSD has support for running Linux binaries via syscall redirection, while that would be enough //for some binaries others depend on emulated linux file systems (procfs & sysfs). //So when we are running FreeBSD and our image OS is Linux we modify our mounts to mount the emulated //filesystems. //We do it here since here we have access to both the OCI spec & the image OS. if runtime.GOOS == "freebsd" && ociimage.OS == "linux" { freebsdLinuxEmulationMounts(s) } Copy link Member @samuelkarp samuelkarp May 29, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Carrying over the comment I left on the previous PR: I'd like to see this extracted out into a function like appendOSMounts, which is then separately defined 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. I'd also like to see 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". If you're not interested in doing that work, I can pick up and carry this PR. Sorry, something went wrong. 2 akhramov and emaste reacted with thumbs up emoji All reactions * 2 reactions Copy link Contributor Author @akhramov akhramov May 30, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I'd like to see this extracted out into a function like appendOSMounts That makes a lot of sense, implemented. As for WithLinuxEmulationMounts I'm not that familiar with the codebase yet, so I would defer it to you Sorry, something went wrong. 1 samuelkarp reacted with thumbs up emoji All reactions * 1 reaction Copy link Member @samuelkarp samuelkarp May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment That works for me, I can pick up WithLinuxEmulationMounts in a follow-up PR. Sorry, something went wrong. All reactions platforms/defaults_freebsd.go Outdated // The Variant field will be empty if arch != ARM. Variant: cpuVariant(), }) } Copy link Member @samuelkarp samuelkarp May 29, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment nit: can you add a newline at the end of this file? Sorry, something went wrong. All reactions Copy link Contributor Author @akhramov akhramov May 30, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Nice catch, thanks! Sorry, something went wrong. All reactions @akhramov akhramov force-pushed the freebsd_linux_containers branch from 6726a95 to 6487912 Compare May 30, 2022 samuelkarp samuelkarp reviewed May 31, 2022 View changes oci/spec_opts.go @@ -377,6 +377,7 @@ func WithImageConfigArgs(image Image, args [] string) SpecOpts { return fmt.Errorf("unknown image config media type %s", ic. MediaType) } appendOSMounts(s, ociimage.OS) Copy link Member @samuelkarp samuelkarp May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Looks like the linter is failing here because it's also attempting to lint & compile on MacOS (darwin), where this new function is not defined. My apologies for not realizing that in my original suggestion, but I think you can fix this by either (a) adding a new spec_opts_notfreebsd.go and moving the definition for everything not-FreeBSD there (including Windows and Linux) or (b) additionally adding a spec_opts_darwin.go for MacOS. Sorry, something went wrong. All reactions @akhramov akhramov force-pushed the freebsd_linux_containers branch 2 times, most recently from 92f180e to e72aeaf Compare May 31, 2022 samuelkarp samuelkarp reviewed May 31, 2022 View changes Copy link Member @samuelkarp samuelkarp left a comment Thanks for continuing to work with me on this! I've got a few more questions and suggestions after looking at the code a bit more closely. Sorry, something went wrong. All reactions oci/spec_opts_freebsd.go Outdated Show resolved Hide resolved oci/spec_opts_freebsd.go Outdated Destination: "/proc", Type: "linprocfs", Source: "linprocfs", Options: []string{}, Copy link Member @samuelkarp samuelkarp May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment In the in-place case above, we're adding "nosuid" and "noexec" as options, but in this case we're not. Is there any particular reason for the difference? Sorry, something went wrong. All reactions Copy link Contributor Author @akhramov akhramov May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment No reason, we should have nosuid & noexec in both ases. Sorry, something went wrong. All reactions oci/spec_opts_freebsd.go Outdated mounts = append(mounts, specs.Mount{ Destination: "/sys", Copy link Member @samuelkarp samuelkarp May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Should we handle /sys in-place like we're doing for /proc and /dev/fd too? Sorry, something went wrong. All reactions Copy link Contributor Author @akhramov akhramov May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I agree, but that is less relevant with the new approach I have taken. I will implement this if the approach does not work. Sorry, something went wrong. All reactions oci/spec_opts_freebsd.go Outdated if !haveDevFd { mounts = append(mounts, specs.Mount{ Destination: "/dev/fd", Copy link Member @samuelkarp samuelkarp May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Should we also be adding devfs for /dev? Sorry, something went wrong. All reactions Copy link Contributor Author @akhramov akhramov May 31, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I suspect we already have both from the default mounts: containerd/oci/mounts_freebsd.go Lines 23 to 38 in 78cd9d3 func defaultMounts() []specs.Mount { return []specs.Mount{ { Destination: "/dev", Type: "devfs", Source: "devfs", Options: []string{"ruleset=4"}, }, { Destination: "/dev/fd", Type: "fdescfs", Source: "fdescfs", Options: []string{}, }, } } Sorry, something went wrong. All reactions @akhramov akhramov force-pushed the freebsd_linux_containers branch from a5163d0 to b9fcb18 Compare May 31, 2022 @akhramov Copy link Contributor Author @akhramov akhramov commented May 31, 2022 @samuelkarp I drastically simplified the code. Let me know if I'm missing something. The reasoning is below. --------------------------------------------------------------------- We should already have /dev & /dev/fd as default mounts: containerd/oci/mounts_freebsd.go Lines 23 to 38 in 78cd9d3 func defaultMounts() []specs.Mount { return []specs.Mount{ { Destination: "/dev", Type: "devfs", Source: "devfs", Options: []string{"ruleset=4"}, }, { Destination: "/dev/fd", Type: "fdescfs", Source: "fdescfs", Options: []string{}, }, } } We need to provide /sys, /proc, and /dev/shm per https:// wiki.freebsd.org/LinuxJails and https://github.com/opencontainers/ runtime-spec/blob/1c3f411f041711bbeecf35ff7e93461ea6789220/ config-linux.md#default-filesystems /sys and /proc are backed by their corresponding file-systems, which I include in appendOSMounts /dev/shm is backed by the linuxulator kernel module (albeit the device is not whitelisted by devfs ruleset 4, but that's a separate issue). All reactions Sorry, something went wrong. @gizahNL @samuelkarp @akhramov Linux containers on FreeBSD ... ae22854 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 Co-authored-by: Gijs Peskens , Samuel Karp Signed-off-by: Artem Khramov @akhramov akhramov force-pushed the freebsd_linux_containers branch from b9fcb18 to ae22854 Compare May 31, 2022 kzys kzys approved these changes Jun 1, 2022 View changes platforms/defaults_freebsd.go // Default returns the default matcher for the platform. func Default() MatchComparer { return Ordered(DefaultSpec(), specs.Platform{ OS: "linux", Copy link Member @kzys kzys Jun 1, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Because most containers image are for Linux today? Sorry, something went wrong. All reactions Copy link Member @samuelkarp samuelkarp Jun 1, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment This is ordered with default (FreeBSD) first and Linux as a fallback. Sorry, something went wrong. 1 akhramov reacted with thumbs up emoji All reactions * 1 reaction @dmcgowan dmcgowan added this to Ready For Review in Code Review Jun 1, 2022 @akhramov Copy link Contributor Author @akhramov akhramov commented Jun 4, 2022 Alright, CI seems green It seems like we are good to go. @samuelkarp is there any items I missed? 1 emaste reacted with hooray emoji All reactions * 1 reaction Sorry, something went wrong. @samuelkarp Copy link Member @samuelkarp samuelkarp commented Jun 7, 2022 @akhramov The previous approach modified the /proc and /dev/fd mounts in-place if they existed rather than appending them. Did you change to append unconditionally because you'd expect the mounts at the end of the spec to hide any previous mount to those mountpoint and the hidden mount is irrelevant? All reactions Sorry, something went wrong. @samuelkarp Copy link Member @samuelkarp samuelkarp commented Jun 7, 2022 Aside from the question about mounts, I've tested this and it works! $ sudo ctr run --rm --runtime wtf.sbk.runj.v1 --tty --snapshotter zfs docker.io/library/alpine:latest test sh -c 'cat /etc/os-release && uname -a' NAME="Alpine Linux" ID=alpine VERSION_ID=3.16.0 PRETTY_NAME="Alpine Linux v3.16" HOME_URL="https://alpinelinux.org/" BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues" Linux 3.17.0 FreeBSD 13.1-RELEASE releng/13.1-n250148-fc952ac2212 GENERIC x86_64 Linux 2 tianon and trasz reacted with hooray emoji [?] 1 tianon reacted with heart emoji 1 tianon reacted with rocket emoji All reactions * 2 reactions * [?] 1 reaction * 1 reaction Sorry, something went wrong. @akhramov Copy link Contributor Author @akhramov akhramov commented Jun 7, 2022 @samuelkarp Did you change to append unconditionally because you'd expect the mounts at the end of the spec to hide any previous mount to those mountpoint and the hidden mount is irrelevant? Not really, but it is up to the runtime (us) to create mountpoints at these locations. Since we don't have these as default mountpoints, I don't expect them to exist at all. Uh, I hope I'm not missing anything. All reactions Sorry, something went wrong. @samuelkarp Copy link Member @samuelkarp samuelkarp commented Jun 7, 2022 * edited Since we don't have these as default mountpoints, I don't expect them to exist at all. Mounts can be arbitrarily defined in the runtime configuration which is an argument to the functional option returned by WithImageConfigArgs (see the signature func(ctx context.Context, client Client, c *containers.Container, s *Spec) error; the s *Spec is an arbitrary configuration which may already have mounts defined). (It's worth keeping in mind that containerd's primary interface/ first-class interface is the thick client library of which this is a part, which then interacts with the daemon over a gRPC API. Clients like ctr and firecracker-containerd and so forth then use this library.) All reactions Sorry, something went wrong. @akhramov Copy link Contributor Author @akhramov akhramov commented Jun 7, 2022 @samuelkarp got it, thanks. So what should happen if a client overrides a default mountpoint? All reactions Sorry, something went wrong. @samuelkarp Copy link Member @samuelkarp samuelkarp commented Jun 7, 2022 A client can choose which functional options to apply (including ones it supplies itself) and can also directly manipulate the object; ultimately the *Spec at the end of those functional options being applied is what gets sent to the underlying runtime (typically runc on Linux and runj on FreeBSD). The question here is: what behavior is appropriate for this functional option? I think it's probably fine for it to append-only and cause any prior mounts at that mountpoint to be shadowed (and on Linux I'd be reasonably confident of the behavior), but I'm not sure if there are footguns I don't know about on FreeBSD (and I think you know more than me). Specific areas I'd be concerned about include performance and what the behavior might be if a shadowed mount was a network filesystem that went away during the lifetime of the container. All reactions Sorry, something went wrong. @akhramov Copy link Contributor Author @akhramov akhramov commented Jun 8, 2022 @samuelkarp I think the shadowing behavior is pretty much the same on FreeBSD as on Linux. @emaste, can you kindly confirm? All reactions Sorry, something went wrong. samuelkarp samuelkarp approved these changes Jun 8, 2022 View changes Copy link Member @samuelkarp samuelkarp left a comment Thanks @akhramov! If the shadowing behavior is roughly the same, I think we can go ahead and merge this and adjust to the more-complicated replace-in-place logic if we discover it being a problem. LGTM! Sorry, something went wrong. All reactions @samuelkarp samuelkarp added ok-to-test and removed needs-ok-to-test labels Jun 8, 2022 @emaste Copy link @emaste emaste commented Jun 8, 2022 @samuelkarp I think the shadowing behavior is pretty much the same on FreeBSD as on Linux. @emaste, can you kindly confirm? That is my expectation. I asked some folks who would know for certain, but haven't heard back. 1 akhramov reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. @samuelkarp Copy link Member @samuelkarp samuelkarp commented Jun 9, 2022 @emaste Thanks! @akhramov and @gizahNL Thank you so much for working on this and pushing through! Now that containerd/project#90 is approved I'll go ahead and merge this myself. 4 akhramov, mikebrow, tianon, and nindanaoto reacted with hooray emoji All reactions * 4 reactions Sorry, something went wrong. Hide details View details @samuelkarp samuelkarp merged commit 2b4b0cf into containerd:main Jun 9, 2022 36 checks passed Code Review automation moved this from Ready For Review to Done Jun 9, 2022 @samuelkarp samuelkarp added impact/changelog kind/feature platform/ freebsd FreeBSD labels Jun 9, 2022 decke added a commit to decke/ports that referenced this issue Jun 11, 2022 @decke sysutils/containerd: Add patch from upstream to support Linux contain... ... 6517366 ...ers on FreeBSD Obtained from: containerd/containerd#7000 CI: yes freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Jun 11, 2022 @decke sysutils/containerd: Add patch to support Linux containers on FreeBSD ... fa5e3e5 Obtained from: containerd/containerd#7000 Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment Reviewers @kzys kzys @samuelkarp samuelkarp Assignees No one assigned Labels impact/changelog kind/feature ok-to-test platform/freebsd FreeBSD Projects Code Review Done Milestone No milestone Development Successfully merging this pull request may close these issues. None yet 6 participants @akhramov @k8s-ci-robot @samuelkarp @emaste @kzys @gizahNL Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. * (c) 2022 GitHub, Inc. * Terms * Privacy * Security * Status * Docs * Contact GitHub * Pricing * API * Training * Blog * About You can't perform that action at this time. You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.