llama-stack persistence #68
Conversation
WalkthroughThe changes transition llama-stack persistence from SQLite to PostgreSQL, updating all relevant configuration files, deployment templates, and container specifications. This includes adding a PostgreSQL container to the pod spec, updating environment variables, and ensuring the necessary Python PostgreSQL driver is installed in all relevant containers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssistedChatPod
participant PostgresContainer
participant LightspeedStackContainer
User->>AssistedChatPod: Deploy pod
AssistedChatPod->>PostgresContainer: Start PostgreSQL with env vars
AssistedChatPod->>LightspeedStackContainer: Start with PostgreSQL env vars
LightspeedStackContainer->>PostgresContainer: Connect to PostgreSQL using env vars
LightspeedStackContainer->>PostgresContainer: Store/retrieve agent, response, metadata, inference data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (2)
template.yaml (2)
373-378: CA certificate env var is injected but never consumed
LLAMA_STACK_POSTGRES_CA_CERTis optional, yet the Llama-stack config never references it (sslmode,rootcert, etc.).
Either wire it into the client config (sslmode: verify-full,sslrootcert: …) or drop the variable to prevent confusion.
235-249: PostgreSQL store definition duplicated – consider YAML anchors to DRY
persistence_storeandresponses_storeshare six identical lines. Repeating blocks makes future edits error-prone.# 👇 one anchor common_pg: &common_pg type: postgres host: ${env.LLAMA_STACK_POSTGRES_HOST} port: ${env.LLAMA_STACK_POSTGRES_PORT} db: ${env.LLAMA_STACK_POSTGRES_NAME} user: ${env.LLAMA_STACK_POSTGRES_USER} password: ${env.LLAMA_STACK_POSTGRES_PASSWORD} persistence_store: *common_pg responses_store: *common_pg
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template.yaml(4 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
🔇 Additional comments (2)
template.yaml (2)
255-256: Telemetry still hard-codes an SQLite pathThe telemetry sink keeps writing to
${STORAGE_MOUNT_PATH}/sqlite/trace_store.db.
If the goal is full Postgres persistence, consider migrating this sink as well or mount a persistent volume; otherwise traces are lost on pod eviction.
348-378: Secret keys with dots are valid – no changes neededKubernetes has always allowed
.in Secret data keys (the spec permits alphanumeric,-,_, and.). The pod’s environment-variable name is taken from thename:field (e.g.LLAMA_STACK_POSTGRES_HOST), not the secret key itself, so there’s no risk of invalid env var names. Bracket-notation is only needed when querying via JSONPath, not for loading into pods.No fixes required.
Likely an incorrect or invalid review comment.
|
/hold
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/llama_stack_client_config.yaml (1)
73-78: Same connection block duplicated againThe
inference_storerepeats the connection stanza verbatim; see earlier recommendation about YAML anchors to keep this DRY.
🧹 Nitpick comments (1)
config/llama_stack_client_config.yaml (1)
35-40: Avoid copy-pasted connection blocks – introduce a YAML anchorThe exact same five key/values are repeated for every Postgres store. Repetition invites drift and makes future rotations (e.g., changing
dbname) error-prone.postgres_conn: &postgres_conn type: postgres host: ${env.POSTGRES_HOST:?missing} port: ${env.POSTGRES_PORT:?missing} db: ${env.POSTGRES_DB:?missing} user: ${env.POSTGRES_USER:?missing} password: ${env.POSTGRES_PASSWORD:?missing} # … persistence_store: <<: *postgres_conn responses_store: <<: *postgres_connOne change, everywhere.
Minor now, lifesaver later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/llama_stack_client_config.yaml(2 hunks)template.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template.yaml
⏰ 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
🔇 Additional comments (1)
config/llama_stack_client_config.yaml (1)
65-71: Confirm table initialisation & permissions forllamastack_kvstoreA dedicated
table_nameis great. Ensure:
- Migration/DDL for creating the table (CI/CD or app-side auto-create).
POSTGRES_USERhasCREATE/ALTERrights if auto-creation is expected.- Proper indexing (primary key) is defined, otherwise metadata look-ups will degrade quickly.
Please verify these DB-level prerequisites are covered before merge.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
assisted-chat-pod.yaml (1)
86-94:pgdatavolume is referenced but never defined – Pod will fail to scheduleThe
postgrescontainer mounts a volume namedpgdata, yetspec.volumesonly definesconfig. Add aemptyDir(or hostPath for dev) volume declaration:volumeMounts: - name: pgdata mountPath: /var/lib/pgsql/data @@ volumes: - name: config hostPath: path: ./config type: Directory + - name: pgdata + emptyDir: {}
🧹 Nitpick comments (1)
assisted-chat-pod.yaml (1)
75-88: Add liveness / readiness probes for the PostgreSQL side-carWithout probes, the main container may attempt to connect before Postgres finishes initialisation, leading to spurious failures. Consider:
readinessProbe: exec: command: ["psql", "-U", "llamastack", "-d", "llamastack", "-c", "SELECT 1"] initialDelaySeconds: 10 periodSeconds: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assisted-chat-pod.yaml(2 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assisted-chat-pod.yaml(2 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
assisted-chat-pod.yaml
[MEDIUM] 1-97: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 1-97: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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
🔇 Additional comments (2)
assisted-chat-pod.yaml (2)
12-21: Verify env-var names match llama-stack client expectations
LLAMA_STACK_POSTGRES_HOST|PORT|USER|PASSWORD|NAMElook reasonable, but the client library recently standardised onLLAMA_STACK_DB_*in the template (LLAMA_STACK_DB_SECRET_NAMEwas added too).
A mismatch will surface only at runtime with a “could not parse DSN” error.Please cross-check the code path that builds the SQLAlchemy URL and align the keys (or export both sets for safety).
75-88: Reuse of./confighostPath for database data risks corruptionThe Postgres container writes its WAL and relation files to
/var/lib/pgsql/data.
Mapping that to the same host directory that already stores YAML configs (./config) mixes mutable database files with source-controlled config and can cause:
- accidental deletion/overwrite when configs are cleaned
- permission clashes between the two containers
- noisy
pg_walgrowth inside your git working treeAllocate a dedicated path (or simply an
emptyDir) for PGDATA.@@ - - name: pgdata - mountPath: /var/lib/pgsql/data + - name: pgdata + mountPath: /var/lib/pgsql/data[ suggest_essential_refactor ]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
assisted-chat-pod.yaml (1)
12-21: Plain-text DB creds were already flagged – no further action unless policy changed
Previous review (see history) covered the risk of embedding credentials directly in the manifest and suggestedvalueFrom.secretKeyRef. Your reply indicated this file is “local-dev only”. No new concern beyond the earlier comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assisted-chat-pod.yaml(2 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
assisted-chat-pod.yaml
[MEDIUM] 1-97: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 1-97: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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
🔇 Additional comments (1)
assisted-chat-pod.yaml (1)
75-88: PostgreSQL container looks correct for the RHEL-9 image
The move toPOSTGRESQL_*variables and a dedicatedpgdatamount aligns with the image contract. Nice catch on the variable prefix.
|
/test images |
maorfr
force-pushed
the
llama-stack-persistence
branch
from
70e5607 to
bf1bb70
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
assisted-chat-pod.yaml (1)
94-97: RelativehostPathblocks scheduling – use an absolute path &DirectoryOrCreateKubernetes rejects
hostPathentries that are not absolute.
Persisting the database on the node also benefits from creating the directory automatically.- - name: pgdata - hostPath: - path: ./pgdata - type: Directory + - name: pgdata + hostPath: + path: /var/lib/assisted-chat/pgdata # any suitable absolute path + type: DirectoryOrCreate
🧹 Nitpick comments (1)
assisted-chat-pod.yaml (1)
12-21: Plain-text DB credentials are fine for local dev – add a warning commentGiven this file is strictly for local development, embedding the credentials is acceptable, but consider adding an explicit comment so future contributors don’t accidentally promote the manifest to a shared cluster.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Containerfile.add_llama_to_lightspeed(1 hunks)Containerfile.assisted-chat(1 hunks)assisted-chat-pod.yaml(2 hunks)config/llama_stack_client_config.yaml(2 hunks)scripts/generate.sh(1 hunks)template.yaml(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- Containerfile.add_llama_to_lightspeed
- scripts/generate.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- config/llama_stack_client_config.yaml
- Containerfile.assisted-chat
🧰 Additional context used
🪛 Checkov (3.2.334)
assisted-chat-pod.yaml
[MEDIUM] 1-97: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 1-97: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (1)
template.yaml (1)
245-258: Verify that${env.*}interpolation actually happens inside the ConfigMapThe YAML embeds placeholders like
${env.LLAMA_STACK_POSTGRES_HOST}which will be written verbatim into the mounted file.
Interpolation only happens at runtime if llama-stack’s config loader expands${env.*}patterns – if that mechanism is absent the application will try to connect to literally"${env.LLAMA_STACK_POSTGRES_HOST}"and fail.Please confirm the library performs this substitution, or pre-render the values before writing the ConfigMap.
Also applies to: 282-296
maorfr
force-pushed
the
llama-stack-persistence
branch
from
c16a57b to
631c357
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
assisted-chat-pod.yaml (1)
25-31:mountPathcontains unsupported:Zsuffix – pod will be rejected
/app-root/lightspeed-stack.yaml:Zis invalid in a Kubernetes manifest; the colon+flag syntax only applies todocker run/podman run.
Kubernetes expects an absolute path without extra modifiers.- - mountPath: /app-root/lightspeed-stack.yaml:Z + - mountPath: /app-root/lightspeed-stack.yaml
♻️ Duplicate comments (1)
assisted-chat-pod.yaml (1)
12-21: Credentials still in plain text (acknowledged as intentional)Hard-coding DB user/password exposes them in the spec. Previous discussion concluded this is acceptable for local-dev only, so no further action taken here.
Also applies to: 78-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Containerfile.add_llama_to_lightspeed(1 hunks)Containerfile.assisted-chat(1 hunks)assisted-chat-pod.yaml(2 hunks)config/llama_stack_client_config.yaml(2 hunks)template.yaml(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- Containerfile.add_llama_to_lightspeed
🚧 Files skipped from review as they are similar to previous changes (3)
- config/llama_stack_client_config.yaml
- Containerfile.assisted-chat
- template.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
assisted-chat-pod.yaml
[MEDIUM] 1-95: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 1-95: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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
🔇 Additional comments (1)
assisted-chat-pod.yaml (1)
75-88: PostgreSQL container block looks correctRenaming to
POSTGRESQL_*matches the RHEL-9 image requirements, and theemptyDirvolume mount is wired properly. LGTM.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, maorfr 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 |
|
/unhold |
2 participants
part of https://issues.redhat.com/browse/MGMT-21299
depends on https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/151196
this PR updates the llama-stack client config to use a postgres database for persistence.
according to https://github.com/meta-llama/llama-stack/tree/main/llama_stack/templates/postgres-demo
Summary by CodeRabbit
New Features
Chores