fix: update global patroni params for systemd#398
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds Patroni DCS-to-dynamic-config conversion and schema updates, wires NodeSize-driven failsafe_mode in the config generator, implements PatroniConfig create/update/delete with dynamic wait computation and optional dynamic-config patching, updates systemd reload wiring, updates golden tests and generator tests, and adds an e2e verifying max_connections update. ChangesPatroni Dynamic Config Update Feature
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 | 2 medium |
🟢 Metrics -4 duplication
Metric Results Duplication -4
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/internal/orchestrator/common/patroni_config.go (1)
252-261: 💤 Low valueComputed wait can be negative when DCSLastSeen is stale but within bounds.
When
lastSeenis betweenloopWaitand2*loopWaitin the past (e.g., 15s ago with a 10s loop),time.Until(lastSeen.Add(loopWait))returns a negative duration. Whileutils.SleepContextlikely treats negative durations as zero, clamping explicitly would clarify intent.Suggested fix
if lastSeen.After(lowerBound) && lastSeen.Before(upperBound) { // Compute the time until the next run cycle wait = time.Until(lastSeen.Add(loopWait)) + if wait < 0 { + wait = 0 + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/orchestrator/common/patroni_config.go` around lines 252 - 261, The computed wait based on DCSLastSeen can be negative; in the block that computes wait using time.Until(lastSeen.Add(loopWait)) (involving status.DCSLastSeen, lastSeen, loopWait and wait), clamp the computed duration to a non-negative value before assigning to wait (e.g., compute d := time.Until(lastSeen.Add(loopWait)); if d < 0 { d = 0 }; wait = d) so the code explicitly prevents negative sleeps (which utils.SleepContext may otherwise treat as zero).e2e/patroni_global_params_test.go (1)
13-50: 💤 Low valueMinor:
DatabaseNamedoesn't reflect what the test validates.The test validates
max_connectionschanges, butDatabaseNameis"test_port_change". Consider renaming to"test_max_connections"or similar for clarity.Suggested fix
db := fixture.NewDatabaseFixture(ctx, t, &api.CreateDatabaseRequest{ Spec: &api.DatabaseSpec{ - DatabaseName: "test_port_change", + DatabaseName: "test_max_connections",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/patroni_global_params_test.go` around lines 13 - 50, Rename the test database name to reflect what the test asserts: update the DatabaseName field in the CreateDatabaseRequest's Spec (inside TestUpdatePatroniGlobalParams) from "test_port_change" to a clearer name like "test_max_connections" so the identifier matches the test intent; update any local references/comments that mention the old name to keep wording consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/patroni_global_params_test.go`:
- Around line 13-50: Rename the test database name to reflect what the test
asserts: update the DatabaseName field in the CreateDatabaseRequest's Spec
(inside TestUpdatePatroniGlobalParams) from "test_port_change" to a clearer name
like "test_max_connections" so the identifier matches the test intent; update
any local references/comments that mention the old name to keep wording
consistent.
In `@server/internal/orchestrator/common/patroni_config.go`:
- Around line 252-261: The computed wait based on DCSLastSeen can be negative;
in the block that computes wait using time.Until(lastSeen.Add(loopWait))
(involving status.DCSLastSeen, lastSeen, loopWait and wait), clamp the computed
duration to a non-negative value before assigning to wait (e.g., compute d :=
time.Until(lastSeen.Add(loopWait)); if d < 0 { d = 0 }; wait = d) so the code
explicitly prevents negative sleeps (which utils.SleepContext may otherwise
treat as zero).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 282e42d3-0776-46a4-9147-ea9e7f712be1
📒 Files selected for processing (13)
e2e/patroni_global_params_test.goserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/enable_fast_basebackup.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/in-place_restore.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/minimal_swarm.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/minimal_systemd.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/with_backup_config.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/with_restore_config.yamlserver/internal/orchestrator/common/patroni_config.goserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/common/patroni_config_generator_test.goserver/internal/orchestrator/systemd/patroni_config.goserver/internal/patroni/client.goserver/internal/patroni/config.go
jason-lynch
force-pushed
the
fix/PLAT-610/update-global-patroni-params
branch
from
d2fa9fa to
775fef7
Compare
jason-lynch
force-pushed
the
fix/PLAT-610/update-global-patroni-params
branch
from
775fef7 to
8cc83a1
Compare
Patroni disallows setting some parameters through the Patroni config file after the cluster is created. Instead, they must be set via the dynamic config API. This commit adds functionality to the systemd Patroni config implementation to patch the dynamic config if the Patroni API is up and if the API reports that it is the primary instance. This commit only affects systemd clusters. I will implement this for Swarm clusters in a subsequent commit by migrating Swarm to use the common Patroni config resource. PLAT-610
jason-lynch
force-pushed
the
fix/PLAT-610/update-global-patroni-params
branch
from
8cc83a1 to
8da1336
Compare
tsivaprasad
approved these changes
Jun 11, 2026
tsivaprasad
left a comment
tsivaprasad
left a comment
Contributor
There was a problem hiding this comment.
Looks Good...!
➜ control-plane git:(fix/PLAT-610/update-global-patroni-params) cp1-req create-database <<EOF | cp-follow-task
{
"id": "storefront",
"spec": {
"database_name": "storefront",
"database_users": [
{
"username": "admin",
"password": "password",
"db_owner": true,
"attributes": ["SUPERUSER", "LOGIN"]
}
],
"port": 0,
"patroni_port": 0,
"nodes": [
{ "name": "n1", "host_ids": ["host-1"] }
]
}
}
EOF
{
"database": {
"created_at": "2026-06-11T19:45:12+05:30",
"id": "storefront",
"spec": {
"database_name": "storefront",
"database_users": [
{
"attributes": [
"SUPERUSER",
"LOGIN"
],
"db_owner": true,
"username": "admin"
}
],
"nodes": [
{
"host_ids": [
"host-1"
],
"name": "n1"
}
],
"patroni_port": 0,
"port": 0,
"postgres_version": "18.4",
"spock_version": "5"
},
"state": "creating",
"updated_at": "2026-06-11T19:45:12+05:30"
},
"task": {
"created_at": "2026-06-11T19:45:12+05:30",
"database_id": "storefront",
"entity_id": "storefront",
"scope": "database",
"status": "pending",
"task_id": "019eb709-7a2a-714a-8440-16aa4b71afb5",
"type": "create"
}
}
[2026-06-11T19:45:13+05:30] refreshing current state
[2026-06-11T19:45:13+05:30] finished refreshing current state (took 192.176787ms)
[2026-06-11T19:45:15+05:30] creating resource common.patroni_cluster::n1
[2026-06-11T19:45:15+05:30] finished creating resource common.patroni_cluster::n1 (took 21.584µs)
[2026-06-11T19:45:15+05:30] creating resource filesystem.dir::storefront-n1-689qacsi-instance
[2026-06-11T19:45:15+05:30] finished creating resource filesystem.dir::storefront-n1-689qacsi-instance (took 2.615176ms)
[2026-06-11T19:45:15+05:30] creating resource common.patroni_member::storefront-n1-689qacsi
[2026-06-11T19:45:15+05:30] finished creating resource common.patroni_member::storefront-n1-689qacsi (took 60.125µs)
[2026-06-11T19:45:15+05:30] creating resource filesystem.dir::storefront-n1-689qacsi-data
[2026-06-11T19:45:15+05:30] finished creating resource filesystem.dir::storefront-n1-689qacsi-data (took 988.378µs)
[2026-06-11T19:45:15+05:30] creating resource filesystem.dir::storefront-n1-689qacsi-certificates
[2026-06-11T19:45:15+05:30] finished creating resource filesystem.dir::storefront-n1-689qacsi-certificates (took 677.253µs)
[2026-06-11T19:45:15+05:30] creating resource filesystem.dir::storefront-n1-689qacsi-configs
[2026-06-11T19:45:15+05:30] finished creating resource filesystem.dir::storefront-n1-689qacsi-configs (took 381.918µs)
[2026-06-11T19:45:16+05:30] creating resource common.etcd_creds::storefront-n1-689qacsi
[2026-06-11T19:45:16+05:30] creating resource common.postgres_certs::storefront-n1-689qacsi
[2026-06-11T19:45:16+05:30] finished creating resource common.postgres_certs::storefront-n1-689qacsi (took 11.029913ms)
[2026-06-11T19:45:16+05:30] finished creating resource common.etcd_creds::storefront-n1-689qacsi (took 78.574354ms)
[2026-06-11T19:45:16+05:30] creating resource systemd.patroni_config::storefront-n1-689qacsi
[2026-06-11T19:45:16+05:30] finished creating resource systemd.patroni_config::storefront-n1-689qacsi (took 3.616929ms)
[2026-06-11T19:45:16+05:30] creating resource systemd.unit::patroni-storefront-n1-689qacsi.service:storefront:host-1
[2026-06-11T19:45:16+05:30] finished creating resource systemd.unit::patroni-storefront-n1-689qacsi.service:storefront:host-1 (took 163.170313ms)
[2026-06-11T19:45:17+05:30] creating resource database.instance::storefront-n1-689qacsi
[2026-06-11T19:45:27+05:30] finished creating resource database.instance::storefront-n1-689qacsi (took 10.038586483s)
[2026-06-11T19:45:27+05:30] creating resource database.node::n1
[2026-06-11T19:45:27+05:30] finished creating resource database.node::n1 (took 79.291µs)
[2026-06-11T19:45:27+05:30] creating resource monitor.instance::storefront-n1-689qacsi
[2026-06-11T19:45:27+05:30] finished creating resource monitor.instance::storefront-n1-689qacsi (took 11.249455ms)
[2026-06-11T19:45:28+05:30] creating resource common.pg_service_conf::storefront-n1-689qacsi
[2026-06-11T19:45:28+05:30] finished creating resource common.pg_service_conf::storefront-n1-689qacsi (took 1.353088ms)
[2026-06-11T19:45:28+05:30] creating resource database.postgres_database::n1:storefront
[2026-06-11T19:45:28+05:30] finished creating resource database.postgres_database::n1:storefront (took 286.266736ms)
database entity storefront task 019eb709-7a2a-714a-8440-16aa4b71afb5 completed
➜ control-plane git:(fix/PLAT-610/update-global-patroni-params) cp-psql -i storefront-n1-689qacsi -U admin -- -c 'SHOW max_connections'
max_connections
-----------------
843
(1 row)
➜ control-plane git:(fix/PLAT-610/update-global-patroni-params) cp1-req update-database storefront <<EOF | cp-follow-task
{
"id": "storefront",
"spec": {
"database_name": "storefront",
"database_users": [
{
"username": "admin",
"db_owner": true,
"attributes": ["SUPERUSER", "LOGIN"]
}
],
"port": 0,
"patroni_port": 0,
"nodes": [
{ "name": "n1", "host_ids": ["host-1"] }
],
"postgresql_conf": {
"max_connections": 900
}
}
}
EOF
{
"database": {
"created_at": "2026-06-11T19:45:12+05:30",
"id": "storefront",
"instances": [
{
"connection_info": {
"addresses": [
"127.0.0.1"
],
"port": 6325
},
"created_at": "2026-06-11T19:45:14+05:30",
"host_id": "host-1",
"id": "storefront-n1-689qacsi",
"node_name": "n1",
"postgres": {
"patroni_state": "running",
"role": "primary",
"version": "18.4"
},
"spock": {
"read_only": "off",
"version": "5.0.8"
},
"state": "available",
"status_updated_at": "2026-06-11T19:46:47+05:30",
"updated_at": "2026-06-11T19:45:27+05:30"
}
],
"spec": {
"database_name": "storefront",
"database_users": [
{
"attributes": [
"SUPERUSER",
"LOGIN"
],
"db_owner": true,
"username": "admin"
}
],
"nodes": [
{
"host_ids": [
"host-1"
],
"name": "n1"
}
],
"patroni_port": 0,
"port": 0,
"postgres_version": "18.4",
"postgresql_conf": {
"max_connections": 900
},
"spock_version": "5"
},
"state": "modifying",
"updated_at": "2026-06-11T19:46:51+05:30"
},
"task": {
"created_at": "2026-06-11T19:46:51+05:30",
"database_id": "storefront",
"entity_id": "storefront",
"scope": "database",
"status": "pending",
"task_id": "019eb70a-fe7b-7791-bba3-50e1be1b6c75",
"type": "update"
}
}
[2026-06-11T19:46:52+05:30] refreshing current state
[2026-06-11T19:47:00+05:30] finished refreshing current state (took 7.999740163s)
[2026-06-11T19:47:02+05:30] updating resource systemd.patroni_config::storefront-n1-689qacsi
[2026-06-11T19:47:09+05:30] finished updating resource systemd.patroni_config::storefront-n1-689qacsi (took 6.877566085s)
[2026-06-11T19:47:09+05:30] updating resource systemd.unit::patroni-storefront-n1-689qacsi.service:storefront:host-1
[2026-06-11T19:47:09+05:30] finished updating resource systemd.unit::patroni-storefront-n1-689qacsi.service:storefront:host-1 (took 176.99861ms)
[2026-06-11T19:47:09+05:30] updating resource database.instance::storefront-n1-689qacsi
[2026-06-11T19:47:26+05:30] finished updating resource database.instance::storefront-n1-689qacsi (took 16.416024886s)
[2026-06-11T19:47:26+05:30] updating resource database.node::n1
[2026-06-11T19:47:26+05:30] finished updating resource database.node::n1 (took 126.417µs)
[2026-06-11T19:47:26+05:30] updating resource monitor.instance::storefront-n1-689qacsi
[2026-06-11T19:47:26+05:30] finished updating resource monitor.instance::storefront-n1-689qacsi (took 15.618679ms)
[2026-06-11T19:47:27+05:30] updating resource common.pg_service_conf::storefront-n1-689qacsi
[2026-06-11T19:47:27+05:30] finished updating resource common.pg_service_conf::storefront-n1-689qacsi (took 903.711µs)
[2026-06-11T19:47:27+05:30] updating resource database.postgres_database::n1:storefront
[2026-06-11T19:47:27+05:30] finished updating resource database.postgres_database::n1:storefront (took 35.183121ms)
database entity storefront task 019eb70a-fe7b-7791-bba3-50e1be1b6c75 completed
➜ control-plane git:(fix/PLAT-610/update-global-patroni-params) cp-psql -i storefront-n1-689qacsi -U admin -- -c 'SHOW max_connections'
max_connections
-----------------
900
(1 row)
| } | ||
| if isPrimary && cfg.Bootstrap != nil && cfg.Bootstrap.DCS != nil { | ||
| _, err := client.PatchDynamicConfig(ctx, cfg.Bootstrap.DCS.ToDynamicConfig()) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
If Patroni rejects the request with a 4xx response (for example, due to an invalid parameter value), this currently treats the update as successful. Should we return an error instead of logging a warning and continuing?
Member
Author
There was a problem hiding this comment.
That's a good question. PatchDynamicConfig should return an error if it gets a 4xx or other unsuccessful response. We have a helper method in that package to validate response codes:
control-plane/server/internal/patroni/client.go
Lines 746 to 748 in 99607a1
And then we call it in each client method, like we do here in PatchDynamicConfig:
control-plane/server/internal/patroni/client.go
Lines 527 to 530 in 99607a1
Does that address your concern, or is there an issue in this implementation?
Contributor
There was a problem hiding this comment.
Oh, okay. Thanks! Looks good now — please go ahead and merge it.
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
Patroni disallows setting some parameters through the Patroni config file after the cluster is created. Instead, they must be set via the dynamic config API.
This commit adds functionality to the systemd Patroni config implementation to patch the dynamic config if the Patroni API is up and reports that it is the primary instance.
This commit only affects systemd clusters. I will implement this for Swarm clusters in a subsequent commit by migrating Swarm to use the common Patroni config resource.
Testing
PLAT-610