Skip to content

fix: invalidate services for systemd clusters#363

Merged
jason-lynch merged 1 commit intomainfrom
fix/invalidate-services-for-systemd
Apr 22, 2026
Merged

fix: invalidate services for systemd clusters#363
jason-lynch merged 1 commit intomainfrom
fix/invalidate-services-for-systemd

Conversation

@jason-lynch
Copy link
Copy Markdown
Member

Summary

We do not yet support services for systemd clusters. This commit adds API validation to return errors early.

Testing

# try making a database with services on a systemd cluster
cp1-req create-database <<EOF
{
  "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"] }
    ],
    "services": [
      {
        "service_id": "mcp",
        "service_type": "mcp",
        "version": "latest",
        "connect_as": "admin",
        "host_ids": ["host-1"],
        "port": 0,
        "config": {
          "llm_enabled": true,
          "llm_provider": "ollama",
          "llm_model": "gpt-oss:120b",
          "ollama_url": "172.17.0.1:11435"
        }
      }
    ]
  }
}
EOF

We do not yet support services for systemd clusters. This commit adds
API validation to return errors early.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

📝 Walkthrough

Walkthrough

The change modifies service validation in validateDatabaseSpec to handle config.OrchestratorSystemD differently. For this orchestrator type, service specs are not validated; instead, a single error is added when services exist. Other orchestrators maintain existing validation behavior.

Changes

Cohort / File(s) Summary
Orchestrator-Specific Service Validation
server/internal/api/apiv1/validate.go
Added branching logic in validateDatabaseSpec that skips service ID uniqueness and port conflict checks for SystemD orchestrator, while retaining full validation for other orchestrator types.

Poem

🐰 Orchestrators now diverge with care,
SystemD skips the service dance,
While others check with thorough flair—
Validation logic takes its chance,
One branch for each, both swift and fair,
This clever diff we hail! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description includes Summary and Testing sections but is missing Changes, Checklist, and Notes for Reviewers sections from the template. Add the missing Changes section with a bulleted list of modifications, complete the Checklist with relevant items, and optionally add Notes for Reviewers if there are important context items.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding validation to reject services on systemd clusters.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/invalidate-services-for-systemd

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codacy-production
Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

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. Give us feedback

@jason-lynch
Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)

137-145: Minor: skip port-owner seeding on SystemD path.

seedPostgresPorts and the portOwner map are built unconditionally but unused when the orchestrator is SystemD (no per-service port validation runs). Consider moving the seed into the default branch to avoid the wasted allocation/iteration.

Proposed tweak
-	// Validate services — seed portOwner with Postgres ports so services can't collide with the database.
-	portOwner := make(servicePortOwnerMap)
-	seedPostgresPorts(spec, portOwner)
-
 	servicesPath := []string{"services"}
 
 	switch orchestrator {
 	case config.OrchestratorSystemD:
 		if len(spec.Services) != 0 {
 			errs = append(errs, newValidationError(errors.New("services are not yet supported for systemd clusters"), servicesPath))
 		}
 	default:
+		// Seed portOwner with Postgres ports so services can't collide with the database.
+		portOwner := make(servicePortOwnerMap)
+		seedPostgresPorts(spec, portOwner)
 		seenServiceIDs := make(ds.Set[string], len(spec.Services))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/internal/api/apiv1/validate.go` around lines 137 - 145, The code
currently allocates portOwner (servicePortOwnerMap) and calls
seedPostgresPorts(spec, portOwner) unconditionally even when orchestrator ==
config.OrchestratorSystemD and those values are never used; move the portOwner
creation and the seedPostgresPorts(spec, portOwner) call into the non-SystemD
path (e.g., the default branch of the switch over orchestrator) so that
seedPostgresPorts is only invoked when per-service validation runs, keeping the
SystemD case untouched (reference: portOwner, servicePortOwnerMap,
seedPostgresPorts, orchestrator, config.OrchestratorSystemD).
🤖 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/validate.go`:
- Around line 140-162: validateDatabaseUpdate currently validates new.Services
without checking the orchestrator, allowing services to be added to systemd
clusters; thread the orchestrator (config.Orchestrator) into
validateDatabaseUpdate (and update its callers in convert.go) and add the same
SystemD guard used in validateDatabaseSpec: if orchestrator ==
config.OrchestratorSystemD then reject non-empty new.Services with a validation
error (same message as in validateDatabaseSpec), otherwise continue to iterate
new.Services and call validateServicePortConflicts and validateServiceSpec as
before; ensure parameter names (validateDatabaseUpdate, validateDatabaseSpec,
new.Services, validateServicePortConflicts, validateServiceSpec, and convert.go
callers) are updated accordingly.

---

Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 137-145: The code currently allocates portOwner
(servicePortOwnerMap) and calls seedPostgresPorts(spec, portOwner)
unconditionally even when orchestrator == config.OrchestratorSystemD and those
values are never used; move the portOwner creation and the
seedPostgresPorts(spec, portOwner) call into the non-SystemD path (e.g., the
default branch of the switch over orchestrator) so that seedPostgresPorts is
only invoked when per-service validation runs, keeping the SystemD case
untouched (reference: portOwner, servicePortOwnerMap, seedPostgresPorts,
orchestrator, config.OrchestratorSystemD).
🪄 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: 7029d778-7c83-4a8c-b165-08f8ee6954f3

📥 Commits

Reviewing files that changed from the base of the PR and between 6fac000 and d9df7c9.

📒 Files selected for processing (1)
  • server/internal/api/apiv1/validate.go

Comment thread server/internal/api/apiv1/validate.go
@jason-lynch jason-lynch merged commit cf14a30 into main Apr 22, 2026
3 checks passed
@jason-lynch jason-lynch deleted the fix/invalidate-services-for-systemd branch April 22, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants