Use docker's new mounts API instead of Binds#1563
Conversation
cpuguy83
force-pushed
the
use_docker_mounts_api
branch
from
24f863f to
c2b9963
Compare
Codecov Report
@@ Coverage Diff @@
## master #1563 +/- ##
==========================================
+ Coverage 54.64% 54.68% +0.04%
==========================================
Files 114 114
Lines 19679 19688 +9
==========================================
+ Hits 10754 10767 +13
+ Misses 7646 7635 -11
- Partials 1279 1286 +7Continue to review full report at Codecov.
|
cpuguy83
force-pushed
the
use_docker_mounts_api
branch
from
c2b9963 to
2efe868
Compare
| } | ||
| case enginemount.TypeBind: | ||
| t.Logf("expected bind opts: %+v, got: %+v", c.x.BindOptions, mounts[0].BindOptions) | ||
| } |
There was a problem hiding this comment.
maybe:
default:
t.Logf("unknown type %+v", c.x.Type)There was a problem hiding this comment.
Type will be in the log call above.
This just makes it so the person running the test can see the actual details of these structs instead of a memory address.
|
LGTM |
agent/exec/container/container.go
Outdated
| case api.MountTypeVolume: | ||
| mount.Type = enginemount.TypeVolume | ||
| default: | ||
| panic("cannot convert type %s") |
There was a problem hiding this comment.
Is panicking really the right thing to do here? The spec is user data, so as I understand it we'd be panicing on bad input.
There was a problem hiding this comment.
ah yep, I meant to take that out.
|
@cpuguy83 This still looks fine but should we wait for 1.13 release before merging into swarmkit or did you want to use this as a test bed? I have no opinion either way expect that it is hard to run dev version on mac. |
|
Friendly ping :) The train for 1.13 is leaving soon, we have to make a quick decision about this |
|
@aluzzardi That is not the issue. The problem is that you won't be able to run swarmkit executor against a 1.12 engine with this change. This isn't production code, so we can merge after 1.13 release. The equivalent change should make it into docker. |
|
What's the status of this PR? 1.13 has come and gone. Do we still want to pursue the changes? |
|
@aaronlehmann This is still valid and relevant. Should be even easier to get working now that 1.13 is out. @cpuguy83 Do you have time to update this? |
Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
use_docker_mounts_api
branch
from
2efe868 to
e0eb0d0
Compare
|
Forgot about this thing. Looks like we'll also be able to move tmpfs over to the new format as well. Can follow up with that. |
|
Rebased. |
|
LGTM |
|
LGTM Are there any analagous changes to make in the executor in docker, or is this specific to the |
These have already been done. I think the issue with making this change is that the API required 1.13 (or 17.03) or later. |
6 participants
Closes #1308 -- though this will be handled in the engine itself instead of the agent/exec interface. Not sure if we also want to check this in the agent.