fix(storageclass): preserve guest-only resources

[weicao]

Summary

• mark virtual StorageClasses created or reconciled by the fromHost controller with its controller ownership annotation
• preserve unowned guest-only StorageClasses when the mapped host object is absent
• retain deletion propagation for controller-owned StorageClasses after host deletion
• preserve unowned guest objects when a host object stops matching the fromHost selector

Fixes the vcluster defect recorded in apecloud/kubeblocks#10614. apecloud/vcluster has Issues disabled, so the linked KubeBlocks issue contains the scene and reproduction evidence.

Root cause

hostStorageClassSyncer.SyncToHost unconditionally deleted the virtual object whenever its mapped host object was missing. The mirror mapper treats host objects as managed, while imported virtual StorageClasses carried no ownership marker. As a result, enabling fromHost StorageClass sync deleted unrelated guest-only StorageClasses, including a generated S3 StorageClass used by BackupRepo. Helm rollback cannot restore such a deletion.

Constructed test scene and reproducibility

1. Start from exact runtime base b98b1ae5a603fa504daf1e745d65784c5a0be0ea with fromHost StorageClass sync disabled.
2. Keep a StorageClass only in guest and no same-name host StorageClass.
3. Enable fromHost StorageClass sync.
4. Before this patch, the guest-only object enters SyncToHost and is deleted.

The regression test failed against the base with virtual state mismatch: expected the guest object, existing state was empty.

Contract

• unowned guest-only StorageClass + host missing => preserve;
• controller-owned synced StorageClass + host deleted => delete virtual object;
• create/update from host => apply and retain the controller ownership marker;
• selector mismatch => only delete controller-owned virtual objects.

Tests

• red before patch: go test -v ./pkg/controllers/resources/storageclasses -run TestFromHostSync -count=1 -> rc1 at Preserve_guest-only_storage_class_when_host_resource_is_absent
• green after patch: go test ./pkg/controllers/resources/storageclasses -count=1
• git diff --check

Scope boundary

This is a vcluster StorageClass lifecycle fix. It is not an etcd restore PASS and does not authorize rollout until a focused review, patched image identity proof, and the previously defined guest/host live gates pass.

[weicao]

Focused review at exact head 61ce65f2982d887e097e3af481a230cf9f6cb9d8: BLOCKED.

1. P0 - update can persist an unowned mirrored StorageClass after a configured translate patch. In Sync, the code marks event.Virtual at host_syncer.go:102-103, but the deferred patcher was created with patcher.TranslatePatches(...) at line 91. SyncerPatcher.Patch deep-copies the object and then calls pro.ApplyPatchesVirtualObject before persistence (pkg/patcher/patcher.go:140-151). A user patch that removes or replaces metadata.annotations["vcluster.loft.sh/controlled-by"] therefore runs after markOwnedVirtual and can erase the invariant. The next host-missing or selector-mismatch reconcile then preserves this formerly mirrored object forever. Keep translate patches before the final ownership write, or otherwise reassert/validate the marker after patches. The GatewayClass syncer already demonstrates the required ordering pattern: construct the patcher without TranslatePatches, apply pro patches explicitly, then enforce the invariant.

2. P1 - the host-delete regression is vacuous. host_syncer_test.go:115 sets ExpectedVirtualState to an empty map. syncertesting.RunTests only iterates map entries, so this asserts no StorageClass GVK and would pass even if the owned guest object remained. Use {StorageClass GVK: {}} so the fake client is actually listed and zero objects are asserted.

3. P1 - the new ownership lifecycle is only exercised by direct syncer calls. There is no selector-mismatch coverage for either branch (owned delete / unowned preserve), no test that an unmarked legacy mirrored object with its host still present is adopted, and no controller-level Reconcile test proving the ownership annotation survives create/update, excludeVirtual, persistence, and later host deletion. These are the boundaries this PR changes; direct Sync* tests bypass generic controller routing and exclusion.

Evidence:

• Base b98b1ae5a603fa504daf1e745d65784c5a0be0ea plus this PR's test file: go test -run TestFromHostSync -count=1 ./pkg/controllers/resources/storageclasses => rc=1; guest-only case shows expected object vs existing [].
• Exact head: package test PASS; go test ./pkg/controllers/resources/... ./pkg/mappings/... ./pkg/syncer/... PASS; git diff --check b98..HEAD PASS.

The ownership primitive itself is compatible with generic excludeVirtual: the annotation value equals syncer name host-storageclass, so this controller accepts it while other owners remain excluded. Migration boundary should remain conservative: unmarked + host absent is preserved; unmarked + host present must be explicitly adopted and tested. PR #5's managed-host filter is orthogonal; do not stack it until this PR closes its own lifecycle tests, then run a combined full-Reconcile gate.

[weicao]

Current-head re-review at 2caf64c198cb1e22b703ec287466f81398d29f3e: still BLOCKED on one P0 error-path invariant.

The previous three findings are fixed on the success path: translate patches now run before the final marker write; the deletion expectation names the StorageClass GVK; selector mismatch and full controller adoption/deletion are covered. Exact-head focused and shared gates pass.

P0 - a translate-patch error still persists a partially mutated, unowned mirrored object. Sync creates the patcher/defer at lines 91-99, then mutates event.Virtual, calls pro.ApplyPatchesVirtualObject, and returns on error before markOwnedVirtual. Because the deferred patch.Patch(...) runs on every return, it persists those partial mutations even when the pro patch failed. A patch implementation that deletes the marker and then returns an error leaves the guest object with host fields applied and no vcluster.loft.sh/controlled-by; later host-missing or selector-mismatch then incorrectly preserves it as guest-owned.

Independent exact-head error injection proves this:

• start with an owned virtual SC using provisioner old-virtual-provisioner;
• host has new-host-provisioner;
• mocked ApplyPatchesVirtualObject deletes the marker and returns injected patch failure;
• Sync returns the expected error, but persisted readback has empty owner marker and new-host-provisioner.

The red assertions are:

assertion failed: "" != host-storageclass
assertion failed: new-host-provisioner != old-virtual-provisioner

Required contract: when translate patching fails, do not persist any virtual mutation. A robust shape is to build/patch/mark a deep copy and only assign it to the object after all fallible transforms succeed, while retaining the patcher's original pre-state for the final write; alternatively, make the defer skip persistence when the transform path has returned an error. Add this error-injection regression so both marker and prior fields are unchanged after the failed reconcile.

Gates at exact head:

• go test -count=1 ./pkg/controllers/resources/storageclasses PASS
• go test -count=1 ./pkg/controllers/resources ./pkg/mappings/... ./pkg/syncer/... PASS
• git diff --check b98..HEAD PASS
• independent patch-error invariant probe RED as above

No PR #5 stacking or live rollout until this closes.

[weicao]

Current-head focused re-review at 0b0186f838ea292ab09847033641ee9786641e5e: APPROVED for the PR #6 guest-only StorageClass ownership lifecycle scope.

The error-path blocker is closed. SyncerPatcher still captures the original virtual object as before-state, while all host-field copying, translate patching, and final ownership marking happen on desired := event.Virtual.DeepCopy(). A transform error returns before assigning desired, so the deferred patcher sees no diff and persists nothing; success assigns the complete desired object once and the deferred patcher writes it.

The deterministic regression now proves both halves of the contract after an injected patch failure that deletes the marker before returning error:

• owner marker remains host-storageclass;
• provisioner remains the prior virtual value.

The earlier findings also remain closed: final marker is after successful translate patches, owned host-delete asserts an explicit empty StorageClass GVK, selector mismatch covers owned/unowned branches, and full SyncController.Reconcile covers legacy unmarked adoption followed by host-delete propagation.

Independent gates at exact head:

• go test -race -count=1 ./pkg/controllers/resources/storageclasses PASS
• go test -count=1 ./pkg/controllers/resources/... ./pkg/mappings/... ./pkg/syncer/... PASS
• git diff --check b98b1ae5a..HEAD PASS
• PR head/base identity readback matches 0b0186f8... / b98b1ae5...

Evidence boundary: this approves PR #6 by itself. PR #5 remains orthogonal and unstacked; a combined current-head full-Reconcile gate is still required before producing or rolling a runtime image. No live rollout is approved by this comment.

[weicao]

Review hardening landed at 6f39f45ea70a84a4de714a4391a6c341f624c398:

• build transformed desired state from a host deep copy and create the syncer patcher only after transforms succeed, preventing failed transforms from mutating host pointer/slice/nested fields or being persisted by deferred patching;
• make owned virtual deletion re-read current state, re-check ownership and event UID, and delete with a current-UID precondition;
• add adversarial coverage for failed-transform alias mutation, owner removal, and same-name UID replacement.

Local gates on the updated PR head:

• go test -race -count=1 ./pkg/controllers/resources/storageclasses PASS
• go test -count=1 ./pkg/controllers/resources/... ./pkg/mappings/... ./pkg/syncer/... PASS
• git diff --check PASS

The stacked PR #5 managed-host deletion path is validated on combined head 0b4650fd61dd03b50898918b15ce491efe72c5ad; it is not deployable until independent re-review/test closes.

[weicao]

Focused design-contract review at exact head 783dec5d338f639e221b851ba809c8d53a085332 (base apecloud-v0.35.1).

BLOCKER — the pre-delete ownership re-read is cached, so the claimed race safety is incomplete

deleteOwnedVirtual correctly re-reads ownership and uses a UID delete precondition, but the read at host_syncer.go:136 uses ctx.VirtualClient, which is the manager client. Its reads can be cache-backed while deletes go to the API server. If the live object removes the ownership annotation in place (same UID) and the cache still returns the prior owned version, lines 143-157 accept the stale owner and the UID precondition still succeeds, deleting the live unowned object.

Direct adversarial evidence on this exact head:

stale cached Get: owned annotation, UID=same-live-uid
live client state: owner annotation removed, UID=same-live-uid
SyncToHost -> deleteOwnedVirtual
expected: live unowned object preserved
actual: delete succeeds; subsequent live Get is NotFound
result: FAIL (rc1)

The PR's existing focused tests and their race run both pass, but the current owner removed from current object fixture uses an immediately consistent fake read. It does not exercise cache staleness, and the race detector cannot detect this distributed read/write race.

Please make the destructive ownership re-read live/uncached (for example, an APIReader captured from the virtual manager, or an equivalent server-authoritative gate), retain the UID precondition, and add the stale-read/same-UID owner-removal case. A same-UID metadata update is exactly the case UID preconditions do not cover.

Pre-flight: current head and two-file scope verified; root-cause contract is coherent; no runtime script/API-version issue. Evidence is source/unit only, not a product/runtime verdict.

Classes 1-8:

1. silent fallback: clean; unowned objects are preserved.
2. ownership write contract: clean; create/update reassert ownership after patches.
3. state continuity: BLOCKER — ownership read from cache is treated as live through the delete boundary.
   4/4a. sentinel/cross-field collision: clean.
4. conditional cleanup pre-state: BLOCKER — cleanup depends on potentially stale owner state.
5. NotFound: explicit and appropriate.
6. terminating vs absent: no release contract in this delta.
7. precedence: clean.

Direct commands: existing focused tests rc0; focused race rc0; stale-cache/live-unowned same-UID adversarial case rc1 as above.