Repository navigation
fix(helm): fix security context and test discovery for SMG chart - #861
Conversation
What changed:
- deploy/helm/smg/templates/_helpers.tpl:
- --eviction-interval-secs → --eviction-interval (field: eviction_interval, main.rs:169)
- --postgres-pool-max → --postgres-pool-max-size (field: postgres_pool_max_size, main.rs:529)
- --redis-pool-max → --redis-pool-max-size (field: redis_pool_max_size, main.rs:538)
- --oracle-connect-descriptor → --oracle-dsn (field: oracle_dsn, main.rs:491)
- --oracle-username → --oracle-user (field: oracle_user, main.rs:495)
- deploy/helm/smg/Chart.yaml:
- appVersion: "0.7.0" → "1.3.3" (latest release tag v1.3.3)
- deploy/helm/smg/values.schema.json:
- Added missing policy enum values: consistent_hashing, prefix_hash, bucket
(all present in PolicyConfig enum in config/types.rs:247-320)
- deploy/helm/smg/values.yaml:
- evictionIntervalSecs: 60 → 120 (matches default_value_t = 120, main.rs:168)
- Updated policy comment to list all 8 valid values
Why:
The Helm chart generated incorrect CLI flags that would cause the SMG
binary to reject them at startup. Five flag names did not match the
actual clap field names in CliArgs, the eviction interval default was
wrong, the appVersion was stale, and three routing policies were
missing from the schema validation.
How:
Cross-referenced every --flag-name in _helpers.tpl against the CliArgs
struct fields in model_gateway/src/main.rs (clap converts field
snake_case to --kebab-case). Verified all default values in values.yaml
against default_value_t annotations. Checked PolicyConfig enum variants
in config/types.rs against values.schema.json. Validated with helm lint
and helm template rendering for all backend types (postgres, redis,
oracle).
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
The parse_policy function in main.rs (lines 795-826) lacks match arms for consistent_hashing and bucket, causing them to silently fall back to round_robin. Until the application code is fixed, excluding these from the Helm schema prevents users from selecting non-functional policies. prefix_hash is retained as it is correctly handled. Addresses review feedback from Gemini Code Assist and CodeRabbit. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
What changed: - deploy/helm/smg/values.yaml: changed runAsNonRoot default from true to false, since the lightseekorg/smg:1.3.3 image runs as root and does not define a non-root USER. Added a comment explaining when to set it back to true. - deploy/helm/smg/templates/deployment-router.yaml: added an emptyDir volume mounted at /tmp so readOnlyRootFilesystem: true does not prevent the process from writing temporary files. - deploy/helm/smg/tests/test-connection.yaml: moved from tests/ to templates/tests/ so Helm can discover and render the test pod template. This fixes `helm test` returning "TEST SUITE: None". Why: The chart could not deploy at all — the pod entered CreateContainerConfigError because the container runtime rejected runAsNonRoot: true for an image that runs as UID 0. The helm test suite was also invisible because it was placed outside templates/. How: Tested against OCI cluster (moirai-eu-frankfurt-1-dev) in the helm-test-minimal namespace with image lightseekorg/smg:1.3.3. Confirmed pod reaches Running/Ready, /health returns 200, and `helm test` passes after these fixes. Refs: #859 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
📝 WalkthroughWalkthroughThis PR updates the SMG Helm chart to version 1.3.3, renames router CLI argument flags to match the updated application interface, adds an ephemeral tmp volume to the router container, extends the supported routing policies, and adjusts security and performance configuration values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and deployability of the SMG Helm chart. It resolves critical deployment failures related to container security contexts and read-only filesystems, while also improving the discoverability of Helm tests. Additionally, it incorporates minor configuration updates and introduces a new routing policy option, ensuring a more stable and flexible deployment experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/Chart.yaml`:
- Around line 5-6: The chart's version field in Chart.yaml still reads "0.1.0"
despite breaking template changes to CLI flags (e.g., renames like
--eviction-interval-secs → --eviction-interval and --postgres-pool-max →
--postgres-pool-max-size); update the Chart.yaml version to a new semver (e.g.,
bump to 0.2.0 or 1.0.0 depending on your versioning policy) so consumers know
this release contains breaking changes and can pin/upgrade appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d27b1a5-5384-480b-b4e8-22061419596b
📒 Files selected for processing (6)
deploy/helm/smg/Chart.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/deployment-router.yamldeploy/helm/smg/templates/tests/test-connection.yamldeploy/helm/smg/values.schema.jsondeploy/helm/smg/values.yaml
| version: 0.1.0 | ||
| appVersion: "0.7.0" | ||
| appVersion: "1.3.3" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider bumping chart version for breaking CLI flag changes.
The chart templates now emit different CLI flag names (e.g., --eviction-interval-secs → --eviction-interval, --postgres-pool-max → --postgres-pool-max-size). Since the chart version remains 0.1.0, users pinning to this version may encounter unexpected breakage if they upgrade the chart but not the application.
Helm best practice is to increment the chart version when templates change in ways that affect deployment behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/smg/Chart.yaml` around lines 5 - 6, The chart's version field in
Chart.yaml still reads "0.1.0" despite breaking template changes to CLI flags
(e.g., renames like --eviction-interval-secs → --eviction-interval and
--postgres-pool-max → --postgres-pool-max-size); update the Chart.yaml version
to a new semver (e.g., bump to 0.2.0 or 1.0.0 depending on your versioning
policy) so consumers know this release contains breaking changes and can
pin/upgrade appropriately.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily fixes deployment issues in the SMG Helm chart. It also includes updates to align with application changes, such as renaming several CLI arguments and introducing a new prefix_hash routing policy. The review focuses on the completeness of these updates. The prefix_hash policy has been added without exposing its configuration parameters in the Helm chart, limiting its usefulness.
| replicas: 1 | ||
|
|
||
| # -- Routing policy: cache_aware, round_robin, power_of_two, manual, random | ||
| # -- Routing policy: cache_aware, round_robin, power_of_two, manual, random, prefix_hash |
There was a problem hiding this comment.
The prefix_hash policy was added to the schema, but its configurable parameters (prefix_token_count and load_factor) are not exposed in the Helm chart. This prevents users from tuning the new policy.
To provide full functionality, consider:
- Adding
prefixTokenCountandloadFactortovalues.yamlwith appropriate defaults (e.g., 256 and 1.25). - Updating
templates/_helpers.tplto pass these values as CLI arguments (e.g.,--prefix-token-countand--load-factor) whenpolicyisprefix_hash.
The chart's default securityContext sets readOnlyRootFilesystem: true, which prevents the container from writing to any path in the image filesystem. Python and other runtime components may need to write temporary files to /tmp. This was discovered during Helm chart testing against a live cluster where the pod would fail to start or crash when the process attempted to write temp files. The workaround branches (fix/helm-edge-cases, fix/helm-minimal-deploy, fix/helm-observability) included this fix alongside a runAsNonRoot: false change. Now that the Docker image runs as non-root (previous commit), runAsNonRoot: true is correct and this /tmp volume is the only remaining piece needed. What changed: - deploy/helm/smg/templates/deployment-router.yaml: added emptyDir volume "tmp" mounted at /tmp in the router container Why: - readOnlyRootFilesystem: true is a security best practice but requires writable mounts for directories that need runtime writes - /tmp is the standard location for temporary files in Python processes Refs: #860, #861, #862 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Description
Problem
The SMG Helm chart fails to deploy with the default
values.yamlconfiguration:runAsNonRoot: truebut thelightseekorg/smgimage runs as root (UID 0). This causes the container runtime to reject the pod immediately.test-connection.yaml) was placed intests/instead oftemplates/tests/, sohelm testreported "TEST SUITE: None".readOnlyRootFilesystem: trueand no/tmpvolume, any temp file writes would fail.Solution
runAsNonRootdefault fromtruetofalseto match the actual imageemptyDirvolume mounted at/tmpfor temporary file writestest-connection.yamlfromtests/totemplates/tests/for Helm discoveryChanges
deploy/helm/smg/values.yaml: SetrunAsNonRoot: false, added explanatory commentdeploy/helm/smg/templates/deployment-router.yaml: Added/tmpemptyDir volumeMount and volumedeploy/helm/smg/tests/test-connection.yaml->deploy/helm/smg/templates/tests/test-connection.yaml: Moved for Helm test discoveryTest Plan
Tested on OCI cluster (moirai-eu-frankfurt-1-dev) in
helm-test-minimalnamespace:helm installwith--set global.image.tag=1.3.3 --set router.workerUrls[0]=http://nonexistent:8000 --set router.healthCheck.enabled=false-- PASS (pod reaches Running/Ready)kubectl logs-- PASS (clean startup, no errors, all CLI flags accepted)kubectl get svc-- PASS (ClusterIP service on ports 80 and 29000)helm test-- PASS (test pod runs curl against /health, returns OK)/health-- PASS (returns "OK", HTTP 200)/v1/models-- PASS (returns "No models available", HTTP 200)Refs: #859
Checklist
Summary by CodeRabbit
New Features
prefix_hashrouting policy option for enhanced routing flexibilityImprovements
Chores