-
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
Added stub / spec for executor interface #36
Conversation
@cbednarski Seems like a solid spec to use as a starting point |
Could we support the idea of wrapping these? I may be overcomplicating things EDIT: I'm not actually sure this would even work, but wanted to throw it out there |
// ones should be at the top and fallbacks at the bottom. | ||
// TODO refactor this to be more lightweight. | ||
executors := []Executor{ | ||
&LinuxExecutor{}, |
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 don't think this will compile on non-Linux, since this type is defined in a _linux.go
file. Maybe instead the executors should register in somehow.
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.
Ah, I will look into this. Registration makes things a bit more complicated because the write is racy and/or I have to reflect to copy the type from the registry (if you have a good example please link me!).
Here each goroutine gets its own (new) resource. The solution I have now is not elegant but it's simple and thread safe so if there is a compiler issue I will try to fix that for now.
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 was thinking more like having a type ExecutorFactory func() Executor
, then each executor could just use like it's init()
method to add its own factory in. Or something like that. Whatever is easiest, I suppose.
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.
Ah I see. That makes sense. Thanks!
8d8bda5
to
a5512c2
Compare
Changed Default executor to use a factory
I've tested this via the exec and java drivers and everything looks good. I'd like to add some more local tests but this is technically tested already. Also exec now works as non-root so this should not break running tests during development. @catsby I'm going to merge this as-is but feel free to tweak if you need to for the |
Added executor interface, includes drop privileges for linux
Fix typos in README
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 very basic but I think will give us a way to write a ulimit executor, cgroup executor, lxc executor, etc. This allows the drivers to be more OS agnostic and also lets us handle fiddly switching logic based on capabilities in the exec package instead of mixing it into the driver code where it gets mixed in with other logic.
Ideally exec stays low level and close to the OS, while the driver can assume that "run this thing with these constraints" just works using best-available techniques on the target platform.
/cc @catsby
Fixes #20