-
Notifications
You must be signed in to change notification settings - Fork 65
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
Enable unlisted_choice
in LEAP hub
#3047
Conversation
Merging this PR will trigger the following deployment actions. Support and Staging deployments
Production deployments
|
3d256f9
to
8d0d3d1
Compare
This unfortunately still has the problem previously reported, where once you use an image as the other choice, you can't use any other image :( This shouldn't be deployed to the LEAP prod hub until we figure that out. |
I reproduce this once before in staging.leap.2i2c.cloud earlier today, but I've not been able to reproduce it later or with a local k3s cluster with jupyterhub 4.0.2 + kubespawner's main branch. |
I've just pushed a commit that I think fixes it. I've manually deployed it to staging https://staging.leap.2i2c.cloud and seems to be working. It looks like it was the same bug, with the same fix as the one in jupyterhub/kubespawner#766. But because we were using a callable |
Yesss, I can confirm that fixes it @GeorgianaElena!!!! \o/ |
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6013781012 |
Nice work @GeorgianaElena!!! I think there is another fix to make in kubespawner also. I think this logic shouldn't just take the value returned by profile_list, but should make a copy of the value returned by a profile_list callable. if callable(self.profile_list):
profile_list = await maybe_future(self.profile_list(self))
else:
# Use a copy of the self.profile_list dict,
# otherwise we might unintentionally modify it
profile_list = copy.deepcopy(self.profile_list)
profile_list = self._populate_profile_list_defaults(profile_list) This upstream fix isn't a problem if the callable profile_list returns a copy at all times as done in the leap hub - but we don't return a copy at all times, for example when we return this:
|
I'll submit a kubespawner PR to make a copy of the returned value of a profile_list callable, and then we should be fine for all hubs - but we will need the fix in b62a46a as well I think. |
@consideRatio I just looked into that, and I don't think that works - the problem is in
profile_list , and modifying that is the problem. In the kubespawner code, the value returned by the callable isn't re-assigned to self.profile_list anywhere I can see, so is probably fine.
I think we should provide guidance that says "if you are using profile_list as a callable, make sure to make a copy!". But curious to see the outcome of your investigation too. Perhaps I'm wrong. |
I agree that the problem we have for the leap hub is the specific one patched by @GeorgianaElena, and would be a problem even if we patched what I consider patching in KubeSpawner. But, I think there is another bug, and I think we are exposed to it because we break the advice of "if you are using profile_list as a callable, make sure to make a copy!".
|
Hmm, I don't fully understand so am curious to see your PR @consideRatio. |
Ref #2146
Just brings in @GeorgianaElena's work in jupyterhub/kubespawner#769