Closed asn1809 closed 4 months ago
Needs envtests to the added. This section hase some tests around this functionality that either is not validating for the current behavior or does not test the same and may need modifications and addition appropriately.
pvcStatusDeleteIfPresent
seems to be doing the same when invoked for a PVC that should no longer be protected. Can you check if that function is not doing the required operation correctly?
Also, instead of clearing the PVCs from the protectedPVCs list at the end of pvcsDeselectedUnprotect
it would be easier to do it how (or what seems to be intended) per PVC that is unprotected in pvcStatusDeleteIfPresent
@ShyamsundarR @Annaraya-Narasagond : In function 'pvcsDeselectedUnprotect' , PVC list is retrieved by 'listPVCsOwnedByVrg' method. Can we consider PVCs not having owner labels for 'pvcsOwned' list here? In the subsequent for loop, PVCs currently filtered out by recipe selector and still present in 'protectedPVC' list of vrg are cleaned up both from S3 store and vrg status as well.
pvcStatusDeleteIfPresent
seems to be doing the same when invoked for a PVC that should no longer be protected. Can you check if that function is not doing the required operation correctly?Also, instead of clearing the PVCs from the protectedPVCs list at the end of
pvcsDeselectedUnprotect
it would be easier to do it how (or what seems to be intended) per PVC that is unprotected inpvcStatusDeleteIfPresent
Responding to this after discussing with @Annaraya-Narasagond for PVCs that were not Bound they do not have the owner label, but are in the protectedPVCs list and hence need garbage collection.
@ShyamsundarR @Annaraya-Narasagond : In function 'pvcsDeselectedUnprotect' , PVC list is retrieved by 'listPVCsOwnedByVrg' method. Can we consider PVCs not having owner labels for 'pvcsOwned' list here? In the subsequent for loop, PVCs currently filtered out by recipe selector and still present in 'protectedPVC' list of vrg are cleaned up both from S3 store and vrg status as well.
This maybe an overkill as we would be listing and processing all PVCs in said namespace. It does resolve the issue, but increases the scope of processing that we can avoid.
Needs envtests to the added. This section hase some tests around this functionality that either is not validating for the current behavior or does not test the same and may need modifications and addition appropriately.
@asn1809 Assume envtests are still pending as part of this PR. Current changes LGTM!
This helps in solving https://github.com/RamenDR/ramen/issues/1335