Skip to content

Implement CacheBackend API#6

Merged
heymrbox merged 1 commit into
mainfrom
weiwzhen-dev
May 27, 2026
Merged

Implement CacheBackend API#6
heymrbox merged 1 commit into
mainfrom
weiwzhen-dev

Conversation

@heymrbox
Copy link
Copy Markdown
Contributor

Summary

Expand the CacheBackend v1alpha1 API to match CAC-45 / tech spec §4.1: add deployment, storage, integration, engine selector, backend config, template override, and status health/index fields. Regenerated deepcopy and CRD manifests, updated the sample manifest, and added schema/deepcopy tests for the new validation surface.

Linear

Closes CAC-45

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 (incl. OCI) 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 (make install-hooks once, then it runs on every commit).

Quality

  • make build and make test pass locally.
  • make lint clean (gofmt + go vet).
  • make manifests generate produces no drift (generated code committed).
  • New/changed behavior has unit tests.
  • CI is green.

Contracts (only if touching CRDs or proto)

  • Change matches the tech spec (or the spec is updated in the same PR).
  • Backward compatibility considered for v1alpha1 consumers (engines, gateway clients).
  • If proto/ changed, docs/design/grpc-contract.md is updated to match (the pre-commit hook enforces this).

@github-actions
Copy link
Copy Markdown

Codex review

Blocking

  • api/v1alpha1/cachebackend_types.go, config/crd/bases/inferencecache.io_cachebackends.yaml: this breaks the existing v1alpha1 CRD contract. Previously spec was optional, spec.type was optional, and spec.type accepted any string; this PR makes top-level spec required, makes spec.type required, and restricts it to a new enum. Existing objects such as the prior sample’s type: memory, or objects relying only on spec.endpoint/empty spec, will be rejected after the CRD update. The project standard says v1alpha1 must remain backward-compatible with no breaking CRD field changes, so these validations need to be relaxed or handled through a compatible migration/versioning path.

  • api/v1alpha1/cachebackend_types_test.go: the new schema test explicitly asserts the breaking spec/type requirements instead of guarding backward compatibility. This makes the regression sticky in CI rather than catching it.

Should-Fix

  • None found.

Nit

  • None.

Verdict: changes-requested.

@github-actions
Copy link
Copy Markdown

Codex review

Blocking
None.

Should-fix
None.

Nit
None.

I did not find violations in the PR diff. The API changes are additive for v1alpha1, spec.endpoint remains present, proto/gRPC contract files are untouched, and the vendor-neutral naming scan only found the banned terms in CONTRIBUTING.md policy text. Generated CRD/deepcopy changes look consistent with the new Go types.

Verification note: git diff --check passed. I could not run go test because the sandbox filesystem is read-only, including /tmp, so Go could not create a build cache.

Verdict: approve.

Copy link
Copy Markdown
Collaborator

@EdHasNoLife EdHasNoLife left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — CacheBackend API

Reviewed at extra-high effort (5 finder angles + verifier + sweep). go build ./..., go vet ./..., and go test ./... all pass. The deepcopy functions and CRD schema were checked by inspection against the Go types/markers and look consistent.

No high-severity correctness bugs. The remaining items are medium/low design and test-quality observations, posted inline. The two I'd actually act on:

  • omitempty on the non-pointer struct fields (Storage, Integration, EngineSelector) — a no-op in Go, so they always serialize as {}. Switch to pointers if absence must be distinguishable (matters for server-side-apply ownership).
  • Endpoint/Health print columns now read .status.*, which the no-op controller never populates — the previously-useful .spec.endpoint display goes blank until status reconciliation lands.

Note (not a finding): CacheBackendType defines six constants but has no +kubebuilder:validation:Enum marker, unlike its sibling enum types — so spec.type accepts any string. The test requireNoEnum asserts this absence is intentional; flagging only to confirm that was deliberate.

Comment thread api/v1alpha1/cachebackend_types.go Outdated
Comment thread api/v1alpha1/cachebackend_types.go Outdated
Comment thread config/crd/bases/inferencecache.io_cachebackends.yaml
Comment thread api/v1alpha1/cachebackend_types_test.go
Comment thread api/v1alpha1/cachebackend_types_test.go Outdated
Comment thread api/v1alpha1/cachebackend_types_test.go Outdated
Comment thread api/v1alpha1/cachebackend_types_test.go Outdated
@github-actions
Copy link
Copy Markdown

Codex review

Blocking

  • api/v1alpha1/cachebackend_types.go: integration.failOpen makes fail-open behavior user-configurable. The project standard requires lookup paths to fail open unconditionally; this API permits failOpen: false, which would bless fail-closed behavior in the core CRD contract. Remove it, make it write-path-specific, or otherwise constrain the API so lookup fail-open cannot be disabled.

Should-fix

  • api/v1alpha1/cachebackend_types.go: spec.template is a runtime.RawExtension with preserved unknown fields while the comment says it is a Kubernetes PodSpec override. This stores arbitrary unvalidated objects in the public API, and tightening it later to a real corev1.PodSpec/PodTemplateSpec schema would be a breaking CRD change. Since this PR is defining the API surface, use the typed Kubernetes struct now unless the design explicitly requires opaque extension data.

Nit

  • None.

Vendor-neutral naming looks clean in the changed core surfaces, and no proto contract files were touched. I could not run tests because this environment is read-only and Go could not create a build cache.

Verdict: changes-requested.

@heymrbox heymrbox force-pushed the weiwzhen-dev branch 2 times, most recently from f49f46e to 534c02f Compare May 27, 2026 17:51
@github-actions
Copy link
Copy Markdown

Codex review

Blocking
None.

Should-Fix

  • api/v1alpha1/cachebackend_types.go: spec.template is documented as an override, but using *corev1.PodSpec makes the generated CRD require spec.template.containers (config/crd/bases/inferencecache.io_cachebackends.yaml). That prevents partial overrides like nodeSelector/tolerations unless users also supply a full container list, which conflicts with managed backend defaults. Use a narrower override type, or make this explicitly a full pod spec.
  • api/v1alpha1/cachebackend_types.go: the Endpoint print column moved from .spec.endpoint to .status.endpoint, but the reconciler still performs no status writes (internal/controller/cachebackend_controller.go). Existing external backends using spec.endpoint will show a blank endpoint in kubectl get cb indefinitely. Either keep the print column on spec.endpoint for now or populate status.endpoint with tests.

Nit
None.

Vendor-neutral naming check: no banned oci / oracle tokens found in the PR diff.

Verdict: changes-requested.

@github-actions
Copy link
Copy Markdown

Codex review

Blocking
None.

Should-Fix

  • api/v1alpha1/cachebackend_types.go:54: this PR adds a substantial public v1alpha1 CRD contract, but there is no matching docs/design/ update for the CacheBackend API shape. The repo standard asks API/contract changes to match the design docs; today docs/design/ only documents the gRPC contract, so reviewers and future implementers have no in-repo spec for these fields.
  • api/v1alpha1/cachebackend_types.go:190 / config/crd/bases/inferencecache.io_cachebackends.yaml:26: status.endpoint is documented as the observed endpoint clients should use, but the kubectl printer column still points at .spec.endpoint. For managed backends like the updated sample, .spec.endpoint is empty, so kubectl get cachebackend will hide the actual observed endpoint once populated. Prefer .status.endpoint, or add a distinct observed endpoint column.

Nit

  • Makefile:115: crd:maxDescLen=0 strips all CRD schema descriptions. That makes the generated CRD much less useful as API documentation; I’d avoid changing this unless the project explicitly wants description-free CRDs.

Verdict: changes-requested.

I could not run tests in this sandbox because the filesystem is read-only, including /home/runner/.cache and /tmp, so Go could not initialize its build cache.

@github-actions
Copy link
Copy Markdown

Codex review

Blocking
None.

Should-Fix

  • internal/controller/cachebackend_controller.go:38: status.endpoint is only updated when spec.endpoint != "", so removing spec.endpoint leaves the old status endpoint forever. Since the CRD exposes status.endpoint as the observed client endpoint and print column, this can keep advertising a stale external backend after the user clears it. Add a clear path and a regression test for endpoint removal.

Nit

  • internal/controller/cachebackend_controller.go:41: the status patch error is returned raw. The project standard asks for wrapped errors; include namespace/name and the operation, e.g. patch CacheBackend status.

Vendor-neutral check: no new core identity/default violations found. Proto was untouched. I could not run tests in this read-only environment because Go could not create a build cache.

Verdict: changes-requested.

@github-actions
Copy link
Copy Markdown

Codex review

Blocking

  • internal/controller/cachebackend_controller.go:39: status.endpoint is mirrored from spec.endpoint for every CacheBackend, not only type: External. This contradicts docs/design/cachebackend-api.md:59, which says managed backends get status.endpoint from the controller-created serving resource. As written, any managed backend with an observed endpoint and empty spec.endpoint will have its status cleared on the next reconcile. Gate the mirror on Spec.Type == External and add a non-external test proving existing status.endpoint is preserved.

Should-fix

  • None.

Nit

  • None.

I did not find vendor-neutral naming violations or proto/doc drift in this PR. I could not run tests because the environment is read-only and Go could not create a temporary build cache.

Verdict: changes-requested.

@github-actions
Copy link
Copy Markdown

Codex review

Blocking: None.

Should-fix: None.

Nit: None.

Verdict: approve.

I verified the touched core-identity surfaces are vendor-neutral; make verify-naming passed, and no proto/ or pkg/server/proto files changed, so the gRPC contract mirror rule is not triggered. git diff --check also passed.

I could not run the package tests because the sandbox is read-only and Go failed to initialize its build cache at /home/runner/.cache/go-build.

@heymrbox heymrbox merged commit 46e3ea1 into main May 27, 2026
4 checks passed
EdHasNoLife added a commit that referenced this pull request May 27, 2026
Resolve the regenerated zz_generated.deepcopy.go conflict (CacheBackend
API from #6 landed on main) by regenerating deepcopy + manifests against
the merged api types, so both CacheBackend and CacheIndex are present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants