-
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
client: support no_pivot_root in exec driver configuration #7149
Conversation
d9a3df2
to
0d8ea8e
Compare
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.
Thank you so much for your contribution. I love that the flag is a plugin flag rather than a task flag: this means that it can be controlled by nomad system administrator rather than have it be a developer concern. Also, good catch in updating executor proto and related client/server files.
I have a couple of suggestions inlined; I would love to add an integration test as well. Given current structure, it might be difficult to assert that executor does actually honor NoPivotRoot, but it's ok - a simple fully integration test is ok imo.
drivers/exec/driver.go
Outdated
@@ -88,6 +93,9 @@ type Driver struct { | |||
// event can be broadcast to all callers | |||
eventer *eventer.Eventer | |||
|
|||
// config is the driver configuration set by the SetConfig RPC | |||
config *Config |
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.
d.config
might be nil in tests - hence one test failure. You may consider avoid using the pointer here.
config *Config | |
config Config |
repeated hashicorp.nomad.plugins.drivers.proto.Mount mounts = 11; | ||
repeated hashicorp.nomad.plugins.drivers.proto.Device devices = 12; | ||
hashicorp.nomad.plugins.drivers.proto.NetworkIsolationSpec network_isolation = 13; | ||
bool no_pivot_root = 11; |
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 would insert append this to the end as item 14. For backward compatibility, field numbers don't change once they are assigned.
e645242
to
56466e7
Compare
I added a test and addressed your comments @notnoop, let me know if that looks good and thanks again for the feedback! |
All checks are green, looks like the |
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.
Perfect, thank you so much. I made a minor doc suggestion but it's up to you - will merge on Monday.
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.
Documentation changes look good
56466e7
to
5a017ac
Compare
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. |
Adds a new
no_pivot_root
plugin option to theexec
plugin.It defaults to
false
, changing it totrue
will pass theNoPivotRoot
configuration option tolibcontainer
which will fall back to using themsMoveRoot
function for isolation.This is useful for systems where the root is on a ramdisk.
Fixes #7136