Workaround for Windows Containers #2574
Conversation
dhirschfeld
changed the title
|
|
||
| if not stat.S_ISREG(st.st_mode) and not stat.S_ISDIR(st.st_mode): | ||
| self.log.debug("%s not a regular file", os_path) | ||
| self.log.warning("%s not a regular file", os_path) |
There was a problem hiding this comment.
Just made this change to confirm why the mapped volume still isn't showing.
Results in warnings being printed to the terminal:
[W 17:07:19.450 LabApp] C:\jupyter\notebooks not a regular file
[W 17:07:29.451 LabApp] C:\jupyter\notebooks not a regular file
[W 17:07:40.452 LabApp] C:\jupyter\notebooks not a regular file
| self.log.warning("%s not a regular file", os_path) | ||
| continue | ||
|
|
||
| if self.should_list(name) and not is_file_hidden(os_path, stat_res=st): |
There was a problem hiding this comment.
is_file_hidden(os_path, stat_res=st) returns True for a host mapped volume.
|
Working around the fact that the mapped directory is hidden results in |
|
🐇 🕳️ |
|
For reference: |
|
...however: [19]: from notebook.utils import is_file_hidden_win
[20]: st = os.stat(r"C:\jupyter\notebooks", follow_symlinks=False)
... : st
Out[20]: os.stat_result(st_mode=41471, st_ino=281474976710694, st_dev=113970060, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1497345137, st_mtime=1497345137, st_ctime=1497345137)
[21]: st.st_file_attributes
Out[21]: 1040
[22]: bool(st.st_file_attributes & stat.FILE_ATTRIBUTE_HIDDEN)
Out[22]: False
[23]: is_file_hidden_win(r"C:\jupyter\notebooks")
Out[23]: TrueFrom: https://stackoverflow.com/a/6365265/2960140 The @takluyver - can you shed any light on the purpose of those lines? Can they just be removed?
Edit: Removing the above lines does allow JupyterLab to list and use the host mapped volume |
dhirschfeld
force-pushed
the
windows-containers
branch
2 times, most recently
from
6e078d5 to
bf1e410
Compare
|
Cleaned up the implementation and (hopefully) made it py27 compatible |
dhirschfeld
force-pushed
the
windows-containers
branch
from
bf1e410 to
9356cac
Compare
|
RFR: @takluyver, @minrk @takluyver - I've included you in the request for review as you appear to be the author of the The use of |
dhirschfeld
changed the title
|
I'm not sure why travis has failed - I can't see anything that should be the case since the changes should only apply on Windows (and when inside a container). Random failure? |
notebook/utils.py
Outdated
| # Replace `os.stat` which can't stat a host mapped volume from inside a Windows Container. | ||
| WINDOWS_CONTAINER = sys.platform == 'win32' and os.environ.get('USERNAME') == 'ContainerAdministrator' | ||
| if WINDOWS_CONTAINER: | ||
| f_stat = os.lstat |
There was a problem hiding this comment.
should we just use lstat on windows in general, rather than trying to check if it's a container?
There was a problem hiding this comment.
Can we do this for Windows in general, and remove the WINDOWS_CONTAINER constant?
|
I restarted Travis. Intermittent failure unrelated to the PR. |
| self.log.warning("%s doesn't exist", os_path) | ||
| else: | ||
| self.log.warning("Error stat-ing %s: %s", os_path, e) | ||
| self.log.warning("Error stat-ing %s: %s", (os_path, e)) |
There was a problem hiding this comment.
Adding these parentheses will cause a formatting error:
TypeError: not enough arguments for format string
There was a problem hiding this comment.
Sorry, not sure how that change crept in - I'll blame it on auto-formatting!
|
Is there a way we can avoid detecting and special-casing Windows containers, and do something that works on Windows and/or catch errors appropriately, instead? |
|
I agree it's a pretty ugly hack - I was aiming to just fix my usecase without changing the existing behaviour for anyone else. There's probably a better fix, but I'm not 100% sure why all the existing checks (some of which fail on Windows containers) are there so I just tried a minimal fix. Specifically, the ishidden = bool(st.st_file_attributes & stat.FILE_ATTRIBUTE_HIDDEN) # py3 only...but it also tries to call # check that dirs can be listed
if os.path.isdir(abs_path):
# can't trust os.access on Windows because it seems to always return True
try:
os.stat(abs_path)
except OSError:
# stat may fail on Windows junctions or non-user-readable dirs
return TrueI assume by calling My initial reaction was to simply delete that block entirely as I'm not sure why its necessary. I think it would be better to show a "broken" symlink dir than to pretend it never existed. When trying to change to a broken directory the notebook could then just let the underlying |
|
IIUC by just using |
|
Other than The The And finally, the check for a regular file: if not stat.S_ISREG(st.st_mode) and not stat.S_ISDIR(st.st_mode):
self.log.debug("%s not a regular file", os_path)
continueThis was resolved by protecting with a Again though I'm not sure why there's a requirement to protect against listing non-regular (??) files. I think for a file browser the requirement could just be if it shows up with ls/dir then display it in the file browser? If that's acceptable you could then just delete the whole checking block in try:
st = os.stat(os_path)
except OSError as e:
# skip over broken symlinks in listing
if e.errno == errno.ENOENT:
self.log.warning("%s doesn't exist", os_path)
else:
self.log.warning("Error stat-ing %s: %s", os_path, e)
continue
if not stat.S_ISREG(st.st_mode) and not stat.S_ISDIR(st.st_mode):
self.log.debug("%s not a regular file", os_path)
continue |
|
So, my recommendation would actually be to remove all checking from both functions and list all files whether regular or not or broken symlinks or not. If the user then tries to use a "broken" file or directory they should get the same OS error as if they tried to do the same from the terminal. Not sure if that makes sense or is reasonable. It's certainly a change in behaviour and thus a design question for the core devs. |
|
...maybe expand the contents definition to include attributes such as hidden/symlink/broken and have a UI element which would allow the user to choose whether or not to display files based on those attributes. |
dhirschfeld
force-pushed
the
windows-containers
branch
from
9356cac to
f5646f7
Compare
|
I'd rather not include hidden files in the listing, since hidden files should be...hidden. Broken links are different, though. I think we hid broken links because they caused errors elsewhere when |
|
I'll look into what happens with broken links on my branch and report back... |
|
On my branch, when run from JupyterHub I get an internal server error dialog when I double-click a directory symbolic link. Will investigate without JupyterHub to try and get more info from the terminal. In contrast an NTFS Junction Point works fine in the file browser. |
|
Trying to start a kernel in a host mapped dir seems to cause an issue: Error Starting Kernel
Traceback (most recent call last):
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 528, in get
value = obj._trait_values[self.name]
KeyError: 'root_dir'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "C:\Miniconda3\lib\site-packages\notebook\base\handlers.py", line 516, in wrapper
result = yield gen.maybe_future(method(self, *args, **kwargs))
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1055, in run
value = future.result()
File "C:\Miniconda3\lib\site-packages\tornado\concurrent.py", line 238, in result
raise_exc_info(self._exc_info)
File "<string>", line 4, in raise_exc_info
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1063, in run
yielded = self.gen.throw(*exc_info)
File "C:\Miniconda3\lib\site-packages\notebook\services\sessions\handlers.py", line 75, in post
type=mtype))
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1055, in run
value = future.result()
File "C:\Miniconda3\lib\site-packages\tornado\concurrent.py", line 238, in result
raise_exc_info(self._exc_info)
File "<string>", line 4, in raise_exc_info
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1063, in run
yielded = self.gen.throw(*exc_info)
File "C:\Miniconda3\lib\site-packages\notebook\services\sessions\sessionmanager.py", line 79, in create_session
kernel_id = yield self.start_kernel_for_session(session_id, path, name, type, kernel_name)
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1055, in run
value = future.result()
File "C:\Miniconda3\lib\site-packages\tornado\concurrent.py", line 238, in result
raise_exc_info(self._exc_info)
File "<string>", line 4, in raise_exc_info
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1063, in run
yielded = self.gen.throw(*exc_info)
File "C:\Miniconda3\lib\site-packages\notebook\services\sessions\sessionmanager.py", line 92, in start_kernel_for_session
self.kernel_manager.start_kernel(path=kernel_path, kernel_name=kernel_name)
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 1055, in run
value = future.result()
File "C:\Miniconda3\lib\site-packages\tornado\concurrent.py", line 238, in result
raise_exc_info(self._exc_info)
File "<string>", line 4, in raise_exc_info
File "C:\Miniconda3\lib\site-packages\tornado\gen.py", line 307, in wrapper
yielded = next(result)
File "C:\Miniconda3\lib\site-packages\notebook\services\kernels\kernelmanager.py", line 92, in start_kernel
kwargs['cwd'] = self.cwd_for_path(path)
File "C:\Miniconda3\lib\site-packages\notebook\services\kernels\kernelmanager.py", line 66, in cwd_for_path
os_path = to_os_path(path, self.root_dir)
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 556, in __get__
return self.get(obj, cls)
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 535, in get
value = self._validate(obj, dynamic_default())
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 593, in _validate
value = self._cross_validate(obj, value)
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 599, in _cross_validate
value = obj._trait_validators[self.name](obj, proposal)
File "C:\Miniconda3\lib\site-packages\traitlets\traitlets.py", line 907, in __call__
return self.func(*args, **kwargs)
File "C:\Miniconda3\lib\site-packages\notebook\services\kernels\kernelmanager.py", line 52, in _update_root_dir
raise TraitError("kernel root dir %r is not a directory" % value)
traitlets.traitlets.TraitError: kernel root dir 'C:\\jupyter\\dhirschf' is not a directory |
|
When double-clicking on a symlink directory JupyterLab pops up an Internal Server Error dialog and I get the below error in the terminal: Note: the directory |
|
...well, that's just odd - On host it Just Works: |
|
If I create instead a junction point it works fine. If I then purposely break the junction point by deleting |
|
My suggestion would be for symlinks (which don't work) and for broken junction points to display the error text: ...rather than just the opaque Internal Server Error. But I suppose that's an issue for JupyterLab rather than the notebook. |
|
File soft-links also fail, but hard links seem to work: |
|
The symlink situation on Windows seems to be a known issue: |
The 👍's on this comment show that there is some support for having this configurable. As a relative power user on Windows one of the first things I do is to tick the Show Hidden Files box in the Explorer config. I'm not advocating it in this PR though - I think it deserves a PR of its own if anyone is interested enough. I'll be happy to just get this working on Windows Containers. |
dhirschfeld
force-pushed
the
windows-containers
branch
from
f5646f7 to
4d850df
Compare
On Windows `os.stat` treats host mapped volumes as broken symlinks
dhirschfeld
force-pushed
the
windows-containers
branch
from
4d850df to
6bb89b7
Compare
|
The latest push fixes the error when trying to start the server in a host mapped directory - #2574 (comment) As it is, this PR allows me to use the Notebook/JupyterLab on NanoServer containers. In this version I've deleted the broken symlink checking from the If possible it would be great to get this into the next release as without these changes you can't use the Notebook/JupyterLab for anything serious on Windows Containers as there's no way to persist any user data outside of the container. cc: @minrk, @takluyver |
| # stat may fail on Windows junctions or non-user-readable dirs | ||
| return True | ||
|
|
||
| win32_FILE_ATTRIBUTE_HIDDEN = 0x02 |
There was a problem hiding this comment.
Made the module level global local to this function as it isn't used elsewhere
| os.stat(abs_path) | ||
| except OSError: | ||
| # stat may fail on Windows junctions or non-user-readable dirs | ||
| return True |
There was a problem hiding this comment.
Removed checking for a broken symlink as I think it's better to let the user know and have them deal with it rather than pretend it doesn't exist
| continue | ||
|
|
||
| if not stat.S_ISREG(st.st_mode) and not stat.S_ISDIR(st.st_mode): | ||
| if (not WINDOWS_CONTAINER |
There was a problem hiding this comment.
just win32 instead of WINDOWS_CONTAINER?
There was a problem hiding this comment.
As it stands the WINDOWS_CONTAINER flag both tests that the platform is win32 and that you're running from inside a container by checking for the default container user ContainerAdministrator.
It's certainly possible to make the changes for all windows users but I was hesitant to potentially break existing code.
There was a problem hiding this comment.
I certainly appreciate aiming for minimal impact. I'm coming from the perspective that anything WINDOWS_CONTAINER specific is going to get minimal testing, so if we can reduce the number of cases it will be easier for us to maintain / catch regressions. Plus, I think showing broken links should be fine, as long as it doesn't cause an error in directory listing (using lstat fixes this, I believe).
Instead of gating on Windows, since it can be a link, maybe it should be adding stat.S_ISLNK to the checks instead of making it conditional on the platform. Does that return True for these mounts? What is os.stat(path).st_mode for these mounts?
There was a problem hiding this comment.
os.stat(path) would throw a FileNotFound error. You'd need to either use lstat or pass the follow_symlinks=False arg (py3 only).
lstat gives a 41471 mode:
[20]: st = os.stat(r"C:\jupyter\notebooks", follow_symlinks=False)
... : st
Out[20]: os.stat_result(st_mode=41471, st_ino=281474976710694, st_dev=113970060, st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1497345137, st_mtime=1497345137, st_ctime=1497345137)
[21]: st.st_file_attributes
Out[21]: 1040If we don't care about checking if a symlink is broken we could just globally use lstat. The only other issue is then the check for a regular file:
if not stat.S_ISREG(st.st_mode) and not stat.S_ISDIR(st.st_mode):
self.log.debug("%s not a regular file", os_path)
continueIn that case doesn't recognise the mapped volume as a directory but sees it as a link instead:
In [2]: stat.S_ISREG(41471)
Out[2]: False
In [3]: stat.S_ISDIR(41471)
Out[3]: False
In [4]: stat.S_ISLNK(41471)
Out[4]: True...so the if not WINDOWS_CONTAINER could be replaced with and not stat.S_ISLNK(st.st_mode)
Happy to go ahead and make those changes if you think that's the right way forward...
|
The CI error is below: The function which failed is: @dec.skip_win32
def test_bad_symlink(self):
with TemporaryDirectory() as td:
cm = FileContentsManager(root_dir=td)
path = 'test bad symlink'
_make_dir(cm, path)
file_model = cm.new_untitled(path=path, ext='.txt')
# create a broken symlink
self.symlink(cm, "target", '%s/%s' % (path, 'bad symlink'))
model = cm.get(path)
self.assertEqual(model['content'], [file_model])IIUC this function is testing that if you create a broken symlink the |
|
I think you are exactly right. Since we're changing how we treat bad symlinks (they show up as regular files, then 404 when one tries to open them), updating the test is appropriate. If the contents manager sees your mounts as bad symlinks, do they look like files in the file listing, or directories? Can you open them? |
|
Host mounted volumes have a directory icon as they should. Opening them in the JupyterLab File Browser works fine and correctly lists the contents. It's just that stat fails for whatever reason on them. Not sure if that's a python issue or expected/correct behaviour. In contrast a broken symlink directory shows up as a directory icon (in this PR) but double-clicking on it in the JupyterLab File Browser pops up an Internal Server Error dialog in the app and logs a FileNotFound error in the terminal. I'll look to finish off this PR by updating the test for the new behaviour... Thanks for the review @minrk! |
dhirschfeld
force-pushed
the
windows-containers
branch
from
798923a to
295294e
Compare
| self.assertEqual(cp_subdir, os.path.join(root, subd, cpm.checkpoint_dir, cp_name)) | ||
|
|
||
| @dec.skip_win32 | ||
| @dec.skipif(sys.platform == 'win32' and sys.version_info[0] < 3) |
There was a problem hiding this comment.
Prior to version 3.2 os.symlink which is required for this test only existed for unix. This enables testing on Python 3 for Windows as all supported versions have this functionality now.
There was a problem hiding this comment.
os.symlink exists now, but I think that in most situations it still raises a permission error, because normal users don't have the right to create symlinks. So I suspect that when we next do a build on Windows, this might fail.
There was a problem hiding this comment.
I wondered if that might be the case but I figured if the tests passed on appveyor it would be fine.
That was based on the assumption that most normal Windows users probably wouldn't be running the tests. In that case it would only affect a Windows developer trying to run the tests without admin privileges where it would fail with a permission error. I think that having some testing on Windows/appveyor is probably worth any hassle that might cause?
OT: In the future it should be possible for developers without admin privileges to create symlinks on Windows 10 - https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/#oWkhfiQvGP6wte4V.97
There was a problem hiding this comment.
Ah, nice. I guess our build servers have got the 'creators update' and therefore are able to create symlinks. :-)
|
For reference, this seems like a bug in docker. I still prefer the new behaviour though as hiding errors from the user doesn't give them a chance to do anything about it. |
3 participants
On Windows
os.stattreats host mapped volumes as broken symlinksxref: jupyterlab/jupyterlab#2426