-
Notifications
You must be signed in to change notification settings - Fork 642
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
[#137] FreeBSD support #361
[#137] FreeBSD support #361
Conversation
1ca61e2
to
14b0025
Compare
Thanks for working on this. Could you update Lines 83 to 84 in 95dd996
Also please add Eventually we should have CI, but it can be another PR if it is difficult. |
cmd/nerdctl/main_freebsd.go
Outdated
return | ||
} | ||
|
||
func bashCompleteNamespaceNames(clicontext *cli.Context) { |
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.
This code shouldn't be OS-dependent
cmd/nerdctl/main_freebsd.go
Outdated
return | ||
} | ||
|
||
func bashCompleteSnapshotterNames(clicontext *cli.Context) { |
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.
This code shouldn't be OS-dependent
cmd/nerdctl/run.go
Outdated
WithoutRunMount(), // unmount default tmpfs on "/run": https://github.com/containerd/nerdctl/issues/157 | ||
) | ||
|
||
if runtime.GOOS != "freebsd" { |
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.
nit: runtime.GOOS == "linux"
would be more accurate check
cmd/nerdctl/run_freebsd.go
Outdated
} | ||
|
||
func WithoutRunMount() func(ctx context.Context, client oci.Client, c *containers.Container, s *oci.Spec) error { | ||
// not valid on windows |
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.
not windows
cmd/nerdctl/run_freebsd.go
Outdated
) | ||
|
||
func runBashComplete(clicontext *cli.Context) { | ||
// noop |
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.
This shouldn't be noop, but can be deferred to another PR
pkg/defaults/defaults_freebsd.go
Outdated
gocni "github.com/containerd/go-cni" | ||
) | ||
|
||
const AppArmorProfileName = "nerdctl-default" |
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.
This should be empty.
Eventually this variable should be only referred from *_linux.go
, though.
pkg/mountutil/mountutil_freebsd.go
Outdated
|
||
// DefaultPropagationMode is the default propagation of mounts | ||
// where user doesn't specify mount propagation explicitly. | ||
// See also: https://github.com/moby/moby/blob/v20.10.7/volume/mounts/windows_parser.go#L440-L442 |
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.
not windows
Probably we need to update nerdctl/cmd/nerdctl/run_runtime.go Line 44 in 95dd996
wtf.sbk.runj.v1
|
cc @samuelkarp |
4e87563
to
856eaf8
Compare
@@ -747,7 +752,7 @@ func withCustomHosts(src string) func(context.Context, oci.Client, *containers.C | |||
} | |||
|
|||
func generateLogURI(dataStore string) (*url.URL, error) { | |||
selfExe, err := os.Readlink("/proc/self/exe") |
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.
@AkihiroSuda This seems to be os-specific. is -d
broken now on windows?
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.
nerdctl run
on Windows is completely WIP (help wanted)
#197
.github/workflows/test.yml
Outdated
@@ -161,3 +161,21 @@ jobs: | |||
command: ssh default -- "CONTAINERD_SNAPSHOTTER=fuse-overlayfs /vagrant/nerdctl.test -test.v -test.kill-daemon" | |||
- name: "Uninstall rootless containerd" | |||
run: ssh default -- containerd-rootless-setuptool.sh uninstall | |||
|
|||
test-unit-freebsd-amd64: |
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.
@AkihiroSuda Instead of using Cirrus, we can run FreeBSD jobs in GH actions this way. What do you think?
@AkihiroSuda thanks for the thorough review. I'm going to focus on resolving the crosscompilation issue. Also I would like to hear your input on doc -- I wasn't sure what to put there. |
1aa2823
to
55768c3
Compare
55768c3
to
327b973
Compare
.github/workflows/test.yml
Outdated
ln -sf hack/Vagrantfile Vagrantfile | ||
vagrant up ${{ matrix.box }} | ||
- name: "Run unit tests" | ||
run: vagrant ssh ${{ matrix.box }} -- "cd /vagrant; go test -v ./pkg/..." |
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.
Would is be possible to run nerdctl run SOME-IMAGE echo hello
here?
cmd/nerdctl/run_freebsd.go
Outdated
|
||
// executablePath returns the absolute path to the current binary | ||
func executablePath() (string, error) { | ||
return filepath.Abs(os.Args[0]) |
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.
42886b4
to
e69cc74
Compare
vagrant up seems extremely flaky. |
e69cc74
to
99274ad
Compare
@AkihiroSuda indeed it's flaky. I'm sorry for the notifications mess I caused. I mitigated the problem using the cache action. Let's test that, otherwise I'll set up a Cirrus pipeline. |
99274ad
to
2af44af
Compare
with: | ||
path: ~/.vagrant.d/boxes | ||
key: ${{ runner.os }}-vagrant-${{ hashFiles('Vagrantfile') }} | ||
restore-keys: | |
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.
Why is this different from key?
hack/Vagrantfile.freebsd13
Outdated
config.vm.provision "shell", inline: <<-SHELL | ||
pkg bootstrap | ||
pkg install -y go containerd runj | ||
cp /usr/local/bin/containerd-shim-runj-v1 /usr/local/bin/containerd-shim-runc-v2 |
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.
This looks slightly invalid.
I’d suggest just setting the default runtime to runj
7521254
to
b0ad974
Compare
Now that containerd supports FreeBSD, we can port nerdctl, too! :rocket:. For full-fledged functionality, we will need to port * buildkit. Status: POC is ready, need to upstream fixes to dependencies. * runtime + containerd shim. runj/knast should work just fine. * CNI bridge plugin for networking. This change fixes FreeBSD compilation errors by renaming linux files to unix where necessary, or otherwise introducing FreeBSD versions for the necessary files. Signed-off-by: Artem Khramov <[email protected]>
b0ad974
to
d93be27
Compare
Signed-off-by: Akihiro Suda <[email protected]>
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.
Pushed follow-up commits.
LGTM if green.
Thanks!
Signed-off-by: Akihiro Suda <[email protected]>
01a3010
to
fef5ff2
Compare
CI seems flaky. Can be fixed later, though
|
Hi Team, Any update on the FreeBSD support on ContainerD/nerdctl? |
@leandroscardua |
Thank you for the information @gmshake |
Now that containerd supports FreeBSD, we can port nerdctl, too!
🚀. For full-fledged functionality, we will need to port
dependencies.
This change fixes FreeBSD compilation errors by renaming linux files
to unix where necessary, or otherwise introducing FreeBSD versions for
the necessary files.
Signed-off-by: Artem Khramov [email protected]