-
Notifications
You must be signed in to change notification settings - Fork 555
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
rbd: add check to getVolumeReplicationInfo #5078
Conversation
@yati1998 please fix the ci problem |
this commit adds a check to getVolumeReplicationInfo to include status not found error while getting the remote status. This helps the failover to be done even if remote site status is not found Signed-off-by: yati1998 <[email protected]>
15e53ab
to
a5f6d89
Compare
@nixpanic PTAL |
@@ -876,6 +876,10 @@ func (rs *ReplicationServer) GetVolumeReplicationInfo(ctx context.Context, | |||
if err != nil { | |||
log.ErrorLog(ctx, err.Error()) |
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.
Should this line be moved after the new check like in GetGlobalMirroringStatus()
or getLastSyncInfo()
cases?
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 agree that this isn't too nice, but the logging of errors in this function is already very inconsistent... It should be cleaned up, but that can be done in an other PR.
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.
done in #5079
@@ -876,6 +876,10 @@ func (rs *ReplicationServer) GetVolumeReplicationInfo(ctx context.Context, | |||
if err != nil { | |||
log.ErrorLog(ctx, err.Error()) |
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 agree that this isn't too nice, but the logging of errors in this function is already very inconsistent... It should be cleaned up, but that can be done in an other PR.
@Mergifyio rebase |
☑️ Nothing to do
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 4101b63 |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/k8s-e2e-external-storage/1.32 |
/test ci/centos/k8s-e2e-external-storage/1.31 |
/test ci/centos/mini-e2e/k8s-1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.32 |
/test ci/centos/mini-e2e-helm/k8s-1.31 |
/test ci/centos/mini-e2e/k8s-1.32 |
/test ci/centos/mini-e2e/k8s-1.31 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
this commit adds a check to getVolumeReplicationInfo to include status not found error while getting the remote status.
This helps the failover to be done even if remote site status is not found