Skip to content

Feat: support sandbox - #22

Merged
InftyAI-Agent merged 22 commits into
InftyAI:mainfrom
kerthcet:feat/remote-workloads-design
Aug 8, 2026
Merged

Feat: support sandbox#22
InftyAI-Agent merged 22 commits into
InftyAI:mainfrom
kerthcet:feat/remote-workloads-design

Conversation

@kerthcet

@kerthcet kerthcet commented Aug 8, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?


kerthcet and others added 21 commits August 7, 2026 22:25
Covers the four workload classes (sandbox/agent, notebook/shell, inference,
training) on one Kubernetes API surface:

- dial-out control plane (one WSS per instance + JWT), replacing the mesh
- kubelet API on the virtual node so kubectl logs/exec work natively
- agent as PID 1 owning the workload's pipes; registry-resolved entrypoint
- Sandbox + SandboxClass CRDs layered above the existing Pod path
- inbound reachability as a per-provider capability, not one mechanism

Reverses the earlier decision to expose logs/exec only from the SandD
controller: the stated requirement is kubectl-native access, so the ergonomics
are the product.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implementation starts now, so the branch-porting notes are no longer useful.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"agent" was overloaded: both a workload class (AI agents, agent-exec) and the
component running in the container.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Sandbox is a sandbox — no class object, no type discriminator. Every field is
meaningful on every sandbox, so there was nothing mutually exclusive to factor
out, and nodePoolRef is already the admin-owned policy object.

Notebook needs its own spec (served port, proxied URL, per-user auth, post-ready
start exec), so it becomes a separate CRD in phase 1.5 reusing the same Pod path.
Inference needs no CRD at all — Deployment + Service already fits.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
StatefulSet-shaped, not Deployment-shaped: replicas serve a warm pool (the answer
to agent-exec cold start) and fan-out, but each keeps its own claim, volume, and
sessions so kubectl exec sandbox-3 is repeatable.

Adds the /scale subresource so kubectl scale and HPA/KEDA work. Records two open
questions: scale-in must pick the least recently active replica rather than the
highest ordinal, and warm-replica checkout needs a claim mechanism once the pool
has more than one client.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mRef

Replaces flat image/accelerator fields with spec.template (a PodSpec), so
resources/env/volumeMounts/securityContext come from the API users already know
and the GPU count stays a standard nvidia.com/gpu resource rather than a parallel
source of truth.

Storage moves out of Sandbox — persistence is a Notebook concern (phase 1.5) and a
provider-seam gap, not part of the sandbox shape. Drops nodeClaimRef from status:
the claim is an internal ledger, not something a consumer addresses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nodeClaimRef

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sandbox is one interactive remote box: the object's name is the box's
stable identity, so per-box RBAC, image, and TTL all work, and a failure
stays visible instead of being replaced underneath its user.

SandboxPool keeps N Sandboxes warm and owns /scale, since provisioning
takes minutes while an exec call wants sub-second. It creates Sandbox
objects rather than replicas, which is what lets Sandbox stay singular.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SandboxSet, not SandboxPool: the controller maintains N boxes and has no
lease semantics (claim/hold/return), so "pool" overpromised. It also
would have been a third sense of "pool" in a group where NodePool
already means placement policy. ReplicaSet is the precedent that matches
- it maintains N and leaves existing members alone.

The Sandbox controller synthesizes the backing Pod and projects its
status back, so placement, the NodeClaim teardown ledger, and quota all
work unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The set prunes terminal boxes so it can replace them: the Sandbox
controller deliberately never resurrects a dead box (a fresh instance
would be a different box under the same name), so replacement has to be
the set's job.

Scale-in picks cheapest-to-lose first (terminal, then not-ready, then
youngest ready) rather than StatefulSet's highest-ordinal, which here
could kill a box in active use while a dead one sits beside it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Pod is the single source of truth for what runs on the instance (the
rule ProvisionRequest already documents for image/env/resources), so the
command belongs here, not duplicated in each provider bootstrap. Every
adapter already reads it off the Pod, so this needs no per-provider code
and cannot drift between providers.

SanddPath is shared because it is a two-ended contract: the controller
writes it as the command, and each bootstrap must make the binary appear
there, since the user's image does not contain it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unit tests for the Sandbox and SandboxSet controllers, using the fake-client
pattern the NodeClaim/NodePool tests already established.

The Sandbox tests pin the synthesized Pod's whole contract, since every field
there is what makes the EXISTING placement path pick the Pod up unchanged — the
opt-in label, the pool label, SandD as the command, and the accelerator count as
an nvidia.com/gpu limit. The rest cover the projection from Pod state to phase
(including gated-vs-provisioning, which is what stops a capacity problem from
looking like a slow boot), ReadyTime being written exactly once so a status blip
cannot restart the TTL clock, TTL deleting the Pod while the object survives as
the record, terminal boxes not being resurrected, and a foreign same-named Pod
being refused rather than adopted.

The SandboxSet tests cover scale-up from the template, the template being unable
to overwrite the ownership label and orphan a box, a labelled-but-foreign box
being neither counted nor deleted, terminal boxes being pruned AND replaced on
one pass, the removal ranking, and the status selector /scale needs.

Two fixes fell out:

- scaleDown returned from inside its loop on any error, and IgnoreNotFound maps
  an already-gone box to nil — so one missing victim abandoned every remaining
  one and reported the scale-in as done, leaving paid instances running.

- spec.image now defaults to ubuntu:24.04 and is no longer required. Unlike the
  accelerator, the image is not a decision a caller must make to get a useful
  box: exec'ing into a bare distro IS the remote-shell case. Defaulting a GPU
  shape would be guessing at spend; defaulting a shell is not. It deliberately
  does not become a CUDA image when an accelerator is requested — a conditional
  default cannot be expressed in a structural schema and would surprise anyone
  reading the object back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task was "add the Sandbox validating webhook", because the API and controller both
claimed a user-set command is rejected at admission with nothing enforcing it.
Checking the claim: SandboxSpec has no command field, and the CRD is a structural
schema, so `command:` is already rejected as an unknown field by the apiserver
itself. The docs were describing a guarantee we get for free; they now say where
it comes from, and an envtest spec pins it — if the schema ever stopped rejecting
the field (a stray x-kubernetes-preserve-unknown-fields would do it) the command
would be silently PRUNED instead, surfacing as "exec does not work" rather than as
a rejected object.

The real admission gap was elsewhere: util.AcceleratorRequest treats an
nvidia.com/gpu count with no acceleratorType as an error, so such a Sandbox was
admitted and then failed at PLACEMENT — minutes later, reported on the synthesized
Pod rather than on the object the user wrote. A CEL rule now rejects it up front,
covering requests as well as limits since gpuCount reads either. The inverse stays
legal: a type with no count means one accelerator. The rule propagates into the
SandboxSet template for free.

CEL is the right mechanism here rather than a webhook: no certs to provision, no
availability to depend on, and it cannot be bypassed by a webhook outage.

Envtest specs cover the CEL rule both ways, image defaulting, `image: ""` still
failing MinLength (via unstructured — the field is omitempty, so a Go zero value
never reaches the wire and would be defaulted instead of rejected), and the
required nodePoolRef. Verified by mutation: neutering the CEL rule fails exactly
the two specs that assert it and no others.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The webhook needs a TLS keypair the manager serves from and that cert's CA in
the MutatingWebhookConfiguration caBundle, and the two must agree exactly. Both
prior ways of getting them were worse:

  - cert-manager: rotates correctly, but is a whole second operator the user has
    to install before Nebula works at all. It also made the e2e suite install
    cert-manager in BeforeSuite, so that suite could not run without network
    access to fetch its manifests — the concrete failure that prompted this.
  - hack/gen-webhook-cert.sh: no dependency, but the cert is minted by a shell
    script at deploy time and NEVER rotates. Valid for CERT_DAYS (default 3650),
    so its expiry is a silent time bomb that fires years later, when nobody
    remembers the script exists.

pkg/cert does both jobs in-process via open-policy-agent/cert-controller: it
mints the keypair into a Secret, writes it to the dir the webhook server reads,
patches the caBundle, and keeps RENEWING it before expiry. Because the CA is
derived from the cert just written, the served cert and the trusted CA cannot
drift.

Rotation is deliberately NOT leader-elected: CertDir is each pod's local disk
and webhook serving is not leader-elected either, so a non-leader would serve
with no keypair. The Secret is the shared source of truth, so later replicas
find a valid cert there rather than minting a competing one.

Controller and webhook registration now waits on certsReady, in a goroutine
because the rotator cannot mint until the manager is STARTED (it is a Runnable
needing a synced cache), so blocking in main would deadlock. The controllers
wait too, not just the webhook: they create Pods, and a Pod admitted while the
webhook is untrusted would either be rejected (failurePolicy=Fail) or, worse,
admitted ungated and scheduled by vanilla Kubernetes — silently bypassing
placement.

The cert volume becomes an emptyDir rather than a Secret projection, since the
rotator writes to that path and a Secret volume is read-only. No
--webhook-cert-path either: it builds a certwatcher that fails at STARTUP when
the files are absent, which they always are on a first install.

cert-controller is pinned to v0.14.0 specifically: it requires
controller-runtime v0.21.0 exactly, so the pin holds (v0.16.0 would drag
controller-runtime to 0.23.3, which the virtual-kubelet v1.11 pin cannot take).
Only k8s.io deps move, 0.33.3 -> 0.33.4.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`go build ./cmd/...` writes manager/keybroker into the repo ROOT, which bin/*
does not cover. They are multi-MB, so an 8.4MB binary can ride along in a
`git add -A` unnoticed — which is exactly what nearly happened.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main lints clean; all three came from this branch.

goconst: the "true" opt-in value appeared in four places (placement controller
and helpers, the Pod webhook, the sandbox controller) because the sandbox work
added a fourth. It is now nebulav1alpha1.EnabledValue, next to EnabledLabel —
worth a named constant beyond silencing the linter, since the label doubles as
the webhook's objectSelector and the API server matches it LITERALLY. A Pod
labelled "True" is not opted in, and having the value written out per call site
is how the controllers and the selector could come to disagree about which Pods
are Nebula's.

unparam: sandboxStatusFromPod took a *Sandbox it never read. Dropped rather than
used — the projection must depend on nothing but observed Pod state, or a stale
value already on Sandbox.Status could feed back into the next projection and
latch. Noted in the doc comment so it does not get "fixed" by adding a use.

lll: a kubebuilder RBAC marker in pkg/cert exceeded 120 chars and cannot be
wrapped — controller-gen parses one marker per comment line and has no
continuation syntax. Excluded by SOURCE pattern rather than by path, so the
exemption covers exactly the lines that are physically unwrappable instead of
turning lll off for a whole directory.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The self-signed cert work landed with three wiring bugs that together meant a
fresh install never came up. Each one hid the next, so they are fixed together.

1. The Secret was never created. cert-controller's rotator does a Get followed
   by an Update on its Secret and never a Create, so an absent Secret is a fatal
   startup error ("acquiring secret to update certificates: not found") that
   crash-loops the manager. Ship it empty from config/webhook, the way gatekeeper
   does — nil Data is exactly what triggers minting.

2. The name did not match what kustomize renders. pkg/cert looked for
   "webhook-server-cert" while config/default's namePrefix produces
   "nebula-webhook-server-cert", so even with the Secret in place the rotator
   looked for one no overlay creates. secretName was the odd one out here;
   serviceName and mutatingWebhookConfName already carried the prefix.

3. certDir was an emptyDir, so the keypair never reached disk. This was the
   subtle one: the rotator does NOT write files — cert-controller has no disk
   writes at all, and CertDir is a path it only os.Stat()s to decide readiness
   (ensureCertsMounted). The kubelet is what puts the keypair there, via a Secret
   projection. With an emptyDir, tls.crt never appeared, IsReady never closed,
   and since controller/webhook registration waits on that channel, NOTHING ever
   started: no reconcilers, no virtual nodes, no webhook. The manager stayed
   1/1 Running and looked healthy the whole time, which is what made this hard
   to see — it surfaced only as the e2e placement spec timing out because no
   virtual node ever registered.

Also drop the stale hack/gen-webhook-cert.sh cabundle step from the e2e suite
(the manager patches its own caBundle now) and fix the Secret name in docs.

e2e: 5 Passed | 0 Failed, down from 160s to 47s — the suite was previously
spending its time waiting on timeouts that can no longer happen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/proposal/architecture.md is the original 114-line design sketch, long
superseded by the 616-line docs/architecture.md that describes what was actually
built. tailnet.png is the headscale mesh diagram — the mesh does not reach the
node counts Nebula targets and is no longer part of the design. Neither file is
referenced from anywhere.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table claimed the optimizer was the next milestone and listed components as
DONE/PLANNED, which has to be hand-updated on every change and was already wrong
(Sandbox/SandboxSet are not in it at all). Git history and the doc body already
say what exists; a second, manually-maintained source of that fact only drifts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The samples no longer set spec.ttl, so a copied sample creates an unbounded box
rather than one that expires after 8h. (ttl remains a valid optional field on both
types — this only changes what the samples demonstrate.)

Also spell out WHY the patch targets name the manager: without a name kustomize
applies the patch to every Deployment in the build, so a second one added later
would silently inherit the manager-only --metrics-bind-address flag (and the
webhook port and cert mount) and fail to start.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 8, 2026 12:34
@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces first-class Sandbox and SandboxSet workload APIs/controllers (to provision interactive “remote box” sandboxes via the existing Pod placement path), and replaces the prior webhook-certificate setup (cert-manager or hack/gen-webhook-cert.sh) with an in-process certificate rotator so deploy/e2e no longer require out-of-band cert steps.

Changes:

  • Add Sandbox + SandboxSet CRDs, controllers, samples, and unit/integration tests.
  • Add pkg/cert (OPA cert-controller-based) webhook serving cert rotation and defer controller/webhook registration until cert readiness.
  • Remove cert-manager and gen-webhook-cert.sh setup from e2e/deploy flows; update docs and dependencies accordingly.

Reviewed changes

Copilot reviewed 34 out of 37 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/utils/utils.go Removes cert-manager helpers from e2e utilities (suite no longer installs cert-manager).
test/e2e/e2e_test.go Updates e2e flow to rely on in-process cert rotation; adjusts secret name assertions and CA injection checks.
test/e2e/e2e_suite_test.go Removes cert-manager setup/teardown logic and related env vars.
pkg/cert/cert.go Adds in-process webhook cert rotator wiring (Secret + caBundle patching).
Makefile Updates e2e comments to reflect cert-manager is no longer needed.
internal/webhook/v1/pod_webhook.go Uses EnabledValue constant for opt-in label comparison.
internal/controller/sandboxset_controller.go Adds SandboxSet reconciler (maintains N Sandboxes; status rollups; scale-in victim selection).
internal/controller/sandboxset_controller_test.go Adds unit tests for SandboxSet scaling, idempotency, pruning, status, and victim selection.
internal/controller/sandbox_validation_test.go Adds envtest-based admission tests for Sandbox CEL/schema behavior.
internal/controller/sandbox_controller.go Adds Sandbox reconciler (synthesizes Pod, projects status, enforces TTL).
internal/controller/sandbox_controller_test.go Adds unit tests for Sandbox pod synthesis, status projection, TTL behavior, and foreign-pod refusal.
internal/controller/pod_placement_helpers.go Uses EnabledValue constant for opt-in label comparison.
internal/controller/pod_placement_controller.go Uses EnabledValue constant for opt-in label comparison.
hack/gen-webhook-cert.sh Removes legacy script-based webhook cert provisioning.
hack/deploy.sh Removes legacy cert/caBundle steps; documents in-process rotation behavior and startup ordering.
go.sum Updates/bumps deps (adds cert-controller; bumps k8s libs, go-restful, jsonreference, etc.).
go.mod Adds cert-controller dep; bumps k8s libs to v0.33.4; adds go.uber.org/atomic indirect.
docs/deploy.md Updates deployment docs to describe in-process cert rotation and Sandbox-era behavior.
docs/architecture.md Removes outdated “Build status” section.
config/webhook/secret.yaml Adds empty Secret manifest intended for rotator-managed keypair storage.
config/webhook/kustomization.yaml Includes the new webhook Secret manifest in webhook kustomization.
config/samples/nebula_v1alpha1_sandboxset.yaml Adds a SandboxSet sample manifest.
config/samples/nebula_v1alpha1_sandbox.yaml Adds a Sandbox sample manifest.
config/rbac/role.yaml Extends manager ClusterRole for new CRDs and cert-rotator needs (secrets + webhook config updates).
config/manager/manager.yaml Adds POD_NAMESPACE env var for cert rotator scoping.
config/default/manager_webhook_patch.yaml Adjusts webhook mount/port wiring for the new cert approach.
config/default/kustomization.yaml Updates cert-manager commentary and patch scoping notes.
config/crd/kustomization.yaml Includes new Sandbox/SandboxSet CRD bases.
config/crd/bases/nebula.inftyai.com_sandboxsets.yaml Adds generated SandboxSet CRD (schema, scale subresource, columns, validations).
config/crd/bases/nebula.inftyai.com_sandboxes.yaml Adds generated Sandbox CRD (schema, columns, CEL validations).
cmd/main.go Registers cert rotator; defers controller/webhook setup until cert readiness; adds Sandbox + SandboxSet controllers.
api/v1alpha1/zz_generated.deepcopy.go Adds generated deepcopy implementations for Sandbox and SandboxSet types.
api/v1alpha1/sandboxset_types.go Defines SandboxSet API types, status, conditions, and markers (incl. /scale).
api/v1alpha1/sandbox_types.go Defines Sandbox API types, phases/reasons, TTL, and markers.
api/v1alpha1/groupversion_info.go Documents new workload types; adds constants (EnabledValue, Sandbox labels, SandD path).
.golangci.yml Exempts kubebuilder marker lines from lll (line-length) linting.
.gitignore Ignores locally built cmd binaries in repo root (e.g., /manager, /keybroker).
Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)

config/default/manager_webhook_patch.yaml:44

  • The patch currently mounts the webhook cert directory from a Secret volume read-only. That conflicts with the rest of this PR (docs/deploy.md and pkg/cert) which states the rotator writes the keypair to CertDir; a Secret projection is read-only and will prevent the rotator from writing cert files, potentially leaving certsReady never closing and controllers/webhook never registering. Use a writable emptyDir for CertDir (and keep the Secret as API storage, not a volume projection).
# The directory the keypair is projected into and the webhook server serves from.
# Must match certDir in pkg/cert (controller-runtime's default path), which is also
# the path the rotator polls to decide readiness.
- op: add
  path: /spec/template/spec/containers/0/volumeMounts/-
  value:
    mountPath: /tmp/k8s-webhook-server/serving-certs
    name: webhook-certs
    readOnly: true


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/controller/sandbox_controller.go
Comment thread internal/controller/sandbox_controller_test.go
Comment thread pkg/cert/cert.go Outdated
Comment thread config/default/manager_webhook_patch.yaml
Copilot AI review requested due to automatic review settings August 8, 2026 14:03
@kerthcet

kerthcet commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

/lgtm
/kind feature

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Aug 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)

internal/controller/sandbox_controller.go:155

  • ensurePod uses (nil, nil) as the sentinel for “a foreign Pod exists”, and Reconcile turns that into a PodConflict status. But on Create returning AlreadyExists (a normal race with a concurrent reconcile), this code also returns (nil, nil), which can incorrectly mark a healthy Sandbox as PodConflict. Re-read the Pod on AlreadyExists and only return nil when it truly isn’t owned by this Sandbox.
	if err := r.Create(ctx, pod); err != nil {
		if apierrors.IsAlreadyExists(err) {
			// Lost a race with another reconcile (or with a foreign creator). Re-read on
			// the next pass rather than guessing which it was.
			return nil, nil
		}

Comment thread config/default/manager_webhook_patch.yaml
@kerthcet

kerthcet commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

/hold

@InftyAI-Agent InftyAI-Agent added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2026
Copilot AI review requested due to automatic review settings August 8, 2026 14:18
@kerthcet
kerthcet force-pushed the feat/remote-workloads-design branch from 693f90c to faa80ef Compare August 8, 2026 14:18
@InftyAI-Agent InftyAI-Agent removed the lgtm Looks good to me, indicates that a PR is ready to be merged. label Aug 8, 2026
65ee48d fixed the volume (emptyDir → Secret projection) but left comments in three
other places still saying the rotator writes the keypair to certDir. It does not:
cert-controller performs no filesystem writes at all, and CertDir is a path it only
os.Stat()s to decide readiness (ensureCertsMounted). The kubelet puts the files there
by projecting the Secret.

This mattered in review: a reader going by these comments concludes the read-only
Secret mount must be a bug and that an emptyDir is the fix — which is exactly the
configuration that hangs the manager (files never appear, IsReady never closes, no
controller or webhook ever registers, pod still reports Running). Verified on Kind:
emptyDir → 0 virtual nodes, Secret projection → 3.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)

internal/controller/sandboxset_controller_test.go:272

  • This test uses for i := range 3, but Go can’t range over an integer. The test file won’t compile as written.
    config/default/manager_webhook_patch.yaml:54
  • config/default/kustomization.yaml sets namePrefix: nebula-, and Kustomize also prefixes name references like volumes[].secret.secretName. Setting secretName: nebula-webhook-server-cert here will likely be transformed into nebula-nebula-webhook-server-cert, breaking the cert Secret mount (and the in-process rotator’s Secret lookup). Use the unprefixed base name so Kustomize can apply the prefix exactly once.
  path: /spec/template/spec/volumes/-
  value:
    name: webhook-certs
    secret:
      secretName: nebula-webhook-server-cert

internal/controller/sandbox_controller.go:156

  • ensurePod returns (nil, nil) on AlreadyExists, but the caller treats pod == nil as a foreign-Pod name conflict and sets ReasonPodConflict. A normal reconcile race (two reconciles creating the same Pod) will therefore incorrectly mark the Sandbox as conflicted instead of just reading the existing owned Pod.
	if err := r.Create(ctx, pod); err != nil {
		if apierrors.IsAlreadyExists(err) {
			// Lost a race with another reconcile (or with a foreign creator). Re-read on
			// the next pass rather than guessing which it was.
			return nil, nil
		}

Comment thread internal/controller/sandboxset_controller.go
Comment thread internal/controller/sandboxset_controller_test.go
Copilot AI review requested due to automatic review settings August 8, 2026 14:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)

internal/controller/sandbox_controller.go:153

  • When Pod creation races and returns AlreadyExists, this returns (nil, nil), which the caller interprets as a foreign-Pod conflict and sets ReasonPodConflict. In the common race case where the Pod was actually created by another reconcile for the same Sandbox, this will incorrectly mark the Sandbox as conflicting. Re-read the Pod on AlreadyExists and return it when it is owned by this Sandbox; only return (nil, nil) for a truly foreign Pod.
	if err := r.Create(ctx, pod); err != nil {
		if apierrors.IsAlreadyExists(err) {
			// Lost a race with another reconcile (or with a foreign creator). Re-read on
			// the next pass rather than guessing which it was.
			return nil, nil
		}

docs/deploy.md:106

  • This states the cert volume is an emptyDir because the rotator writes files there, but the rest of the PR (pkg/cert and config/default/manager_webhook_patch.yaml) explicitly relies on a Secret projection and notes cert-controller does not write to disk. This documentation is internally inconsistent and will mislead operators into deploying a non-functional webhook cert mount.
The cert volume is an `emptyDir`, not a Secret projection, because the rotator
*writes* to that path; a Secret volume is read-only and its kubelet refresh would
fight the rotator.

internal/controller/sandboxset_controller.go:329

  • For spec.replicas == 0, the Ready condition reason/message is set to ScaledToZero but the condition Status stays False. The API doc on SandboxSetConditionReady says it is True when every desired box is Ready; when replicas is 0, that condition should be True (vacuously) so kubectl wait --for=condition=Ready works for a parked set.

@kerthcet

kerthcet commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

/unhold

@InftyAI-Agent InftyAI-Agent removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2026
@kerthcet

kerthcet commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

/lgtm

@InftyAI-Agent InftyAI-Agent added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Aug 8, 2026

@InftyAI-Agent InftyAI-Agent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved: PR has both lgtm and approved labels

@InftyAI-Agent
InftyAI-Agent merged commit 44456b5 into InftyAI:main Aug 8, 2026
18 checks passed
@kerthcet
kerthcet deleted the feat/remote-workloads-design branch August 8, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants