updateing the template patch #124
Conversation
WalkthroughMigrates several persistence stores from PostgreSQL to SQLite in the deployment template, removes a related exporter container, replaces explicit DB envs with secretKeyRefs, removes the default-provider Postgres block, adjusts an agents store namespace, and adds a ConfigMap-backed lightspeed-config volume. Changes
Sequence Diagram(s)sequenceDiagram
participant Pod as Assisted Chat Pod
participant Secret as K8s Secret (ASSISTED_CHAT_DB_SECRET_NAME)
participant CM as ConfigMap (lightspeed-config)
participant SQLite as SQLite Files
participant Postgres as PostgreSQL
Note over Pod: Startup
Pod->>Secret: Read DB credentials via env.valueFrom
Pod->>CM: Mount lightspeed-config volume
Note over Pod,Postgres: Previous flow
Pod-xPostgres: Connect (removed for stores)
Note over Pod,SQLite: New flow
Pod->>SQLite: Open db_path files (agents_store.db, responses_store.db, registry.db)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
test/prow/template_patch.diff (3)
47-55: Inference_store shares the same SQLite file as registry (registry.db) — likely unintendedBoth the registry store (context lines 44-45) and inference_store (lines 54-55) point to the same SQLite file ${STORAGE_MOUNT_PATH}/sqlite/registry.db. This couples unrelated stores and increases locking contention. Recommend giving inference_store its own db file.
Apply:
- db_path: ${STORAGE_MOUNT_PATH}/sqlite/registry.db + db_path: ${STORAGE_MOUNT_PATH}/sqlite/inference_store.db
130-133: Incomplete ConfigMap volume and missing volumeMountThe volume definition lacks configMap.name, which makes the manifest invalid. Also, no container mounts this volume, so it won’t have any effect.
Add the ConfigMap name:
- name: lightspeed-config configMap: + name: lightspeed-configAdditionally, mount it in the main container (showing a representative snippet; adjust the path/subPath as needed):
# Under the main container's volumeMounts: - name: lightspeed-config mountPath: /etc/lightspeed/config.yaml subPath: config.yaml readOnly: trueIf you expect a different file layout, replace mountPath/subPath accordingly.
32-39: Responses store moved to SQLite: ensure storage mount and SQLite directory exist
- Verified that
${STORAGE_MOUNT_PATH}is already mounted intemplate.yaml(lines 473 and 517) for the main container.- You still need to create the
${STORAGE_MOUNT_PATH}/sqlitefolder at runtime (e.g. via aninitContaineror in your application startup) so thatdb_path: ${STORAGE_MOUNT_PATH}/sqlite/responses_store.dbcan be written.
🧹 Nitpick comments (1)
test/prow/template_patch.diff (1)
28-30: Agents persistence_store switched to SQLite: ensure dir exists, permissions, and single-writer assumptions
- The db_path under ${STORAGE_MOUNT_PATH}/sqlite/ requires the directory to exist and be writable by the runtime UID. Consider an initContainer to mkdir -p and set ownership/permissions, or ensure fsGroup is set.
- If this deployment can be scaled (>1 replica), SQLite will not support multi-writer across pods. Enforce replicas: 1 or document constraints.
- If the store implementation ignores namespace for sqlite, consider omitting the key entirely instead of setting null.
Would you like me to propose an initContainer snippet and a securityContext.fsGroup addition?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/prow/template_patch.diff(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
| - database: | ||
| - postgres: | ||
| - host: ${env.ASSISTED_CHAT_POSTGRES_HOST} | ||
| - port: ${env.ASSISTED_CHAT_POSTGRES_PORT} | ||
| - db: ${env.ASSISTED_CHAT_POSTGRES_NAME} | ||
| - user: ${env.ASSISTED_CHAT_POSTGRES_USER} | ||
| - password: ${env.ASSISTED_CHAT_POSTGRES_PASSWORD} | ||
| - namespace: lightspeed-stack |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removal of default-provider Postgres block: confirm no remaining references
Looks intentional given the SQLite migration. Please double-check no other parts of the template still read from the removed keys.
Run:
🏁 Script executed:
#!/bin/bash
# Find any lingering references to postgres DB settings
rg -n -A 2 -i 'env\.ASSISTED_CHAT_POSTGRES|ASSISTED_CHAT_POSTGRES|database:\s*$\n\s*postgres:' Length of output: 288
🏁 Script executed:
#!/bin/bash
# Search for any lingering references to ASSISTED_CHAT_POSTGRES env vars
rg -n 'ASSISTED_CHAT_POSTGRES'
# Search for any occurrences of “postgres” in the repository
rg -n -i 'postgres'Length of output: 11430
Remove all remaining Postgres references
The patch only removes the Postgres block from the prow test, but there are still live references elsewhere. Please remove or migrate these to SQLite (or delete if no longer needed):
• template.yaml (lines ~175–180, 301–306, 308–313, 341–346, and env var definitions at 424–449)
• assisted-chat-pod.yaml (env var names at lines 12–20; “postgres” container at 85–92)
• Makefile (psql target at lines 84–86)
• config/llama_stack_client_config.yaml (all postgres entries at 28–35 and 68–73)
Ensure no code or CI templates still reference ASSISTED_CHAT_POSTGRES_* or “type: postgres.”
| - - name: ASSISTED_CHAT_POSTGRES_HOST | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.host | ||
| - - name: ASSISTED_CHAT_POSTGRES_PORT | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.port | ||
| - - name: ASSISTED_CHAT_POSTGRES_NAME | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.name | ||
| - - name: ASSISTED_CHAT_POSTGRES_USER | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.user | ||
| - - name: ASSISTED_CHAT_POSTGRES_PASSWORD | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.password | ||
| - - name: ASSISTED_CHAT_POSTGRES_CA_CERT | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${ASSISTED_CHAT_DB_SECRET_NAME} | ||
| - key: db.ca_cert | ||
| - optional: true | ||
| resources: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removal of Postgres env vars looks good; please verify no stragglers
This aligns with the SQLite migration and reduces secret surface area.
Run:
🏁 Script executed:
#!/bin/bash
# Ensure no remaining env or ${env.*} references exist for Postgres
rg -n -A 2 -i 'ASSISTED_CHAT_POSTGRES|env\.ASSISTED_CHAT_POSTGRES'Length of output: 7011
Remove remaining Postgres env vars and references
While the prow test diff drops the ASSISTED_CHAT_POSTGRES_* env vars, we still have live references in the actual deployment templates. Please purge or replace all of the following with your new SQLite settings:
• In template.yaml:
– Lines 176–180 (host, port, db, user, password)
– Lines 302–306 (host, port, db, user, password)
– Lines 309–313 (host, port, db, user, password under responses_store)
– Lines 342–346 (host, port, db, user, password under models)
• In assisted-chat-pod.yaml:
– Lines 12–22 (env vars for HOST, PORT, USER, PASSWORD, NAME)
After removal, ensure you’ve wired in the SQLite type: sqlite and correct db_path for each section.
| - - name: lightspeed-to-dataverse-exporter | ||
| - image: quay.io/lightspeed-core/lightspeed-to-dataverse-exporter:${LIGHTSPEED_EXPORTER_IMAGE_TAG} | ||
| - imagePullPolicy: Always | ||
| - args: | ||
| - - "--mode" | ||
| - - "manual" | ||
| - - "--config" | ||
| - - "/etc/config/config.yaml" | ||
| - - "--log-level" | ||
| - - "INFO" | ||
| - env: | ||
| - - name: INGRESS_SERVER_AUTH_TOKEN | ||
| - valueFrom: | ||
| - secretKeyRef: | ||
| - name: ${INSIGHTS_INGRESS_SECRET_NAME} | ||
| - key: auth_token | ||
| - resources: | ||
| - limits: | ||
| - memory: "512Mi" | ||
| - cpu: "200m" | ||
| - requests: | ||
| - memory: "256Mi" | ||
| - cpu: "100m" | ||
| - volumeMounts: | ||
| - - name: lightspeed-exporter-config | ||
| - mountPath: /etc/config/config.yaml | ||
| - subPath: config.yaml | ||
| - - name: data-storage | ||
| - mountPath: ${STORAGE_MOUNT_PATH} | ||
| - |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Exporter container removal: check for orphaned volumes/ConfigMaps and dead secrets
- If a lightspeed-exporter-config volume or ConfigMap remains elsewhere, remove it.
- Check whether INSIGHTS_INGRESS_SECRET_NAME is used anywhere; if not, drop it from the template/docs.
Run:
🏁 Script executed:
#!/bin/bash
# Look for remnants related to the removed exporter
rg -n -A 2 -i 'lightspeed-to-dataverse-exporter|lightspeed-exporter-config|INSIGHTS_INGRESS_SECRET_NAME'Length of output: 2133
Cleanup orphaned exporter resources in template.yaml
Please remove all remaining references to the lightspeed-to-dataverse-exporter container, its config volume, and the associated secret, since the exporter has been dropped:
- Drop the
INSIGHTS_INGRESS_SECRET_NAMEparameter (template.yaml:75–77) - Remove the exporter container spec (template.yaml:489–492)
- Remove the
secretKeyRefunderINSIGHTS_INGRESS_SECRET_NAME(template.yaml:503–505) - Remove the
lightspeed-exporter-configvolumeMount (template.yaml:513–515) - Remove the
lightspeed-exporter-configvolume (template.yaml:523–526) - Remove the standalone ConfigMap data block for
lightspeed-exporter-config(template.yaml:570–572)
Also search your docs and README for any lingering mentions of the exporter or its image tag and drop them.
🤖 Prompt for AI Agents
In test/prow/template_patch.diff around lines 100 to 129, leftover references to
the removed lightspeed-to-dataverse-exporter container, its config
volumeMount/volume, and the ingress secret remain; remove the exporter container
block (image, imagePullPolicy, args, env, resources, volumeMounts), delete the
INSIGHTS_INGRESS_SECRET_NAME parameter and any secretKeyRef entries that
reference it, remove the lightspeed-exporter-config volumeMount and
corresponding volume definition, and delete the standalone ConfigMap data block
for lightspeed-exporter-config; after removing these blocks, run a project-wide
search for any occurrences of the exporter name or its image tag in docs/README
and delete or update those references to fully clean up orphaned exporter
resources.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrej1991, carbonin, eranco74, keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 9e9d4df
into
rh-ecosystem-edge:main
4 participants
updateing the template patch
Summary by CodeRabbit
New Features
Refactor