Add playground build and deployment #1
Conversation
akram
force-pushed
the
containerize-chatbot
branch
3 times, most recently
from
5e4f075 to
8f7c0f8
Compare
| ## Getting manifests | ||
|
|
||
| ``` | ||
| oc eksporter is > 01-is.yaml |
There was a problem hiding this comment.
What does this eksporter supposed to mean here?
There was a problem hiding this comment.
hi @Ygnas ,
it is a krew plugin that does the export of kubernetes resources and clean all namespace dependant, or status dependent data.
There was a problem hiding this comment.
Should we mention about it in the README?
akram
force-pushed
the
containerize-chatbot
branch
2 times, most recently
from
c785556 to
5ffb4ad
Compare
| - "--server.address=0.0.0.0" | ||
| env: | ||
| - name: LLAMA_STACK_ENDPOINT | ||
| value: http://llama-test-milvus-service-llamastack.apps.rosa.akram.vsil.p3.openshiftapps.com |
There was a problem hiding this comment.
This looks like your own private cluster address should we instead provide a generic http://example-llama-stack-service.com example instead?
akram
force-pushed
the
containerize-chatbot
branch
from
5ffb4ad to
fc87971
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis update introduces a set of OpenShift and Kubernetes resource manifests, a deployment automation script, and a README for deploying the "playground" UI component. The changes define image streams, build and deployment configurations, service exposure, and routing, along with documentation and export automation for these resources. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OpenShift
participant GitHub
participant ImageRegistry
participant PlaygroundPod
User->>OpenShift: Apply manifests (oc create -f .)
OpenShift->>ImageRegistry: Import python-312 image (ImageStream)
OpenShift->>GitHub: Clone repo for BuildConfig
OpenShift->>ImageRegistry: Build playground image (BuildConfig)
OpenShift->>ImageRegistry: Store built image (ImageStreamTag)
OpenShift->>PlaygroundPod: Deploy container (Deployment)
User->>OpenShift: Access Route
OpenShift->>PlaygroundPod: Forward request via Service
PlaygroundPod-->>User: Responds to request
Suggested labels
Poem
✨ Finishing Touches
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: 6
♻️ Duplicate comments (1)
ui/03-dc.yaml (1)
33-36: Environment variable hard-codes a private cluster URL
Previous feedback noted this (see past review). The valuellama-test-milvus-service-llamastack.apps.rosa.akram...binds the manifest to your personal ROSA cluster.Replace with a placeholder and document patching steps, e.g.:
- value: http://llama-test-milvus-service-llamastack.apps.rosa.akram.vsil.p3.openshiftapps.com + value: http://{{ LLAMA_STACK_ENDPOINT }}
🧹 Nitpick comments (8)
ui/04-svc.yaml (2)
11-18: No health-check port names reused by probes or route
While the port block is fine, consider addingappProtocol: http(K8s ≥1.26) to improve service mesh / gateway awareness:ports: - name: http port: 8501 protocol: TCP + appProtocol: http
16-17: Selector couples service to Deployment labeldeployment=playground
If you ever scale viakubectl rollout restartthe new RS label will still match, but switching to a more semantic label (app: playground) gives future flexibility (Helm charts, Kustomize overlays).ui/02-bc.yaml (2)
8-9: Trailing whitespace & emptynodeSelectorkey
Line 8 has a trailing space and an empty mapping value – YAML parsers accept it but it’s noisy in diffs.- nodeSelector: + # nodeSelector intentionally left blank; remove key until used
24-27: Pinned builder image tag is cluster-specific – build will break on another cluster
python-312:1-40.1747189120includes a build number from the current OpenShift registry namespace. Use a semantic tag (python-312:latest) or parameterise via aBuildConfigenvironment to ensure portability.ui/03-dc.yaml (1)
22-22: Trailing whitespace
Line 22 contains a single trailing space – clean it to silenceyamllint.ui/01-is.yaml (1)
18-18: Trim trailing whitespace & omit empty keyLine 18 ends with superfluous spaces and the
annotations:key is empty.
YAML parsers tolerate this, but it creates needless diffs and violates the repo’s linting rules (see yaml-lint error).- - annotations: + - # annotations: <none>Either drop the key or add real content to keep the file clean.
ui/README.md (2)
5-15: Specify language for fenced blocks to satisfy MD040All code snippets lack a language hint, triggering markdown-lint and disabling syntax highlighting.
-``` +```bash git clone https://github.com/meta-llama/llama-stack.git cd llama_stack/distribution/uiApply the same `bash` (or `sh`) tag to every other fenced block. --- `28-31`: **Consider splitting the long `oc patch` into a yaml patch file** Inline JSON patches are error-prone and unreadable. Storing the patch in `patch/deployment-streamlit.json` (or using `oc apply -f`) improves maintainability and allows CI diffing. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 98f060957571912b04905ab0c2aaf15a670be860 and fc879712b82319db6786d85a325c69de8fc7bfa8. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `ui/01-is.yaml` (1 hunks) * `ui/02-bc.yaml` (1 hunks) * `ui/03-dc.yaml` (1 hunks) * `ui/04-svc.yaml` (1 hunks) * `ui/05-route.yaml` (1 hunks) * `ui/README.md` (1 hunks) * `ui/export-manifests.sh` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Shellcheck (0.10.0)</summary> <details> <summary>ui/export-manifests.sh</summary> [error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive. (SC2148) </details> </details> <details> <summary>🪛 YAMLlint (1.37.1)</summary> <details> <summary>ui/01-is.yaml</summary> [error] 18-18: trailing spaces (trailing-spaces) </details> <details> <summary>ui/02-bc.yaml</summary> [error] 8-8: trailing spaces (trailing-spaces) </details> <details> <summary>ui/03-dc.yaml</summary> [error] 22-22: trailing spaces (trailing-spaces) </details> </details> <details> <summary>🪛 Checkov (3.2.334)</summary> <details> <summary>ui/03-dc.yaml</summary> [MEDIUM] 2-49: Containers should not run with allowPrivilegeEscalation (CKV_K8S_20) --- [MEDIUM] 2-49: Minimize the admission of root containers (CKV_K8S_23) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>ui/README.md</summary> 5-5: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 12-12: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 19-19: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 28-28: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 34-34: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 42-42: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 52-52: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>ui/export-manifests.sh (1)</summary> `1-5`: **Hard-coded filenames may overwrite handcrafted manifests** If a developer keeps modified versions of `01-is.yaml` etc., this script will overwrite them without notice. Emit to a `tmp/` dir or require an explicit `--force` flag. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| spec: | ||
| port: | ||
| targetPort: http | ||
| to: | ||
| kind: Service | ||
| name: playground | ||
| weight: 100 | ||
| wildcardPolicy: None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Route lacks TLS termination and explicit host – external access will default to HTTP on a generated hostname
You are creating a public-facing entry-point but leave both spec.host and spec.tls unset.
• Users will be redirected to an automatically generated *.apps.<cluster> hostname with plain HTTP.
• Most browsers display security warnings or block mixed-content calls when the UI later fetches HTTPS back-end endpoints.
Add an explicit host (or parameterise it) and enable edge termination:
to:
kind: Service
name: playground
weight: 100
+ tls:
+ termination: edge # or re-encrypt/passthrough
+ insecureEdgeTerminationPolicy: Redirect
+ host: playground.example.com # 👉 make this configurable in overlays / Helm valuesConfirm whether the cluster is front-ended by a load balancer that injects TLS automatically; if not, the above is required.
Route lacks TLS termination and explicit host – external access will default to HTTP on a generated hostname
You are creating a public-facing entry-point but leave both spec.host and spec.tls unset.
• Users will be redirected to an automatically generated *.apps.<cluster> hostname with plain HTTP.
• Most browsers display security warnings or block mixed-content calls when the UI later fetches HTTPS back-end endpoints.
Add an explicit host (or parameterise it) and enable edge termination:
to:
kind: Service
name: playground
weight: 100
+ tls:
+ termination: edge # or re-encrypt/passthrough
+ insecureEdgeTerminationPolicy: Redirect
+ host: playground.example.com # 👉 make this configurable in overlays / Helm valuesConfirm whether the cluster is front-ended by a load balancer that injects TLS automatically; if not, the above is required.
🤖 Prompt for AI Agents
In ui/05-route.yaml around lines 6 to 13, the route definition lacks both an
explicit host and TLS termination, causing the route to default to HTTP on a
generated hostname, which can lead to browser security warnings. To fix this,
add a `spec.host` field with a specific hostname or parameterize it, and include
a `spec.tls` section with `termination: edge` to enable TLS termination at the
edge. Verify if the cluster uses a load balancer that handles TLS automatically;
if not, these changes are necessary to ensure secure external access.
| oc eksporter is > 01-is.yaml | ||
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | ||
| oc eksporter deployment playground > 03-dc.yaml | ||
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | ||
| oc eksporter route playground --drop spec.host > 05-route.yaml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing shebang and defensive flags – script fails shellcheck & may silently succeed with partial output
SC2148– add#!/usr/bin/env bash.- Use
set -euo pipefailso CI stops on failures (e.g., missingeksporterplugin). - Consider validating that each target exists before export.
+#!/usr/bin/env bash
+set -euo pipefail
+
oc eksporter is > 01-is.yaml
oc eksporter bc playground --drop spec.triggers > 02-bc.yaml
oc eksporter deployment playground > 03-dc.yaml
oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml
oc eksporter route playground --drop spec.host > 05-route.yaml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc eksporter is > 01-is.yaml | |
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | |
| oc eksporter deployment playground > 03-dc.yaml | |
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | |
| oc eksporter route playground --drop spec.host > 05-route.yaml | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| oc eksporter is > 01-is.yaml | |
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | |
| oc eksporter deployment playground > 03-dc.yaml | |
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | |
| oc eksporter route playground --drop spec.host > 05-route.yaml |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In ui/export-manifests.sh lines 1 to 5, the script lacks a shebang and defensive
shell options, causing shellcheck failures and potential silent errors. Add the
shebang line `#!/usr/bin/env bash` at the top, then add `set -euo pipefail` to
ensure the script exits on errors or unset variables. Additionally, before each
`oc eksporter` command, add checks to verify the target resource exists to
prevent partial or empty output.
| containers: | ||
| - command: | ||
| - streamlit | ||
| - run | ||
| - app.py | ||
| - "--server.port=8501" | ||
| - "--server.address=0.0.0.0" | ||
| env: | ||
| - name: LLAMA_STACK_ENDPOINT | ||
| value: http://llama-test-milvus-service-llamastack.apps.rosa.akram.vsil.p3.openshiftapps.com | ||
| image: image-registry.openshift-image-registry.svc:5000/llamastack/playground@sha256:00a3008aa70832bc679513b21cacc4e2ae5e875ee648dfbd60264a8b2b42d65b | ||
| imagePullPolicy: IfNotPresent | ||
| name: playground | ||
| ports: | ||
| - containerPort: 8501 | ||
| protocol: TCP | ||
| resources: {} | ||
| terminationMessagePath: "/dev/termination-log" | ||
| terminationMessagePolicy: File |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No security context, probes or resource limits – fails basic hardening policies
Static analysis flags privilege escalation & root user. Also missing liveness/readiness probes, which Streamlit readily supports via /_stcore/health.
Minimal hardened snippet:
resources:
- {}
+ limits:
+ memory: "512Mi"
+ cpu: "500m"
+ requests:
+ memory: "256Mi"
+ cpu: "100m"
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
+ seccompProfile:
+ type: RuntimeDefault
+ livenessProbe:
+ httpGet:
+ path: /_stcore/health
+ port: 8501
+ initialDelaySeconds: 30
+ periodSeconds: 10
+ readinessProbe:
+ httpGet:
+ path: /_stcore/health
+ port: 8501
+ initialDelaySeconds: 5
+ periodSeconds: 10🤖 Prompt for AI Agents
In ui/03-dc.yaml between lines 26 and 44, the container spec lacks security
context settings to prevent privilege escalation and avoid running as root, and
it also misses liveness and readiness probes for health checks. Add a
securityContext section to disable privilege escalation and run as a non-root
user, and define livenessProbe and readinessProbe using HTTP GET requests to the
Streamlit health endpoint /_stcore/health on port 8501. Also, specify resource
limits and requests to enforce resource usage constraints.
| tags: | ||
| - annotations: | ||
| from: | ||
| kind: DockerImage | ||
| name: registry.redhat.io/ubi8/python-312 | ||
| generation: 1 | ||
| importPolicy: | ||
| importMode: Legacy | ||
| name: latest | ||
| referencePolicy: | ||
| type: Source |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the base image by digest for deterministic builds
Tagging registry.redhat.io/ubi8/python-312:latest means every rebuild may silently pick up a newer image, breaking reproducibility and SBOM requirements. Prefer an immutable digest:
- name: registry.redhat.io/ubi8/python-312
+ # sha256 taken from `podman pull --quiet` output
+ name: registry.redhat.io/ubi8/python-312@sha256:<digest>Importing by digest still lets you tag it as latest locally while ensuring provenance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tags: | |
| - annotations: | |
| from: | |
| kind: DockerImage | |
| name: registry.redhat.io/ubi8/python-312 | |
| generation: 1 | |
| importPolicy: | |
| importMode: Legacy | |
| name: latest | |
| referencePolicy: | |
| type: Source | |
| tags: | |
| - annotations: | |
| from: | |
| kind: DockerImage | |
| # sha256 taken from `podman pull --quiet` output | |
| name: registry.redhat.io/ubi8/python-312@sha256:<digest> | |
| generation: 1 | |
| importPolicy: | |
| importMode: Legacy | |
| name: latest | |
| referencePolicy: | |
| type: Source |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 18-18: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In ui/01-is.yaml around lines 17 to 27, the base image is referenced using the
tag 'latest', which can lead to non-deterministic builds. Replace the image tag
with a specific immutable digest to ensure reproducibility and compliance with
SBOM requirements. Update the 'name' field to use the digest format (e.g.,
'registry.redhat.io/ubi8/python-312@sha256:<digest>') instead of 'latest' while
keeping the local tag as 'latest' if needed.
| LS_ROUTE=$(oc get route llamastack -ojsonpath={.spec.host}) | ||
| oc set env deployment/playground LLAMA_STACK_ENDPOINT=http://$LS_ROUTE |
There was a problem hiding this comment.
Quote the jsonpath expression to avoid shell brace-expansion
Unquoted {} is interpreted by most shells (e.g., zsh) and may fail.
-LS_ROUTE=$(oc get route llamastack -ojsonpath={.spec.host})
+LS_ROUTE=$(oc get route llamastack -o jsonpath='{.spec.host}')Keeps the command POSIX-portable and immune to globbing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LS_ROUTE=$(oc get route llamastack -ojsonpath={.spec.host}) | |
| oc set env deployment/playground LLAMA_STACK_ENDPOINT=http://$LS_ROUTE | |
| LS_ROUTE=$(oc get route llamastack -o jsonpath='{.spec.host}') | |
| oc set env deployment/playground LLAMA_STACK_ENDPOINT=http://$LS_ROUTE |
🤖 Prompt for AI Agents
In ui/README.md around lines 20 to 21, the jsonpath expression in the oc get
route command is unquoted, which can cause shell brace-expansion issues in some
shells like zsh. To fix this, enclose the jsonpath expression {.spec.host} in
single or double quotes to prevent shell interpretation and ensure POSIX
portability and correct command execution.
| ``` | ||
| oc eksporter is > 01-is.yaml | ||
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | ||
| oc eksporter deployment playground > 03-dc.yaml | ||
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | ||
| oc eksporter route playground --drop spec.host > 05-route.yaml | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the eksporter dependency
Previous feedback asked for clarification, but the README still assumes the plugin exists. Add installation instructions or an alternative (kubectl neat, oc adm export).
+# Prerequisite
+kubectl krew install eksporter # https://github.com/…/eksporterWithout this, newcomers hit “command not found”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| oc eksporter is > 01-is.yaml | |
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | |
| oc eksporter deployment playground > 03-dc.yaml | |
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | |
| oc eksporter route playground --drop spec.host > 05-route.yaml | |
| ``` | |
| # Prerequisite | |
| kubectl krew install eksporter # https://github.com/…/eksporter | |
| oc eksporter is > 01-is.yaml | |
| oc eksporter bc playground --drop spec.triggers > 02-bc.yaml | |
| oc eksporter deployment playground > 03-dc.yaml | |
| oc eksporter svc playground --drop spec.clusterIPs > 04-svc.yaml | |
| oc eksporter route playground --drop spec.host > 05-route.yaml |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
42-42: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In ui/README.md around lines 42 to 48, the README uses the `oc eksporter`
command without explaining how to install or obtain the `eksporter` plugin,
which causes confusion for new users. Add a section before these commands that
provides installation instructions for the `eksporter` plugin or suggest
alternative tools like `kubectl neat` or `oc adm export` with brief usage notes.
This will ensure users know how to get the required tool before running the
commands.
Description
Add playground build and deployment
How Has This Been Tested?
Documentation, just repeating the tasks
Merge criteria:
Summary by CodeRabbit
New Features
Documentation