review: comment budget and named func types in pkg/scale and controllers

api/v1alpha1/sandbox_conversion.go

@@ -25,9 +25,7 @@ import (
 
 const v1alpha1SandboxStateAnnotation = "api.agents.x-k8s.io/v1alpha1-sandbox-state"
 
-// v1alpha1State is the round-trip payload: the fields v1beta1 cannot represent
-// (replica counts collapse into OperatingMode). Legacy annotations carrying a
-// full v1alpha1 Sandbox JSON decode into this shape too.
+// v1alpha1State is the round-trip payload for the fields v1beta1 cannot represent.
 type v1alpha1State struct {
 	Spec struct {
 		Replicas *int32 `json:"replicas,omitempty"`

controllers/sandbox_controller.go

@@ -1002,11 +1002,7 @@ func (r *SandboxReconciler) deleteExpiredChild(ctx context.Context, sandbox *san
 	return nil
 }
 
-// CacheByObject scopes a manager cache to the child objects this controller
-// labels, so the Pod/Service/PVC informers never watch the whole cluster. Every
-// cached read this controller makes targets a labeled child, and the PVC entry
-// also stops the first volumeClaimTemplates Sandbox from spinning up a
-// cluster-wide informer mid-reconcile.
+// CacheByObject scopes the manager cache to this controller's labeled children, so no child informer watches the whole cluster.
 func CacheByObject() (map[client.Object]cache.ByObject, error) {
 	sel, err := labels.Parse(sandboxLabel)
 	if err != nil {
@@ -1039,10 +1035,7 @@ func MergeVolumeClaimVolumes(existing, pvcVolumes []corev1.Volume) []corev1.Volu
 	return append(filtered, pvcVolumes...)
 }
 
-// checkOwnership determines whether a Kubernetes resource is owned by the given Sandbox,
-// has no controller, or is owned by a different controller.
-// It returns both the ownership classification and the controller reference (if any),
-// so callers can log owner details without redundant GetControllerOf calls.
+// checkOwnership classifies obj's ownership relative to sandbox and returns the controller reference it read.
 func checkOwnership(obj client.Object, sandbox *sandboxv1beta1.Sandbox) (resourceOwnership, *metav1.OwnerReference) {
 	controllerRef := metav1.GetControllerOf(obj)
 	if controllerRef == nil {
@@ -1141,6 +1134,11 @@ func podIPsFromStatus(podIPs []corev1.PodIP) []string {
 	return ips
 }
 
+type (
+	keyPredicate func(string) bool
+	keyCallback  func(string)
+)
+
 // hasSystemReservedPrefix reports whether a key uses a label/annotation prefix
 // reserved for the sandbox system or its extensions.
 func hasSystemReservedPrefix(key string) bool {
@@ -1178,7 +1176,7 @@ func isControllerManagedPodAnnotation(key string) bool {
 
 // filterSystemKeys copies src minus system-reserved keys, returning the copy and
 // the sorted-by-caller list of keys it carries.
-func filterSystemKeys(src map[string]string, isSystem func(string) bool, onSkip func(string)) (map[string]string, []string) {
+func filterSystemKeys(src map[string]string, isSystem keyPredicate, onSkip keyCallback) (map[string]string, []string) {
 	out := make(map[string]string, len(src))
 	var kept []string
 	for k, v := range src {
@@ -1236,7 +1234,7 @@ func setEntry(m map[string]string, key, want string) bool {
 
 // propagateKeys copies template into live, skipping system-reserved keys, and
 // returns the keys it now manages plus whether live changed.
-func propagateKeys(live, template map[string]string, isSystem func(string) bool, onSkip func(string)) ([]string, bool) {
+func propagateKeys(live, template map[string]string, isSystem keyPredicate, onSkip keyCallback) ([]string, bool) {
 	var managed []string
 	updated := false
 	for k, v := range template {
@@ -1255,7 +1253,7 @@ func propagateKeys(live, template map[string]string, isSystem func(string) bool,
 
 // prunePropagated drops previously propagated keys the template no longer carries.
 // A system key recorded by an older controller is scrubbed unless keep claims it.
-func prunePropagated(live map[string]string, recorded string, template map[string]string, isSystem, keep func(string) bool, onScrub func(string)) bool {
+func prunePropagated(live map[string]string, recorded string, template map[string]string, isSystem, keep keyPredicate, onScrub keyCallback) bool {
 	updated := false
 	for k := range strings.SplitSeq(recorded, ",") {
 		if k == "" {

pkg/scale/apiserver/lifecycle_storage.go

@@ -42,7 +42,6 @@ func (r *lifecycleREST) Destroy() {}
 
 func (r *lifecycleREST) NamespaceScoped() bool { return true }
 
-// GroupVersionKind pins the kind the request pipeline decodes the body as.
 func (r *lifecycleREST) GroupVersionKind(schema.GroupVersion) schema.GroupVersionKind {
 	gvks, _, err := Scheme.ObjectKinds(r.newOptions())
 	if err != nil || len(gvks) == 0 {
@@ -51,8 +50,6 @@ func (r *lifecycleREST) GroupVersionKind(schema.GroupVersion) schema.GroupVersio
 	return gvks[0]
 }
 
-// Create runs the verb. The subresource's parent name arrives separately from
-// the body, which is why this is a NamedCreater.
 func (r *lifecycleREST) Create(
 	ctx context.Context,
 	name string,

pkg/scale/apiserver/storage.go

@@ -100,11 +100,6 @@ func (r *sandboxREST) Watch(ctx context.Context, options *metainternalversion.Li
 	return r.store.Watch(ctx, toScaleListOptions(ctx, options))
 }
 
-// Create is the L3 write path: it derives the warm pool from the submitted
-// Sandbox and asks the store to hand over an already-running microVM from a node
-// with warm capacity for that pool (a synchronous node-local claim). It writes NO
-// per-sandbox object to etcd; it synthesizes and returns a Ready Sandbox carrying
-// the claim id + connection address. With no warm node it returns a retryable 503.
 func (r *sandboxREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, _ *metav1.CreateOptions) (runtime.Object, error) {
 	sb, ok := obj.(*sandboxv1beta1.Sandbox)
 	if !ok {
@@ -149,12 +144,8 @@ func (r *sandboxREST) Create(ctx context.Context, obj runtime.Object, createVali
 	return synthesizeClaimedSandbox(namespace, name, sb, assignment), nil
 }
 
-// Delete is the L3 release path. It resolves the sandbox from live node inventory
-// (so it knows the owning node), reads the sandboxd claim id the node published
-// on that entry, and releases the node-local claim via the store. Per the delete-
-// authorization contract this is owner-authorized teardown of the Sandbox resource
-// itself; it never destroys a VM on pod state alone. Returns (object, true).
 func (r *sandboxREST) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, _ *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	// owner-authorized teardown of the Sandbox resource only; pod state never reaches here
 	namespace := genericapirequest.NamespaceValue(ctx)
 	sb, err := r.store.Get(ctx, namespace, name)
 	if err != nil {

pkg/scale/claimgateway_impl.go

@@ -195,16 +195,8 @@ func (g *nodeClaimGateway) Claim(ctx context.Context, req ClaimRequest) (Assignm
 }
 
 // Release destroys the node-local microVM backing the Assignment.
-//
-// DELETE-AUTHORIZATION CONTRACT (G-0131 hard constraint). Release is invoked ONLY
-// on owner-authorized teardown — when the SandboxClaim CR itself is being released
-// or deleted by its owner. It is NEVER wired to Pod deletion or any pod-level
-// state: a Pod disappearing must not destroy the VM. There is deliberately no code
-// path in this package from Pod state to Release. As a structural guard the
-// gateway only releases sandboxes it actually delivered (it holds the sandbox's
-// own token, the Release credential); an Assignment the gateway never handed out
-// — e.g. one synthesized from stale pod state — is refused without any sandboxd
-// call, so no such state can drive a destroy.
+// It runs only on owner-authorized teardown, never off pod state — the same
+// delete-authorization contract the aggregated apiserver's Delete honors.
 func (g *nodeClaimGateway) Release(ctx context.Context, a Assignment) error {
 	g.mu.Lock()
 	d, ok := g.holdings[a.SandboxName]
@@ -382,7 +374,6 @@ type ReviewAuthorizer struct {
 	Verb     string
 }
 
-// Authorize denies missing identities and runs SubjectAccessReview.
 func (a *ReviewAuthorizer) Authorize(ctx context.Context, req ClaimRequest) error {
 	user := req.Selector[RequestUserSelectorKey]
 	if user == "" {

pkg/scale/sandboxstore_impl.go

@@ -187,6 +187,8 @@ func NewSandboxdClientFactory() SandboxdClientFactory {
 	}
 }
 
+type inventoryMatch func(inv *NodeInventory, i int) bool
+
 var _ SandboxStore = (*scatterGatherStore)(nil)
 
 // scatterGatherStore is the concrete SandboxStore: List/Get/Watch synthesize
@@ -381,7 +383,7 @@ func (s *scatterGatherStore) Watch(ctx context.Context, opts ListOptions) (watch
 // It reads the node the index last saw holding key and only sweeps the whole
 // fleet on a miss, canceling the rest of the sweep on the hit. Nil with a nil
 // error means no entry matched.
-func (s *scatterGatherStore) findEntry(ctx context.Context, op, key string, match func(inv *NodeInventory, i int) bool) (*sandboxv1beta1.Sandbox, error) {
+func (s *scatterGatherStore) findEntry(ctx context.Context, op, key string, match inventoryMatch) (*sandboxv1beta1.Sandbox, error) {
 	if node, ok := s.index.lookup(key); ok {
 		if sb := s.matchOnNode(ctx, node, match); sb != nil {
 			return sb, nil
@@ -439,7 +441,7 @@ func (s *scatterGatherStore) findEntry(ctx context.Context, op, key string, matc
 
 // matchOnNode resolves match against one node's inventory, returning nil when
 // that node is unreadable or no longer holds the entry.
-func (s *scatterGatherStore) matchOnNode(ctx context.Context, node string, match func(inv *NodeInventory, i int) bool) *sandboxv1beta1.Sandbox {
+func (s *scatterGatherStore) matchOnNode(ctx context.Context, node string, match inventoryMatch) *sandboxv1beta1.Sandbox {
 	inv, err := s.src.NodeInventory(ctx, node)
 	if err != nil {
 		return nil
@@ -706,8 +708,6 @@ func (s *StaticInventorySource) Put(inv *NodeInventory) {
 	s.inv[inv.Node] = inv.DeepCopy()
 }
 
-// Apply implements InventoryApplier: it stores the inventory and counts the
-// apply, modeling the single O(nodes) server-side-apply write per node.
 func (s *StaticInventorySource) Apply(_ context.Context, inv *NodeInventory) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -732,15 +732,12 @@ func (s *StaticInventorySource) Remove(node string) {
 	delete(s.partition, node)
 }
 
-// ListNodes returns the known node names in stable order.
 func (s *StaticInventorySource) ListNodes(_ context.Context) ([]string, error) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 	return slices.Sorted(maps.Keys(s.inv)), nil
 }
 
-// NodeInventory returns a copy of one node's inventory, or an error if the node
-// is partitioned or unknown.
 func (s *StaticInventorySource) NodeInventory(_ context.Context, node string) (*NodeInventory, error) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
@@ -754,7 +751,6 @@ func (s *StaticInventorySource) NodeInventory(_ context.Context, node string) (*
 	return inv.DeepCopy(), nil
 }
 
-// NodeCapacity returns one node's address and pools without copying its entries.
 func (s *StaticInventorySource) NodeCapacity(_ context.Context, node string) (string, []PoolCapacity, error) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
@@ -800,7 +796,6 @@ func NewClientInventorySource(reader client.Reader) *ClientInventorySource {
 	return &ClientInventorySource{reader: reader}
 }
 
-// ListNodes lists NodeInventory objects (O(nodes)) and returns their names.
 func (s *ClientInventorySource) ListNodes(ctx context.Context) ([]string, error) {
 	ul := &unstructured.UnstructuredList{}
 	ul.SetGroupVersionKind(NodeInventoryGVK.GroupVersion().WithKind(NodeInventoryGVK.Kind + "List"))
@@ -815,7 +810,6 @@ func (s *ClientInventorySource) ListNodes(ctx context.Context) ([]string, error)
 	return nodes, nil
 }
 
-// NodeInventory fetches and decodes one node's NodeInventory object.
 func (s *ClientInventorySource) NodeInventory(ctx context.Context, node string) (*NodeInventory, error) {
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(NodeInventoryGVK)
@@ -829,8 +823,6 @@ func (s *ClientInventorySource) NodeInventory(ctx context.Context, node string)
 	return inv, nil
 }
 
-// NodeCapacity decodes only the address and pools fields, so the claim and
-// routing paths never pay for the node's whole entry list.
 func (s *ClientInventorySource) NodeCapacity(ctx context.Context, node string) (string, []PoolCapacity, error) {
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(NodeInventoryGVK)

pkg/scale/sandboxstore_lifecycle.go

@@ -7,7 +7,6 @@ import (
 	"github.com/cocoonstack/sandbox-operator/pkg/sandboxd"
 )
 
-// Pause hibernates a delivered sandbox on its owning node.
 func (s *scatterGatherStore) Pause(ctx context.Context, node, id string) error {
 	cl, err := s.nodeClient(ctx, node, "pause", id)
 	if err != nil {
@@ -19,7 +18,6 @@ func (s *scatterGatherStore) Pause(ctx context.Context, node, id string) error {
 	return nil
 }
 
-// Resume restores a paused sandbox through cocoon's mmap fast path.
 func (s *scatterGatherStore) Resume(ctx context.Context, node, id string) error {
 	cl, err := s.nodeClient(ctx, node, "resume", id)
 	if err != nil {
@@ -31,8 +29,6 @@ func (s *scatterGatherStore) Resume(ctx context.Context, node, id string) error
 	return nil
 }
 
-// Fork branches a sandbox into count children on its owning node. Children are
-// fresh claims with their own ids; the parent keeps running.
 func (s *scatterGatherStore) Fork(ctx context.Context, node, id string, count, ttlSeconds int) ([]Assignment, error) {
 	if count < 1 {
 		return nil, fmt.Errorf("scale: fork count must be >= 1, got %d", count)
@@ -58,7 +54,6 @@ func (s *scatterGatherStore) Fork(ctx context.Context, node, id string, count, t
 	return out, nil
 }
 
-// Snapshot captures a sandbox's state as a checkpoint on its owning node.
 func (s *scatterGatherStore) Snapshot(ctx context.Context, node, id, name string) (Snapshot, error) {
 	cl, err := s.nodeClient(ctx, node, "snapshot", id)
 	if err != nil {
@@ -71,7 +66,6 @@ func (s *scatterGatherStore) Snapshot(ctx context.Context, node, id, name string
 	return snapshotFrom(ck, node), nil
 }
 
-// Snapshots lists a node's checkpoints.
 func (s *scatterGatherStore) Snapshots(ctx context.Context, node string) ([]Snapshot, error) {
 	cl, err := s.nodeClient(ctx, node, "list snapshots", "")
 	if err != nil {
@@ -88,7 +82,6 @@ func (s *scatterGatherStore) Snapshots(ctx context.Context, node string) ([]Snap
 	return out, nil
 }
 
-// DeleteSnapshot removes a checkpoint from its node.
 func (s *scatterGatherStore) DeleteSnapshot(ctx context.Context, node, snapshotID string) error {
 	cl, err := s.nodeClient(ctx, node, "delete snapshot", snapshotID)
 	if err != nil {
@@ -100,7 +93,6 @@ func (s *scatterGatherStore) DeleteSnapshot(ctx context.Context, node, snapshotI
 	return nil
 }
 
-// Stats reports a sandbox's resource usage from its owning node.
 func (s *scatterGatherStore) Stats(ctx context.Context, node, id string) (SandboxStats, error) {
 	cl, err := s.nodeClient(ctx, node, "stats", id)
 	if err != nil {

pkg/scale/warmpool/driver.go

@@ -116,13 +116,6 @@ func New(kube client.Client, inv scale.InventorySource, token string, factory Cl
 	return &Driver{kube: kube, inv: inv, token: token, factory: factory, interval: opts.Interval, log: opts.Log}
 }
 
-// Reconcile runs a full pool reconcile on ANY SandboxWarmPool or NodeInventory
-// event — so a `kubectl apply/patch/delete` reacts in milliseconds, not after a
-// poll tick. It ignores the request key (the loop is global, O(pools+nodes)) and
-// requeues after d.interval, which doubles as the sampling period of the warm
-// count written back to status. Target changes therefore land in milliseconds
-// (event-driven), while the fill they kick off — the node side provisions at
-// O(100)/s per node — becomes visible one interval at a time.
 func (d *Driver) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
 	if err := d.reconcileOnce(ctx); err != nil {
 		return ctrl.Result{}, err