fix: use PV/SC mapper for volumeName and storageClassName translation#3889
fix: use PV/SC mapper for volumeName and storageClassName translation#3889ChellaVigneshKP wants to merge 2 commits into
Conversation
The PVC syncer's translateSelector and the PV syncer's translate call translate.Default.HostNameCluster() directly for volumeName and storageClassName, bypassing the PV and SC mappers. This unconditionally mangles these names even for PVs synced from the host (with the vcluster.loft.sh/host-pv annotation), making it impossible to bind a PVC to a pre-existing host PV by name. It also causes SC name mismatches between PVs and PVCs on the host when toHost.storageClasses.enabled is false (mirror mapper scenario). Route the 5 call sites through mappings.VirtualToHostName() so translation respects resource origin — host-synced resources retain their original names, vCluster-created resources are still translated.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aef667b67
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // TODO: translate the storage secrets | ||
| if pPV.Spec.StorageClassName != "" { | ||
| pPV.Spec.StorageClassName = translate.Default.HostNameCluster(vPv.Spec.StorageClassName) | ||
| pPV.Spec.StorageClassName = mappings.VirtualToHostName(ctx, vPv.Spec.StorageClassName, "", mappings.StorageClasses()) |
There was a problem hiding this comment.
With sync.toHost.storageClasses.enabled=false, this new call resolves through the StorageClass mirror mapper and keeps the original class name, but persistentvolumes/syncer.go still forces translate.Default.HostNameCluster(...) in Sync (event.Host.Spec.StorageClassName at lines 249-251). That makes the create path and steady-state reconcile path disagree for the same PV, and after this commit PVC translation now also keeps the unmodified class, so host PV/PVC storage class names can diverge and prevent binding (or cause repeated reconcile attempts if the field update is rejected).
Useful? React with 👍 / 👎.
The Sync reconcile path in persistentvolumes/syncer.go still called translate.Default.HostNameCluster() for StorageClassName, while the create path and the PVC syncer now route through mappings.StorageClasses(). When sync.toHost.storageClasses.enabled=false the mapper is a mirror and keeps the original name, so the create and reconcile paths would disagree on the same PV — causing host PV/PVC class names to diverge or repeated reconcile attempts if the update was rejected.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
ChellaVigneshKP
changed the title
ChellaVigneshKP
changed the title
|
@FabianKramm could you take a look when you have a moment? This resolves #3810 . Happy to address any feedback. |
1 participant
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
What does this pull request do? Which issues does it resolve?
The PVC syncer's translateSelector function calls translate.Default.HostNameCluster() directly to translate volumeName and storageClassName, bypassing the PV and SC mappers. This unconditionally mangles these names even when the PV was synced from the host (has vcluster.loft.sh/host-pv annotation), making it impossible to bind a PVC to a pre-existing host PV by name.
Additionally, the PV syncer's translate function has the same pattern for storageClassName, causing SC name mismatches between PVs and PVCs on the host when toHost.storageClasses.enabled is false.
This fix replaces 5 instances of HostNameCluster() with mappings.VirtualToHostName() across 2 files, routing through the proper mappers which conditionally translate based on resource origin — host-synced resources retain their original names, vCluster-created resources still get translated normally.
resolves #3810
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vCluster unconditionally mangled volumeName and storageClassName when syncing PVCs to the host, even for PVs synced from the host with the vcluster.loft.sh/host-pv annotation. PVCs with explicit volumeName pointing to a pre-existing host PV now bind correctly.
What else do we need to know?
The PV mapper (pkg/mappings/resources/persistentvolumes.go:18-29) already had the correct annotation-aware logic — this fix simply ensures the PVC and PV syncers use it instead of bypassing it.
The fix also addresses the PV syncer's storageClassName translation (persistentvolumes/translate.go:20) to prevent SC name mismatches between PVs and PVCs on the host when toHost.storageClasses.enabled is false (mirror mapper scenario).
E2E Tests
Default Test Execution
The mandatory PR suite runs automatically. Only specify additional test suites below if needed.
Adding New Test Suites
When adding a new ginkgo test suite:
Additional test suites
Additional test suite(s) that will be executed before the mandatory PR suite: