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

Fix stack compose bind-mount volumes for Windows #136

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Projects
None yet
7 participants
@johnstep
Contributor

johnstep commented May 27, 2017

For stack compose files, use filepath.IsAbs instead of path.IsAbs, for
bind-mounted service volumes, because filepath.IsAbs handles Windows
paths, while path.IsAbs does not.

Signed-off-by: John Stephens johnstep@docker.com

@johnstep johnstep added this to the 17.06.0 milestone May 27, 2017

@johnstep johnstep requested a review from dnephin May 27, 2017

@dnephin

Thanks! LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 29, 2017

Collaborator

Unfortunately doing this on the client side just does not work at all.

Collaborator

cpuguy83 commented May 29, 2017

Unfortunately doing this on the client side just does not work at all.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 30, 2017

Collaborator

@cpuguy83 I believe this works correctly on the client. absPath() is used in 4 places right now. 3 of the 4 I know are safe (env_file path, secret path, config path) because they always refer to paths handled by the client.

The last one (bind mount source path) is also a path relative to the client, so I think that's correct as well.

Collaborator

dnephin commented May 30, 2017

@cpuguy83 I believe this works correctly on the client. absPath() is used in 4 places right now. 3 of the 4 I know are safe (env_file path, secret path, config path) because they always refer to paths handled by the client.

The last one (bind mount source path) is also a path relative to the client, so I think that's correct as well.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 30, 2017

Collaborator

The last one (bind mount source path) is also a path relative to the client, so I think that's correct as well.

This should never be a path relative to the client. Works great for compose as a relative path, does not really work on a swarm.

I'm also wondering how this affects docker4windows (windows client -> linux daemon).

Collaborator

cpuguy83 commented May 30, 2017

The last one (bind mount source path) is also a path relative to the client, so I think that's correct as well.

This should never be a path relative to the client. Works great for compose as a relative path, does not really work on a swarm.

I'm also wondering how this affects docker4windows (windows client -> linux daemon).

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 30, 2017

Collaborator

This should never be a path relative to the client.

What about when there is a single swarm node which is the localhost? Then local paths are just fine. We need to continue to support that workflow.

In every other case a client path will be wrong, so the user should use an absolute path.

Collaborator

dnephin commented May 30, 2017

This should never be a path relative to the client.

What about when there is a single swarm node which is the localhost? Then local paths are just fine. We need to continue to support that workflow.

In every other case a client path will be wrong, so the user should use an absolute path.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 30, 2017

Collaborator
Collaborator

cpuguy83 commented May 30, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin May 30, 2017

Collaborator

To clarify. I meant we need to continue to support it for stack deploy. We should be able to use the same tool (stack deploy) to develop on a local cluster of 1, and a production cluster.

Collaborator

dnephin commented May 30, 2017

To clarify. I meant we need to continue to support it for stack deploy. We should be able to use the same tool (stack deploy) to develop on a local cluster of 1, and a production cluster.

@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Jun 1, 2017

Contributor

This fix makes it possible to bind-mount volumes in stacks from the Windows client.

I verified this works with Windows Server 2016 containers, and with Docker for Windows with Linux containers. I tried to verify Windows containers as well, but am encountering an unrelated HNS error. Regardless, I can see that this corrects the problem by inspecting the service created for the stack.

However, this does not handle a Windows client creating a stack service from a Linux daemon, where the volume source path is a directory local to the daemon system. In the Docker for Windows case, that would be bind-mounting a volume from within the LinuxKit VM (normally the Windows path is translated automatically); the client working directory is irrelevant in that case. As @cpuguy83 suggested earlier (offline), it might be reasonable for the client to check for both Linux and Windows absolute paths.

Contributor

johnstep commented Jun 1, 2017

This fix makes it possible to bind-mount volumes in stacks from the Windows client.

I verified this works with Windows Server 2016 containers, and with Docker for Windows with Linux containers. I tried to verify Windows containers as well, but am encountering an unrelated HNS error. Regardless, I can see that this corrects the problem by inspecting the service created for the stack.

However, this does not handle a Windows client creating a stack service from a Linux daemon, where the volume source path is a directory local to the daemon system. In the Docker for Windows case, that would be bind-mounting a volume from within the LinuxKit VM (normally the Windows path is translated automatically); the client working directory is irrelevant in that case. As @cpuguy83 suggested earlier (offline), it might be reasonable for the client to check for both Linux and Windows absolute paths.

Fix stack compose bind-mount volumes for Windows
For stack compose files, use filepath.IsAbs instead of path.IsAbs, for
bind-mounted service volumes, because filepath.IsAbs handles Windows
paths, while path.IsAbs does not.

Signed-off-by: John Stephens <johnstep@docker.com>
@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Jun 1, 2017

Contributor

I updated this to do a Unix absolute path check first (via the previous check, path.IsAbs), and only perform the platform-specific check if that returns false. This has no effective impact on Linux/Unix clients, but should allow a Windows client to work with Linux/Unix and Windows daemons in all cases, while preserving support for relative paths for a single-nodes swarm.

Contributor

johnstep commented Jun 1, 2017

I updated this to do a Unix absolute path check first (via the previous check, path.IsAbs), and only perform the platform-specific check if that returns false. This has no effective impact on Linux/Unix clients, but should allow a Windows client to work with Linux/Unix and Windows daemons in all cases, while preserving support for relative paths for a single-nodes swarm.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 1, 2017

Codecov Report

Merging #136 into master will decrease coverage by 0.1%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   44.96%   44.85%   -0.11%     
==========================================
  Files         169      169              
  Lines       11378    11379       +1     
==========================================
- Hits         5116     5104      -12     
- Misses       5970     5982      +12     
- Partials      292      293       +1

codecov-io commented Jun 1, 2017

Codecov Report

Merging #136 into master will decrease coverage by 0.1%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   44.96%   44.85%   -0.11%     
==========================================
  Files         169      169              
  Lines       11378    11379       +1     
==========================================
- Hits         5116     5104      -12     
- Misses       5970     5982      +12     
- Partials      292      293       +1

@johnstep johnstep requested a review from cpuguy83 Jun 1, 2017

@johnstep johnstep referenced this pull request Jun 1, 2017

Closed

17.06.0 RC2 tracker #2

23 of 23 tasks complete
@dnephin

dnephin approved these changes Jun 1, 2017

LGTM

@mlaventure

LGTM

@cpuguy83

I think this works for now, but note that path really only applies to strings with / separators and not really unix paths specifically... however path.IsAbs and the unix implementation of filepath.IsAbs are essentially the same.

@cpuguy83 cpuguy83 merged commit bf22fc6 into docker:master Jun 1, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 75% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -0.1% but relative coverage increased by +30.03% compared to efaadcf
Details
dco-signed All commits are signed
@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Jun 1, 2017

Contributor

@cpuguy83 Yes, true. A better solution might be to add a function that specifically checks for an absolute path on any platform, if that can be done in a reasonable way.

On a side note, the code that handles ~ is not going to work on Windows since the HOME environment variable is not generally set, but it's probably not something users expect on Windows anyway (though PowerShell supports it), and is just a minor convenience.

Contributor

johnstep commented Jun 1, 2017

@cpuguy83 Yes, true. A better solution might be to add a function that specifically checks for an absolute path on any platform, if that can be done in a reasonable way.

On a side note, the code that handles ~ is not going to work on Windows since the HOME environment variable is not generally set, but it's probably not something users expect on Windows anyway (though PowerShell supports it), and is just a minor convenience.

@johnstep johnstep deleted the johnstep:fix_stack_service_volumes branch Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment