Opened 11 years ago
Closed 8 years ago
#11679 closed enhancement (fixed)
Allow different directory for server_pool communication
Reported by: | jdemeyer | Owned by: | jason, mpatel, was |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | notebook | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | Jeroen Demeyer, Karl-Dieter Crisman | |
Report Upstream: | Fixed upstream, but not in a stable release. | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
In a remote notebook setup using server_pool
, all communication between the main Sage process and the workers goes through
/tmp
. I suggest to introduce an environment variable
SAGENB_TMPDIR
(with fallback to
TMPDIR
) to configure this.
I have a succesful setup with one server node and two worker nodes using this, so it works.
Attachments (1)
Change History (22)
comment:1 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 11 years ago by
comment:3 in reply to: ↑ 2 Changed 11 years ago by
- Status changed from needs_review to needs_work
Replying to rkirov:
shouldn't we use python's tempfile.gettempdir() as a fallback and never have hardcoded directories.
I will look into that.
Also seems there are changes to the ulimit docs? Were they wrong?
Maybe not technically wrong but at least I think I clarified it a lot. In particular, the ulimit
options which are documented are the only ones which actually work, while the documentation might suggest that all
ulimit
options from
bash
or whathever work.
comment:4 follow-up: ↓ 5 Changed 11 years ago by
I'm concerned -- I think this would first time any environment variable is used to configure the notebook. I think there is a much better and more standard way to implement this feature, which is unfortunately more work and requires knowing a lot more about how the notebook works. However, it is better in the longrun, since it will be much less confusing to admins and easier to maintain. For the notebook, implementing the following would be much more natural:
notebook(local_directory = '/path/you/want', ...)
This would involve modifying the notebook.py file by editing "new_worksheet_process" and run_notebook.py, etc. Also, this new option would be part of the server configuration database (currently a pickle, but someday a SQL table), so it would automatically be something that can be edited graphically by the notebook admin from the notebook configuration settings webpage.
So a big +1 to having this feature, but -1 to using environment variables in the longrun to implement it, since we already have a standard system for configuring the server, and it isn't via environment variables.
Thoughts?
comment:5 in reply to: ↑ 4 Changed 11 years ago by
Replying to was:
I'm concerned -- I think this would first time any environment variable is used to configure the notebook. I think there is a much better and more standard way to implement this feature, which is unfortunately more work and requires knowing a lot more about how the notebook works. However, it is better in the longrun, since it will be much less confusing to admins and easier to maintain. For the notebook, implementing the following would be much more natural:
notebook(local_directory = '/path/you/want', ...)This would involve modifying the notebook.py file by editing "new_worksheet_process" and run_notebook.py, etc. Also, this new option would be part of the server configuration database (currently a pickle, but someday a SQL table), so it would automatically be something that can be edited graphically by the notebook admin from the notebook configuration settings webpage.
So a big +1 to having this feature, but -1 to using environment variables in the longrun to implement it, since we already have a standard system for configuring the server, and it isn't via environment variables.
Thoughts?
Well, using environment variables for temporary directories is not unusual, see the fairly standard use of TPMDIR
.
On the other hand, you are right that it would be a new way to configure the notebook. So my answer is "Yes, you are probably right but I'm not going to implement it". I do think this is a very useful feature.
comment:6 Changed 11 years ago by
On the other hand, you are right that it would be a new way to configure
the notebook. So my answer is "Yes, you are probably right but I'm not going
to implement it". I do think this is a very useful feature.
I very much agree with your answer above. It would make sense for me, or Rado, or Mike to "properly" implement this feature, and you can spend your valuable time on release management (which you are doing a fantastic job on).
comment:7 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
Changed 9 years ago by
comment:8 Changed 9 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:9 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:11 Changed 8 years ago by
- Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
This was reported at https://github.com/sagemath/sagenb/issues/32 a while ago.
comment:12 follow-up: ↓ 13 Changed 8 years ago by
Unfortunately not yet fixed. 3 years ago William wrote in 4 that my patch was no good. Given that there hasn't surfaced a better fix and given that I don't know much about the notebook, wouldn't it be possible to just apply my patch?
I have been using it in a production environment at my university and it hasn't broken anything.
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Unfortunately not yet fixed. 3 years ago William wrote in 4 that my patch was no good. Given that there hasn't surfaced a better fix and given that I don't know much about the notebook, wouldn't it be possible to just apply my patch?
Sure, we can try that. My previous comment was mostly just to keep tabs on things so we don't lose issues. I may take a look, but I don't know much about the actual communication part of web apps.
By the way, Rado's comment refers to a previous version of this patch which had some doc on ulimit
. I don't see it in this version; if you have some info on that it would be helpful to add to the sagenb doc.
comment:14 Changed 8 years ago by
I think the ulimit
thing refered to an old version of this patch. I don't remember if that was merged as part of a different ticket.
comment:15 Changed 8 years ago by
I finally looked at this patch. I really don't see how it can harm anything, though I will point out that on Macs at least this would mean a change of folder.
$ echo $TMPDIR /var/folders/k8/nj0z1bkd11dcs1v5hbh_tknc92p43w/T/
Can you make a branch upstream so I can review it? I know you said somewhere that would make things easier for you, though I don't recall where...
comment:16 follow-up: ↓ 17 Changed 8 years ago by
I wonder if the backup to $TMPDIR
is okay...
$ stat $TMPDIR 234881026 42865026 drwx------ ... $ stat /tmp 234881026 33176 lrwxr-xr-x ...
Would having fewer permissions cause problems for processes? I know that is sort of the point, but I don't know what minimum permissions are needed.
By the way, you can just reference https://github.com/sagemath/sagenb/issues/32 in said pull request.
comment:17 in reply to: ↑ 16 Changed 8 years ago by
Replying to kcrisman:
Would having fewer permissions cause problems for processes?
Yes, but only when running a multi-user sagenb server using the server_pool
option. Given that such a setup requires some non-trivial configuration anyway, I don't see this as a problem.
comment:18 Changed 8 years ago by
Agreed.
comment:19 Changed 8 years ago by
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.
Just need an upstream package now, which will come soon for #12212 in any case.
comment:20 Changed 8 years ago by
- Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
- Reviewers set to Jeroen Demeyer, Karl-Dieter Crisman
- Status changed from needs_work to positive_review
This has been merged, n'est pas?
comment:21 Changed 8 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
shouldn't we use python's tempfile.gettempdir() as a fallback and never have hardcoded directories.
Also seems there are changes to the ulimit docs? Were they wrong?
Other than that the patch seems ok.