Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rbd: return error if last sync time not present #3489

Merged
merged 3 commits into from
Nov 3, 2022
Merged

rbd: return error if last sync time not present #3489

merged 3 commits into from
Nov 3, 2022

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Nov 1, 2022

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

Note:- Replacement for #3478 after discussion in csiaddons spec.

@Madhu-1 Madhu-1 added ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures labels Nov 1, 2022
@mergify mergify bot added the component/rbd Issues related to RBD label Nov 1, 2022
Madhu-1
Madhu-1 commented Nov 1, 2022
View reviewed changes
go.mod Outdated
@@ -27,7 +27,7 @@ require (
github.com/pkg/xattr v0.4.7
github.com/prometheus/client_golang v1.12.2
github.com/stretchr/testify v1.8.1
golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd
golang.org/x/crypto v0.1.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this got updated as part of go get

ShyamsundarR
ShyamsundarR reviewed Nov 1, 2022
View reviewed changes
Copy link
Contributor

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Nov 1, 2022

@ShyamsundarR PTAL at last commit. This check was missing, and cephcsi was returning an empty response.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Nov 1, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Nov 1, 2022

rebase

✅ Branch has been successfully rebased

@ShyamsundarR
Copy link
Contributor

@ShyamsundarR PTAL at last commit. This check was missing, and cephcsi was returning an empty response.

Thanks, I am only looking at overall structural correctness (hence not hitting the approved button). @yati1998 assuming you are taking a closer look?

nixpanic
nixpanic requested changes Nov 1, 2022
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, but (a nit) the formatting of the error messages should be nicer.

internal/rbd/replicationcontrollerserver.go Outdated
if description == "" {
return nil, nil
return nil, fmt.Errorf("%w empty description", ErrLastSyncTimeNotFound)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing : after %w?

internal/rbd/replicationcontrollerserver.go Outdated
}
splittedString := strings.SplitN(description, ",", 2)
if len(splittedString) == 1 {
return nil, nil
return nil, fmt.Errorf("%w no local snapshot timestamp", ErrLastSyncTimeNotFound)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, formatting of error messages matter

@Madhu-1 Madhu-1 requested a review from nixpanic November 2, 2022 07:38
nixpanic
nixpanic previously approved these changes Nov 2, 2022
View reviewed changes
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@Madhu-1
rbd: return error if last sync time not present
5504208 
As per the csiaddon spec last sync time is
required parameter in the GetVolumeReplicationInfo
if we are failed to parse the description, return
not found error message instead of nil
which is empty response

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
rebase: update csi-addons spec to latest
12f98b3 
csi-addons/spec#47
defines the error messages for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
rbd: check for empty lastSyncTime
9f7624f 
Sometime the json unmarshal might
get success and return empty time
stamp. add a check to make sure the
time is not zero always.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1 Madhu-1 requested a review from nixpanic November 2, 2022 13:50
@mergify mergify bot dismissed nixpanic’s stale review November 2, 2022 13:50

Pull request has been modified.

yati1998
yati1998 approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@yati1998 yati1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Nov 2, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

rebase

☑️ Nothing to do

  • any of:
    • #commits-behind>0 [:pushpin: rebase requirement]
    • -linear-history [:pushpin: rebase requirement]
  • -closed [:pushpin: rebase requirement]

@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Nov 2, 2022
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

/test ci/centos/upgrade-tests-rbd

@mergify mergify bot merged commit 3e1f602 into ceph:devel Nov 3, 2022
@Madhu-1 Madhu-1 mentioned this pull request Nov 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@yati1998 yati1998 yati1998 approved these changes

@Rakshith-R Rakshith-R Rakshith-R approved these changes

@ShyamsundarR ShyamsundarR Awaiting requested review from ShyamsundarR

Assignees
No one assigned
Labels
ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures component/rbd Issues related to RBD ok-to-test Label to trigger E2E tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Madhu-1 @ShyamsundarR @nixpanic @yati1998 @Rakshith-R