Integration branch for next release #112

rkdarst · 2018-08-07T21:49:39Z

I started this as integration branch of all patches for next release. Not everything is here yet, but I will try to integrate on here. Feedback from community about what to integrate is welcome.

I will use #103 to list what is integrated into this already.

Runs OK on my dev instance. Let's see if tests pass.

- It would be possible to have exec_prefix use string formatting and the command itself use jinja2 formatting, and when combined the string formatting would fail. To handle this, template them separately and then concatenate.

- These options first appeared in dff4482 for SlurmSpawner - Add tests for this option - Not currently implemented for CondorSpawner, since it uses a different method using `bash -c`. It could be added there, but someone with local knowledge should do that.

- Existing keepvars will whitelist environment variables, but is all-or-nothing: you can't append to existing required environemnt variables that JupyterHub requires. - This allows a comma separated list of the same format as req_keepvars that gets appended to keepvars, with one extra comma between them.

…ency with spacing

…atch_script first

…on call signature For consistency with jupyterhub itself

@krinsman

- Thanks to @krinsman in jupyterhub#90.

@krinsman

- Reading from stdout and stderr separately can produce a deadlock. I assume that the separate proc.wait_for_exit() doesn't matter here. - Thanks to @krinsman in jupyterhub#90.

…_debug', 'keepvars_extra', 'spawner_detail', 'prologue_epilogue' and 'exec_prefix_template_separately' into dev

Conflicts: .travis.yml (keep travis_jh9.0.1 side)

- Previously, only the query command got the full enivornment. - If this is used to authenticate to the submit/query/cancel commands, then the user's environment gets this also. - Closes: jupyterhub#108

Conflicts: batchspawner/batchspawner.py

Conflicts: batchspawner/batchspawner.py batchspawner/tests/test_spawners.py

willingc · 2018-08-08T00:48:31Z

@rkdarst I appreciate the work that you are doing to push the release forward. I'm a bit confused why move to an integration branch for development instead of merging existing PRs. Perhaps I'm missing some subtle interactions between PRs. We've typically cut releases from the master branch.

I'm copying @minrk for his thoughts.

I'm sorry that I've been a bit out of the loop recently with teaching, travel and vacation taking me away from the day-to-day dev stuff.

willingc

Thanks @rkdarst. I've made a few suggestions.

willingc · 2018-08-08T00:53:58Z

.travis.yml


 before_install:
    - npm install -g configurable-http-proxy
    - git clone --quiet --depth 1 https://github.com/minrk/travis-wheels travis-wheels
    - git clone --quiet --branch $JHUB_VER https://github.com/jupyter/jupyterhub.git jupyterhub
 install:
 # Don't let requirements pull in tornado 5 yet except for jupyterhub master
-    - if [ $JHUB_VER != "master" -a $JHUB_VER != "0.9.0b2" ]; then pip install "tornado<5.0"; fi
+    - if [ $JHUB_VER != "master" -a $JHUB_VER != "0.9.1" ]; then pip install "tornado<5.0"; fi


Would be good to update the comment here to add clarity that 0.9.0 and later should install tornado>=5.0. This line should be updated as well.

willingc · 2018-08-08T00:54:44Z

README.md

+
+Unless otherwise stated for a specific spawner, assume that spawners
+*do* evaluate shell environment for users and thus the [security
+requriemnts of JupyterHub security for untrusted


typo: requirements

thanks, updated.

willingc · 2018-08-08T00:55:46Z

README.md

+*do* evaluate shell environment for users and thus the [security
+requriemnts of JupyterHub security for untrusted
+users](https://jupyterhub.readthedocs.io/en/stable/reference/websecurity.html)
+are not fulfilled.  This is something which we are working on.


We should probably be a bit more precise as to what is not fulfilled.

Thanks! done.

willingc · 2018-08-08T00:59:25Z

README.md

@@ -146,6 +157,14 @@ clusters, as well as an option to run a local notebook directly on the jupyterhu
 * Add new option exec_prefix, which defaults to `sudo -E -u {username}`.  This replaces explicit `sudo` in every batch command - changes in local commands may be needed.
 * Add many more tests.
 * Update minimum requirements to JupyterHub 0.8.1 and Python 3.4.
+* New option: `req_keepvars_extra`, which allows keeping extra variables in addition to what is defined by JupyterHub itself (addition of variables to keep instead of replacement).  #99


To help the end user, we should divide into subsections: Added, Changed, Fixed, Removed

Updated in the changelog PR.

willingc · 2018-08-08T01:00:25Z

SPAWNERS.md

+# Checklist for making spawners
+
+- Does your spawner read shell environment before starting?  (See
+  [Jupyterhub Security](https://jupyterhub.readthedocs.io/en/stable/reference/websecurity.html).


If so, please add documentation so that the user is aware of this possibility.

Thanks! Added as a note at the start of the section.

willingc · 2018-08-08T01:04:59Z

batchspawner/batchspawner.py

-        """Add user environment variables"""
+        """Add user environment variables.
+
+        Everything here should be passed to the user's job.  If it is


All user environment variables will be passed to the user's job.

Caution: If these variables are used for authentication to the batch system commands as an admin,
be aware that the user will receive access to these as well.

Thanks!

This had made me realize one possible solution is to separate req_keepvars and get_env more. Right now they are basically the same, but by using them more cleverly we can separate admin variables and user variables. It's already possible in fact, but not very convenient. Will comment in the other security tracking issue.

willingc · 2018-08-08T01:07:19Z

batchspawner/batchspawner.py

+        ).tag(config=True)
+
+    req_srun = Unicode('srun',
+        help="Job step wrapper, default 'srun'.  Set to '' you do not want "


Perhaps: Set req_srun='' to disable running in job step, and note that this affects environment handling.

Done, thanks.

rkdarst · 2018-08-08T07:36:19Z

I agree that it is better to merge PRs individually. But... PRs are not really merged or commented on. Practically everything in here was a separate pull request that doesn't seem to have any objections, and it got to the point where things can not be tested individually anymore (for example, #110 means all tests break, several of these resulted in merge conflicts). I'm trying to keep everything separated into branches as much as possible, and I'm happy to do enough git magic to separate out again if someone would like.

You can review all of the separate PRs instead of this branch, and internally I will keep this up to date and tested. Also, if someone merges all of the other separate PRs, this PR becomes obsolete (but that will require manual merging in some places).

Thanks for the reviews! I will go update the individual branches and re-merge into here.

@willingc

- Thanks @willingc

@willingc

- Thanks @willingc

rkdarst · 2018-08-08T08:00:28Z

Also I need to make a new branch to test locally anyway, and i I will be doing that work, may as well make it public to save effort... dealing with long-running branches is not one of my favorite things to do.

@willingc

- Thanks to @willingc for the suggestion.

minrk · 2018-08-10T12:09:45Z

Thanks for getting this started! I think we should land this one to keep things moving, and try to stay more on top of individual PRs in the future. Whenever folks are depending on multiple unmerged PRs, things get tricky.

willingc · 2018-08-10T18:49:39Z

Thanks @rkdarst for persisting and keeping this moving.

mbmilligan · 2018-08-10T21:28:30Z

Agreed. Thanks to everyone for the help in keeping things moving!

Gwen and others added 29 commits June 15, 2018 17:02

Add reservation option.

5201449

Fix spacing.

370af5b

Add SPAWNERS.md, a list of specific info on spawners

afbf5d3

Add tests for req_keepvars and req_keepvars_extra

359fc35

ENH (minor): bool() instead of True/False, import, a bit more consist…

05a747e

…ency with spacing

RF: consistency for SlurmSpawner - removed dated __doc__ and placed b…

9165b0e

…atch_script first

RF(minor): remove not needed \ for newline continuation within functi…

35f0919

…on call signature For consistency with jupyterhub itself

RF: revert back to if ... return True else return False

6546ca9

RF: another removal of trailing \

5a9d6e8

run_command: Increase debugging output in case of error

9a00bee

- Thanks to @krinsman in jupyterhub#90.

run_command: Fix possible deadlock

a23b3d4

- Reading from stdout and stderr separately can produce a deadlock. I assume that the separate proc.wait_for_exit() doesn't matter here. - Thanks to @krinsman in jupyterhub#90.

Travis: update for jupyterhub 9.0.1 and python 3.7

c6b34a9

Merge branches 'travis_jh9.0.1', 'run_command_deadlock', 'run_command…

1b4f03d

…_debug', 'keepvars_extra', 'spawner_detail', 'prologue_epilogue' and 'exec_prefix_template_separately' into dev

Travis: update for jupyterhub 9.0.1 and python 3.7

327d9bd

Pin jsonschema=2.6.0 to fix test errors

fa11cb1

Merge branch 'travis_jh9.0.1' into dev

61a4a0b

Conflicts: .travis.yml (keep travis_jh9.0.1 side)

Pass the environment dict to all commands (query and cancel)

228107e

- Previously, only the query command got the full enivornment. - If this is used to authenticate to the submit/query/cancel commands, then the user's environment gets this also. - Closes: jupyterhub#108

Merge branch 'env_dict_to_commands' into dev

14287ac

SPAWNERS.md: update SlurmSpawner and general info.

a696601

Merge branch 'spawner_detail' into dev

b8d43de

Merge remote-tracking branch 'upstream/pr/100' into dev

8633785

Conflicts: batchspawner/batchspawner.py

SlurmSpawner: Add reservation option to batch script

02c33a6

SlurmSpawner: add test for reservation option

81ff85a

Merge branch 'slurm_reservation' into dev

46fc2bd

Conflicts: batchspawner/batchspawner.py batchspawner/tests/test_spawners.py

Update changelog for release

4cf3732

Merge branch 'changelog' into dev

c91dc0b

rkdarst mentioned this pull request Aug 7, 2018

Time for a new Batchspawner release #103

Closed

willingc requested a review from minrk August 8, 2018 00:44

willingc reviewed Aug 8, 2018

View reviewed changes

rkdarst force-pushed the dev branch from 431f065 to c91dc0b Compare August 8, 2018 07:27

rkdarst added 5 commits August 8, 2018 10:47

get_env: clarify documentation and security risk

b9f71ab

Spawner detail list: updates from code review

d4e21f4

- Thanks @willingc

Merge branches 'env_dict_to_commands' and 'spawner_detail' into dev

5192b98

README.md: Typo fix

594ba77

- Thanks @willingc

Merge branch 'spawner_detail' into dev

2045a9b

rkdarst added 2 commits August 8, 2018 11:13

Changelog: Split into sections by topic.

21af0af

- Thanks to @willingc for the suggestion.

Merge branch 'changelog' into dev

755f4fe

minrk merged commit 4747946 into jupyterhub:master Aug 10, 2018

rkdarst deleted the dev branch August 11, 2018 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration branch for next release #112

Integration branch for next release #112

rkdarst commented Aug 7, 2018

willingc commented Aug 8, 2018

willingc left a comment

willingc Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

willingc Aug 8, 2018

rkdarst Aug 8, 2018

rkdarst commented Aug 8, 2018

rkdarst commented Aug 8, 2018

minrk commented Aug 10, 2018

willingc commented Aug 10, 2018

mbmilligan commented Aug 10, 2018

Integration branch for next release #112

Integration branch for next release #112

Conversation

rkdarst commented Aug 7, 2018

willingc commented Aug 8, 2018

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkdarst commented Aug 8, 2018

rkdarst commented Aug 8, 2018

minrk commented Aug 10, 2018

willingc commented Aug 10, 2018

mbmilligan commented Aug 10, 2018