replication: reduce the reqeue time for GetReplicationInfo #263
Conversation
mergify
bot
requested review from
nixpanic,
Rakshith-R,
yati1998 and
Yuggupta27
| // Reduce the schedule time by half to get the latest update and also to | ||
| // avoid the inconsistency between the last sync time in the VR and the | ||
| // Storage system. | ||
| return scheduleTime / 2 |
There was a problem hiding this comment.
Shouldn't this be done by the entity creating VRC in the first place?
I don't think it is logical to to add this logic at this level.
There was a problem hiding this comment.
Please explain why you dont think this logic should not be added here. Please check the commit message for the proper explanation of why this is done.
There was a problem hiding this comment.
Reduce the schedule time by half to get the latest update and also to avoid the inconsistency between the last sync time in
the VR and the Storage system.
SchedulingTime from the VRC implies the time interval between which we will schedule a re-queue to check for updates right ?
Can the reducing the time by half done at a above level ?
The user can see updates for RPO that are not stuck in a bad schedule race, i.e, VolumeReplication checks and finds sync time as t-5m, and just after that storage system updates it to t+x. If we checked every 1/2 of the schedule we will update it to t+x in t+s/2
By checking 1/2 of the schedule, we are not obeying the provided scheduling time at all?
Is there a reason that the entity handling this cannot create VRC with half of the required shedule?
There was a problem hiding this comment.
SchedulingTime from the VRC implies the time interval between which we will schedule a re-queue to check for updates right ?
Can the reducing the time by half done at a above level ?
This is not true, This is the time specific to the storage, what is the interval to take the snapshot of the rbd image(for example ceph). we are here using the same time to Requeue to check the LastSyncTime/Update time of the replication.
By checking 1/2 of the schedule, we are not obeying the provided scheduling time at all?
Is there a reason that the entity handling this cannot create VRC with half of the required schedule?
Please Check the above explanation, the schedule interval is not meant for GetReplicationInfo Requeue time it's for the Storage Replication interval time, we are using the same time to get the updates on the replication time. if we keep both the same, there is a kind of diff between the VR LastSyncTime and the storage LastSyncTime. to keep both the same, we are decreasing the time by half so that we can keep the LastSyncTime and the storageLastSyncTime to almost the same.
There was a problem hiding this comment.
okay,
https://github.com/csi-addons/kubernetes-csi-addons/blob/main/docs/volumereplicationclass.md
Can we add description here and how it used in parameters section passed to csi-driver
And for requeue time of reconcile
and mention it in function description too .
Madhu-1
force-pushed
the
improve-lastUpdateTime
branch
from
7cb01f6 to
3342712
Compare
Madhu-1
force-pushed
the
improve-lastUpdateTime
branch
from
3342712 to
5d909ac
Compare
Madhu-1
force-pushed
the
improve-lastUpdateTime
branch
from
5d909ac to
76add31
Compare
Madhu-1
force-pushed
the
improve-lastUpdateTime
branch
from
76add31 to
b176a51
Compare
docs/volumereplicationclass.md
Outdated
| @@ -21,5 +21,8 @@ spec: | |||
| parameters: | |||
| replication.storage.openshift.io/replication-secret-name: secret-name | |||
| replication.storage.openshift.io/replication-secret-namespace: secret-namespace | |||
| # This is storage vendor specific configuration. if present, half of the schedulingInterval | |||
There was a problem hiding this comment.
(nits)
- VolumeReplication spell
- (reword?)
Will be used to make the requeue of the GetVolumeReplication Info to update the LastSyncTime in the VolumeReplicaiton CRtowill be used to requeue VolumeReplication resource for lastSyncTime info updates- I would actually not state this at all... Just state something like: schedulingInterval is a vendor specific parameter. It is used to set the replication scheduling interval for storage volumes that are replication enabled using related VolumeReplication resource
Reduce the schedule time by half to get the latest update and also to avoid the inconsistency between the last sync time in the VR and the Storage system. The user can see updates for RPO that are not stuck in a bad schedule race i.e VR checks and finds sync time as t-5m and just after that storage system updates it to t+x. If we checked every 1/2 of schedule we will update it to t+x in t+s/2 Signed-off-by: Madhu Rajanna <[email protected]>
Madhu-1
force-pushed
the
improve-lastUpdateTime
branch
from
b176a51 to
54dd513
Compare
| @@ -21,5 +21,8 @@ spec: | |||
| parameters: | |||
| replication.storage.openshift.io/replication-secret-name: secret-name | |||
| replication.storage.openshift.io/replication-secret-namespace: secret-namespace | |||
| # schedulingInterval is a vendor specific parameter. It is used to set the | |||
| # replication scheduling interval for storage volumes that are replication | |||
| # enabled using related VolumeReplication resource | |||
There was a problem hiding this comment.
Add a line about re queueing of VR if specified too?
Reduce the schedule time by half to get the latest update and also to avoid the inconsistency between the last sync time in
the VR and the Storage system.
The user can see updates for RPO that are not stuck in a bad schedule race, i.e, VolumeReplication checks and finds sync time as t-5m, and just after that storage system updates it to t+x. If we checked every 1/2 of the schedule we will update it to t+x in t+s/2
Signed-off-by: Madhu Rajanna [email protected]