Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tickets/DM-34218: expand user reset env beyond .local #133

Merged
merged 8 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ repos:
- toml

- repo: https://github.com/psf/black
rev: 22.1.0
rev: 22.3.0
hooks:
- id: black

Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM jupyterhub/jupyterhub:2.1.1 as base-image
FROM jupyterhub/jupyterhub:2.2.0 as base-image

# Update system packages
COPY scripts/install-base-packages.sh .
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
.PHONY: update-deps
update-deps:
pip install --upgrade pip-tools 'pip<22' setuptools
pip install --upgrade pip-tools pip setuptools
pip-compile --upgrade --build-isolation --generate-hashes --output-file requirements/main.txt requirements/main.in
pip-compile --upgrade --build-isolation --generate-hashes --output-file requirements/dev.txt requirements/dev.in

Expand Down
3 changes: 2 additions & 1 deletion requirements/dev.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#
# This file is autogenerated by pip-compile with python 3.10
# This file is autogenerated by pip-compile with python 3.9
# To update, run:
#
# pip-compile --generate-hashes --output-file=requirements/dev.txt requirements/dev.in
Expand Down Expand Up @@ -374,6 +374,7 @@ pytest==7.1.1 \
# -r requirements/dev.in
# pytest-asyncio
pytest-asyncio==0.18.3 \
--hash=sha256:16cf40bdf2b4fb7fc8e4b82bd05ce3fbcd454cbf7b92afc445fe299dabb88213 \
--hash=sha256:7659bdb0a9eb9c6e3ef992eef11a2b3e69697800ad02fb06374a210d85b29f91 \
--hash=sha256:8fafa6c52161addfd41ee7ab35f11836c5a16ec208f93ee388f752bea3493a84
# via -r requirements/dev.in
Expand Down
12 changes: 4 additions & 8 deletions requirements/main.in
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,14 @@ jupyterhub-idle-culler
psycopg2
ruamel.yaml
tornado

kubernetes-asyncio

# Until changes are accepted upstream and a new kubespawner release happens:
rubin-kubespawner
# Until z2jh adopts kubespawner-asyncio:
kubernetes
jupyterhub-kubespawner

# Always pin jupyterhub to a specific version. We don't want it to be
# upgraded without our explicit approval, and it should be upgraded in
# lockstep with the base image in Dockerfile.
jupyterhub==2.1.1
# lockstep with the base image in Dockerfile and with the app version
# in the JupyterHub helm chart (https://jupyterhub.github.io/helm-chart/)
jupyterhub==2.2.0

# Required by alembic for Python 3.8, so install it unconditionally until
# Python 3.8 support is dropped so that we have consistent dependencies.
Expand Down
162 changes: 59 additions & 103 deletions requirements/main.txt

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions src/nublado2/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,10 @@
name="enable_debug" value="true">
<label for="enable_debug">Enable debug logs</label><br />

<input type="checkbox" id="clear_dotlocal"
name="clear_dotlocal" value="true">
<label for="clear_dotlocal">
Clear <tt>.local</tt> directory (caution!)
<input type="checkbox" id="reset_user_env"
name="reset_user_env" value="true">
<label for="reset_user_env">
Reset user environment: relocate .cache, .jupyter, and .local
</label><br />
</div>
</td>
Expand Down
13 changes: 6 additions & 7 deletions src/nublado2/resourcemgr.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,11 @@ def __init__(self) -> None:
self.provisioner = Provisioner()
self.yaml = YAML()
self.yaml.indent(mapping=2, sequence=4, offset=2)
# You can't create the shared_clients here; they fail to
# serialize and the Hub won't start.

async def create_user_resources(
self, spawner: KubeSpawner, options: SelectedOptions
) -> None:
"""Create the user resources for this spawning session."""
# This actually gets called in a pre-spawn hook, which means no one
# has yet called start() or poll() on the spawner, which means
# we need to initialize its resources (particularly its API
# client) ourselves.
await spawner.initialize_reflectors_and_clients()
await self.provisioner.provision_homedir(spawner)
try:
await exponential_backoff(
Expand Down Expand Up @@ -119,6 +112,9 @@ async def _create_kubernetes_resources(
self, spawner: KubeSpawner, options: SelectedOptions
) -> None:
api_client = shared_client("ApiClient")
# This works around an infelicity in upstream Kubespawner's
# shared client implementation
api_client.api_client = api_client
custom_api = shared_client("CustomObjectsApi")
template_values = await self._build_template_values(spawner, options)

Expand Down Expand Up @@ -190,6 +186,9 @@ async def _create_kubernetes_resources(
async def _build_dask_template(self, spawner: KubeSpawner) -> str:
"""Build a template for dask workers from the jupyter pod manifest."""
api_client = shared_client("ApiClient")
# This works around an infelicity in upstream Kubespawner's
# shared client implementation
api_client.api_client = api_client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you show me the error message that you get if you don't do this? I've read the relevant source code for both kubespawner and kubernetes_asyncio and can't find any reason why this would be necessary. The kubernetes_asyncio ApiClient object has a sanitize_for_serialization method that's an entirely normal method and doesn't use an api_client attribute of itself in any way, and shared_client("ApiClient") should just return a kubernetes_asyncio ApiClient. And you don't seem to have to do this down below in the code that's making actual Kubernetes calls. I also don't see anything like this code in kubespawner with a search.

So I'm fairly confused, and the commit message and comment didn't really help unconfuse me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-81' coro=<shared_client.<locals>.close_client_task() done, defined at /usr/local/lib/python3.8/dist-packages/kubespawner/clients.py:55> exception=AttributeError("'ApiClient' object has no attribute 'api_client'")>
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/kubespawner/clients.py", line 57, in close_client_task
    async with client.api_client:
AttributeError: 'ApiClient' object has no attribute 'api_client'

Copy link
Member Author

@athornton athornton Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the shared_client's close_client_task() uses an async with on the api_client attr of the client object to do its thing and cleanly shut down the shared client when it goes out of scope (that is, when the Spawner object is finalized).

client objects that point to actual K8s APIs (e.g. CoreV1AP1) have such an attr. shared_client("ApiClient") does not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@rra rra Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! The problem is that shared_client("ApiClient") is simply invalid; the only thing that should be retrieved via shared_client are specific API clients. That makes sense. I think the fix is to replace every instance of api_client = shared_client("ApiClient") with:

api_client = shared_client("CoreV1Api").api_client

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better still: I already have one of those, because I have a spawner everywhere I use that client, and its api attr is already a CoreV1Api shared client.

dask_template = await spawner.get_pod_manifest()

# Here we make a few mangles to the jupyter pod manifest
Expand Down
13 changes: 7 additions & 6 deletions src/nublado2/selectedoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def __init__(self, options: Dict[str, Any]) -> None:
self._size = nc.sizes[size_name]

self._debug = "TRUE" if "enable_debug" in options else ""
self._clear_dotlocal = "TRUE" if "clear_dotlocal" in options else ""
self._reset_user_env = "TRUE" if "reset_user_env" in options else ""

@property
def debug(self) -> str:
Expand All @@ -43,12 +43,13 @@ def debug(self) -> str:
return self._debug

@property
def clear_dotlocal(self) -> str:
"""String to pass in for CLEAR_DOTLOCAL variable in the lab.
def reset_user_env(self) -> str:
"""String to pass in for RESET_USER_ENV variable in the lab.

This gets rid of the user's .local directory which may
cause issues during startup."""
return self._clear_dotlocal
This moves the user's .local, .cache, and .jupyter environments
aside. That in turn allows getting out of the common case where
local package installation conflicts with the RSP machinery."""
return self._reset_user_env

@property
def image_info(self) -> ImageInfo:
Expand Down