Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThis PR refactors instance resource management across database orchestration by splitting resources into instance dependencies, database dependencies, and node dependents categories. The change introduces 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 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
|
| Metric | Results |
|---|---|
| Complexity | 30 |
| Duplication | -3 |
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: 2
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/common/pgbackrest_stanza.go (1)
47-51:⚠️ Potential issue | 🔴 CriticalPgBackRestStanza's Dependencies() should explicitly include InstanceResourceIdentifier.
resource.FromContextdoes not enforce declared dependency constraints—it retrieves any resource present in state usingstate.Get(identifier)without validation. BothRefresh()andCreate()calldatabase.GetPrimaryInstance, which internally executesresource.FromContexttwice: first forNodeResource(declared), then forInstanceResource(not declared). While the current state-building order (end.go) ensuresInstanceResourceis in state, this dependency is implicit and fragile. If state loading, reconstruction, or orchestration order changes, the undeclared dependency could fail intermittently. DeclareInstanceResourceIdentifieras an explicit dependency.This applies to the other methods at lines 57–69 and 97–109 that similarly call
GetPrimaryInstanceor access instance data without declaring the dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/pgbackrest_stanza.go` around lines 47 - 51, PgBackRestStanza.Dependencies currently only returns NodeResourceIdentifier but the code paths in Refresh() and Create() call database.GetPrimaryInstance (and otherwise access instance data) which relies on InstanceResource being present; update PgBackRestStanza.Dependencies to also include database.InstanceResourceIdentifier(p.ClusterName, p.NodeName) (or the correct InstanceResourceIdentifier constructor used elsewhere) so InstanceResource is an explicit dependency; also audit the Refresh() and Create() methods to ensure any other implicit instance accesses correspond to declared identifiers and add InstanceResourceIdentifier entries where missing.
🧹 Nitpick comments (2)
server/internal/database/orchestrator.go (1)
63-71: Consider renamingAddResourcestoAddInstanceDependenciesfor API consistency.Now that the backing field is
InstanceDependenciesand there are sibling methodsAddDatabaseDependencies/AddNodeDependents, the nameAddResourcesis misleading — it suggests it adds to "resources" generically but it only appends toInstanceDependencies. Renaming would make the three add-methods symmetric with the three fields and reduce confusion at call sites (e.g. line 41 inNewInstanceResources).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/orchestrator.go` around lines 63 - 71, Rename the method AddResources on the InstanceResources type to AddInstanceDependencies to match the backing field InstanceDependencies and sibling methods AddDatabaseDependencies/AddNodeDependents; update any call sites (e.g., NewInstanceResources) to use AddInstanceDependencies, preserving the current behavior of converting inputs via resource.ToResourceDataSlice and appending to r.InstanceDependencies, and keep the original signature and error handling so implementation logic does not change.server/internal/orchestrator/swarm/orchestrator.go (1)
1026-1033: Hardcoded container paths and ignoredpgVersion— worth a comment.A few observations on the swarm
InstancePathsimplementation:
pgVersionis accepted but intentionally unused (hence_), whereas the systemd implementation requires it (returns an error if empty). Since the interface contract now allowspgVersionto be significant, consider adding a brief comment explaining why it's irrelevant for swarm (paths are fixed by the container image layout, not by PG major version). This will spare future maintainers from guessing.The container-internal paths
/opt/pgedge,/usr/bin/pgbackrest, and/usr/local/bin/patroniare hardcoded here. They're correct for the current pgedge image, but if the image layout ever changes they'll silently drift. Consider promoting them to package-levelconsts (e.g. nearOverlayDriveron line 44) so they're discoverable alongside other swarm constants and easier to update.🧹 Suggested cleanup
const ( OverlayDriver = "overlay" + + // Container-internal paths for the pgedge postgres image. + containerPgEdgeBaseDir = "/opt/pgedge" + containerPgBackRestPath = "/usr/bin/pgbackrest" + containerPatroniPath = "/usr/local/bin/patroni" )-func (o *Orchestrator) InstancePaths(_ *ds.Version, instanceID string) (database.InstancePaths, error) { +// InstancePaths returns filesystem paths for a swarm-managed instance. pgVersion is +// ignored because the pgedge container image has a fixed layout regardless of PG major. +func (o *Orchestrator) InstancePaths(_ *ds.Version, instanceID string) (database.InstancePaths, error) { return database.InstancePaths{ - Instance: database.Paths{BaseDir: "/opt/pgedge"}, + Instance: database.Paths{BaseDir: containerPgEdgeBaseDir}, Host: database.Paths{BaseDir: filepath.Join(o.cfg.DataDir, "instances", instanceID)}, - PgBackRestPath: "/usr/bin/pgbackrest", - PatroniPath: "/usr/local/bin/patroni", + PgBackRestPath: containerPgBackRestPath, + PatroniPath: containerPatroniPath, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/orchestrator.go` around lines 1026 - 1033, The InstancePaths method on Orchestrator currently ignores the pgVersion parameter and returns hardcoded container-internal paths; add a short comment inside Orchestrator.InstancePaths explaining that pgVersion is intentionally unused for swarm because container image layout fixes binary locations, and replace the literal strings "/opt/pgedge", "/usr/bin/pgbackrest", and "/usr/local/bin/patroni" with package-level consts (e.g. define PGEDGE_BASE_DIR, PGBACKREST_PATH, PATRONI_PATH near OverlayDriver) so the values are discoverable and maintainable across the package; update InstancePaths to reference those consts and keep behavior unchanged.
🤖 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/instance_resource.go`:
- Around line 161-165: InstanceResource.PostgresVersion currently returns an
empty ds.Version when Spec.PgEdgeVersion.PostgresVersion is nil, which hides the
real error; change PostgresVersion to return (*ds.Version, error) and return a
descriptive error when the version is missing, then update callers (notably the
callsite in pgbackrest_stanza.go that passes the result into
orchestrator.InstancePaths) to handle the error and avoid passing an empty
version into orchestrator.InstancePaths; ensure error messages clearly identify
the InstanceResource and missing PostgresVersion so the root cause is surfaced.
In `@server/internal/orchestrator/common/pgbackrest_stanza.go`:
- Around line 26-29: The PgBackRestStanza struct was changed to remove the Paths
field, which will cause diffs against stored resources that still include
`paths`; update the resource handling to avoid noisy updates by either
incrementing the resource version or ignoring the old field: bump the
ResourceVersion from "1" to "2" in the resource registration that exposes
PgBackRestStanza (so the controller performs a one-time migration), or add the
JSONPath `/paths` to the DiffIgnore() list used when comparing stored vs desired
state; locate the PgBackRestStanza type and the resource/version registration
code and update the ResourceVersion constant or update the DiffIgnore()
implementation to include `/paths` so existing deployments won’t generate
unnecessary Update operations.
---
Outside diff comments:
In `@server/internal/orchestrator/common/pgbackrest_stanza.go`:
- Around line 47-51: PgBackRestStanza.Dependencies currently only returns
NodeResourceIdentifier but the code paths in Refresh() and Create() call
database.GetPrimaryInstance (and otherwise access instance data) which relies on
InstanceResource being present; update PgBackRestStanza.Dependencies to also
include database.InstanceResourceIdentifier(p.ClusterName, p.NodeName) (or the
correct InstanceResourceIdentifier constructor used elsewhere) so
InstanceResource is an explicit dependency; also audit the Refresh() and
Create() methods to ensure any other implicit instance accesses correspond to
declared identifiers and add InstanceResourceIdentifier entries where missing.
---
Nitpick comments:
In `@server/internal/database/orchestrator.go`:
- Around line 63-71: Rename the method AddResources on the InstanceResources
type to AddInstanceDependencies to match the backing field InstanceDependencies
and sibling methods AddDatabaseDependencies/AddNodeDependents; update any call
sites (e.g., NewInstanceResources) to use AddInstanceDependencies, preserving
the current behavior of converting inputs via resource.ToResourceDataSlice and
appending to r.InstanceDependencies, and keep the original signature and error
handling so implementation logic does not change.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 1026-1033: The InstancePaths method on Orchestrator currently
ignores the pgVersion parameter and returns hardcoded container-internal paths;
add a short comment inside Orchestrator.InstancePaths explaining that pgVersion
is intentionally unused for swarm because container image layout fixes binary
locations, and replace the literal strings "/opt/pgedge", "/usr/bin/pgbackrest",
and "/usr/local/bin/patroni" with package-level consts (e.g. define
PGEDGE_BASE_DIR, PGBACKREST_PATH, PATRONI_PATH near OverlayDriver) so the values
are discoverable and maintainable across the package; update InstancePaths to
reference those consts and keep behavior unchanged.
🪄 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: dc2dbb2c-68c8-46e2-8af4-f7a3a1e7ff01
📒 Files selected for processing (27)
e2e/backup_restore_test.goserver/internal/database/instance_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/helpers_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/paths.goserver/internal/database/paths_test.goserver/internal/orchestrator/common/etcd_creds.goserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/common/patroni_config_generator_test.goserver/internal/orchestrator/common/pgbackrest_config.goserver/internal/orchestrator/common/pgbackrest_stanza.goserver/internal/orchestrator/common/postgres_certs.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/systemd/orchestrator.goserver/internal/orchestrator/systemd/patroni_unit.goserver/internal/orchestrator/systemd/pgbackrest_restore.goserver/internal/orchestrator/systemd/unit_options_test.goserver/internal/pgbackrest/config.go
pgBackRest stanza is a per-node resource, but it had an instance paths property, which can be unique per instance. This commit refactors the recently-added instance paths types and makes it so that the instance paths are computed at run time in the pgBackRest stanza resource. Note that we haven't yet refactored the swarm package to use the new common resources, so this bug and change only affected the systemd orchestrator. PLAT-547
jason-lynch
force-pushed
the
fix/PLAT-547/node-dependent-resources-ii
branch
from
d13512f to
880ff1b
Compare
Node resources aren't created until after all instances are available. This means that node-dependent resources, such as the pgBackRest stanza resource, cannot be created until after all instances are available. This change adds a mechanism to treat node-dependent instance resources separately and ensure they're created after the node resource. This fixes a bug where you could not create a new node from a backup if the new node would have more than one instance. PLAT-547
jason-lynch
force-pushed
the
fix/PLAT-547/node-dependent-resources-ii
branch
from
880ff1b to
bab94e3
Compare
mmols
approved these changes
Apr 22, 2026
Member
mmols
left a comment
mmols
left a comment
There was a problem hiding this comment.
LGTM, E2E worked locally as well
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
Node resources aren't created until after all instances are available. This means that node-dependent resources, such as the pgBackRest stanza resource, cannot be created until after all instances are available.
This change adds a mechanism to treat node-dependent instance resources separately and ensure they're created after the node resource. This fixes a bug where you could not create a new node from a backup if the new node would have more than one instance.
Changes
common.InstancePathsto thedatabasepackage so that it can be used by thedatabase.Orchestratorinterface.InstanceResources.NodeDependentsfield for node-dependent resources and rename theResourcesfield toInstanceDependenciesto clarify that these are resources that the instance depends on.Testing
There is a unit test for this case, and I've added a read replica to the
TestS3CreateDBFromBackupE2E test to exercise this case.# Only works on the lima and EC2 environments because of the S3 dependency. make update-lima-fixture use-lima make test-e2e E2E_RUN=TestS3CreateDBFromBackupPLAT-547