feat: add POST /v1/databases/{id}/upgrade endpoint#418
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (17)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds a database upgrade capability: a new ChangesDatabase Apply-Upgrade Feature
Related PRs: None identified. Suggested labels: enhancement, database, api Suggested reviewers: None identified. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 15 duplication
Metric Results Duplication 15
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 pls review this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR details and file list
gh pr view 418 --repo pgEdge/control-plane --json files,additions,deletions,changedFiles 2>/dev/null || echo "Could not fetch PR details"
# Get the list of changed files
gh pr diff 418 --repo pgEdge/control-plane --name-only 2>/dev/null || echo "Could not fetch file list"Length of output: 5025 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get full diff
gh pr diff 418 --repo pgEdge/control-plane 2>/dev/null | head -3000Length of output: 122777 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Fetch the key non-generated source files
echo "=== server/internal/database/service.go ==="
cat server/internal/database/service.go | grep -A 100 "ApplyUpgrade"
echo "=== server/internal/database/orchestrator.go ==="
cat server/internal/database/orchestrator.go
echo "=== server/internal/orchestrator/swarm/orchestrator.go ==="
cat server/internal/orchestrator/swarm/orchestrator.go | grep -A 80 "FindUpgrade"
echo "=== server/internal/orchestrator/swarm/images.go ==="
cat server/internal/orchestrator/swarm/images.go
echo "=== server/internal/orchestrator/systemd/orchestrator.go ==="
cat server/internal/orchestrator/systemd/orchestrator.go | tail -30
echo "=== server/internal/task/task.go ==="
cat server/internal/task/task.go | grep -B2 -A5 "TypeUpgrade\|TypeUpdate"
echo "=== server/internal/workflows/service.go ==="
cat server/internal/workflows/service.go | grep -A 40 "UpgradeDatabase"
echo "=== server/internal/api/apiv1/post_init_handlers.go ==="
cat server/internal/api/apiv1/post_init_handlers.go | grep -A 60 "ApplyUpgrade"
echo "=== server/internal/api/apiv1/pre_init_handlers.go ==="
cat server/internal/api/apiv1/pre_init_handlers.go | tail -20
echo "=== server/internal/api/apiv1/errors.go ==="
cat server/internal/api/apiv1/errors.go | tail -20
echo "=== server/internal/orchestrator/swarm/find_upgrade_test.go ==="
cat server/internal/orchestrator/swarm/find_upgrade_test.goLength of output: 34975 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== errors.go full new section ==="
gh pr diff 418 --repo pgEdge/control-plane -- server/internal/api/apiv1/errors.go
echo "=== pre_init_handlers.go ApplyUpgrade stub ==="
gh pr diff 418 --repo pgEdge/control-plane -- server/internal/api/apiv1/pre_init_handlers.go
echo "=== systemd orchestrator FindUpgrade ==="
gh pr diff 418 --repo pgEdge/control-plane -- server/internal/orchestrator/systemd/orchestrator.go
echo "=== database service.go full ApplyUpgrade context (broader) ==="
grep -n "UpdateDatabaseState\|ErrUpgradeNotAvailable\|prevState\|rollback" server/internal/database/service.go || true
grep -n "UpdateDatabaseState\|ErrUpgradeNotAvailable\|prevState\|rollback" server/internal/api/apiv1/post_init_handlers.go || trueLength of output: 1181 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== errors.go (full) ==="
cat server/internal/api/apiv1/errors.go
echo "=== pre_init_handlers.go ApplyUpgrade grep ==="
grep -n "ApplyUpgrade\|ErrUninitialized" server/internal/api/apiv1/pre_init_handlers.go || true
echo "=== systemd orchestrator.go FindUpgrade + AvailableUpgrades ==="
grep -n "FindUpgrade\|AvailableUpgrades\|ErrUpgradeNotAvailable" server/internal/orchestrator/systemd/orchestrator.go || true
echo "=== post_init_handlers.go state rollback lines ==="
grep -n "prevState\|UpdateDatabaseState\|rollback\|restorationErr" server/internal/api/apiv1/post_init_handlers.go || trueLength of output: 7906
SummaryThe feature is well-structured: clean API design, correct Goa type generation, solid swarm-level validation logic, good test coverage for 🔴 High: Spec update is not rolled back on workflow trigger failureFile:
// ApplyUpgrade atomically writes: spec.PostgresVersion=17.10 + state=modifying
db, err := s.dbSvc.ApplyUpgrade(ctx, databaseID, req.Request.Image)
t, err := s.workflowSvc.UpgradeDatabase(ctx, db)
if err != nil {
// ✅ state rolled back: modifying → available
// ❌ spec NOT rolled back: spec still says PostgresVersion=17.10
s.dbSvc.UpdateDatabaseState(ctx, databaseID, database.DatabaseStateModifying, prevState)
return nil, apiErr(err)
}Impact: After this failure path, the database appears Suggested fix options:
🟡 Medium: Pre-flight
|
moizpgedge
left a comment
moizpgedge
left a comment
There was a problem hiding this comment.
I ran this end to end on a dev cluster. The endpoint works: upgrade returned 200, all four containers moved to 17.10 and came back healthy, and the error cases were correct (empty image 400, unknown db 404, fake image 400).
But the upgrade finished in a failed state, twice, with the same error:
failed to update subscription on node n1:
ERROR: cannot assign TransactionIds during recovery (SQLSTATE XX000)
the upgrade reuses the UpdateDatabase workflow, which restarts each node and then reconciles Spock subscriptions. A just restarted node is briefly in recovery (read only), so the subscription write is rejected and the whole upgrade is marked failed, even though every container already upgraded and is healthy. Nothing waits for the node to leave recovery and the step does not retry.
The logs show the trigger clearly: the upgrade does a switchover on each node, and the subscription write then runs against the just promoted primary while it is still finishing recovery.
Result of my two runs:
| author log | my run 1 | my run 2 | |
|---|---|---|---|
| containers on 17.10 | yes | yes | yes |
| final db state | available | failed | failed |
Thanks @moizpgedge! That issue is now resolved. Please check now. |
tsivaprasad
force-pushed
the
PLAT-603-apply-image-upgrade-to-a-database
branch
2 times, most recently
from
c11eb50 to
6286ea4
Compare
moizpgedge
left a comment
moizpgedge
left a comment
There was a problem hiding this comment.
Approved.It looks good to me
| // | ||
| // Both conditions are transient and clear within seconds of promotion | ||
| // completing. Callers should retry until this function returns false. | ||
| func IsRecoveryError(err error) bool { |
There was a problem hiding this comment.
What is the use of IsRecoveryError? I could not find any callers in the code or tests
tsivaprasad
force-pushed
the
PLAT-603-apply-image-upgrade-to-a-database
branch
3 times, most recently
from
b75d238 to
12f219a
Compare
|
@moizpgedge Pls reverify this PR and the issue we saw will be addressed as part of PLAT-665 |
tsivaprasad
force-pushed
the
PLAT-602-check-available-image-upgrades-for-a-database
branch
from
e5b577d to
6413d13
Compare
tsivaprasad
force-pushed
the
PLAT-603-apply-image-upgrade-to-a-database
branch
from
12f219a to
2a5cb06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/images.go (1)
143-231: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDuplicate
AvailableUpgradesmethod declaration breaks compilation.
Versions.AvailableUpgradesis declared twice in this file — once at Line 146 (newly added alongsideFindByImage) and again, identically, at Line 191 (pre-existing). This is a compile error, confirmed by static analysis:method Versions.AvailableUpgrades already declared at server/internal/orchestrator/swarm/images.go:146:20. The entireswarmpackage (and its test package, includingfind_upgrade_test.go) fails to build as a result.🐛 Proposed fix: remove the duplicate definition
-func (v *Versions) AvailableUpgrades(current *ds.PgEdgeVersion) []*database.AvailableUpgrade { - if current == nil { - return nil - } - currentPGMajor, ok := current.PostgresVersion.Major() - if !ok { - return nil - } - currentSpockMajor, ok := current.SpockVersion.Major() - if !ok { - return nil - } - - var upgrades []*database.AvailableUpgrade - for _, ver := range v.supportedVersions { - pgMajor, ok := ver.PostgresVersion.Major() - if !ok || pgMajor != currentPGMajor { - continue - } - spockMajor, ok := ver.SpockVersion.Major() - if !ok || spockMajor != currentSpockMajor { - continue - } - if ver.PostgresVersion.Compare(current.PostgresVersion) <= 0 { - continue - } - img, err := v.GetImages(ver) - if err != nil { - continue - } - if img.Stability != "" && img.Stability != "stable" { - continue - } - upgrades = append(upgrades, &database.AvailableUpgrade{ - PostgresVersion: ver.PostgresVersion.String(), - SpockVersion: ver.SpockVersion.String(), - Image: img.PgEdgeImage, - }) - } - return upgrades -}(remove whichever of the two identical blocks — Line 143-186 or Line 191-231 — is redundant; keep exactly one.)
🤖 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/swarm/images.go` around lines 143 - 231, The file has two identical Versions.AvailableUpgrades method declarations, which causes a compile-time duplicate symbol error. Remove one of the duplicate AvailableUpgrades implementations in images.go and keep only a single definition alongside FindByImage, ensuring the remaining method still contains the full existing upgrade-filtering logic and matches the ds.PgEdgeVersion to database.AvailableUpgrade flow.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@server/internal/orchestrator/swarm/images.go`:
- Around line 143-231: The file has two identical Versions.AvailableUpgrades
method declarations, which causes a compile-time duplicate symbol error. Remove
one of the duplicate AvailableUpgrades implementations in images.go and keep
only a single definition alongside FindByImage, ensuring the remaining method
still contains the full existing upgrade-filtering logic and matches the
ds.PgEdgeVersion to database.AvailableUpgrade flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71741655-a112-41f6-bd20-c082e19291cf
⛔ Files ignored due to path filters (17)
api/apiv1/gen/control_plane/client.gois excluded by!**/gen/**api/apiv1/gen/control_plane/endpoints.gois excluded by!**/gen/**api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/cli/control_plane/cli.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/cli.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/client.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/paths.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/paths.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/server.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 (14)
api/apiv1/design/api.goapi/apiv1/design/database.goserver/internal/api/apiv1/errors.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/pre_init_handlers.goserver/internal/database/apply_upgrade_test.goserver/internal/database/orchestrator.goserver/internal/database/service.goserver/internal/orchestrator/swarm/find_upgrade_test.goserver/internal/orchestrator/swarm/images.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/systemd/orchestrator.goserver/internal/task/task.goserver/internal/workflows/service.go
tsivaprasad
force-pushed
the
PLAT-603-apply-image-upgrade-to-a-database
branch
from
2481427 to
0ffa2b0
Compare
2 participants
Summary
This PR adds POST
/v1/databases/{id}/upgrade, a dedicated endpoint for applying minor-version image upgrades to a database. Validation ensures the target image is a stable manifest entry, belongs to the same PostgreSQL major and Spock major version bucket, and is strictly newer than the database's current version.Changes
apply-upgrademethod withApplyUpgradeRequest/ApplyUpgradeResponseGoa types; regenerated OpenAPI spec and transportlayer
TypeUpgradefor audit-trail distinction fromTypeUpdateFindUpgrade(current, targetImage)method +ErrUpgradeNotAvailablesentinel errorVersions.FindByImagereverse-lookup +FindUpgradevalidation (same PG major, same Spock major, strictly newer, stable
stability only)
FindUpgradereturnsErrUpgradeNotAvailabledirectingusers to the OS package manager
ApplyUpgradevalidates, clears per-node versionoverrides, sets state to
modifying, and commits atomically to etcdUpgradeDatabasecreates aTypeUpgradetask andreuses the existing
UpdateDatabaseworkflowApplyUpgradeinPostInitHandlerswithprevStaterollback if workflow trigger fails; pre-init stub returns
ErrUninitializedTesting
Verification:
create_db_lower_image.jsonChecklist
PLAT-603