-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
Don't follow Windows ContainerMappedDirectories #9475
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
Don't follow Windows ContainerMappedDirectories #9475
Conversation
Signed-off-by: Stefan Scherer <scherer_stefan@icloud.com>
if (linkTarget === null) { | ||
fs.statSync(base); | ||
linkTarget = fs.readlinkSync(base); | ||
if (linkTarget.startsWith('\\ContainerMappedDirectories')) { |
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.
This seems a bit hacky. Wouldn't this prevent any application from using \ContainerMappedDirectories
? I imagine an isWindows
check should be added too.
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.
@cjihrig is being too kind. This is not an acceptable solution.
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 took the effort and had a look where things go wrong. I don't have expected that this PR gets merged in. It's just tried to get started understand what Node.js is doing here and start a discussion and giving a first idea to find a fix for Node.js in Windows containers.
The idea is to just stop the symlink expansion at the container volume shares.
I believe the work should be done by some Node.js core maintainer with more knowledge of the code base. I just forked the node repo for the first time, tried to compile the sources and find a fix and not just filing issues.
As a user of Node.js I just wonder why Node.js does so many symlink expansion for such simple tasks like node.exe c:\mount\hello.js
and not finding the source code. I just wonder why this is all needed ( just thinking "code that does not exist could not have bugs" ). I'm sure there is a good reason why this is done in node.exe and npm at a lot of places, but it seems that it complicate things at some point.
Maybe a more general approach to paths starting with |
Yes, I also thought of libuv. Just reading through the source code of libuv, planning to build a |
Any status updates on this? |
@jasnell Well, over the last couple of months I learned that other languages have similar problems retrieving the real path of a file in such volume mount points. See moby/moby#27537 for a discussion.
# escape=`
FROM stefanscherer/node-windows:7.7.4-nano
RUN npm install -g nodemon
VOLUME C:\code
RUN set-itemproperty -path `
'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager\DOS Devices' `
-Name 'G:' -Value '\??\C:\code' -Type String
WORKDIR G:\
CMD ["nodemon.cmd", "--debug=5858", "app.js"]
|
Closing this PR |
cc @digitalinfinity @nodejs/platform-windows |
As of moby/moby#27537 (comment) it seems we have two options. Live with the drive mapping workaround or make node/libuv aware of these |
A interesting comment in Golang's fix for a similar issue inside a Windows container: https://go-review.googlesource.com/c/41834/
|
@StefanScherer that interesting info. Since |
@refack Do you have any updates on libuv? In the meantime I had the chance to try out the first Windows Server Insider build and it seems that Node.js is running fine in a mounted folder (there's no C:\ContainerMappedDirectories symlink). And nanoserver base image got much smaller, so a Node.js image on Windows is 89 ... 92 MB ( https://hub.docker.com/r/stefanscherer/node-windows/tags/ ). |
@StefanScherer I've been playing with the "using IO manager" trick a bit locally... It has allot of other advantages (huge maximal path length, UNC compatible, Direct device access) but it needs some massaging and I need more than 24h in a day... |
Windows Server on the outside or the inside (i.e. host OS or contained OS)? |
There isa new ISO file to build a VM with the Windows Server 2016 Insider preview. I'm using this Vagrant environment https://github.com/StefanScherer/insider-docker-machine for these tests. |
@refack The symlink ContainerMappedDirectories is still there in HyperV containers. This means in all Windows containers running on a Windows 10 machine where the typical developer wants to run code and live debug code in a container. I just verified it with |
So there's a slight complication with using the "Windows I/O manager". AFAICT it requires all paths to use the "proper" syntax that includes only backslashes, but currently libuv supports mixed forward slash paths on Windows... I have to make sure all code paths normalize the path before doing the system call... |
Just got into this issue with official https://hub.docker.com/r/microsoft/windowsservercore/ container. Unfortunately kubernetes cannot mount emptyDir into driver letter, therefore I end up setting my temp dir to C:\Users\ContainerAdministrator\Downloads within container. In the case of kubernetes Docker container has to be built before disk is mounted. |
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
fs
Description of change
This is my proposal fixing the problem #8897 using
fs.realpathSync()
on Docker shared volumes in Windows containers. The additional check if a symlink target is\ContainerMappedDirectories
than just ignore that link.Without that change:
After applying the change:
Any ideas how to add a test for this as it only occurs in Windows containers?