Initial blueprint changes for preserving VRG as Secondary always #1652
Conversation
- Start by not needing to delete VRG as secondary from DRPC reconciler - For VR based VRG reconciliation ensure PVC is released when requested state is Secondary - Some trivial rename of functions to handle as secondary than as secondary for volsync - Adjust test cases to expect a secondary manifest work to be always present There is more work to do in terms of cleanup and refactoring, but the bulk of changes to address the need should be present with this commit. NOTE: Posted for reviews, refactor of the commit into more logical units is possible as work progresses Signed-off-by: Shyamsundar Ranganathan <[email protected]>
There was a problem hiding this comment.
Looks good to me. In regards to progression changes. We talked about it, but I want to capture it here. Instead of EnsuringVolSyncSetup, it might be clearer to use EnsuringSecondarySetup. Similarly, CleaningUp could be more descriptive, something like; MovingOldPrimaryToSecondary. Alternatively, you could consider combining both progressions into a single one. Something like; PreparingSecondary.
Signed-off-by: Shyamsundar Ranganathan <[email protected]>
…condary Signed-off-by: Shyamsundar Ranganathan <[email protected]>
|
Addressed the function split. Also add one more commit to remove PVCs form protectedPVCs once processing as secondary is complete. Additionally addressed some of the Unused messages, to be a little more ambiguous at present. Running mixed mode workload tests and will report on it here. |
There was a problem hiding this comment.
I added two additional comments regarding potential issues with deleting the ProtectedPVCs. However, any leftover ProtectedPVC will only result in a display issue and won’t cause any harm, even if the secondary becomes the primary again. I think this is fine.
| if !containsString(pvc.Finalizers, PvcVRFinalizerProtected) { | ||
| log.Info("pvc does not contain VR protection finalizer. Skipping it") | ||
|
|
||
| v.pvcStatusDeleteIfPresent(pvc.Namespace, pvc.Name, log) |
There was a problem hiding this comment.
Isn't this call too late for the protectedPVC to be deleted? You have it on line 177, which should cover it; unless we successfully remove the finalizer from the PVC but fail to update the VRG. In that scenario, the PVC will be gone, and we would never reach this point because volRepPVCs most likely wouldn’t have it.
There was a problem hiding this comment.
True on both comments, I added it in 2 places to be sure, but the PVC disappears quite fast, and also VRG updates are not a gurantee. Saw this with env tests as well in one case.
Overall we need a better place the garbage collect the protected PVC. Currently as it exists it is not added(appended) again etc. but in cases where a PVC is deleted (which is not yet supported) we would need to remove it from the ProtectedPVCs as well.
IOW, this status delete may work at times, but does not impact the functionality at present. Planning to deal with this subsequently.
| continue | ||
| } | ||
|
|
||
| v.pvcStatusDeleteIfPresent(pvc.Namespace, pvc.Name, log) |
There was a problem hiding this comment.
This is a good place to delete the ProtectedPVC for this PVC. However, reaching this point indicates that line 171 was successful and the PV/PVC were updated, but there is still a possibility that the VRG update might fail. See the comment above for more context.
Done with the tests, ran a sequence of deploy->enableDR->failover->relocate->failover->failover->relocate->relocate->disable->(re)enableDR->relocate->disable->undeploy for a mixed mode app and tests passed. We can merge this in. |
There is more work to do in terms of cleanup and refactoring, but the bulk of changes to address the need should be present with this commit.
NOTE: Posted for reviews, refactor of the commit into more logical units is possible as work progresses