Add volumes config to LXC driver #3687
Conversation
|
The failing check looks like a Travis-only error. I was unable to reproduce it locally. |
client/driver/lxc.go
Outdated
| @@ -250,6 +270,13 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |||
| fmt.Sprintf("%s alloc none rw,bind,create=dir", ctx.TaskDir.SharedAllocDir), | |||
| fmt.Sprintf("%s secrets none rw,bind,create=dir", ctx.TaskDir.SecretsDir), | |||
| } | |||
|
|
|||
| for _, volDesc := range driverConfig.Volumes { | |||
There was a problem hiding this comment.
Please guard this in a similar fashion to how it is done in Docker: https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L85
https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L496
https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go#L1003
client/driver/lxc.go
Outdated
|
|
||
| for _, volDesc := range driverConfig.Volumes { | ||
| // the format was checked in Validate() | ||
| paths := strings.Split(volDesc, ":") |
|
My apologies if overwriting the first commit made the review hard to go back to. Let me know if there's a workflow you prefer to follow for Nomad. |
Nope! That's fine! |
client/driver/lxc_test.go
Outdated
| []string{ | ||
| ":def", | ||
| }, | ||
| []string{ |
There was a problem hiding this comment.
Tests are failing because our linter prefers the simplified version of this code. You can fix it with either gofmt -s lxc_test.go or just by using this:
brokenVolumeConfigs := [][]string{
{
"foo:/var",
},
{
":",
},
{
"abc:",
},
{
":def",
},
{
"abc:def:ghi",
},
}A bit of a silly code style quibble, but it keeps our code consistent and linters happy!
|
Hi, just checking if there's anything else you need me to do here. Thanks! |
|
|
||
| } | ||
|
|
||
| func TestLxcDriver_Start_NoVolumes(t *testing.T) { |
There was a problem hiding this comment.
It would be nice to get one that tests actually mounting a volume
|
@mikemccracken Everything looks great. I would just like to see a test actually mount a volume |
Allow lxc driver to accept bind mount config similarly to the docker driver. Includes some static sanity checks in Validate step Signed-off-by: Michael McCracken <[email protected]>
Signed-off-by: Michael McCracken <[email protected]>
Signed-off-by: Michael McCracken <[email protected]>
Signed-off-by: Michael McCracken <[email protected]>
Signed-off-by: Michael McCracken <[email protected]>
Ensure that bind mounting via the volumes config really did work. Signed-off-by: Michael McCracken <[email protected]>
mikemccracken
force-pushed
the
add-lxc-bindmounts
branch
from
7f95551 to
561376e
Compare
|
Rebased onto master, and added an extra check: The original changes to |
|
@mikemccracken Really great PR! Thank you |
|
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. |
Allow lxc driver to accept bind mount config similarly to the docker
driver.
Includes some static sanity checks in Validate step
Signed-off-by: Michael McCracken [email protected]