Opened 10 years ago

Closed 7 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:

Status badges

Description (last modified by jdemeyer)

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)

11679.patch (4.1 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 follow-up: Changed 10 years ago by rkirov

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.

comment:3 in reply to: ↑ 2 Changed 10 years ago by jdemeyer

  • 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: Changed 10 years ago by 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?

comment:5 in reply to: ↑ 4 Changed 10 years ago by jdemeyer

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 10 years ago by was

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 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 8 years ago by jdemeyer

comment:8 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:11 Changed 7 years ago by kcrisman

  • 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: Changed 7 years ago by jdemeyer

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 7 years ago by kcrisman

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 7 years ago by jdemeyer

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 7 years ago by kcrisman

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: Changed 7 years ago by kcrisman

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 7 years ago by jdemeyer

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 7 years ago by kcrisman

Agreed.

comment:19 Changed 7 years ago by kcrisman

  • Authors Jeroen Demeyer deleted
  • 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 7 years ago by kcrisman

  • 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 7 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.