rbd: return error if last sync time not present #3478
Conversation
As per the csiaddon spec last sync time is required parameter in the GetVolumeReplicationInfo if we are failed to parse the description, return error message instead of nil which is empty response Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
|
Added DNM for now, as @yati1998 needs to check a few things. |
|
Thinking More about it, keeping the LastSyncTime as optional makes more sense than a generic CSI Driver as well. Update the spec to make it optional. |
|
csi-addons/spec#47 and csi-addons/kubernetes-csi-addons#260 are alternative for this one, @yati1998 PTAL |
The intention should not be to error (unless there is actually an error). For example if remote status is missing, then determining the lastSyncTime is not possible in this reconcile, and would be reported as epoch (or 0). If we error out because the volume is not synced yet or the remote is not reporting a sync time (and report the errors on the resource), cases where remote is down or due to failover is still Primary etc. will result in VolRep resource potentially reporting errors and not a healthy primary state and cause issues. Overall, I think we expect a lastSyncTime, and if we are unable to determine the same it is epoch OR prior lastSyncTime value for the resource, iff it is from the same generation as the current CR (which is applicable at the VR layer and not at the gRPC). IOW, what does the caller (VolRep reconciler in this case) do when this errors out is critical IMHO. |
|
Closing this one as per discussion and csi-addons/kubernetes-csi-addons#260 solves the problem |
As per the csiaddon spec, last sync time is a required parameter in the GetVolumeReplicationInfo if we are failed to parse the description, we return an error message instead of nil which is an empty response.
Signed-off-by: Madhu Rajanna madhupr007@gmail.com
Note:- skipping E2E as we dont have CI for DR