feat: generate RAG service YAML config resource#313
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds RAG configuration generation and resource management to the orchestrator swarm module. It introduces functions to generate pgedge-rag-server.yaml files, manages host-side configuration resources, registers resource types, integrates with the orchestrator, and includes comprehensive test coverage. 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 docstrings
🧪 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 review |
|
✅ Actions performedReview triggered.
|
![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/swarm/orchestrator.go (1)
404-409:⚠️ Potential issue | 🟠 Major
raginstances are now provisionable but never deployed.
GenerateServiceInstanceResourcesroutes"rag"intogenerateRAGInstanceResources, but that path only returns filesystem/user/key/config resources. There is noServiceInstanceSpecResourceorServiceInstanceResource, so nothing ever creates a Swarm service for the persistedServiceInstance. In practice this leaves RAG instances stuck increatingwith no container that can ever become ready. Either keep the"rag"path disabled until deployment lands, or add the deployment resources in this PR.Also applies to: 531-594
🤖 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 404 - 409, The "rag" branch in GenerateServiceInstanceResources currently calls generateRAGInstanceResources which only returns filesystem/user/key/config resources and omits ServiceInstanceSpecResource and ServiceInstanceResource, so RAG instances are never deployed; either remove/guard the "rag" case until deployment support lands, or modify generateRAGInstanceResources to create and return the same deployment resources as other service types (add ServiceInstanceSpecResource and ServiceInstanceResource entries, including container/service spec needed for Swarm creation) so the persisted ServiceInstance can be turned into a running Swarm service; update the switch in GenerateServiceInstanceResources and the body of generateRAGInstanceResources (and mirrored logic around lines 531–594) accordingly.
🤖 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/orchestrator/swarm/rag_config.go`:
- Around line 94-98: GenerateRAGConfig currently builds pipelines by calling
buildRAGPipelineYAML without validating that per-stage credentials match; this
lets a pipeline with EmbeddingLLM.APIKey and RAGLLM.APIKey set to different
values silently prefer one key and misauthenticate. Add a guard where pipelines
are iterated (in GenerateRAGConfig, and similarly in ParseRAGServiceConfig if
present) to detect when both EmbeddingLLM.APIKey and RAGLLM.APIKey are non-empty
for the same provider (e.g., "openai", "anthropic") and differ, then return an
error (fail fast) describing the mismatched keys and include the pipeline
identifier; update buildRAGPipelineYAML callers to rely on this validation so no
silent overwrite occurs.
- Around line 24-35: The YAML model is collapsing explicit zeros because
TokenBudget and TopN are plain ints with `omitempty`; update ragPipelineYAML to
use `*int` for TokenBudget and TopN (and change the corresponding numeric fields
in any related YAML structs such as ragDatabaseYAML and the RAGDefaults YAML
struct to `*int` as well), and preserve/propagate the pointers when mapping
between internal structs and these YAML structs so nil vs. explicit 0 is
preserved (look for all mappings that build/return ragPipelineYAML,
ragDatabaseYAML, and the RAGDefaults YAML structs and assign pointer values
instead of converting to plain ints).
---
Outside diff comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 404-409: The "rag" branch in GenerateServiceInstanceResources
currently calls generateRAGInstanceResources which only returns
filesystem/user/key/config resources and omits ServiceInstanceSpecResource and
ServiceInstanceResource, so RAG instances are never deployed; either
remove/guard the "rag" case until deployment support lands, or modify
generateRAGInstanceResources to create and return the same deployment resources
as other service types (add ServiceInstanceSpecResource and
ServiceInstanceResource entries, including container/service spec needed for
Swarm creation) so the persisted ServiceInstance can be turned into a running
Swarm service; update the switch in GenerateServiceInstanceResources and the
body of generateRAGInstanceResources (and mirrored logic around lines 531–594)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c6dcd4a-44fe-4d12-9fe1-c4b54b1b6cc8
📒 Files selected for processing (8)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_config.goserver/internal/orchestrator/swarm/rag_config_resource.goserver/internal/orchestrator/swarm/rag_config_resource_test.goserver/internal/orchestrator/swarm/rag_config_test.goserver/internal/orchestrator/swarm/rag_service_user_role_test.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_spec.go
tsivaprasad
force-pushed
the
PLAT-490-rag-service-api-key-file-management
branch
2 times, most recently
from
7a20ebb to
da808b3
Compare
tsivaprasad
force-pushed
the
PLAT-491-rag-service-yaml-config-generation-swarm-config
branch
from
64e5c45 to
f0b6b0a
Compare
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 3 medium |
🟢 Metrics 58 complexity · 14 duplication
Metric Results Complexity 58 Duplication 14
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/rag_service_user_role_test.go (1)
207-210: Strengthen this case by asserting trailing resource types too.Line 207 updates the expected count, but this test still doesn’t verify that the appended resources are
Dir -> RAGServiceKeys -> RAGConfig. Adding those checks will catch order/type regressions in this path as well.Proposed test hardening
@@ if !perNodeNames["n1"] || !perNodeNames["n3"] { t.Errorf("per-node resources = %v, want n1 and n3", perNodeNames) } + + // Data dir, keys, and config resource are appended last. + if result.Resources[3].Identifier.Type != filesystem.ResourceTypeDir { + t.Errorf("Resources[3].Identifier.Type = %q, want %q", + result.Resources[3].Identifier.Type, filesystem.ResourceTypeDir) + } + if result.Resources[4].Identifier.Type != ResourceTypeRAGServiceKeys { + t.Errorf("Resources[4].Identifier.Type = %q, want %q", + result.Resources[4].Identifier.Type, ResourceTypeRAGServiceKeys) + } + if result.Resources[5].Identifier.Type != ResourceTypeRAGConfig { + t.Errorf("Resources[5].Identifier.Type = %q, want %q", + result.Resources[5].Identifier.Type, ResourceTypeRAGConfig) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go` around lines 207 - 210, The test currently only asserts len(result.Resources) == 6 but should also verify the trailing resource types/order to catch regressions; update the test (in rag_service_user_role_test.go) to assert that result.Resources[len(result.Resources)-3].Type == "Dir", result.Resources[len(result.Resources)-2].Type == "RAGServiceKeys", and result.Resources[len(result.Resources)-1].Type == "RAGConfig" (or equivalent fields used to identify resource type in the Resource struct) so the final three appended resources are validated in sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go`:
- Around line 207-210: The test currently only asserts len(result.Resources) ==
6 but should also verify the trailing resource types/order to catch regressions;
update the test (in rag_service_user_role_test.go) to assert that
result.Resources[len(result.Resources)-3].Type == "Dir",
result.Resources[len(result.Resources)-2].Type == "RAGServiceKeys", and
result.Resources[len(result.Resources)-1].Type == "RAGConfig" (or equivalent
fields used to identify resource type in the Resource struct) so the final three
appended resources are validated in sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edeeca6b-a1ee-4d95-b72b-f1bfacc301b7
📒 Files selected for processing (2)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_service_user_role_test.go
✅ Files skipped from review due to trivial changes (1)
- server/internal/orchestrator/swarm/orchestrator.go
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Nice work! The only suggestion I have is very minor.
rshoemaker
left a comment
rshoemaker
left a comment
There was a problem hiding this comment.
Looks good, just the one question on config updates below...
| return fmt.Errorf("failed to get service data dir path: %w", err) | ||
| } | ||
|
|
||
| return r.writeConfigFile(fs, dirPath, rc) |
There was a problem hiding this comment.
Do you need to do anything else after a config update (like send a SIGHUP to the RAG server)?
There was a problem hiding this comment.
Yes — no SIGHUP handling yet in RAG Server. We have PLAT-497 tracked for this in Control Plane. The plan is to work on it in parallel:
RAG server & Control Plane — same pattern as MCP
tsivaprasad
changed the base branch from
PLAT-490-rag-service-api-key-file-management
to
main
RAGConfigResource was missing from the RAG resource chain due to a rebase conflict resolution that took the HEAD version of generateRAGInstanceResources (which predated RAGConfigResource). The resource is now appended after DirResource and RAGServiceKeysResource, completing the chain required to write pgedge-rag-server.yaml. PLAT-491
tsivaprasad
force-pushed
the
PLAT-491-rag-service-yaml-config-generation-swarm-config
branch
from
fbf00c8 to
2719035
Compare
tsivaprasad
force-pushed
the
PLAT-491-rag-service-yaml-config-generation-swarm-config
branch
from
2719035 to
a86d831
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/internal/orchestrator/swarm/rag_config_test.go (1)
11-19: These assertions are round-tripping the production YAML schema.
parseRAGYAMLunmarshals back intoragYAMLConfig, so tag/schema regressions can still pass because the same tags are used on both sides. It also collapses omitted string fields to zero values, so checks likedescription == "",system_prompt == "", andid_column == ""do not actually proveomitemptybehavior. For schema-sensitive cases, decode into an independent test struct or a rawmap[string]any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_config_test.go` around lines 11 - 19, The test currently round-trips production struct ragYAMLConfig via parseRAGYAML which masks tag/omitempty regressions; change parseRAGYAML (and tests that call it) to unmarshal the generated YAML into an independent representation (either a dedicated test struct that does NOT reuse ragYAMLConfig or into a raw map[string]any / yaml.Node) and assert on presence/absence of keys and actual YAML values (e.g., check that keys like "description", "system_prompt", "id_column" are omitted rather than empty strings). Update references to parseRAGYAML and any assertions in the tests that rely on ragYAMLConfig to use the new independent type or map checks so schema and omitempty behavior are validated independently of GenerateRAGConfig.server/internal/orchestrator/swarm/rag_config_resource_test.go (1)
10-76: The behaviorful resource paths still need coverage.This suite only exercises metadata helpers.
Create/Update/Refresh—dir resolution,ServiceUserRolelookup, file write, and ownership change—remain untested, so the main regression surface for this resource can slip through. A smallafero-backed lifecycle test would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_config_resource_test.go` around lines 10 - 76, Add an afero-backed lifecycle test that exercises RAGConfigResource.Create, Update and Refresh to cover directory resolution, ServiceUserRole lookup, key file write and ownership change: instantiate an afero.NewMemMapFs (or mem fs) and a fake resource manager that returns filesystem.DirResourceIdentifier("...") for DirResourceID and ServiceUserRoleIdentifier("rag", ServiceUserRoleRO) for the service role, call r.Create(ctx, mgr) then r.Update/Refresh and assert the expected file(s) exist with correct contents, modes and ownership semantics (owner checks can be simulated via metadata on the fake fs or mock owner-changer), and ensure RAGServiceKeysResourceIdentifier("...") interactions are exercised; use the same unique symbols (RAGConfigResource.Create, Update, Refresh, DirResourceIdentifier, ServiceUserRoleIdentifier, RAGServiceKeysResourceIdentifier) so the test hooks the real code paths rather than only metadata helpers.
🤖 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/orchestrator/swarm/rag_config_resource.go`:
- Around line 149-153: The write path using afero.WriteFile leaves existing file
modes unchanged; after calling afero.WriteFile(fs, configPath, content, 0o600)
ensure you explicitly enforce strict permissions by calling fs.Chmod(configPath,
0o600) before changing ownership — add fs.Chmod(configPath, 0o600) immediately
after the WriteFile call (and before fs.Chown(configPath, ragContainerUID,
ragContainerUID)) so pgedge-rag-server.yaml always ends up with 0o600 regardless
of prior mode.
---
Nitpick comments:
In `@server/internal/orchestrator/swarm/rag_config_resource_test.go`:
- Around line 10-76: Add an afero-backed lifecycle test that exercises
RAGConfigResource.Create, Update and Refresh to cover directory resolution,
ServiceUserRole lookup, key file write and ownership change: instantiate an
afero.NewMemMapFs (or mem fs) and a fake resource manager that returns
filesystem.DirResourceIdentifier("...") for DirResourceID and
ServiceUserRoleIdentifier("rag", ServiceUserRoleRO) for the service role, call
r.Create(ctx, mgr) then r.Update/Refresh and assert the expected file(s) exist
with correct contents, modes and ownership semantics (owner checks can be
simulated via metadata on the fake fs or mock owner-changer), and ensure
RAGServiceKeysResourceIdentifier("...") interactions are exercised; use the same
unique symbols (RAGConfigResource.Create, Update, Refresh,
DirResourceIdentifier, ServiceUserRoleIdentifier,
RAGServiceKeysResourceIdentifier) so the test hooks the real code paths rather
than only metadata helpers.
In `@server/internal/orchestrator/swarm/rag_config_test.go`:
- Around line 11-19: The test currently round-trips production struct
ragYAMLConfig via parseRAGYAML which masks tag/omitempty regressions; change
parseRAGYAML (and tests that call it) to unmarshal the generated YAML into an
independent representation (either a dedicated test struct that does NOT reuse
ragYAMLConfig or into a raw map[string]any / yaml.Node) and assert on
presence/absence of keys and actual YAML values (e.g., check that keys like
"description", "system_prompt", "id_column" are omitted rather than empty
strings). Update references to parseRAGYAML and any assertions in the tests that
rely on ragYAMLConfig to use the new independent type or map checks so schema
and omitempty behavior are validated independently of GenerateRAGConfig.
🪄 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: d35b942f-f641-45bf-9420-5d13ab97faa9
📒 Files selected for processing (9)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_config.goserver/internal/orchestrator/swarm/rag_config_resource.goserver/internal/orchestrator/swarm/rag_config_resource_test.goserver/internal/orchestrator/swarm/rag_config_test.goserver/internal/orchestrator/swarm/rag_service_keys_resource.goserver/internal/orchestrator/swarm/rag_service_user_role_test.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_spec.go
✅ Files skipped from review due to trivial changes (3)
- server/internal/orchestrator/swarm/service_spec.go
- server/internal/orchestrator/swarm/rag_service_keys_resource.go
- server/internal/orchestrator/swarm/resources.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/orchestrator/swarm/orchestrator.go
| if err := afero.WriteFile(fs, configPath, content, 0o600); err != nil { | ||
| return fmt.Errorf("failed to write %s: %w", configPath, err) | ||
| } | ||
| if err := fs.Chown(configPath, ragContainerUID, ragContainerUID); err != nil { | ||
| return fmt.Errorf("failed to change ownership for %s: %w", configPath, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Go's os.WriteFile, and github.com/spf13/afero.WriteFile which mirrors it, apply the provided file mode when rewriting an existing file, or is the mode only used when the file is created?
💡 Result:
For Go’s os.WriteFile: the perm argument is only used when the file is created. If the file already exists, os.WriteFile truncates and overwrites it without changing its permissions/mode. [1]
For github.com/spf13/afero.WriteFile / Afero.WriteFile: it follows the same rule—creates the file with perm if it doesn’t exist; otherwise it truncates before writing (i.e., it does not apply perm as a chmod step when overwriting an existing file). [2]
Sources:
[1] Go os.WriteFile docs (pkg.go.dev) (pkg.go.dev)
[2] Afero WriteFile docs (pkg.go.dev) (pkg.go.dev)
Citations:
Add explicit Chmod after WriteFile to enforce strict permissions on every write.
The perm argument in afero.WriteFile is only applied when creating a new file. When the file already exists, WriteFile truncates and overwrites it without resetting the mode. If pgedge-rag-server.yaml previously had broader permissions (e.g., 0o644), the DB credentials remain readable by unintended users. Call fs.Chmod(configPath, 0o600) immediately after the write to guarantee 0o600 on both create and update.
🔧 Suggested fix
if err := afero.WriteFile(fs, configPath, content, 0o600); err != nil {
return fmt.Errorf("failed to write %s: %w", configPath, err)
}
+ if err := fs.Chmod(configPath, 0o600); err != nil {
+ return fmt.Errorf("failed to change mode for %s: %w", configPath, err)
+ }
if err := fs.Chown(configPath, ragContainerUID, ragContainerUID); err != nil {
return fmt.Errorf("failed to change ownership for %s: %w", configPath, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/swarm/rag_config_resource.go` around lines 149 -
153, The write path using afero.WriteFile leaves existing file modes unchanged;
after calling afero.WriteFile(fs, configPath, content, 0o600) ensure you
explicitly enforce strict permissions by calling fs.Chmod(configPath, 0o600)
before changing ownership — add fs.Chmod(configPath, 0o600) immediately after
the WriteFile call (and before fs.Chown(configPath, ragContainerUID,
ragContainerUID)) so pgedge-rag-server.yaml always ends up with 0o600 regardless
of prior mode.
3 participants
Summary
This PR adds
RAGConfigResource, which generatespgedge-rag-server.yamlfrom pipeline configuration and writes it to the host data directory, completing the file-based config layer for the RAG service.Changes
rag_config.go— YAML struct definitions mirroring the RAG server'sConfigstruct andGenerateRAGConfig()generator;api_keyspathsreference bind-mounted key files at
/app/keys/{pipeline}_{embedding|rag}.keyrag_config_resource.go—RAGConfigResourcelifecycle (Create/Update/Refresh/Delete);
orchestrator.go— addsDirResource(host-side data directory) andRAGConfigResourcetogenerateRAGInstanceResources; resource chain is now:DirResource → RAGServiceUserRole + RAGServiceKeysResource → RAGConfigResourceresources.go— registersResourceTypeRAGConfigTesting
Verification:
Created a cluster
Configured a database with RAG service (single-host and multi-host)
rag_create_db.json
rag_create_multi_host_db.json
Confirmed successful database creation and generation of
pgedge-rag-server.yamlwith the expected datapgedge-rag-server.yaml
Checklist
Notes for Reviewers
Container deployment is not part of this. The config file is written correctly but no container starts yet.
PLAT-491