feat: add patroni_port field to database api#281
Conversation
📝 WalkthroughWalkthroughAdds a patroni_port integer to API schema and internal database models, propagates it through conversion functions and instance-generation logic, adds validation to prevent Patroni/Postgres port conflicts, and adds tests and a changelog entry documenting the feature. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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 (1)
server/internal/database/spec.go (1)
63-63: Avoid pointer aliasing in clone forPatroniPort.
Line 63 and Line 375 copy the pointer directly, so clone and source can share the same underlying int value.♻️ Proposed fix
func (n *Node) Clone() *Node { @@ - PatroniPort: n.PatroniPort, + PatroniPort: utils.ClonePointer(n.PatroniPort), @@ } func (s *Spec) Clone() *Spec { @@ - PatroniPort: s.PatroniPort, + PatroniPort: utils.ClonePointer(s.PatroniPort), @@ }Also applies to: 375-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec.go` at line 63, The clone is copying the PatroniPort pointer directly which causes aliasing; in the Clone/DeepCopy implementation where PatroniPort is assigned (the lines that set PatroniPort: n.PatroniPort), allocate a new int, copy the value (*n.PatroniPort) into it and assign the new pointer to the cloned struct (e.g., tmp.PatroniPort = &newVal) so the clone has its own independent int; apply the same fix for the other occurrence that directly copies PatroniPort (the second Clone/DeepCopy spot around 375).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apiv1/design/database.go`:
- Line 47: Fix the typo and wording in the g.Description strings for the Patroni
port: replace "This is field" with "This field" and remove "for this node" from
the database-level description; update the g.Description(...) in database.go
(the strings passed to g.Description that reference Patroni port and
DatabaseSpec) to read something like "The port used by Patroni. Overrides the
Patroni port set in the DatabaseSpec. NOTE: This field is not currently
supported for Docker Swarm." so both occurrences (the node-level and
database-level descriptions) are grammatically correct and accurate.
---
Nitpick comments:
In `@server/internal/database/spec.go`:
- Line 63: The clone is copying the PatroniPort pointer directly which causes
aliasing; in the Clone/DeepCopy implementation where PatroniPort is assigned
(the lines that set PatroniPort: n.PatroniPort), allocate a new int, copy the
value (*n.PatroniPort) into it and assign the new pointer to the cloned struct
(e.g., tmp.PatroniPort = &newVal) so the clone has its own independent int;
apply the same fix for the other occurrence that directly copies PatroniPort
(the second Clone/DeepCopy spot around 375).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (4)
api/apiv1/design/database.gochanges/unreleased/Added-20260225-091421.yamlserver/internal/api/apiv1/convert.goserver/internal/database/spec.go
jason-lynch
force-pushed
the
feat/PLAT-417/core-systemd-resources-pt1
branch
2 times, most recently
from
7c2858e to
e320e28
Compare
rshoemaker
left a comment
rshoemaker
left a comment
There was a problem hiding this comment.
This looks good. I called out a question about "port 0" semantics in-line. The only other thing is to consider whether or not you think API level validation makes sense here - something that prevents the user from specifying the same port for the db and patroni.
| }) | ||
| g.Attribute("patroni_port", g.Int, func() { | ||
| g.Description("The port used by Patroni for this node. Overrides the Patroni port set in the DatabaseSpec. NOTE: This field is not currently supported for Docker Swarm.") | ||
| g.Minimum(0) |
There was a problem hiding this comment.
Does port 0 have meaningful semantics for Patroni on SystemD hosts, or should this be Minimum(1) since there's no random-port-assignment mechanism like Docker provides?
There was a problem hiding this comment.
Good callout! Yes, it has the same meaning: 0 will assign a random port. You're right that we can't rely on OS features to do that for us, so I'm building that feature. I've kept this description vague for now since I'm still working out the details, and I don't want it to be misleading if we need to cut a release before I'm done. We will also eventually add support for this field in Swarm because it's useful for deploying a proxy in front of the instances. That'll be a separate follow-up ticket.
jason-lynch
force-pushed
the
feat/PLAT-417/core-systemd-resources-pt1
branch
from
e320e28 to
72b56b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api/apiv1/convert.go`:
- Around line 747-748: In validateDatabaseSpec, add validation to ensure Port
and PatroniPort (fields from DatabaseSpec/DatabaseNodeSpec) do not conflict:
check that PatroniPort != Port and that neither Port nor PatroniPort equals any
port declared in the Services array (iterate Services and compare against each
Service's Port/TargetPort). When a conflict is detected return a descriptive
error from validateDatabaseSpec indicating which ports conflict (e.g., "Port X
conflicts with service Y" or "PatroniPort equals Port"), so callers receive a
validation failure; keep the checks alongside existing port-range validations
and ensure they run for both node-level and spec-level port fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d49b099-35da-49ee-8526-d0eb3a28a868
⛔ Files ignored due to path filters (9)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (4)
api/apiv1/design/database.gochanges/unreleased/Added-20260225-091421.yamlserver/internal/api/apiv1/convert.goserver/internal/database/spec.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/internal/database/spec.go
- api/apiv1/design/database.go
- changes/unreleased/Added-20260225-091421.yaml
| Port: apiSpec.Port, | ||
| PatroniPort: apiSpec.PatroniPort, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the specific lines mentioned in the review
echo "=== Lines 747-748 in convert.go ==="
sed -n '740,760p' server/internal/api/apiv1/convert.go
echo ""
echo "=== Searching for validateDatabaseSpec function ==="
rg -n "func validateDatabaseSpec" server/ --glob '*.go'
echo ""
echo "=== Searching for PatroniPort validation ==="
rg -n -A3 -B3 "PatroniPort" server/internal/api/apiv1/ --glob '*.go' | head -100
echo ""
echo "=== Checking for port validation in general ==="
rg -n "Port.*validat|validat.*Port" server/ --glob '*.go' -i | head -50Repository: pgEdge/control-plane
Length of output: 4309
🏁 Script executed:
#!/bin/bash
echo "=== Full validateDatabaseSpec function ==="
sed -n '64,200p' server/internal/api/apiv1/validate.go
echo ""
echo "=== Check for API proto/schema definitions ==="
find server -name "*.proto" -o -name "*api*.go" | grep -i "design\|schema\|proto" | head -20
echo ""
echo "=== Search for DatabaseSpec definition in API ==="
rg -n "type DatabaseSpec struct" server/internal/api/ --glob '*.go' -A30Repository: pgEdge/control-plane
Length of output: 4603
🏁 Script executed:
#!/bin/bash
echo "=== Continue reading validateNode function ==="
sed -n '200,300p' server/internal/api/apiv1/validate.go
echo ""
echo "=== Search for Goa design files ==="
find server -type f -name "*design*.go" | head -20
echo ""
echo "=== Search for Port validation in validate.go ==="
rg -n "Port" server/internal/api/apiv1/validate.go -B2 -A2
echo ""
echo "=== Find API design package ==="
ls -la server/internal/api/ | grep -E "^d"Repository: pgEdge/control-plane
Length of output: 4010
🏁 Script executed:
#!/bin/bash
echo "=== Search for any 'design' related Go files ==="
find server -name "*.go" | xargs grep -l "design\|Design" | head -10
echo ""
echo "=== Search for Port constraints in entire codebase ==="
rg -n "Port.*int|port.*int" server/internal/api/ --glob '*.go' -B2 -A2 | head -80
echo ""
echo "=== Look for API schema/proto files ==="
find server -type f \( -name "*.proto" -o -name "design.go" \) 2>/dev/null
echo ""
echo "=== Search for DatabaseSpec type definition ==="
rg -n "type DatabaseSpec" server/ --glob '*.go' -A20 | head -100
echo ""
echo "=== Check if Port is validated anywhere ==="
rg -n "validat.*[Pp]ort|[Pp]ort.*validat" server/ --glob '*.go' | head -30Repository: pgEdge/control-plane
Length of output: 1191
🏁 Script executed:
#!/bin/bash
echo "=== Search for DatabaseSpec definition anywhere ==="
rg -n "DatabaseSpec" server/ --glob '*.go' | grep -E "type|struct" | head -20
echo ""
echo "=== Check apiv1 service.go for API design ==="
head -150 server/internal/api/apiv1/service.go
echo ""
echo "=== Search for where api.DatabaseSpec comes from ==="
rg -n "import.*api" server/internal/api/apiv1/ --glob '*.go' | head -10
echo ""
echo "=== Look for generated code or package definitions ==="
ls -la server/pkg/ 2>/dev/null | head -20Repository: pgEdge/control-plane
Length of output: 2239
🏁 Script executed:
#!/bin/bash
echo "=== Find api/apiv1 directory structure ==="
find server/api -type f -name "*.go" | head -30
echo ""
echo "=== Look for design.go files ==="
find . -name "design.go" | head -20
echo ""
echo "=== Search for DatabaseSpec in generated files ==="
find . -path "*/gen/*" -name "*.go" | xargs grep -l "DatabaseSpec" 2>/dev/null | head -10
echo ""
echo "=== Check actual api package location ==="
ls -la api/ 2>/dev/null || ls -la server/api/ 2>/dev/null | head -30Repository: pgEdge/control-plane
Length of output: 1065
🏁 Script executed:
#!/bin/bash
echo "=== List api/apiv1 directory ==="
ls -la api/apiv1/
echo ""
echo "=== Find all files in api/apiv1 (non-generated) ==="
find api/apiv1 -type f -name "*.go" | grep -v gen | head -30
echo ""
echo "=== Check for design.go or similar ==="
find api -name "design.go" -o -name "*design*.go" 2>/dev/null
echo ""
echo "=== Look in api/apiv1 root for any .go files ==="
ls -la api/apiv1/*.go 2>/dev/null || echo "No root go files"
echo ""
echo "=== Find DatabaseSpec definition in types.go ==="
head -200 api/apiv1/gen/http/control_plane/server/types.goRepository: pgEdge/control-plane
Length of output: 8714
🏁 Script executed:
#!/bin/bash
echo "=== Check database.go design file ==="
cat api/apiv1/design/database.goRepository: pgEdge/control-plane
Length of output: 50376
Port conflict validation is missing and should be added.
The API schema correctly validates port ranges (0-65535) for both Port and PatroniPort in DatabaseSpec and DatabaseNodeSpec. However, validation logic should be added to validateDatabaseSpec to ensure that PatroniPort does not equal Port and that neither conflicts with service ports defined in the Services array. Given the note that PatroniPort is not yet supported for Docker Swarm, consider adding this validation now to prevent misconfiguration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/api/apiv1/convert.go` around lines 747 - 748, In
validateDatabaseSpec, add validation to ensure Port and PatroniPort (fields from
DatabaseSpec/DatabaseNodeSpec) do not conflict: check that PatroniPort != Port
and that neither Port nor PatroniPort equals any port declared in the Services
array (iterate Services and compare against each Service's Port/TargetPort).
When a conflict is detected return a descriptive error from validateDatabaseSpec
indicating which ports conflict (e.g., "Port X conflicts with service Y" or
"PatroniPort equals Port"), so callers receive a validation failure; keep the
checks alongside existing port-range validations and ensure they run for both
node-level and spec-level port fields.
Adds the ability to set the Patroni port via the database spec. It will be necessary to explicitly set the Patroni port for SystemD instances until we add a port allocation mechanism PLAT-417
jason-lynch
force-pushed
the
feat/PLAT-417/core-systemd-resources-pt1
branch
from
72b56b1 to
522c473
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/database/spec.go`:
- Line 603: GetInstanceConnectionInfo currently hardcodes PatroniPort to 8888
and patroni_config.go builds endpoints using 0.0.0.0:8888 / {hostname}:8888;
update GetInstanceConnectionInfo (the function that constructs ConnectionInfo)
to set ConnectionInfo.PatroniPort from the InstanceSpec.PatroniPort (fall back
to existing default if nil/zero) instead of 8888, and change patroni_config.go
where endpoints are assembled to use the ConnectionInfo.PatroniPort (or the spec
value) rather than the literal 8888 so the orchestrator honors the spec's
PatroniPort.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c52f4dd-08de-4787-87a7-94d9ad99c815
⛔ Files ignored due to path filters (9)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (6)
api/apiv1/design/database.gochanges/unreleased/Added-20260225-091421.yamlserver/internal/api/apiv1/convert.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/database/spec.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/api/apiv1/convert.go
- changes/unreleased/Added-20260225-091421.yaml
| NodeOrdinal: nodeOrdinal, | ||
| PgEdgeVersion: nodeVersion, | ||
| Port: overridableValue(s.Port, node.Port), | ||
| PatroniPort: overridableValue(s.PatroniPort, node.PatroniPort), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'PatroniPort' --type=go -C3Repository: pgEdge/control-plane
Length of output: 42073
🏁 Script executed:
rg -n -i 'patroni.*port|8888' --type=go server/internal/orchestrator/Repository: pgEdge/control-plane
Length of output: 678
🏁 Script executed:
# Check patroni_config.go specifically for hardcoded port values
find . -name 'patroni_config.go' -type f | head -5Repository: pgEdge/control-plane
Length of output: 118
🏁 Script executed:
# Check how ConnectionInfo is instantiated in orchestrator
rg -n 'PatroniPort\s*:' server/internal/orchestrator/ -A2 -B2Repository: pgEdge/control-plane
Length of output: 579
🏁 Script executed:
# Check how orchestrator.go creates ConnectionInfo - look for the full context
sed -n '560,585p' server/internal/orchestrator/swarm/orchestrator.goRepository: pgEdge/control-plane
Length of output: 1173
🏁 Script executed:
# Check if there's a method that converts spec to ConnectionInfo
rg -n 'ConnectionInfo' server/internal/orchestrator/swarm/orchestrator.go | head -20Repository: pgEdge/control-plane
Length of output: 242
🏁 Script executed:
# Check the full signature and context of GetInstanceConnectionInfo
sed -n '533,590p' server/internal/orchestrator/swarm/orchestrator.goRepository: pgEdge/control-plane
Length of output: 2585
🏁 Script executed:
# Check if InstanceSpec is passed to GetInstanceConnectionInfo or if PatroniPort from spec is used anywhere
rg -n 'GetInstanceConnectionInfo|InstanceSpec.*Patroni' server/internal/orchestrator/Repository: pgEdge/control-plane
Length of output: 548
🏁 Script executed:
# Check how patroni_config.go uses the port - does it read from ConnectionInfo or hardcode?
sed -n '315,330p' server/internal/orchestrator/swarm/patroni_config.goRepository: pgEdge/control-plane
Length of output: 873
PatroniPort field is not consumed by the orchestrator layer; the port is hardcoded to 8888.
The PatroniPort field is correctly propagated through the database spec layers and validated in the API, but the orchestrator implementation completely ignores it. When GetInstanceConnectionInfo() creates the ConnectionInfo object (line 574 in orchestrator.go), it hardcodes PatroniPort: 8888 regardless of the value provided in the instance spec. Similarly, patroni_config.go hardcodes the Patroni REST API endpoints to 0.0.0.0:8888 and {hostname}:8888 (lines 323-324), never reading from the spec's PatroniPort field.
To complete this feature, pass the PatroniPort value from InstanceSpec into the orchestrator when creating ConnectionInfo and configure Patroni with the provided port instead of hardcoding 8888.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/database/spec.go` at line 603, GetInstanceConnectionInfo
currently hardcodes PatroniPort to 8888 and patroni_config.go builds endpoints
using 0.0.0.0:8888 / {hostname}:8888; update GetInstanceConnectionInfo (the
function that constructs ConnectionInfo) to set ConnectionInfo.PatroniPort from
the InstanceSpec.PatroniPort (fall back to existing default if nil/zero) instead
of 8888, and change patroni_config.go where endpoints are assembled to use the
ConnectionInfo.PatroniPort (or the spec value) rather than the literal 8888 so
the orchestrator honors the spec's PatroniPort.
2 participants
Summary
Adds the ability to set the Patroni port via the database spec. It will be necessary to explicitly set the Patroni port for SystemD instances until we add a port allocation mechanism
Testing
No functionality should change for Swarm databases.
PLAT-417