-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
Some cleanups and improvements in VolRep and VolSync code #1587
Conversation
24d5daf
to
c102a9e
Compare
internal/controller/vrg_volrep.go
Outdated
} | ||
|
||
func (v *VRGInstance) updatePVCLastSyncTime(pvcNamespace, pvcName string, lastSyncTime *metav1.Time) { | ||
protectedPVC := v.findProtectedPVC(pvcNamespace, pvcName) | ||
protectedPVC := FindProtectedPVC(v.instance, pvcNamespace, pvcName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like an improvement, I would keep the previous code.
Same for other instances of this change
internal/controller/vrg_volsync.go
Outdated
@@ -48,15 +48,12 @@ func (v *VRGInstance) restorePVsAndPVCsForVolSync() (int, error) { | |||
err = v.volSyncHandler.EnsurePVCfromRD(rdSpec, failoverAction) | |||
} | |||
|
|||
srcPVC := rdSpec.ProtectedPVC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a PVC, why call it PVC?
internal/controller/vrg_volsync.go
Outdated
rdSpec.ProtectedPVC.DeepCopyInto(protectedPVC) | ||
v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *protectedPVC) | ||
} | ||
protectedPVC := v.getOrCreateProtectedPVC(rdSpec.ProtectedPVC.Namespace, rdSpec.ProtectedPVC.Name, &srcPVC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this use case (2 instances, we can create another helper copying rdSpec.ProtectedPVC, like:
v.copyProtectedPVC(rdSpec.ProtectedPVC)
protectedPVC = &ramen.ProtectedPVC{Namespace: pvcNamespace, Name: pvcName} | ||
if srcPVC != nil { | ||
srcPVC.DeepCopyInto(protectedPVC) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper is too complicated, doing different unrelated things. I think the previous code was better, it was clear what the code is doing.
If we have repeated code we can eliminate them with small and simpler helpers that do one thing.
if protectedPVC == nil { | ||
protectedPVC = &ramendrv1alpha1.ProtectedPVC{Namespace: pvc.GetNamespace(), Name: pvc.GetName()} | ||
v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *protectedPVC) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding s simpler helper like
func (v *VRGInstance) addProtectedPVC(pvc *corev1.PersistentVolumeClaim)
*ramendrv1alpha1.ProtectedPVC {
protectedPVC = &ramendrv1alpha1.ProtectedPVC{
Namespace: pvc.GetNamespace(),
Name: pvc.GetName(),
}
v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *protectedPVC)
return &v.instance.Status.ProtectedPVCs[len(v.instance.Status.ProtectedPVCs)-1]
}
This make the code more clear:
protectedPVC := v.findProtectedPVC(...)
if protectedPVC == nil {
protectedPVC = v.addProctectedPVC(...)
}
v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *protectedPVC) | ||
} | ||
|
||
return protectedPVC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not the same object added to the slice. Change to this object will be missing from the protectedPVC int the slice.
c102a9e
to
c019ed0
Compare
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
c019ed0
to
e834132
Compare
No description provided.