fix for synced pvs and retain SC #1723
Conversation
✅ Deploy Preview for vcluster-docs canceled.Built without sensitive environment variables
|
|
Can do a review here (lmk if not necessary), could you write a brief description? |
|
@facchettos your tests are failing, can you fix them? |
|
@rohantmp @FabianKramm I know the tests are failing, didn't have time to do more before I left for my PTO :) |
facchettos
force-pushed
the
pv-fix
branch
3 times, most recently
from
7422401
to
ece23b9
Compare
|
@rohantmp make sense. please review it. |
|
@facchettos since the test only takes a couple of seconds, I feel like a separate job is too much. Can we include it in an existing test suite? |
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| if syncBack { |
There was a problem hiding this comment.
Why make a variable for this, can we not just put the condition here?
There was a problem hiding this comment.
I thought it was more readable this way, but I can change it if really needed
| } | ||
|
|
||
| pPvc := &corev1.PersistentVolumeClaim{} | ||
| err = s.physicalClient.Get(ctx, types.NamespacedName{Namespace: pObj.Spec.ClaimRef.Namespace, Name: pObj.Spec.ClaimRef.Name}, pPvc) |
There was a problem hiding this comment.
Shouldn't we validate here that the ClaimRef.Namespace actually matches the namespace we are working in as vCluster?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| @@ -124,6 +129,10 @@ func (s *persistentVolumeSyncer) translateUpdate(ctx context.Context, vPv *corev | |||
| } | |||
|
|
|||
| translatedStorageClassName := translateStorageClass(vPv.Spec.StorageClassName) | |||
| if !s.useFakePersistentVolumes { | |||
| translatedStorageClassName = vPv.Spec.StorageClassName | |||
There was a problem hiding this comment.
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
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? (use
resolves #<issue_number>if possible)resolves #1464
resolves eng-2701
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster would not make persistent volumes available when
retainpolicy was set on the storage class.What else do we need to know?
The PV syncing in the host->vcluster direction was not syncing at the right time because it was relying exclusively on claimrefs, but did not detect the claimref being deleted from the vcluster object, and thus was forcing it back onto it