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 containerd_binary to shim config #2922

Closed
wants to merge 1 commit into from

Conversation

andrewrynhard
Copy link

@andrewrynhard andrewrynhard commented Jan 12, 2019

When using musl and gcompat to run containerd, the shim command has the value /lib/libc.so for the -containerd-binary flag. This is due to the use of os.Executable when building the shim command. This PR introduces a new item in the shim config (containerd_binary) to set the path to containerd explicitly.

See https://github.com/AdelieLinux/gcompat/blob/master/loader/loader.c#L75-L81 for details on why it is this way with gcompat.

@andrewrynhard andrewrynhard force-pushed the binary-path branch 2 times, most recently from 5637b69 to d338409 Compare January 12, 2019 04:43
@andrewrynhard andrewrynhard changed the title [WIP] Add containerd_binary_path to shim config [WIP] Add containerd_binary to shim config Jan 12, 2019
Signed-off-by: Andrew Rynhard <andrew@andrewrynhard.com>
@codecov-io
Copy link

Codecov Report

Merging #2922 into master will decrease coverage by 2.77%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2922      +/-   ##
==========================================
- Coverage   44.19%   41.41%   -2.78%     
==========================================
  Files         101       70      -31     
  Lines       10808     9457    -1351     
==========================================
- Hits         4777     3917     -860     
+ Misses       5297     4979     -318     
+ Partials      734      561     -173
Flag Coverage Δ
#linux ?
#windows 41.41% <ø> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/native/native.go 1.78% <0%> (-41.52%) ⬇️
archive/tar.go 16.99% <0%> (-26.8%) ⬇️
metadata/snapshot.go 21.53% <0%> (-24.28%) ⬇️
content/local/writer.go 56.86% <0%> (-0.99%) ⬇️
gc/scheduler/scheduler.go 66.34% <0%> (-0.97%) ⬇️
oci/spec_opts.go 26.8% <0%> (-0.27%) ⬇️
mount/temp_unix.go
sys/reaper_linux.go
services/server/server_linux.go
sys/env.go
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adfaa69...141aafa. Read the comment docs.

@andrewrynhard andrewrynhard changed the title [WIP] Add containerd_binary to shim config Add containerd_binary to shim config Jan 14, 2019
@Ace-Tang
Copy link
Contributor

Ace-Tang commented Jan 17, 2019

Do you mean When using musl and gcompat to run containerd, should set -containerd-binary /usr/bin/contaianerd, since when execute os.Executable() in containerd, it will get value /lib/libc.so, not real containerd path ?

@andrewrynhard
Copy link
Author

andrewrynhard commented Jan 17, 2019

@Ace-Tang correct.

@crosbymichael
Copy link
Member

Just wondering, why do you think this is a containerd issue that should be fixed within our codebase and not something wrong with musl and gcompat?

@andrewrynhard
Copy link
Author

andrewrynhard commented Jan 17, 2019

@crosbymichael I don't neccessarily think it is a containerd issue, but I thought it was reasonable to add this config option considering that the golang standard library warns here that:

Executable returns the path name for the executable that started the current process. There is no guarantee that the path is still pointing to the correct executable.

and that nearly all the other flags to the shim can be supplied via the config.

@crosbymichael
Copy link
Member

@andrewrynhard thanks. I'm still thinking on this...

My initial thought would be not to add this for this specific issue. It's kinda a one off flag for this specific environment. Maybe people that have more context on musl like @justincormack or other maintainers could give some feedback first.

@estesp
Copy link
Member

estesp commented Jan 17, 2019

I think the issue is that we are relying on the Go runtime reporting what we expect it to; that comment from gcompat clearly shows in that environment the /proc/self/exe is not reliable for the binary actually being executed. I assume there are other LD_PRELOAD-like use cases where this might happen as well (where the loader will show up, not the effective binary name).

A long shot (and something that would take a cycle or two of Go releases) would be to get gcompat awareness into Go's os package. Until then I'm not sure what other workarounds there can be other than allowing these situations to have a way to say "no, the binary is really this..Go isn't going to report it correctly"

@crosbymichael
Copy link
Member

@estesp go just reports /proc/self/exe which is pretty standard for linux. From what I understand, gcompat is the actual binary launching things, therefore, its causing issues. It does not look like gcompat is go releated, but musl related so I doubt anything will change in the os package.

/proc/self/exe works well for 99% of the usecases. This seems to be like a 0.1% usecase :p

@estesp
Copy link
Member

estesp commented Jan 17, 2019

Actually, now I'm curious to back up a bit. LinuxKit seems to use alpine with containerd + runc without any need for glibc compat layers; at least the package list doesn't reference anything that would make me think that; so that means containerd is compiled with musl-libc.

Is there a reason containerd is being used with a glibc compat layer in this instance?

(Edit: Just for clarity; here is the containerd build step in Alpine, with the compiler and other deps being added, but no glibc from what I can tell: https://github.com/linuxkit/linuxkit/blob/master/tools/alpine/Dockerfile#L56-L65)

@andrewrynhard
Copy link
Author

andrewrynhard commented Jan 17, 2019

It is indeed compiled: https://github.com/linuxkit/linuxkit/blob/eb7e07542f839961b4611ee10f86f524ec9c39e9/tools/alpine/Dockerfile#L53-L65

I would like to avoid having to build containerd. That would also require building golang to run in the musl env I am working on. Maybe I could run golang using gcompat (will try), but I would still like to leverage the officially built binaries and use gcompat as a compatibility layer.

EDIT: I didn't refresh and missed the above edit.

@crosbymichael
Copy link
Member

Do we have glibc binaries compiles and downloadable on the release page of this repo.

@estesp
Copy link
Member

estesp commented Jan 17, 2019

Yes, for all recent releases we have linux/amd64 binaries compiled with glibc, and 1.2.2 has a darwin (macOS) binary as well. The latest Docker release also ships a containerd package on Ubuntu that is glibc-based, and the CRI team publishes all binary releases (glibc-based) on a google storage bucket.

@crosbymichael
Copy link
Member

Lets close this for now.

We do offer binaries for libc and we do have a pretty clean build pipeline if you did need to compile containerd yourself, just type make.

Feel free to ask anymore questions because we are happy to help. We know the pains that musl gives us all ;)

@estesp
Copy link
Member

estesp commented Jan 17, 2019

Long term option for musl-libc official binaries: looks like Alpine will have containerd as a community package in the next release, v3.9 (@ncopa can correct me if necessary); but looking at the main mirror in the v3.9 dir I see containerd-1.2.2 and runc-1.0.0-rc6 as APKs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants