fix(icms-translate): run LLM sidecars as a shared UID so pylon can read the worker-token#409
Closed
Max-NV wants to merge 3 commits into
Closed
fix(icms-translate): run LLM sidecars as a shared UID so pylon can read the worker-token#409Max-NV wants to merge 3 commits into
Max-NV wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughLLM workload configuration now propagates a configurable sidecar UID. Both LLM sidecars receive matching user/group security contexts, and generated LLM pods set the corresponding filesystem group when applicable. Tests cover defaults, overrides, and negative values. ChangesLLM sidecar security
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorkloadConfig
participant TranslateConfig
participant LLMTranslator
participant LLMRouterClient
participant CredentialManager
WorkloadConfig->>TranslateConfig: LLMSidecarRunAsUser
TranslateConfig->>LLMTranslator: configured UID
LLMTranslator->>LLMRouterClient: RunAsUser/RunAsGroup
LLMTranslator->>CredentialManager: RunAsUser/RunAsGroup
LLMTranslator->>LLMTranslator: apply pod FSGroup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Max-NV
requested review from
estroz and
kristinapathak
and removed request for
balajinvda
July 24, 2026 03:06
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libraries/go/lib/pkg/icms-translate/translate/function/llm.go`:
- Around line 50-57: Add a permission mechanism for the shared LLM emptyDir
mounted at /var/run/llm, such as configuring the pod security context with the
sidecars’ shared group via fsGroup or adding an initContainer to chown the
directory before startup. Update the relevant pod construction alongside
newLLMSidecarSecurityContext and add a test verifying worker-token is writable
by the non-root sidecars.
🪄 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: Enterprise
Run ID: 4718fbcb-08cb-4c72-a788-b52a8c568a86
⛔ Files ignored due to path filters (3)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/function/llm.gois excluded by!**/vendor/**src/libraries/go/lib/testdata/icms-translate/function/container/llm/exp.yamlis excluded by!**/testdata/**src/libraries/go/lib/testdata/icms-translate/function/helmchart/llm/exp.yamlis excluded by!**/testdata/**
📒 Files selected for processing (2)
src/libraries/go/lib/pkg/icms-translate/translate/function/llm.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/llm_test.go
apartha-nv
requested changes
Jul 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/libraries/go/lib/pkg/icms-translate/translate/function/llm_test.go`:
- Around line 466-472: Update the “configured UID overrides the default on both
sidecars” test to first assert both pylon.SecurityContext and
cred.SecurityContext are non-nil, then verify both sidecars’ RunAsUser and
RunAsGroup values match the configured shared UID/GID. Avoid dereferencing
either security context before the nil checks.
🪄 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: Enterprise
Run ID: 008c7220-8296-4790-94fd-6b8bf66d22d5
⛔ Files ignored due to path filters (9)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/function/llm.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/function/translate.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/function/translate_container.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/function/translate_helm.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/types.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/vendor/**,!**/zz_generated.*src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/zz_generated.*src/libraries/go/lib/testdata/icms-translate/function/container/llm/exp.yamlis excluded by!**/testdata/**src/libraries/go/lib/testdata/icms-translate/function/helmchart/llm/exp.yamlis excluded by!**/testdata/**
📒 Files selected for processing (9)
src/compute-plane-services/nvca/internal/miniservice/translate_workload.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/storage/translate_workload.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/llm.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/llm_test.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/translate.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/translate_container.gosrc/libraries/go/lib/pkg/icms-translate/translate/function/translate_helm.gosrc/libraries/go/lib/pkg/types/nvca/config/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libraries/go/lib/pkg/icms-translate/translate/function/llm.go
…ad the worker-token The llm-credential-manager writes the worker auth token to the shared "llm" emptyDir with mode 0600, but pylon (llm-router-client) runs as the distroless non-root user nvs (1000) while the credential-manager runs as root, so pylon cannot read it. Registration fails at token read before pylon dials the router, leaving the model unregistered and gateway invokes returning no_eligible_candidates. Render both sidecars with runAsUser/runAsGroup 1000 so they share the token owner while keeping 0600; the customer workload container runs as its own user and still cannot read it. Regenerate the container/llm and helmchart/llm goldens and sync the NVCA vendored copy. NO-REF
…Group Addressing review. Hardcoding runAsUser 1000 conflicts with OpenShift SCC UID-range admission, and the non-root sidecars need the shared emptyDir writable. - Add TranslateConfig.LLMSidecarRunAsUser and the NVCA WorkloadConfig field. Defaults to 1000; a negative value leaves runAsUser unset so the platform (e.g. OpenShift) can assign a UID from its allowed range. - Set the pod fsGroup to the same UID so the credential-manager can write the worker-token to the shared emptyDir. NO-REF
Max-NV
force-pushed
the
fix/llm-worker-token-runasuser
branch
from
July 24, 2026 17:23
4f410ca to
67f2cf2
Compare
apartha-nv
approved these changes
Jul 24, 2026
Contributor
|
I'm hoping we can just use 0644 perms on the token mount, although i'm not completely against the changes as-is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Managed LLM workers reach the request router but never register, so gateway invokes return
no_eligible_candidates. The llm-credential-manager writes the worker auth token to the sharedllmemptyDir as mode 0600, but pylon (llm-router-client) runs as the distroless non-root usernvs(1000) while the credential-manager runs as root, so pylon cannot read it and fails before dialing the router.What changed
Run both LLM sidecars as one UID so they share the token owner; the token stays 0600 and the customer workload container (its own user) still cannot read it. Also set the pod fsGroup to that UID so the non-root credential-manager can write the shared emptyDir.
The UID is configurable via
TranslateConfig.LLMSidecarRunAsUserand the NVCAWorkloadConfigfield: default 1000, and a negative value leaves runAsUser unset so a platform such as OpenShift can assign a UID from its SCC range. Regenerated thecontainer/llmandhelmchart/llmgoldens and synced the NVCA vendored copies.Customer Release Notes
LLM function workers now register with the request router in managed deployments; OpenAI-compatible invocations no longer fail with no_eligible_candidates due to the worker-token permission mismatch.
Testing
go test ./pkg/icms-translate/translate/function/...(default, configured-override, and platform-managed UID cases, plus the pod fsGroup helper) and the NVCApkg/storageworkload translation test. Golden diff is the sidecar securityContexts plus the pod fsGroup.References
Closes #410
Summary by CodeRabbit