Skip to content
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

Volume binds for windows containers #1321

Merged
merged 1 commit into from
Jul 18, 2016
Merged

Volume binds for windows containers #1321

merged 1 commit into from
Jul 18, 2016

Conversation

mwieczorek
Copy link
Contributor

Windows containers needs separate definition of paths for mounted volumes (with drive letters).
Without this patch we get an error during creating container, f.e.:

2016/06/18 13:50:54 [ERR] client: failed to start task 'redis' for alloc 'f66e0366-7137-7e29-9e11-3d21dceeaa4e': 
Failed to create container from image microsoft/sample-redis:nanoserver: API error (500): 
Invalid bind mount spec "C:\\Users\\vagrant\\AppData\\Local\\Temp\\NomadClient916164026\\f66e0366-7137-7e29-9e11-3d21dceeaa4e\\alloc:/alloc": 
Invalid volume specification: 'C:\Users\vagrant\AppData\Local\Temp\NomadClient916164026\f66e0366-7137-7e29-9e11-3d21dceeaa4e\alloc:/alloc'

@diptanu
Copy link
Contributor

diptanu commented Jun 21, 2016

@mwieczorek Thanks for the PR! We will test this out once we make the 0.4 release over the next few days and merge this.

@mwieczorek
Copy link
Contributor Author

@diptanu Any thoughts about it? Is it ok, or should I change something?


var (
//Path inside container for mounted directory that is shared across tasks in a task group.
SharedAllocContainerPath = filepath.Join("c:\\", SharedAllocName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we hardcoding c:\\ here? I understand that this is the default, but users can specify any other drive such as D, E, etc as their Data Dir. Is this going to work in all cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am not sure how bind mount sharing between containers and hosts work in windows. Can you please explain how the file system is laid out inside a windows container? So if it's always going to be C:\\ I think this is fine.

@diptanu
Copy link
Contributor

diptanu commented Jul 8, 2016

@mwieczorek Sorry for taking so long. The PR looks good, but I have a question regarding the hard coded C:\\, once we can ensure that it's going to work properly I can merge this PR.

@mwieczorek
Copy link
Contributor Author

@diptanu If I try with other drives (D:, E:, etc.) I get an error (something with win32).
I couldn't find anything in MS documentation so I asked about it here

Let's wait for the reply. I'll let you know about it.

@diptanu
Copy link
Contributor

diptanu commented Jul 8, 2016

@mwieczorek Sounds good!

@diptanu
Copy link
Contributor

diptanu commented Jul 8, 2016

@mwieczorek Also, probably worth asking the question on the docker project's mailing list/issues as well.

@mwieczorek
Copy link
Contributor Author

@diptanu It's confirmed - we can only use C: drive.
Of course in the future if other drives will be supported we can change it also in Nomad.

@diptanu
Copy link
Contributor

diptanu commented Jul 18, 2016

@mwieczorek Thanks for shepherding this!

@diptanu diptanu merged commit 51149e4 into hashicorp:master Jul 18, 2016
@mwieczorek mwieczorek deleted the f-windows-binds branch July 18, 2016 19:09
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants