rbd: consider lastSyncTimeNotFound as image not syncing #5498
Conversation
Rakshith-R
added
ci/skip/e2e
|
|
||
| if req.GetForce() && st.Equal(*creationTime) && !syncInfo.IsSyncing() { | ||
| // consider the image is not syncing if either | ||
| // - the last sync time was not found or |
There was a problem hiding this comment.
please add a comment that when last sync time is not found for example when in split brain state etc. it will help us in future why we used this check.
There was a problem hiding this comment.
please add a comment that when last sync time is not found for example when in split brain state etc. it will help us in future why we used this check.
I've added example as when images on both peered clusters are in secondary state.
The actual check is to skip resync if the image is syncing.
The last sync time not found error can be considered equivalent to the image is not in syncing state.
There was a problem hiding this comment.
I've added example as when images on both peered clusters are in secondary state.
I think this is wrong, because if the images are secondary state on both cluster, both will be in up+unknown state (we have check for this even before this state). This is more of an error state when not in healthy secondary state. when both are in secondary state its not possible to resync as there is no primary image.
There was a problem hiding this comment.
I've added example as when images on both peered clusters are in secondary state.
I think this is wrong, because if the images are secondary state on both cluster, both will be in up+unknown state (we have check for this even before this state). This is more of an error state when not in healthy secondary state. when both are in secondary state its not possible to resync as there is no primary image.
resync with both images in secondary is a no-op AFAIK.
There was a problem hiding this comment.
I've added example as when images on both peered clusters are in secondary state.
I think this is wrong, because if the images are secondary state on both cluster, both will be in up+unknown state (we have check for this even before this state). This is more of an error state when not in healthy secondary state. when both are in secondary state its not possible to resync as there is no primary image.
The behavior of resync execution when both images are secondary is not changed by this pr.
There was a problem hiding this comment.
i think you are missing my point, my request is to add a comment not to change any code flow. For example that the last sync might be missing when image is in up+error or split brain state
| // consider the image is not syncing if either | ||
| // - the last sync time was not found or | ||
| // - the sync info indicates the image is not syncing | ||
| IsNotSyncing := (errors.Is(sErr, rbderrors.ErrLastSyncTimeNotFound) || !syncInfo.IsSyncing()) |
There was a problem hiding this comment.
| IsNotSyncing := (errors.Is(sErr, rbderrors.ErrLastSyncTimeNotFound) || !syncInfo.IsSyncing()) | |
| isNotSyncing := errors.Is(sErr, rbderrors.ErrLastSyncTimeNotFound) || !syncInfo.IsSyncing() |
|
please describe in the commit message and PR what kind of problem can be seen without this change |
This commit modifies code to consider image as not syncing when the lastSyncTime is not found. Currently replication's Resync call fails if the lastSyncTime of an image is not found. Instead, we should treat missing lastSyncTime as the image not being in Syncing state. Signed-off-by: Rakshith R <[email protected]>
|
Added a lot of comments at various steps PTAL |
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at fa9e254 |
3 participants
Describe what this PR does
This commit modifies code to consider image as not syncing when the lastSyncTime is not found.
Currently replication's Resync call fails if the lastSyncTime of an image is not found
when deciding if an image is in syncing state or not.
Instead, we should treat missing lastSyncTime as the image not being
in Syncing state.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)