feat: cpu and memory limits for systemd#330
Merged
jason-lynch merged 1 commit intomainfrom Apr 6, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request introduces typed data structures for systemd unit file generation and enhances the Patroni unit configuration to support CPU and memory resource limits. Golden test fixtures validate the generated systemd units under various resource specifications using a new test framework with an Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 30 complexity · 0 duplication
Metric Results Complexity 30 Duplication 0
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/internal/orchestrator/systemd/patroni_unit.go (1)
18-21: Use a deterministic servicePATHinstead of inheriting the orchestrator process environment.Lines 18-21 persist whatever
PATHthe control-plane process happens to have at render time into the unit file. The new goldens already show root-specific entries like/root/.local/bin, so unit contents and runtime command resolution now vary by host even though the service itself runs aspostgres. Prefer a fixed system fallback (or a config-supplied one) afterpgBinPathinstead ofos.Getenv("PATH").♻️ Example direction
- pathEnv := pgBinPath - if p := os.Getenv("PATH"); p != "" { - pathEnv += ":" + p - } + const defaultServicePath = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + pathEnv := pgBinPath + ":" + defaultServicePath🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/systemd/patroni_unit.go` around lines 18 - 21, The code sets pathEnv by appending the current process PATH via os.Getenv("PATH"), causing non-deterministic unit files; change the logic that builds pathEnv (the variable in patroni_unit.go that currently uses pgBinPath + os.Getenv("PATH")) to append a fixed system fallback PATH (e.g. "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") or a configurable default instead of reading os.Getenv("PATH"), ensuring deterministic unit content while still prefixing pgBinPath.server/internal/orchestrator/systemd/unit_options_test.go (1)
56-86: Consider adding a combined CPU + memory limit test case.The current test cases validate CPU and memory limits independently. Adding a case that sets both
cpusandmemoryByteswould ensure the options compose correctly without conflicts in the generated unit file.💡 Suggested additional test case
{ name: "memory limit", paths: paths, pgBinPath: pgBinPath, memoryBytes: 8_589_934_592, // 8GiB in bytes }, + { + name: "cpu and memory limit", + paths: paths, + pgBinPath: pgBinPath, + cpus: 2, + memoryBytes: 4_294_967_296, // 4GiB in bytes + }, } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/systemd/unit_options_test.go` around lines 56 - 86, Add a new test case to the table in unit_options_test.go (the for loop over []struct{name, paths, pgBinPath, cpus, memoryBytes}) that sets both cpus and memoryBytes (e.g., name "cpu + memory limit", cpus: 2 or 14, memoryBytes: 8_589_934_592) so the test exercises composition of CPU and memory options when generating the unit file; ensure the new case uses the same paths and pgBinPath variables as the other cases and follows the existing test assertions for generated unit content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/systemd/orchestrator.go`:
- Line 262: The Patroni/Postgres config generator is still using host totals
instead of the cgroup-enforced limits; update the call site that creates
common.NewPatroniConfigGenerator to pass the effective per-instance limits
(spec.CPUs and spec.MemoryBytes) when they are set so the generated Patroni
config matches the unit limits used in PatroniUnitOptions(paths,
o.packageManager.BinDir(pgMajor), spec.CPUs, spec.MemoryBytes); ensure
NewPatroniConfigGenerator receives and uses those same CPU/memory values
(falling back to host totals only if spec.CPUs/spec.MemoryBytes are unset) so
worker/memory settings are bounded by the service cgroup.
---
Nitpick comments:
In `@server/internal/orchestrator/systemd/patroni_unit.go`:
- Around line 18-21: The code sets pathEnv by appending the current process PATH
via os.Getenv("PATH"), causing non-deterministic unit files; change the logic
that builds pathEnv (the variable in patroni_unit.go that currently uses
pgBinPath + os.Getenv("PATH")) to append a fixed system fallback PATH (e.g.
"/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") or a
configurable default instead of reading os.Getenv("PATH"), ensuring
deterministic unit content while still prefixing pgBinPath.
In `@server/internal/orchestrator/systemd/unit_options_test.go`:
- Around line 56-86: Add a new test case to the table in unit_options_test.go
(the for loop over []struct{name, paths, pgBinPath, cpus, memoryBytes}) that
sets both cpus and memoryBytes (e.g., name "cpu + memory limit", cpus: 2 or 14,
memoryBytes: 8_589_934_592) so the test exercises composition of CPU and memory
options when generating the unit file; ensure the new case uses the same paths
and pgBinPath variables as the other cases and follows the existing test
assertions for generated unit content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 160fc849-f9e0-458a-b4d7-90cad8e73458
📒 Files selected for processing (9)
server/internal/orchestrator/systemd/golden_test/TestUnitOptions/PatroniUnitOptions/cpu_limit.serviceserver/internal/orchestrator/systemd/golden_test/TestUnitOptions/PatroniUnitOptions/fractional_cpu_limit.serviceserver/internal/orchestrator/systemd/golden_test/TestUnitOptions/PatroniUnitOptions/memory_limit.serviceserver/internal/orchestrator/systemd/golden_test/TestUnitOptions/PatroniUnitOptions/minimal.serviceserver/internal/orchestrator/systemd/main_test.goserver/internal/orchestrator/systemd/orchestrator.goserver/internal/orchestrator/systemd/patroni_unit.goserver/internal/orchestrator/systemd/unit_options.goserver/internal/orchestrator/systemd/unit_options_test.go
Adds CPU and memory limits for databases deployed with systemd using these service properties: - `CPUQuota`: Expressed as a percentage where 100% = 1 CPU core. - `MemoryLimit`: Expressed as an integer with optional units, such as `1G`. Defaults to bytes when units are unspecified. These are populated from existing properties on our instance spec. This commit includes a small refactor to the way we produce and test unit file options. These changes are aimed at making it easier to add new types of unit files when we implement supporting services for systemd. PLAT-417
jason-lynch
force-pushed
the
feat/PLAT-417/cpu-memory-limits
branch
from
6c770b3 to
dc4509a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds CPU and memory limits for databases deployed with systemd using these service properties:
CPUQuota: Expressed as a percentage where 100% = 1 CPU core.MemoryMax: Expressed as an integer with optional units, such as1G. Defaults to bytes when units are unspecified.These are populated from existing properties on our instance spec.
This commit includes a small refactor to the way we produce and test unit file options. These changes are aimed at making it easier to add new types of unit files when we implement supporting services for systemd.
Testing
PLAT-417