-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Reorder executor lookupBin ordering #5511
Conversation
defer executor.Shutdown("", 0) | ||
|
||
// need to configure path in chroot with that file if using isolation executor | ||
if _, ok := executor.(*UniversalExecutor); !ok { |
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 sure how to best customize test for executors. I could have done a string matching against LibcontainerExecutor
name instead (maybe with use of const) but couldn't check against LibcontainerExecutor
in macOS where type isn't defined.
// as exec.LookPath only considers files already marked as executable | ||
// and only consider this for absolute paths to avoid depending on | ||
// current directory of nomad which may cause unexpected behavior | ||
if _, err := os.Stat(bin); err == nil && filepath.IsAbs(bin) { |
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.
I considered allowing relative paths containing /
that don't require search through PATH; however, I felt that reliance on nomad process current directory can be risky and uninuitive. Given that this is a fix for a bug in 0.8, I felt it's fine to start with absolute paths first, then support other types of path if it's an issue.
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.
LGTM - the way you broke it down and the links to failed tests made it easy to follow.
Thanks! 🙇 |
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.
Amazing analysis. Thanks for digging in!
nonExecutablePath := filepath.Join(tmpDir, "nonexecutablefile") | ||
ioutil.WriteFile(nonExecutablePath, | ||
[]byte("#!/bin/sh\necho hello world"), | ||
0600) |
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 is a great approach!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is a follow up work to #4813 to fix #4809 and fix a regression introduced in 0.9 in marking files in libcontainer executable.
#4809 bug is that
lookupBin
usesexec.LookPath
when not inspecting task dir files.exec.LookPath
only returns a file if it's already marked as an executable path in https://github.com/golang/go/blob/go1.12.1/src/os/exec/lp_unix.go#L24-L27 . This affects raw exec as if passed an absolute path to file,lookupBin
returns an error if file isn't already an executable. This explains why the error manifests when an absolute interpolated path is used (e.g.${NOMAD_TASK_DIR}/hellov1
) but not when using a task rel dir (e.g.local/hellov1
) in the above examples used in ticket.PR #4813 remedied this problem for raw exec but inadvertably broke libcontainer executor, as it made
lookupBin
returns the paths to host files rather than ones found inside chroot.This PR reorders the evaluation, so we go back to 0.8 behavior of looking up task directories first, but then check for host paths before using
exec.LookPath
.This PR is broken into three commits to illustrate evolution and confirming hypothesis:
exec.LookPath(...)
for absolute paths and have a green job in https://travis-ci.org/hashicorp/nomad/jobs/514945024Future work
Inspecting
lookupBin
in 0.8 and 0.9 case, we have a bug in usingexec.LookPath
for the libcontainer executor case. We should be looking up paths based on the container chroot and container PATH rather than the host's. However, this is not a 0.9.0 regression and was present in 0.8; so punting to fix it post 0.9.