Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix for synced pvs and retain SC #1723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix for synced pvs and retain SC #1723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9bf500d
fix for synced pvs and retain SC and add e2e
facchettos Apr 29, 2024
d9af0e5
improved the ppvc handling
facchettos May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions pkg/controllers/resources/persistentvolumeclaims/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ const (
)

func New(ctx *synccontext.RegisterContext) (syncer.Object, error) {
storageClassesEnabled := ctx.Config.Sync.ToHost.StorageClasses.Enabled
syncSCToHost := ctx.Config.Sync.ToHost.StorageClasses.Enabled
syncSCFromHost := ctx.Config.ControlPlane.Advanced.VirtualScheduler.Enabled || ctx.Config.Sync.FromHost.StorageClasses.Enabled == "true"
excludedAnnotations := []string{bindCompletedAnnotation, boundByControllerAnnotation, storageProvisionerAnnotation}
return &persistentVolumeClaimSyncer{
NamespacedTranslator: translator.NewNamespacedTranslator(ctx, "persistent-volume-claim", &corev1.PersistentVolumeClaim{}, excludedAnnotations...),

storageClassesEnabled: storageClassesEnabled,
syncSCToHost: syncSCToHost,
syncSCFromHost: syncSCFromHost,
schedulerEnabled: ctx.Config.ControlPlane.Advanced.VirtualScheduler.Enabled,
useFakePersistentVolumes: !ctx.Config.Sync.ToHost.PersistentVolumes.Enabled,
}, nil
Expand All @@ -49,7 +51,8 @@ func New(ctx *synccontext.RegisterContext) (syncer.Object, error) {
type persistentVolumeClaimSyncer struct {
translator.NamespacedTranslator

storageClassesEnabled bool
syncSCToHost bool
syncSCFromHost bool
schedulerEnabled bool
useFakePersistentVolumes bool
}
Expand Down
7 changes: 2 additions & 5 deletions pkg/controllers/resources/persistentvolumeclaims/translate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (s *persistentVolumeClaimSyncer) translateSelector(ctx *synccontext.SyncCon
}

// translate storage class if we manage those in vcluster
if s.storageClassesEnabled && storageClassName != "" {
if s.syncSCToHost && storageClassName != "" {
translated := translate.Default.PhysicalNameClusterScoped(storageClassName)
delete(vPvc.Annotations, deprecatedStorageClassAnnotation)
vPvc.Spec.StorageClassName = &translated
Expand All @@ -61,11 +61,8 @@ func (s *persistentVolumeClaimSyncer) translateSelector(ctx *synccontext.SyncCon
if vPvc.Spec.Selector != nil {
vPvc.Spec.Selector = translate.Default.TranslateLabelSelectorCluster(vPvc.Spec.Selector)
}
if vPvc.Spec.VolumeName != "" {
vPvc.Spec.VolumeName = translate.Default.PhysicalNameClusterScoped(vPvc.Spec.VolumeName)
}
// check if the storage class exists in the physical cluster
if !s.storageClassesEnabled && storageClassName != "" {
if !(s.syncSCToHost || s.syncSCFromHost) && storageClassName != "" {
// Should the PVC be dynamically provisioned or not?
if vPvc.Spec.Selector == nil && vPvc.Spec.VolumeName == "" {
err := ctx.PhysicalClient.Get(ctx.Context, types.NamespacedName{Name: storageClassName}, &storagev1.StorageClass{})
Expand Down
73 changes: 48 additions & 25 deletions pkg/controllers/resources/persistentvolumes/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ func NewSyncer(ctx *synccontext.RegisterContext) (syncertypes.Object, error) {
return &persistentVolumeSyncer{
Translator: translator.NewClusterTranslator(ctx, "persistentvolume", &corev1.PersistentVolume{}, NewPersistentVolumeTranslator(), HostClusterPersistentVolumeAnnotation),

virtualClient: ctx.VirtualManager.GetClient(),
virtualClient: ctx.VirtualManager.GetClient(),
physicalClient: ctx.PhysicalManager.GetClient(),
useFakePersistentVolumes: !ctx.Config.Sync.ToHost.PersistentVolumes.Enabled,
isMultiNS: ctx.Config.Experimental.MultiNamespaceMode.Enabled,
}, nil
}

Expand Down Expand Up @@ -66,7 +69,10 @@ func NewPersistentVolumeTranslator() translate.PhysicalNameTranslator {
type persistentVolumeSyncer struct {
translator.Translator

virtualClient client.Client
virtualClient client.Client
physicalClient client.Client
useFakePersistentVolumes bool
isMultiNS bool
}

var _ syncertypes.IndicesRegisterer = &persistentVolumeSyncer{}
Expand Down Expand Up @@ -142,7 +148,7 @@ func (s *persistentVolumeSyncer) Sync(ctx *synccontext.SyncContext, pObj client.
}

// check if the persistent volume should get synced
sync, vPvc, err := s.shouldSync(ctx.Context, pPersistentVolume)
sync, vPvc, pPvc, err := s.shouldSync(ctx.Context, pPersistentVolume)
if err != nil {
return ctrl.Result{}, err
} else if !sync {
Expand All @@ -151,17 +157,19 @@ func (s *persistentVolumeSyncer) Sync(ctx *synccontext.SyncContext, pObj client.
}

// check if there is a corresponding virtual pvc
updatedObj := s.translateUpdateBackwards(vPersistentVolume, pPersistentVolume, vPvc)
if updatedObj != nil {
ctx.Log.Infof("update virtual persistent volume %s, because spec has changed", vPersistentVolume.Name)
translator.PrintChanges(vPersistentVolume, updatedObj, ctx.Log)
err = ctx.VirtualClient.Update(ctx.Context, updatedObj)
if err != nil {
return ctrl.Result{}, err
}
if s.useFakePersistentVolumes || vPersistentVolume.Spec.ClaimRef != nil || pPersistentVolume.Spec.ClaimRef == nil || pPvc != nil {
updatedObj := s.translateUpdateBackwards(vPersistentVolume, pPersistentVolume, vPvc)
if updatedObj != nil {
ctx.Log.Infof("update virtual persistent volume %s, because spec has changed", vPersistentVolume.Name)
translator.PrintChanges(vPersistentVolume, updatedObj, ctx.Log)
err = ctx.VirtualClient.Update(ctx.Context, updatedObj)
if err != nil {
return ctrl.Result{}, err
}

// we will reconcile anyways
return ctrl.Result{}, nil
// we will reconcile anyways
return ctrl.Result{}, nil
}
}

// check status
Expand All @@ -180,7 +188,7 @@ func (s *persistentVolumeSyncer) Sync(ctx *synccontext.SyncContext, pObj client.
}

// update the physical persistent volume if the virtual has changed
if vPersistentVolume.Annotations == nil || vPersistentVolume.Annotations[HostClusterPersistentVolumeAnnotation] == "" {
if vPersistentVolume.Annotations == nil || vPersistentVolume.Annotations[HostClusterPersistentVolumeAnnotation] == "" || vPersistentVolume.Spec.ClaimRef == nil {
if vPersistentVolume.DeletionTimestamp != nil {
if pPersistentVolume.DeletionTimestamp != nil {
return ctrl.Result{}, nil
Expand Down Expand Up @@ -221,7 +229,7 @@ var _ syncertypes.ToVirtualSyncer = &persistentVolumeSyncer{}

func (s *persistentVolumeSyncer) SyncToVirtual(ctx *synccontext.SyncContext, pObj client.Object) (ctrl.Result, error) {
pPersistentVolume := pObj.(*corev1.PersistentVolume)
sync, vPvc, err := s.shouldSync(ctx.Context, pPersistentVolume)
sync, vPvc, _, err := s.shouldSync(ctx.Context, pPersistentVolume)
if err != nil {
return ctrl.Result{}, err
} else if translate.Default.IsManagedCluster(pObj) {
Expand All @@ -240,33 +248,48 @@ func (s *persistentVolumeSyncer) SyncToVirtual(ctx *synccontext.SyncContext, pOb
return ctrl.Result{}, nil
}

func (s *persistentVolumeSyncer) shouldSync(ctx context.Context, pObj *corev1.PersistentVolume) (bool, *corev1.PersistentVolumeClaim, error) {
func (s *persistentVolumeSyncer) shouldSync(ctx context.Context, pObj *corev1.PersistentVolume) (shouldSync bool, vpvc *corev1.PersistentVolumeClaim, ppvc *corev1.PersistentVolumeClaim, err error) {
// is there an assigned PVC?
if pObj.Spec.ClaimRef == nil {
if translate.Default.IsManagedCluster(pObj) {
return true, nil, nil
return true, nil, nil, nil
}

return false, nil, nil
return false, nil, nil, nil
}

vPvc := &corev1.PersistentVolumeClaim{}
err := clienthelper.GetByIndex(ctx, s.virtualClient, vPvc, constants.IndexByPhysicalName, pObj.Spec.ClaimRef.Namespace+"/"+pObj.Spec.ClaimRef.Name)
err = clienthelper.GetByIndex(ctx, s.virtualClient, vPvc, constants.IndexByPhysicalName, pObj.Spec.ClaimRef.Namespace+"/"+pObj.Spec.ClaimRef.Name)
if err != nil {
if !kerrors.IsNotFound(err) {
return false, nil, err
return false, nil, nil, err
} else if translate.Default.IsManagedCluster(pObj) {
return true, nil, nil
return true, nil, nil, nil
}

// this will only work if multi ns isn't enabled, because translator returns an error
// as it is not a valid call
if s.isMultiNS {
return true, nil, nil, nil
}
namespace, err := translate.Default.LegacyGetTargetNamespace()
if err != nil {
return false, nil, nil
return false, nil, nil, nil
}
return pObj.Spec.ClaimRef.Namespace == namespace && pObj.Spec.PersistentVolumeReclaimPolicy == corev1.PersistentVolumeReclaimRetain, nil, nil
return pObj.Spec.ClaimRef.Namespace == namespace && pObj.Spec.PersistentVolumeReclaimPolicy == corev1.PersistentVolumeReclaimRetain, nil, nil, nil
}

pPvc := &corev1.PersistentVolumeClaim{}
err = s.physicalClient.Get(ctx, types.NamespacedName{Namespace: pObj.Spec.ClaimRef.Namespace, Name: pObj.Spec.ClaimRef.Name}, pPvc)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we validate here that the ClaimRef.Namespace actually matches the namespace we are working in as vCluster?

Copy link
Member

Choose a reason for hiding this comment

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

e.g. this could cause problematic errors if we cannot list in that namespace, which would make everything fail as a non not found error is thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but isn't that what we want though? if the pv has a claimref with a pvc that we don't have access to, we don't own the pv

switch {
case kerrors.IsNotFound(err):
return true, vPvc, nil, nil
case err != nil, !translate.Default.IsManaged(pPvc):
// we don't own the physical pvc here so we don't sync
return false, vPvc, nil, nil
}

return true, vPvc, nil
return true, vPvc, pPvc, nil
}

func (s *persistentVolumeSyncer) IsManaged(ctx context.Context, pObj client.Object) (bool, error) {
Expand All @@ -275,7 +298,7 @@ func (s *persistentVolumeSyncer) IsManaged(ctx context.Context, pObj client.Obje
return false, nil
}

sync, _, err := s.shouldSync(ctx, pPv)
sync, _, _, err := s.shouldSync(ctx, pPv)
if err != nil {
return false, nil
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/controllers/resources/persistentvolumes/translate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ func (s *persistentVolumeSyncer) translateUpdate(ctx context.Context, vPv *corev
updated.Spec.Capacity = vPv.Spec.Capacity
}

if !s.useFakePersistentVolumes && vPv.Spec.ClaimRef == nil {
// if the virtual claimref is removed, we should also remove it in the
// updated
updated = translator.NewIfNil(updated, pPv)
updated.Spec.ClaimRef = nil
}

if !equality.Semantic.DeepEqual(pPv.Spec.AccessModes, vPv.Spec.AccessModes) {
updated = translator.NewIfNil(updated, pPv)
updated.Spec.AccessModes = vPv.Spec.AccessModes
Expand All @@ -124,6 +131,10 @@ func (s *persistentVolumeSyncer) translateUpdate(ctx context.Context, vPv *corev
}

translatedStorageClassName := translateStorageClass(vPv.Spec.StorageClassName)
if !s.useFakePersistentVolumes {
translatedStorageClassName = vPv.Spec.StorageClassName
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because if we use real persistent volumes, they need to reference the proper storage class, for some reasons the storage class was updated to something different than the one at creation time when the persistent volume was updated

}

if !equality.Semantic.DeepEqual(pPv.Spec.StorageClassName, translatedStorageClassName) {
updated = translator.NewIfNil(updated, pPv)
updated.Spec.StorageClassName = translatedStorageClassName
Expand Down
1 change: 1 addition & 0 deletions test/e2e_node/e2e_node_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
_ "github.com/loft-sh/vcluster/test/e2e/syncer/services"
_ "github.com/loft-sh/vcluster/test/e2e/webhook"
_ "github.com/loft-sh/vcluster/test/e2e_node/node"
_ "github.com/loft-sh/vcluster/test/e2e_node/pvcretain"
)

var (
Expand Down