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

Transition towards passing through config to Systemd spawned services without providing dedicated config? #128

Open
consideRatio opened this issue May 31, 2023 · 4 comments

Comments

@consideRatio
Copy link
Member

Learning more about systemd, I understand that we may want to expose a lot of configuration that could just go straight to how we configure the started systemd transient service. If its just passthrough configuration, should this project then provide a dedicated python traitlets based config option for it that we need to maintain? We do so for a few things now, but should we keep adding config to pass through systemd config?

Consider for example, in #125 we have a proposal to add SystemdSpawner.cpu_weight which maps directly to configuring CPUWeight with associated docs, and we provide similar docs as Systemd does.

In the jupyterhub distribution https://github.com/jupyterhub/zero-to-jupyterhub-k8s, we have had a similar situations that I found to be unsustainable. We provided the same config options for the Helm chart the project represents to configure the underlying GitLabOAuthenticator class for example. As soon as there was something new to configure, and a user wanted it, we had to expose it as a Helm chart config option. We decided that we simply shouldn't try to provide a 1:1 mapping of chart config, but instead provide a way to passthrough config, so that no matter what new functionality was introduced in some software dependency, it could be configured without the Helm chart knowing about it.

SystemdSpawner is in a similar situation I think, where Systemd has a lot of config options, and SystemdSpawner creates python based config to map to it.

I suggest we overview the existing config, and start ensuring users become confident on how to configure the spawned Systemd services with passthrough config directly, instead of relying on SystemdSpawner to be aware of all options provided by Systemd as it changes.

@ticoneva
Copy link

ticoneva commented Jun 1, 2023

In general, I agree that adding more systemd options to the spawner is not a sustainable solution. I am not sure why the base jupyterhub/spawner has cpu_limit and mem_limit set as unimplemented traitlet options---perhaps it's because they both involve changing two properties? If that's the reason, then cpu_weight is in the same situation.

I personally find cpu_limit of very llimited use in comparison to cpu_weight---because affected processes are unaware of the limit, multithread performance is generally very poor. It just feels weird to me that the former is a traitlet option when the latter is not.

@behrmann
Copy link
Contributor

behrmann commented Jun 1, 2023

Yes, I too think that mirroring all options is unsustainable. It's also quite limiting. To be honest I've never used vanilla systemd-spawner in production, but always my private forks, which you can find in two PRs, the most recent one being #100. The general reasoning behind that is that I don't want jupyterhub to be running as root, which it needs to be (or at least need to be for all intents and purposes) to use systemd-run, which is makes the spawner just a dispatcher for a single type of service that then can use whatever systemd offers.

That being said in the context of using systemd-run, it would probably make most sense to deprecate all options and just have a config dict of systemd unit properties and values and use that.

@manics
Copy link
Member

manics commented Jun 1, 2023

I agree with moving to pass-through configuration where possible. I think we could also make the case that pass-though configuration should override the traitlets configuration so that configuration doesn't have to be split across two interfaces.

@ticoneva Many of the existing traitlets in the parent class are based on what's generically useful across multiple spawners. CPU/memory limits are supporting in many environments such as containers, HPC, local systems without systemd, and are easy to understand.

@consideRatio consideRatio pinned this issue Jun 1, 2023
@yuvipanda
Copy link
Collaborator

I'm generally in favor. jupyterhub/kubespawner#248 is the relavant issue in kubespawner.

As the person who made this choice in both KubeSpawner and SystemdSpawner initially, I want to apologize for making this particular choice. My intent was that folks should be able to use KubeSpawner or SystemdSpawner without having to fully understand kubernetes or systemd. However, that is a function of JupyterHub distributions (like z2jh or TLJH), which did not exist at that time. Hindsight 20/20, etc.

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

No branches or pull requests

5 participants