Skip to content

fix(storageclass): ignore vcluster-managed host mirrors - #5

Open
weicao wants to merge 2 commits into
apecloud-v0.35.1from
fix/filter-managed-host-storageclasses
Open

fix(storageclass): ignore vcluster-managed host mirrors#5
weicao wants to merge 2 commits into
apecloud-v0.35.1from
fix/filter-managed-host-storageclasses

Conversation

@weicao

@weicao weicao commented Jul 11, 2026

Copy link
Copy Markdown

Problem

With sync.fromHost.storageClasses.enabled=true and an empty selector on a shared host, host StorageClasses created by another vCluster carry vcluster.loft.sh/managed-by and vcluster.loft.sh/object-name. The mirror mapper can map those objects onto a guest name before the host StorageClass syncer evaluates its selector. This can import stale long-name mirrors, overwrite an annotation-mapped guest StorageClass, or make selector-mismatch cleanup delete the wrong guest object.

Current-state evidence from the TiDB KB1.2 lane shows one baseline host default StorageClass, one old vCluster-managed translated host collision, the corresponding guest collision, and host-storageclass delete attempts failing on RBAC. This is a syncer contract issue, not a TiDB product failure.

Change

  • Reject host StorageClasses carrying the vCluster managed marker before physical-event mapping/admission.
  • Defensively ignore managed host objects in SyncToVirtual.
  • During an already-enqueued reconcile, delete only a stale guest mirror whose name exactly equals the managed host name.
  • Never delete or overwrite a different guest name reached through object-name annotation mapping.
  • Leave the managed host StorageClass untouched; no host delete permission is required.

Tests

  • Managed host StorageClass admission is false while a baseline host StorageClass remains admitted.
  • Managed host StorageClass cannot create a guest object.
  • Same-name stale guest mirror is removed while the managed host UID/object remains unchanged.
  • Annotation-mapped different-name guest collision is neither deleted nor overwritten.
  • Existing baseline host-to-guest create/update/delete behavior remains covered.

Validated on aaff9e25ea2650a4429e5d46ae04bb248e6023ca:

go test ./pkg/controllers/resources/storageclasses

go test ./pkg/controllers/resources/...

go test ./pkg/mappings/... ./pkg/syncer/...

git diff --check

All pass. Runtime evidence is intentionally not claimed by this PR; the pre-release image is for a separate focused live gate.

@weicao

weicao commented Jul 13, 2026

Copy link
Copy Markdown
Author

Independent adversarial review found the managed-host owned-delete path must not use name-only deletion on a stale event. The race-safe helper now lives on PR #6 head 6f39f45ea70a84a4de714a4391a6c341f624c398 and the stacked managed-host path is updated/tested on combined head 0b4650fd61dd03b50898918b15ce491efe72c5ad.

Do not merge/deploy this PR standalone before PR #6's helper is included (or this branch is rebased onto it). Independent re-review/test of the combined head is in progress.

@weicao
weicao changed the base branch from fix/restore-populate-complete-contract to apecloud-v0.35.1 July 28, 2026 15:51

@weicao weicao left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

BLOCKER — deletion is not ownership-bound

pkg/controllers/resources/storageclasses/host_syncer.go:98-100 deletes any virtual StorageClass whose name equals a vCluster-managed host StorageClass name. Name equality alone does not prove that the virtual object is the stale mirror created from that host object. A user-owned guest StorageClass can independently have the same long name and no managed marker; reconciling the other vCluster's host object then deletes that user object.

Direct adversarial evidence on the exact head:

TestReviewManagedHostDoesNotDeleteUnownedSameNameVirtualStorageClass
host: name=baseline-x-other-vcluster, managed-by=other-vcluster
virtual: same name, no managed marker, provisioner=user-provisioner
expected: virtual preserved
actual: virtual deleted
result: FAIL

The existing focused tests and their race run pass, but the current "same-name stale mirror" fixtures do not prove mirror ownership: staleVirtualObject / staleVirtual have no marker or other ownership evidence and therefore encode the unsafe behavior as expected.

Please require explicit mirror ownership before DeleteVirtualObject (using the contractually preserved marker/ownership metadata, not name alone), and add both cases:

  1. owned stale mirror is deleted;
  2. same-name unowned virtual StorageClass is preserved.

Pre-flight: purpose and two-file scope are coherent; current head verified; no runtime shell/API-version issue. Evidence is source/unit only, not a runtime or product verdict.

Classes 1-8:

  1. silent fallback: clean; managed host objects fail closed from import.
  2. non-empty/write contract: blocker equivalent here — destructive ownership precondition is not enforced.
  3. commit-state continuity: clean in the reviewed path.
    4/4a. sentinel/cross-field collision: clean; empty NamespacedName is checked by enqueue logic.
  4. conditional cleanup pre-state: blocker at lines 98-100; cleanup is conditioned only on name equality, not revalidated ownership.
  5. NotFound: clean for this delta.
  6. terminating vs absent: no release contract in this delta.
  7. precedence: clean.

Direct commands: existing focused tests rc0; focused race rc0; adversarial ownership-negative case rc1 as above.

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.

1 participant