Fix mounts for FreeBSD #5472

gizahNL · 2021-05-10T14:58:43Z

Move the default mounts definition to platform specific function & implement the correct mounts definition for FreeBSD

k8s-ci-robot · 2021-05-10T14:58:53Z

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.

estesp · 2021-05-10T15:00:44Z

All files in containerd need the proper copyright/license header; this checked by CI so you will need update the commit for the 2 new files. Thanks!

gizahNL · 2021-05-10T15:10:39Z

All files in containerd need the proper copyright/license header; this checked by CI so you will need update the commit for the 2 new files. Thanks!

Will do, thx!
I also figured this likely will not build on Windows, so I'll have to rework for that.

theopenlab-ci · 2021-05-10T15:45:12Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 10s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 27s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 23m 02s (non-voting)

dmcgowan · 2021-05-10T17:00:06Z

Thanks for the contribution! Please sign your commit using git commit -s, see https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work

Signed-off-by: Gijs Peskens <[email protected]>

theopenlab-ci · 2021-05-10T20:14:30Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 44s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 20s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 22m 46s (non-voting)

estesp · 2021-05-10T20:38:03Z

/ok-to-test

samuelkarp · 2021-05-13T07:52:38Z

oci/mounts_freebsd.go

+		{
+			Destination: "/dev/fd",
+			Type:        "fdescfs",
+			Source:      "fdescfs",
+			Options:     []string{},
+		},
+		{
+			Destination: "/dev/mqueue",
+			Type:        "mqueue",
+			Source:      "mqueue",
+			Options:     []string{"nosuid", "noexec"},
+		},
+		{
+			Destination: "/dev/shm",
+			Type:        "tmpfs",
+			Source:      "shm",
+			Options:     []string{"nosuid", "noexec", "mode=1777"},
+		},


I admit to being very much a newbie with FreeBSD, but I'm wondering whether these are the right defaults. On my FreeBSD host (outside a jail) I only have devfs mounted at /dev. /dev/fd appears to be provided by devfs, and the same is true in the jails that I've set up with runj. I don't have /dev/shm or /dev/mqueue at all on my host either.

$ mount zroot/ROOT/default on / (zfs, local, noatime, nfsv4acls) devfs on /dev (devfs, local, multilabel) zroot/tmp on /tmp (zfs, local, noatime, nosuid, nfsv4acls) zroot/usr/home on /usr/home (zfs, local, noatime, nfsv4acls) zroot/var/log on /var/log (zfs, local, noatime, noexec, nosuid, nfsv4acls) zroot on /zroot (zfs, local, noatime, nfsv4acls) zroot/usr/ports on /usr/ports (zfs, local, noatime, nosuid, nfsv4acls) zroot/var/audit on /var/audit (zfs, local, noatime, noexec, nosuid, nfsv4acls) zroot/usr/src on /usr/src (zfs, local, noatime, nfsv4acls) zroot/var/crash on /var/crash (zfs, local, noatime, noexec, nosuid, nfsv4acls) zroot/var/mail on /var/mail (zfs, local, nfsv4acls) zroot/var/tmp on /var/tmp (zfs, local, noatime, nosuid, nfsv4acls) zroot/containerd on /var/lib/containerd/io.containerd.snapshotter.v1.zfs (zfs, local, noatime, nfsv4acls) zroot/containerd/44 on /run/containerd/io.containerd.runtime.v2.task/default/test2/rootfs (zfs, local, noatime, nfsv4acls) devfs on /run/containerd/io.containerd.runtime.v2.task/default/test2/rootfs/dev (devfs, local, multilabel)

The man page for fdescfs answers at least the question of /dev/fd being provided by devfs, but I'd still like to understand more about whether these are the appropriate defaults.

Looking at my jails (setup via cbsd), I have fdescfs mounted for all of them, the other mounts are indeed not present.

/dev/shm is a Linuxism, and I should remove that, my first intention was to make sure mount doesn't explode when it tries to mount filesystems that are not native to FreeBSD. /dev/mqueue is also save to remove I'd say, it doesn't seem to be mounted anywhere either.

I'll fixup tonight.

Looking at my jails (setup via cbsd), I have fdescfs mounted for all of them, the other mounts are indeed not present.

Should fdescfs be present by default though? It doesn't seem to be mounted by default in the FreeBSD host I've been using that I installed without much customization.

According to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216985 and FreeBSD's ports, bash and OpenJDK need fdescfs.

So questions are;

Do we want to provide a minimal set or good enough default to run common programs?

Are bash and/or OpenJDK considered "common" to mount fdescfs by default?

bash and OpenJDK needing fdescfs sounds like a good reason to mount it by default to me.

Signed-off-by: Gijs Peskens <[email protected]>

theopenlab-ci · 2021-05-13T20:00:57Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 48s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 31s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 21m 36s (non-voting)

samuelkarp

~~LGTM~~ (See note below)

One of the committers will need to hit the approve button so CI can run. I also haven't implemented mount in runj yet (though I see you did in your fork) so I haven't been able to test it.

samuelkarp · 2021-05-23T07:05:13Z

Integration test failures appear to be unrelated.

samuelkarp · 2021-05-24T07:42:23Z

oci/mounts_freebsd.go

+			Destination: "/dev",
+			Type:        "devfs",
+			Source:      "devfs",
+			Options:     []string{},


I've been using ruleset=4 in runj so far. A somewhat more-restrictive default might be useful here and the jail(8) manual page suggests using 4.

It has been my intention to pass devfs rule options in a (yet to be defined) OCI specification for FreeBSD.
ruleset=4 sounds like a safe default for the meantime though :)

It's not completely relevant for this PR, but I'm curious to know why you are thinking of embedding devfs rule options in a FreeBSD section rather than mount options. Were you planning to allow for full ruleset definitions rather than referencing existing rulesets?

(Note: dismissing previous LGTM to track the addition of the default ruleset option prior to merging.)

Yes allowing to build a complete devfs ruleset has been the intention.
FreeBSD allows the ruleset to be specified at mount time, but also allows later modifications of a devfs mountpoint via the CLI devfs tool, even changing the ruleset (rulesets in essence basically just being a series of show and hide commands).
So I'd think having a node in a FreeBSD OCI spec that allows specification of a ruleset & a list of actions to take on the dev mountpoint would be usefull.

samuelkarp

(Note: dismissing previous LGTM to track the addition of the default ruleset option prior to merging.)

samuelkarp · 2021-05-24T16:32:38Z

oci/mounts_freebsd.go

+			Destination: "/dev",
+			Type:        "devfs",
+			Source:      "devfs",
+			Options:     []string{},


(Note: dismissing previous LGTM to track the addition of the default ruleset option prior to merging.)

samuelkarp · 2021-05-25T06:58:41Z

I've now implemented mount support in runj and tested this change. I'd still like to see the ruleset=4 option included in the default devfs mount, but otherwise this change is looking good.

Signed-off-by: Gijs Peskens <[email protected]>

gizahNL · 2021-05-25T07:18:13Z

I've now implemented mount support in runj and tested this change. I'd still like to see the ruleset=4 option included in the default devfs mount, but otherwise this change is looking good.

done.

theopenlab-ci · 2021-05-25T07:19:05Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-test-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-integration-test-arm64 : NODE_FAILURE in 0s (non-voting)

samuelkarp

LGTM

kzys · 2021-05-25T17:27:09Z

@estesp Can you hit the approve button to run GitHub Actions?

One of the committers will need to hit the approve button so CI can run. I also haven't implemented mount in runj yet (though I see you did in your fork) so I haven't been able to test it.

Neither @samuelkarp nor I can do that.

estesp

LGTM

k8s-ci-robot added the needs-ok-to-test label May 10, 2021

gizahNL added 2 commits May 10, 2021 21:49

Fix mounts for FreeBSD

e1fd6be

Signed-off-by: Gijs Peskens <[email protected]>

Add copyright header & make sure compilation succeeds on all platforms

de04b32

Signed-off-by: Gijs Peskens <[email protected]>

gizahNL force-pushed the freebsd_mounts branch from 0ef645a to de04b32 Compare May 10, 2021 19:50

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 10, 2021

kzys approved these changes May 10, 2021

View reviewed changes

AkihiroSuda requested a review from samuelkarp May 11, 2021 01:55

gizahNL mentioned this pull request May 11, 2021

Freebsd linux containers #5480

Closed

AkihiroSuda added the platform/freebsd FreeBSD label May 11, 2021

samuelkarp reviewed May 13, 2021

View reviewed changes

Remove mountpoints not commonly mounted on FreeBSD

1442fee

Signed-off-by: Gijs Peskens <[email protected]>

samuelkarp approved these changes May 22, 2021

View reviewed changes

samuelkarp reviewed May 24, 2021

View reviewed changes

samuelkarp requested changes May 24, 2021

View reviewed changes

Add ruleset=4 option

a4f97d4

Signed-off-by: Gijs Peskens <[email protected]>

samuelkarp approved these changes May 25, 2021

View reviewed changes

kzys approved these changes May 25, 2021

View reviewed changes

estesp approved these changes May 25, 2021

View reviewed changes

estesp merged commit ba4fa32 into containerd:master May 25, 2021

Fix mounts for FreeBSD #5472

Fix mounts for FreeBSD #5472

Uh oh!

Conversation

gizahNL commented May 10, 2021

Uh oh!

k8s-ci-robot commented May 10, 2021

Uh oh!

estesp commented May 10, 2021

Uh oh!

gizahNL commented May 10, 2021

Uh oh!

theopenlab-ci bot commented May 10, 2021

Uh oh!

dmcgowan commented May 10, 2021

Uh oh!

theopenlab-ci bot commented May 10, 2021

Uh oh!

estesp commented May 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelkarp May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented May 13, 2021

Uh oh!

samuelkarp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelkarp commented May 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gizahNL May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelkarp commented May 25, 2021

Uh oh!

gizahNL commented May 25, 2021

Uh oh!

theopenlab-ci bot commented May 25, 2021

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented May 25, 2021

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

samuelkarp May 14, 2021 •

edited

Loading

samuelkarp left a comment •

edited

Loading

gizahNL May 24, 2021 •

edited

Loading