Skip to content

Don't delete PVC if it may be shared by others #883

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+92 −29
Open

Don't delete PVC if it may be shared by others #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
438e398
Don't delete user PVC if shared or from old template
manics Feb 24, 2025
ba9d142
Test PVC isn't deleted if pvc template changes
manics Feb 26, 2025
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
18 changes: 18 additions & 0 deletions kubespawner/spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3815,6 +3815,24 @@ async def delete_forever(self):
)
return

if (
not self.name
and '{user_server}' not in self.pvc_name_template
and '{username}' not in self.pvc_name_template
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for leaving this hanging for so long, but these aren't the only username-sensitive template fields (anymore). We could remove the leading { on both and it would capture {escaped_username}, {safe_user_server}, etc.. There is also the userid field.

Another, looser check would be to look for the strings username and user_server without the bran.

Another, more rigorous check is to use Formatter.parse to extract the fields and check them directly, rather than substring matching:

from string import Formatter

format_fields = [f[1] for f in Formatter().parse(s) if f[1] is not None]
user_sensitive = False
for (_literal_text, field_name, _format_spec, _conversion) in Formatter().parse(template):
    if field_name and 'user' in field_name:
        # template is user-sensitive
        user_sensitive = True

Otherwise, 👍 to the change.

):
# PVC is shared by multiple users
self.log.info(
f"Not deleting shared pvc for user {log_name}: {self.pvc_name}"
)
return

if self.pvc_name != self._expand_user_properties(self.pvc_name_template):
# PVC was created using a different template
self.log.info(
f"Not deleting pvc for user {log_name}: {self.pvc_name} does not match {self.pvc_name_template}"
)
return

await exponential_backoff(
partial(
self._make_delete_pvc_request,
Expand Down
103 changes: 74 additions & 29 deletions tests/test_spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
import base64
import json
import os
from copy import deepcopy
from functools import partial
from unittest.mock import Mock
from uuid import uuid4

import jupyterhub
import pytest
Expand Down Expand Up @@ -1856,45 +1858,85 @@ async def test_url_changed(kube_ns, kube_client, config, hub_pod, hub):
await spawner.stop()


async def test_delete_pvc(kube_ns, kube_client, hub, config):
@pytest.mark.parametrize(
"pvc_template1,pvc_template2,should_delete",
[
(None, None, True),
("not-templated", "not-templated", False),
(None, "modified-{user_server}", False),
("{user_server}-custom", "{user_server}-custom", True),
("{username}-custom", "{username}-custom", True),
# This is currently allowed even though the templates are different,
# because we're testing with the default server so user_server==username
("{user_server}-custom", "{username}-custom", True),
# TODO: Test that deleting a named server doesn't delete "{username}-custom"
],
)
async def test_delete_pvc(
kube_ns, kube_client, hub, config, pvc_template1, pvc_template2, should_delete
):

config.KubeSpawner.storage_pvc_ensure = True
config.KubeSpawner.storage_capacity = '1M'
config2 = deepcopy(config)
if pvc_template1:
config.KubeSpawner.pvc_name_template = pvc_template1
if pvc_template2:
config2.KubeSpawner.pvc_name_template = pvc_template2
mockusername = "u-" + str(uuid4()).split("-")[0]

async def start_stop_pod(spawner):
spawner.api = kube_client

# start the spawner
await spawner.start()

# verify the pod exists
pod_name = "jupyter-%s" % spawner.user.name
pods = (await kube_client.list_namespaced_pod(kube_ns)).items
pod_names = [p.metadata.name for p in pods]
assert pod_name in pod_names

# verify PVC is created
pvc_name = spawner.pvc_name
pvc_list = (
await kube_client.list_namespaced_persistent_volume_claim(kube_ns)
).items
pvc_names = [s.metadata.name for s in pvc_list]
assert pvc_name in pvc_names

spawner = KubeSpawner(
state = spawner.get_state()

# stop the pod
await spawner.stop()

# verify pod is gone
pods = (await kube_client.list_namespaced_pod(kube_ns)).items
pod_names = [p.metadata.name for p in pods]
assert "jupyter-%s" % spawner.user.name not in pod_names
return pvc_name, state

spawner1 = KubeSpawner(
hub=hub,
user=MockUser(name="mockuser"),
user=MockUser(name=mockusername),
config=config,
_mock=True,
)
spawner.api = kube_client

# start the spawner
await spawner.start()
pvc_name, state = await start_stop_pod(spawner1)

# verify the pod exists
pod_name = "jupyter-%s" % spawner.user.name
pods = (await kube_client.list_namespaced_pod(kube_ns)).items
pod_names = [p.metadata.name for p in pods]
assert pod_name in pod_names

# verify PVC is created
pvc_name = spawner.pvc_name
pvc_list = (
await kube_client.list_namespaced_persistent_volume_claim(kube_ns)
).items
pvc_names = [s.metadata.name for s in pvc_list]
assert pvc_name in pvc_names

# stop the pod
await spawner.stop()
spawner2 = KubeSpawner(
hub=hub,
user=MockUser(name=mockusername),
config=config2,
_mock=True,
)
spawner2.load_state(state)
pvc_name2, state2 = await start_stop_pod(spawner2)

# verify pod is gone
pods = (await kube_client.list_namespaced_pod(kube_ns)).items
pod_names = [p.metadata.name for p in pods]
assert "jupyter-%s" % spawner.user.name not in pod_names
assert pvc_name == pvc_name2

# delete the PVC
await spawner.delete_forever()
await spawner2.delete_forever()

# verify PVC is deleted, it may take a little while
for i in range(5):
Expand All @@ -1906,7 +1948,10 @@ async def test_delete_pvc(kube_ns, kube_client, hub, config):
await asyncio.sleep(1)
else:
break
assert pvc_name not in pvc_names
if should_delete:
assert pvc_name not in pvc_names
else:
assert pvc_name in pvc_names


async def test_ipv6_addr():
Expand Down