-
Notifications
You must be signed in to change notification settings - Fork 57
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
create RS irrespective of PV mount status. #1724
Conversation
How this is tested:
ramen-dr-cluster-operator pod log:
|
return false, nil | ||
l.Info("PVC is not in use by ready pod, yet creating RS ...") | ||
} else { | ||
l.Info("PVC is use by ready pod, proceeding to create RS ...") | ||
} |
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.
I would use same log, just change the part describing the pvc:
PVC is use by ready pod, proceeding to create RS"
PVC is not used by ready pod, proceeding to create RS"
The "..." part is not helpful, so you can remove it while changing the logs. It can also be done in a preparation commit to keep one logic change for every commit.
Since we don't skip if inUseByReadyPod is false, we can use more natual check:
if inUseByReadyPod {
log for used pvc...
} else {
log for unused pvc ...
}
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.
I would just replace all lines from line 370 to 399 and replace it with this:
_, err := v.getRS(getReplicationSourceName(rsSpec.ProtectedPVC.Name), rsSpec.ProtectedPVC.Namespace)
if err != nil {
if !kerrors.IsNotFound(err) {
return false, err
}
}
return true, nil
d18d555
to
841d754
Compare
if err != nil { | ||
// Err looking up the RS, return it | ||
return false, err | ||
l.Info("ReplicationSource does not exist, creating it now. ") |
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.
If you want to log here, just say "ReplicationSource does not exist". If you say "creating it now" it implies that this function will create it. But this function does do that. So I would ignore that part.
4412732
to
b206162
Compare
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.
LGTM
return true, nil | ||
l.Info("Good to proceed with final sync.") | ||
|
||
return true, nil // Good to proceed - PVC is not in use, not mounted to node (or does not exist-should not happen) |
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.
The last thing is to remove this comment
Fix for [[2319824] [RDR] Replication via Volsync requires PVC to be mounted before PVC is synchronized](https://issues.redhat.com/browse/DFBUGS-580) * Create ReplicationSource irrespective of PV being mounted to running pod. * Renamed "validatePVCBeforeRS" as "validatePVCForFinalSync" Signed-off-by: pruthvitd <[email protected]> create RS irrespective of PV mount status. Fix for [[2319824] [RDR] Replication via Volsync requires PVC to be mounted before PVC is synchronized](https://issues.redhat.com/browse/DFBUGS-580) Create ReplicationSource irrespective of PV being mounted to running pod. Signed-off-by: pruthvitd <[email protected]>
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.
LGTM.
Thanks @pruthvitd
Fix for [2319824] [RDR] Replication via Volsync requires PVC to be mounted before PVC is synchronized
* Create ReplicationSource irrespective of PV being mounted to running pod.
* Renamed "validatePVCBeforeRS" as "validatePVCForFinalSync"