Allow different directory for server_pool communication

[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.

[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.

[jdemeyer]

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.

[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?

[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.

[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).

[kcrisman]

This was reported at ​https://github.com/sagemath/sagenb/issues/32 a while ago.

[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.

[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.

[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.

[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...

[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.

[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.

[kcrisman]

Agreed.

[kcrisman]

Just need an upstream package now, which will come soon for #12212 in any case.

[kcrisman]

This has been merged, n'est pas?