Add cpu_weight config, relates to Systemd's CPUAccounting and CPUWeight
#125
Conversation
consideRatio
changed the title
cpu_weight config, relates to Systemd's CPUAccounting and CPUWeight
… into cpu-weight
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Nice @ticoneva, this makes sense to me!!
- I believe but don't know that CPUAccounting and CPUWeight are supported by systemd v245 and later. Do you know that we can assume these to be around in systemd v245?
- Resolution to comment about setting CPU_WEIGHT environment variable
| if self.cpu_weight: | ||
| env["CPU_WEIGHT"] = str(self.cpu_weight) | ||
|
|
There was a problem hiding this comment.
To add a new env var here sets a new precendece for systemdspawner of passing environment variables related to the config set.
I suspect this can add to maintenance burden long term, so I suggest this is removed unless there is a solid motivation for retaining it.
I know some information about system constraints can be detected in a standardized ways, for example as done in dask/distributed. So if this is relevnat information for the user environment, maybe it can be captured in a similar way?
| @@ -304,6 +319,11 @@ async def start(self): | |||
| properties["CPUAccounting"] = "yes" | |||
| properties["CPUQuota"] = f"{int(self.cpu_limit * 100)}%" | |||
|
|
|||
| if self.cpu_weight is not None: | |||
| # FIXME: Detect & use proper properties for v1 vs v2 cgroups | |||
There was a problem hiding this comment.
I've seen similar comments, but I don't know much about it, so I feel quite inactionable about these FIXME comments overall.
Are these systemd settings only valid in certain contexts with regards to available cgroup versions?
| This info is exposed to the single-user server as the environment variable | ||
| `CPU_WEIGHT` as an integer. | ||
|
|
There was a problem hiding this comment.
Related to discussion in another comment about not setting this env var.
They are around in that form since at least systemd 241, but my memory is a bit hazy since they might require cgroup v2, which legacy versions of distros might not yet use, but the hybrid cgroup layout should be fine. |
|
Sorry for the much delayed review here, @ticoneva. I agree with @consideRatio here that we should move towards allowing people to pass through params directly to systemd, instead of special casing them here. With that, I'm going to close this PR. |
This PR introduces a new configuration option
cpu_weight. This sets the systemd propertyCPUWeight, which causes available CPU time to be sliced in proportion to each user's weight.This is a follow-up to PR #98.