Skip to content

Fix #761 expand pvc_name outside of init - #820

Open
Ph0tonic wants to merge 3 commits into
jupyterhub:mainfrom
Ph0tonic:main
Open

Fix #761 expand pvc_name outside of init#820
Ph0tonic wants to merge 3 commits into
jupyterhub:mainfrom
Ph0tonic:main

Conversation

@Ph0tonic

@Ph0tonic Ph0tonic commented Jan 16, 2024

Copy link
Copy Markdown

Partial fix for #761 by moving the expansion of pvc_name and secret_name outside of __init__.

\cc @yuvipanda

@welcome

welcome Bot commented Jan 16, 2024

Copy link
Copy Markdown

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@manics

manics commented Jan 16, 2024

Copy link
Copy Markdown
Member

I've added the breaking label since this changes method signatures that may be used by subclasses.

@consideRatio consideRatio reopened this Jun 1, 2024
Comment thread kubespawner/spawner.py

async def _start(self):
"""Start the user's pod"""
pvc_name = self._expand_user_properties(self.pvc_name_template)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've been looking at this in #744, and pvc_name should actually be one of the things persisted in Spawner state, because the pvc name should probably be preserved to avoid data loss even if it. Resolving the template here would ensure the new template is used and the old pvc is orphaned. But that's intended when overrides are used as in #761.

Should we:

  1. not try to track existing pvcs (status quo, this PR), or
  2. try not to lose data (what I'm trying to do in add 'safe' slug scheme #744)

If we should try not to lose data, how do we distinguish between a changed pvc for a profile (should change) vs config (should not change if prior pvc already created)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants