Skip to content

CAC-262: Add NFS remote storage for SGLang HiCache - #172

Open
fredericsun wants to merge 9 commits into
mainfrom
cac-262-sglang-hicache-nfs
Open

CAC-262: Add NFS remote storage for SGLang HiCache#172
fredericsun wants to merge 9 commits into
mainfrom
cac-262-sglang-hicache-nfs

Conversation

@fredericsun

@fredericsun fredericsun commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add NFS as an externally owned remote-storage provider, with initial runtime support for SGLang HiCache.

This PR introduces the following canonical configuration:

spec:
  runtime: SGLang
  type: SGLangHiCache
  hiCache:
    ratio: "2.0"
    storagePrefetchPolicy: wait_complete
  remoteStorage:
    provider: NFS
    ownership: External
    nfs:
      server: 192.0.2.10
      path: /hicache
      mountPath: /mnt/hicache

NFS is modeled as a generic remoteStorage.provider, rather than as an SGLang-specific API. The currently supported runtime/cache pair is (SGLang, SGLangHiCache).

For matching SGLang engine Pods, the webhook injects:

  • --hicache-storage-backend file
  • --hicache-storage-prefetch-policy <value>
  • SGLANG_HICACHE_FILE_BACKEND_STORAGE_DIR=<mountPath>
  • An inline NFS volume using the configured server and export path
  • A volume mount on the SGLang engine container at mountPath

The NFS mount target is externally owned. inference-cache does not create a PV, PVC, StorageClass, NFS export, mount target, Deployment, Service, or HPA.

Because NFS is mounted directly into the engine Pod, it has no CacheBackend network endpoint. The controller acknowledges the current generation but does not publish an endpoint or synthetic Ready condition. HiCache readiness remains a separate contract tracked by CAC-260.

This PR does not claim an NFS or SGLang data-plane test. The configured NFS server must be reachable from the engine nodes, and runtime store/read behavior should be verified in the target cluster.

Change breakdown

  1. feat: add NFS remote storage for SGLang HiCache (891d949)

    • Adds NFS to the generic remote-storage provider API.
    • Adds typed remoteStorage.nfs fields:
      • server
      • path
      • mountPath
    • Adds the typed HiCache storagePrefetchPolicy field.
    • Introduces a generic file/NFS backend binding that does not require a network endpoint.
    • Registers External NFS as a provider while limiting runtime consumption to the adapters that explicitly support it.
    • Extends SGLang HiCache validation and injection for file-backed L3 storage.
    • Preserves all-or-nothing and idempotent Pod mutation:
      • Matching existing arguments, environment variables, volumes, and mounts are preserved.
      • Conflicting, malformed, or duplicate reserved configuration causes fail-open admission without partially mutating the Pod.
    • Adds admission validation for:
      • NFS requiring ownership: External
      • NFS rejecting remoteStorage.endpoint
      • Required and valid server, export path, and mount path
      • storagePrefetchPolicy being required with NFS and rejected without NFS
      • Runtime/cache/provider compatibility
    • Handles endpoint-free External bindings without provisioning a managed backend workload or publishing endpoint readiness.
    • Updates doctor classification so NFS is treated as mounted storage rather than a dialable endpoint.
    • Adds a committed NFS sample and updates the CacheBackend design documentation.
    • Adds API, provider, runtime-adapter, admission, Pod-webhook, controller, doctor, sample-admission, and installed-cluster smoke assertions.

Linked issues

CAC-262

Validation

Passed locally:

  • make build
  • make test
  • make lint
  • make verify-naming
  • make manifests generate — generated CRD and deepcopy code are committed
  • make verify-samples — 22 passed, 2 intentional skips
  • make ci
    • Vendor-neutral naming guard
    • Internal issue-reference guard
    • gofmt
    • go vet
    • golangci-lint
    • Prometheus rule validation
    • Documentation synchronization checks
    • Full race-test suite
    • Build
  • bash -n docs/reference-stack/scripts/default_install_smoke.sh
  • git diff --check

The optional golden-vector verification was skipped by the local CI script because the local Python environment does not have xxhash installed.

The installed-cluster smoke script now asserts the NFS lifecycle and server-side dry-run Pod mutation, but the full kind install-smoke was not executed locally for this commit.

Checklist

Vendor-neutral naming (required — see CONTRIBUTING.md)

  • No oci / oracle / *.oci.com / oraclecloud.com in any API group, CRD group, proto package, gRPC service/package, Kubernetes namespace, image registry, Helm chart, or Go module path.
  • Any cloud-specific integration lives in an isolated, optional adapter (pkg/adapters/.../) — never in core controllers, CRD types, the proto contract, or default config.
  • No Oracle/OCI domain or namespace in sample manifests, README, or default values.
  • Pre-commit naming guard passed.

Quality

  • make build and make test pass locally.
  • make lint is clean.
  • make manifests generate produces no uncommitted generated drift.
  • New and changed behavior has unit tests.
  • The install-smoke gate contains assertions for the operator-facing CRD, controller, and Pod-webhook behavior.
  • CI is green.

Contracts (only if touching CRDs or proto)

  • The CacheBackend design documentation is updated with the NFS contract.
  • Backward compatibility was considered for v1alpha1 consumers; the API changes are additive.
  • N/A — proto/ is unchanged.

@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown

CAC-262

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

None.

Should-fix

  • pkg/cli/doctor/checks/cachebackend.go:60 — Doctor requires Ready=True for NFS-backed HiCache even though this PR’s controller deliberately removes that condition. Consequently every real NFS backend reports CB001, while the new test passes only because healthyBackend supplies a synthetic state the controller never publishes.
  • config/samples/cachebackend-sglang-hicache-l3-nfs.yaml:2 — The generic NFS sample instructs users to replace the address with a “VCN mount target,” introducing provider-specific terminology into a vendor-neutral default sample. Use generic language such as “NFS server or mount target.”

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: findings above at: config/samples/cachebackend-sglang-hicache-l3-nfs.yaml:2
  2. Contract & spec / docs sync: findings above at: pkg/cli/doctor/checks/cachebackend.go:60
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/cli/doctor/checks/cachebackend.go:60

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

  • config/samples/cachebackend-sglang-hicache-l3-nfs.yaml:2 — The generic NFS sample instructs users to replace the address with a “VCN mount target,” an Oracle-specific concept. User-facing samples must remain vendor-neutral; say “NFS server” or “mount target” instead.

Should-fix

  • internal/webhook/v1alpha1/cachebackend_webhook.go:411 — The validator claims to require a hostname or IP address but accepts arbitrary strings such as @, host?query, or option-like values beginning with -. Validate with Kubernetes DNS-name validation or net.ParseIP, accounting explicitly for IPv6, and add rejection tests so invalid objects do not reach kubelet mount failures.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: findings above at: config/samples/cachebackend-sglang-hicache-l3-nfs.yaml:2
  2. Contract & spec / docs sync: findings above at: internal/webhook/v1alpha1/cachebackend_webhook.go:411
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/webhook/v1alpha1/cachebackend_webhook.go:411

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

None.

Should-fix

  • pkg/cli/doctor/checks/cachebackend.go:115 — Doctor emits CodeBackendHealthy for NFS even though this PR explicitly says NFS/HiCache readiness cannot be established. Successful engine matching and event participation do not prove that the NFS mount or L3 store/read path works, so this produces the synthetic health claim the controller deliberately avoids.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/cli/doctor/checks/cachebackend.go:115
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go

Findings

Blocking

None.

Should-fix

  • pkg/adapters/runtime/sglang/hicache.go:437 — The adapter’s admission-bypass validation accepts invalid NFS servers such as @, host?query, and -option, although the validating webhook rejects them. This contradicts the adapter-boundary defense-in-depth contract and can inject an unusable inline NFS volume instead of failing open; reuse the DNS/IP validation used by validateNFSRemoteStorage and add direct adapter tests.

  • pkg/cli/doctor/checks/cachebackend.go:65 — Doctor classifies any declaration with provider=NFS as supported endpoint-free HiCache, without checking ownership=External or the (SGLang, SGLangHiCache) pair. Admission-bypassed or grandfathered malformed resources consequently skip Ready and endpoint diagnostics and may receive the misleading CB008 result; scope this special case to the complete supported contract.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/runtime/sglang/hicache.go:437, pkg/cli/doctor/checks/cachebackend.go:65
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/adapters/runtime/sglang/hicache.go:437, pkg/cli/doctor/checks/cachebackend.go:65

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

  • pkg/adapters/runtime/sglang/hicache.go:452 — The injected inline NFS volume makes NFS availability a hard Pod-start prerequisite even though this pair requires integration.failOpen: true. If the export is unavailable, kubelet cannot mount the volume and the inference engine never starts, violating the contract that an unavailable remote/shared cache tier must fall back to local prefill rather than become a serving dependency.

Should-fix

  • docs/design/cachebackend-api.md:812 — The contract notes still claim exactly one fail-open exception, (SGLang, LMCache), while the new NFS section at line 468 acknowledges another serving dependency. Either redesign the NFS wiring to preserve fail-open behavior or explicitly revise the top-level contract and API field documentation to describe and justify this second exception.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/runtime/sglang/hicache.go:452, docs/design/cachebackend-api.md:812
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

  • internal/webhook/pod/podinjector.go:183 — The pod webhook constructs an NFS binding without validating remoteStorage.ownership or the endpoint prohibition. An admission-bypassed or grandfathered provider: NFS, ownership: Managed object can therefore be marked unsupported by the controller while still injecting an inline NFS mount into new engine pods.

Should-fix

  • internal/controller/cachebackend_controller.go:490 — The endpoint-free external branch validates only protocol compatibility, not the typed NFS server/path/mount contract. Invalid stored NFS configuration is consequently acknowledged at the current generation with no degraded condition, even though the pod webhook later rejects injection and fails open.
  • internal/webhook/pod/podinjector_test.go:1 — Add a bypass-defense test covering NFS with unsupported ownership and forbidden endpoint data. Existing tests do not exercise the disagreement where the controller rejects the resource but pod admission still mutates matching pods.
  • internal/controller/cachebackend_controller_test.go:1 — Add controller coverage for malformed admission-bypassed NFS fields. The defensive reconciliation path should demonstrate that such resources are surfaced as unsupported/degraded rather than silently acknowledged as valid endpoint-free storage.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: internal/webhook/pod/podinjector.go:183, internal/controller/cachebackend_controller.go:490
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: internal/webhook/pod/podinjector_test.go:1, internal/controller/cachebackend_controller_test.go:1

Verdict

changes-requested

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • cmd/controller/
    • [reviewed] main.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go

Findings

Blocking

None.

Should-fix

  • pkg/cli/doctor/checks/cachebackend.go:64 — Any declaration with provider=NFS is treated as successfully reconciled endpoint-free storage when Ready is absent. A fresh, admission-bypassed unsupported pair or invalid NFS declaration can consequently receive CB008 instead of CB001; require a current acknowledged generation or independently verify the supported NFS shape before exempting missing readiness.
  • pkg/adapters/backend/backend.go:48 — Verify: raw IPv6 literals are admitted as NFS servers, while kubelet/mount implementations commonly require bracketed IPv6 in the resulting server:path source. Confirm the target Kubernetes versions format this correctly; otherwise reject unsupported literals or accept/document the required bracketed representation.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/backend/backend.go:48
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/cli/doctor/checks/cachebackend.go:64

Verdict

changes-requested

@EdHasNoLife

EdHasNoLife commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Review — high quality; no blocking defects, main item is merge coordination with #171

Read the full diff and ran the suite on the PR head (079ba90): builds clean, all webhook/adapter/controller/doctor/api packages green. This is a well-structured, thoroughly-tested change. Findings are coordination/awareness-level, not blocking.

Correctness — strong

  • Layered validation. validateCleanAbsolutePath rejects non-absolute, non-clean (/hicache/.. → Clean != value, good traversal defense), and whitespace; mountPath can't be /; ValidateNFSServer accepts only IPv4/IPv6/DNS-1123; NFS rejects endpoint, forces ownership: External, and gates storagePrefetchPolicy required-with / forbidden-without NFS; the new args + storage-dir env are added to the reserved set. The webhook checks are re-run at the adapter boundary as defense-in-depth against admission bypass — deliberate and correct.
  • Idempotent injection with real conflict detection. planHiCacheNFSWiring matches env by name, volume by reserved name, mount by name-or-path, DeepEqual-compares before add-vs-skip, and rejects conflicts/dups fail-open with no partial mutation. The mount guard correctly catches a foreign mount squatting the target path. Re-injection is a no-op.
  • Endpoint-free binding abstraction is clean: ProtocolFile + BindingRequiresEndpoint + AdapterRequiresEndpointFor thread consistently through the webhook (effectiveEndpoint → ""), dispatch, and doctor.

Findings (all non-blocking)

🟡 Coordinate the merge with #171. This branch predates #171; both edit the same HiCache dispatch branch, hicache.go, podinjector.go, and cachebackend-api.md, so expect textual conflicts. Substantively: in the merged code NFS-HiCache stays on the endpoint-free-External path → reconcileUnmanaged, so it never reaches #171's reconcileEngineLocal and gets no readiness reporting (consistent with "NFS readiness = CAC-260"). Fine today — but #171's engineConfigConverged hardcodes a nil binding, so whenever NFS-HiCache is later routed through engine-local readiness, that convergence check will inject without the NFS wiring and mis-Degrade every NFS pod unless it's made binding-aware. Worth a note on CAC-260 so it isn't a surprise.

🟡 reconcileUnsupported broadens the status surface beyond NFS. Several previously-silent paths (events-only no-adapter, no-runtime-adapter, unsupported external binding, unsupported host-only, no-provider, unsupported managed binding) now publish Ready=False/Degraded=True instead of no conditions. Genuine UX improvement and it's tested — but it's a status-behavior change wider than "add NFS," so anything keying on condition presence for those cases should be aware. Additive + v1alpha1 → low risk.

🟢 Minor — failOpen defaults true but NFS requires false. A minimal NFS manifest that omits failOpen is defaulted to true, then rejected; the operator must set failOpen: false explicitly. Deliberate (inline NFS mount is a startup dependency), documented, and tested — just an ergonomic wrinkle when copying a host-only HiCache sample. Also note every NFS backend emits the FailClosedEnabled Warning event by design.

🟢 Minor — inline NFSVolumeSource gives no mount-option control. It exposes only server/path/readOnly — no hard/soft/timeo. Under the fail-closed model an NFS outage mid-serving can hang engine I/O (default hard mount) rather than fail-fast. Inherent to inline NFS, not a code issue — worth an operator-facing line in the design doc.

🟢 Nits. Two near-identical path validators (validateCleanAbsolutePath vs validateHiCachePath) could share a helper. ValidateNFSServer rejects uppercase hostnames (DNS-1123 strictness) — NFS.example.com would be refused; fine, but slightly surprising given DNS is case-insensitive.

Tests / docs

Excellent lockstep: deepcopy + CRD + schema assertions + validation table + full args/env/volume/mount injection test + binding + provider + doctor branches + controller + committed sample + design doc + install-smoke NFS lifecycle. The PR is upfront that no real NFS/SGLang data-plane test runs — appropriate given the mount is externally owned.

Verdict

Approve-worthy on its own — nothing blocking. Main actionable item is merge coordination with #171 (overlap + the nil-binding convergence assumption for any future NFS readiness); the reconcileUnsupported broadening and the two operational notes are worth a mention but not gating.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • cmd/controller/
    • [reviewed] main.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go

Findings

Blocking

None.

Should-fix

None.

Nit

  • internal/controller/cachebackend_controller.go:142 — The comment says caches are “never a serving dependency,” immediately before documenting fail-closed mode and despite NFS now requiring it. Qualify this as the default posture to keep code documentation consistent with the API contract.
  • internal/webhook/pod/podinjector.go:89 — “Always an optimization, never a serving dependency” is no longer universally true because NFS-backed HiCache requires failOpen: false. The comment should distinguish admission’s fail-open behavior from the configured data-plane dependency.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: internal/controller/cachebackend_controller.go:142, internal/webhook/pod/podinjector.go:89
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

approve-with-nits

@fredericsun

fredericsun commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@EdHasNoLife Thanks for the thorough review.

I addressed the two in-scope operational/documentation notes in 925ef56:

  • Qualified the controller and Pod-webhook comments: fail-open is the default/admission-failure posture, while a successfully injected External NFS backend is an explicitly acknowledged fail-closed serving dependency.
  • Documented that inline NFSVolumeSource exposes no mount-option control, Linux defaults to a hard mount, a mid-serving outage can block HiCache file I/O, and storagePrefetchPolicy cannot interrupt an NFS syscall already blocked in a storage thread.

For the #171 coordination point, I’m intentionally leaving #172’s dispatch unchanged: NFS-backed HiCache remains on the endpoint-free External/reconcileUnmanaged path. The nil-binding assumption in engineConfigConverged needs to become binding-aware when CAC-260 (or the #171 integration) routes NFS-backed HiCache through engine-local readiness; handling it there avoids changing today’s dispatch contract in this PR.

The broader reconcileUnsupported status behavior is intentional and covered by the tests already in this branch.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review

Files reviewed

  • api/v1alpha1/
    • [reviewed] cachebackend_types.go
    • [reviewed] cachebackend_types_test.go
    • [skipped — generated] zz_generated.deepcopy.go
  • cmd/controller/
    • [reviewed] main.go
  • config/crd/bases/
    • [skipped — generated] inferencecache.io_cachebackends.yaml
  • config/samples/
    • [reviewed] README.md
    • [reviewed] cachebackend-sglang-hicache-l3-nfs.yaml
  • docs/cli/
    • [reviewed] doctor.md
  • docs/design/
    • [reviewed] cachebackend-api.md
  • docs/reference-stack/scripts/
    • [reviewed] default_install_smoke.sh
  • internal/controller/
    • [reviewed] cachebackend_controller.go
    • [reviewed] cachebackend_controller_test.go
  • internal/webhook/pod/
    • [reviewed] podinjector.go
    • [reviewed] podinjector_test.go
  • internal/webhook/v1alpha1/
    • [reviewed] cachebackend_webhook.go
    • [reviewed] cachebackend_webhook_test.go
  • pkg/adapters/backend/
    • [reviewed] backend.go
    • [reviewed] backend_test.go
  • pkg/adapters/backend/provider/
    • [reviewed] provider.go
    • [reviewed] provider_test.go
  • pkg/adapters/runtime/
    • [reviewed] adapter.go
  • pkg/adapters/runtime/sglang/
    • [reviewed] hicache.go
    • [reviewed] hicache_test.go
  • pkg/cli/doctor/
    • [reviewed] finding.go
  • pkg/cli/doctor/checks/
    • [reviewed] cachebackend.go
    • [reviewed] checks_test.go

Findings

Blocking

None.

Should-fix

None.

Nit

None.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fredericsun @EdHasNoLife