feat: add kserve - #56
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates model serving in the Thalamus Helm chart from custom Deployment/EPP/InferencePool templates to KServe's LLMInferenceService/LLMInferenceServiceConfig resources, reworks values schema (configs/models), updates gateway/httproute policy rendering, adds cert-manager/KServe helmfile releases, downgrades the gateway-api-inference-extension to v1.4.0, and updates E2E readiness checks accordingly. ChangesKServe LLMInferenceService migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: One nit worth flagging directly: the gateway-api-inference-extension version change is a downgrade (v1.5.0 → v1.4.0), not an upgrade — worth double-checking that's intentional given it also swaps out the InferencePool CRD schema wholesale. Also, models.yaml/epp.yaml removal with zero backward-compat shim means any consumer still relying on those Deployment/Service/InferencePool resources breaks outright on upgrade. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
811b564 to
92e2da7
Compare
92e2da7 to
26ff33e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/e2e.yaml (1)
59-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify the wait timeout still fits inside the job's overall budget.
The step now allows up to 600s (10 min) for a single wait, but the job-level
timeout-minutesis still 20 (line 24). Combined with cluster bootstrap,helmfile apply(image pulls), port-forward, and validation retries, a slow model pull could push the total runtime close to or past the 20-minute cap, killing the job beforevalidate.sheven runs.Consider bumping
timeout-minutesor trimming the wait timeout to preserve headroom for the remaining steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml around lines 59 - 65, The LLMInferenceService readiness wait can consume too much of the job’s total runtime, leaving insufficient headroom for later bootstrap and validation steps. Review the e2e workflow job that contains the kubectl wait for LLMInferenceService/vllm-arnir0-tiny-llm and either raise the job-level timeout-minutes or reduce the wait --timeout so the overall pipeline still comfortably fits within the budget, especially before validate.sh runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/example.values.cpu.yaml`:
- Around line 11-14: The example still contains legacy thalamus.epp.resources
configuration even though it now uses the KServe model schema. Remove that dead
EPP override from the example values file, or migrate the resource settings into
the new config model if they are still needed, and keep the example aligned with
the updated schema used by the arnir0/Tiny-LLM entry.
In `@helm/thalamus/templates/llmisvc.yaml`:
- Line 7: The LLMInferenceService name generation in the templating logic for
the vllm resource can exceed the DNS label limit and break downstream KServe
child resource creation. Update the name construction around the vllm-{{
regexReplaceAll ... }} expression to cap the base slug length, leaving room for
generated suffixes, and if needed append a stable hash so names remain unique
while staying within the 63-character limit.
In `@helm/thalamus/values.yaml`:
- Line 148: The documented default for replicas is not actually applied, because
the llmisvc template reads $m.replicas directly instead of falling back to 1.
Update the rendering logic in the llmisvc template to use a default value when
replicas is omitted, and make sure the values.yaml documentation stays
consistent with the template behavior by referencing the existing $m.replicas
field and the template’s replica count output.
- Around line 133-138: The default CPU security settings are too permissive
because the CPU model config currently sets an unconfined seccomp profile and
adds SYS_NICE for every deployment. Update the Helm values in the CPU default
securityContext to use RuntimeDefault and remove elevated capabilities from the
default path, then move any special privileges into an explicit opt-in override
or dedicated config. Keep the change localized to the securityContext block so
the default chart remains locked down.
---
Nitpick comments:
In @.github/workflows/e2e.yaml:
- Around line 59-65: The LLMInferenceService readiness wait can consume too much
of the job’s total runtime, leaving insufficient headroom for later bootstrap
and validation steps. Review the e2e workflow job that contains the kubectl wait
for LLMInferenceService/vllm-arnir0-tiny-llm and either raise the job-level
timeout-minutes or reduce the wait --timeout so the overall pipeline still
comfortably fits within the budget, especially before validate.sh runs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 996a90c6-77db-475e-8b60-e5ca8444251e
📒 Files selected for processing (15)
.github/workflows/e2e.yamlhelm/Makefilehelm/example.values.cpu.yamlhelm/example.values.yamlhelm/gateway-api-inference-extension/Chart.yamlhelm/gateway-api-inference-extension/templates/crds.yamlhelm/helmfile.yaml.gotmplhelm/thalamus/ci/test-values.yamlhelm/thalamus/templates/configs.yamlhelm/thalamus/templates/epp.yamlhelm/thalamus/templates/gateway.yamlhelm/thalamus/templates/httproute.yamlhelm/thalamus/templates/llmisvc.yamlhelm/thalamus/templates/models.yamlhelm/thalamus/values.yaml
💤 Files with no reviewable changes (3)
- helm/thalamus/ci/test-values.yaml
- helm/thalamus/templates/epp.yaml
- helm/thalamus/templates/models.yaml
| - name: arnir0/Tiny-LLM | ||
| config: cpu | ||
| replicas: 1 | ||
| extraArgs: | ||
| args: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Finish removing the legacy EPP override from this example.
This example now uses the KServe model schema, but the same file still contains thalamus.epp.resources below. Since this PR removes the legacy EPP values/templates, that block is dead config and should be deleted or migrated to the new config model.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/example.values.cpu.yaml` around lines 11 - 14, The example still
contains legacy thalamus.epp.resources configuration even though it now uses the
KServe model schema. Remove that dead EPP override from the example values file,
or migrate the resource settings into the new config model if they are still
needed, and keep the example aligned with the updated schema used by the
arnir0/Tiny-LLM entry.
| apiVersion: serving.kserve.io/v1alpha2 | ||
| kind: LLMInferenceService | ||
| metadata: | ||
| name: vllm-{{ regexReplaceAll "[^a-z0-9-]+" (lower $m.name) "-" }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether KServe truncates/hashes derived child resource names itself,
# which would make this non-issue.
rg -n "trunc|63|DNS1035|GenerateName" --type=go -C3 -g '*llmisvc*'Repository: cobaltcore-dev/thalamus
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target template and locate related naming logic.
git ls-files 'helm/thalamus/templates/*' 'helm/**' | sed -n '1,120p'
echo '--- llmisvc.yaml ---'
cat -n helm/thalamus/templates/llmisvc.yaml | sed -n '1,120p'
echo '--- search ---'
rg -n "LLMInferenceService|GenerateName|GenerateName:|metadata.name|trunc|trimSuffix|DNS1035|DNS-1035|name:" helm -g '*.yaml' -g '*.yml' -g '*.tpl' -g '*.gotmpl'Repository: cobaltcore-dev/thalamus
Length of output: 26981
🌐 Web query:
KServe LLMInferenceService child resource name truncation DNS1035 generated Service Deployment EndpointSlice 63 characters
💡 Result:
In Kubernetes, resources requiring DNS-compliant names (such as Services and certain label values) must adhere to the RFC 1035 standard, which mandates a maximum length of 63 characters [1]. When KServe creates child resources—such as Kubernetes Services, Deployments, or EndpointSlices—based on the name of an LLMInferenceService (LLMISVC) resource, the combined length of the parent name and the appended suffix often exceeds this 63-character limit [2][3]. If the resulting resource name exceeds 63 characters, the Kubernetes API server will reject the creation of the child resource because it violates the DNS-1035 label constraint [2][3][4]. Key points regarding this limitation: 1. Resource Creation Failure: When a user defines an LLMInferenceService with a long name (e.g., nearing or exceeding 50+ characters), KServe's automatic generation of child resources (e.g., appending suffixes like -predictor, -service, or -inference-pool) frequently causes the final name to surpass the 63-character limit [2][5][3]. 2. Validation Challenges: Because Kubernetes validation is strict regarding DNS-1035 labels, KServe must account for these limits when generating names [6][4]. While some attempts have been made to shorten internal component names (such as webhooks) to comply with label limits, these can occasionally conflict with existing installations or OLM requirements, leading to reverts or more complex solutions like label sanitization (hashing) by operators rather than truncation [7][8]. 3. Recommended Practice: To avoid these failures, users should ensure that the names of their LLMInferenceService resources are kept sufficiently short to allow for the additional characters appended by the controller during resource generation [2][3]. As a rule of thumb, developers should subtract the length of the expected suffix (e.g., the length of "-predictor" or other operational suffixes) from 63 to determine the maximum allowed length for their custom resource names [3].
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 2: Fix service name more than 63 kserve/kserve#4208
- 3: Long Service/Configuration names cause k8s service creation to fail knative/serving#3236
- 4: KService name validation should follow the k8s service restrictions knative/serving#2942
- 5: https://kserve.github.io/website/docs/model-serving/generative-inference/llmisvc/llmisvc-envoy-ai-gateway
- 6: Use RFC 1123 instead of RFC 1035 for names validation knative/serving#16007
- 7: fix(llmisvc): shorten webhook name to comply with 63-char label limit kserve/kserve#5381
- 8: revert(llmisvc): restore original webhook names, reverts #5381 kserve/kserve#5398
Cap the LLMInferenceService name length. The slugified model id can push the base name past the 63-char DNS label limit, and KServe-derived child resource names can then be rejected. Truncate or hash the base name so there is room for suffixes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/thalamus/templates/llmisvc.yaml` at line 7, The LLMInferenceService name
generation in the templating logic for the vllm resource can exceed the DNS
label limit and break downstream KServe child resource creation. Update the name
construction around the vllm-{{ regexReplaceAll ... }} expression to cap the
base slug length, leaving room for generated suffixes, and if needed append a
stable hash so names remain unique while staying within the 63-character limit.
| securityContext: | ||
| seccompProfile: | ||
| type: Unconfined | ||
| capabilities: | ||
| add: | ||
| - SYS_NICE |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not ship the default CPU config with elevated runtime privileges.
seccompProfile: Unconfined plus SYS_NICE weakens container isolation for every CPU model using this default. Move this into an explicit opt-in config, or keep the default on RuntimeDefault with dropped capabilities.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/thalamus/values.yaml` around lines 133 - 138, The default CPU security
settings are too permissive because the CPU model config currently sets an
unconfined seccomp profile and adds SYS_NICE for every deployment. Update the
Helm values in the CPU default securityContext to use RuntimeDefault and remove
elevated capabilities from the default path, then move any special privileges
into an explicit opt-in override or dedicated config. Keep the change localized
to the securityContext block so the default chart remains locked down.
There was a problem hiding this comment.
Got this configuration from official vllm docs to improve docker compatibility.
There was a problem hiding this comment.
@henrichter-sap Thanks for the reference — that's a well-known and documented requirement for vLLM's CPU backend (NUMA memory policy calls need SYS_NICE, and the default Docker seccomp profile blocks get_mempolicy). Since this is scoped to the cpu config only and doesn't affect the nvidia config, I'm withdrawing this concern.
🐰💭
✏️ Learnings added
Learnt from: henrichter-sap
Repo: cobaltcore-dev/thalamus PR: 56
File: helm/thalamus/values.yaml:133-138
Timestamp: 2026-07-07T19:44:27.966Z
Learning: In helm/thalamus/values.yaml, the `configs.cpu.spec.template.containers[].securityContext` intentionally sets `seccompProfile.type: Unconfined` and adds the `SYS_NICE` capability for the vLLM CPU backend. This is required per official vLLM docs (https://docs.vllm.ai/en/stable/getting_started/installation/cpu/) to avoid `get_mempolicy: Operation not permitted` errors caused by NUMA memory policy calls being blocked by Docker's default seccomp profile. This is not a security oversight and should not be flagged as such in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
9ad404a to
9492ecf
Compare
1 participant
Summary by CodeRabbit