Use a merge patch, not a full Update, for finalizer add/remove

[ayuskauskas]

Summary

SkyhookReconciler.HandleFinalizer adds and removes SkyhookFinalizer with a full-object r.Update. Both writes should be merge patches scoped to metadata.finalizers.

• add: skyhook_controller.go#L1854-L1858
• remove: skyhook_controller.go#L2032-L2035

Why it matters

1. It silently rewrites user-authored spec.

r.Update marshals the whole typed object and PUTs it. Every resource.Quantity field re-serializes through Quantity.MarshalJSON(), which emits the canonical String() form — so the add-finalizer write replaces values that are semantically identical but textually different from what the user applied.

Concrete case: NVIDIA/aicr ships a CR with per-package resource overrides:

resources:
  cpuLimit: 4000m
  cpuRequest: 2000m
  memoryLimit: 8192Mi
  memoryRequest: 4096Mi

Admission leaves these alone (the mutating webhook's Default() is a no-op, so controller-runtime short-circuits before computing a patch). Then the first reconcile's finalizer Update rewrites the stored CR to cpuLimit: "4", cpuRequest: "2", memoryLimit: 8Gi, memoryRequest: 4Gi.

Nothing is numerically wrong, but:

• kubectl get ... -o yaml no longer matches what was applied, which is confusing when debugging a package's cgroup limits.
• A subsequent Helm/Flux/Argo sync re-sends 4000m. The add path is guarded by !ContainsFinalizer, so the operator never re-canonicalizes — the field flips to the source form and stays there. GitOps tooling sees perpetual drift on a field nobody is editing.
• Any assertion (chainsaw, e2e, CI gate) written against either spelling is unstable depending on when it samples.

2. Whole-object last-writer-wins.

A PUT of the full object clobbers concurrent spec changes made between the controller's cached read and the write. The file already treats this as a known hazard elsewhere — the migration-marker write re-Gets and uses client.MergeFrom with the comment "because of conflict issues (409) we need to do things a bit differently here" (L645-L658). Finalizer handling should follow the same pattern.

Proposed change

obj := skyhook.GetSkyhook().NodeWright
patch := client.MergeFrom(obj.DeepCopy())
controllerutil.AddFinalizer(obj, SkyhookFinalizer)
if err := r.Patch(ctx, obj, patch); err != nil {
    return false, fmt.Errorf("error patching skyhook to add finalizer: %w", err)
}

MergeFrom diffs the two typed objects and sends only the delta, so the wire payload carries metadata.finalizers alone — spec quantities are byte-identical on both sides and never appear in it. Same shape for the RemoveFinalizer branch.

There is ample precedent on this very object: L651, L907, L1246 all patch the CR with client.MergeFrom.

Optional: client.MergeFromWithOptimisticLock if a stale-cache add/remove race is a concern — note it reintroduces 409s that the caller must requeue on.

Acceptance criteria

• Both the add and remove finalizer paths use r.Patch with client.MergeFrom.
• Test: a CR created with cpuLimit: 4000m / memoryLimit: 8192Mi still reads those exact strings after reconcile completes.
• Deletion ordering is unchanged — the status update must still precede finalizer removal (see the existing comment about removing the last finalizer racing the delete into a spurious NotFound).

Notes

Found while tracing why AICR's committed tuning.yaml resource values did not match the live CR. Behavior confirmed against operator/v0.17.0 (the chart version AICR pins) and current main; the same two call sites exist in both, only renamed from .Skyhook to .NodeWright.