fix: update global patroni params for systemd#398
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 56 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 (13)
📝 WalkthroughWalkthroughThis PR extends Patroni orchestration to support dynamic updates of PostgreSQL global configuration on primary nodes. It introduces DCS-to-dynamic-config conversion, adds failsafe mode control based on cluster size, implements the ChangesPatroni Dynamic Config Update Feature
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 6 complexity · -4 duplication
Metric Results Complexity 6 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
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
d2fa9fa to
775fef7
Compare
1 participant
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