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

fixed csi-rbdplugin crashes when decoding volume ID(CSI identifier) failed #4099

Merged
merged 1 commit into from
Sep 5, 2023
Merged

fixed csi-rbdplugin crashes when decoding volume ID(CSI identifier) failed #4099

merged 1 commit into from
Sep 5, 2023

Conversation

crazytaxii
Copy link
Contributor

@crazytaxii crazytaxii commented Sep 4, 2023 •
edited by nixpanic
Loading

Describe what this PR does

csi-rbdplugin will crash when decoding volume ID failed due to nil pointer

Related issues

#4098
Closes: #4002

@mergify mergify bot added the bug Something isn't working label Sep 4, 2023
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Sep 4, 2023

Duplicate of #4002

@Madhu-1 Madhu-1 marked this as a duplicate of #4002 Sep 4, 2023
@crazytaxii
Copy link
Contributor Author

Duplicate of #4002

I think my solution is better.

@nixpanic nixpanic added the component/rbd Issues related to RBD label Sep 4, 2023
nixpanic
nixpanic previously approved these changes Sep 4, 2023
View reviewed changes
@nixpanic nixpanic requested a review from a team September 4, 2023 12:38
Madhu-1
Madhu-1 reviewed Sep 4, 2023
View reviewed changes
internal/rbd/controllerserver.go Outdated
if err != nil {
return cs.checkErrAndUndoReserve(ctx, err, volumeID, rbdVol, cr)
}
defer rbdVol.Destroy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that this case lead to the memory leak problem as rbdVol is returned even in case of error from GenVolFromVolID function

Copy link
Member

Choose a reason for hiding this comment

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

If there is an error, GenVolFromVolID() should not return a connected rbdVol... If that is the case, GenVolFromVolID() should be corrected too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that this case lead to the memory leak problem as rbdVol is returned even in case of error from GenVolFromVolID function

Oh I see, maybe GenVolFromVolID funtion returns error though, the connection has established.

@mergify mergify bot dismissed nixpanic’s stale review September 4, 2023 15:08

Pull request has been modified.

karthik-us
karthik-us approved these changes Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@karthik-us karthik-us left a comment

Choose a reason for hiding this comment

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

LGTM.

@riya-singhal31
Copy link
Contributor

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 5, 2023 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

Pull request #4099 has been dequeued due to failing checks or checks timeout.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@nixpanic
Copy link
Member

nixpanic commented Sep 5, 2023

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 5, 2023

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Sep 5, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Sep 5, 2023
@nixpanic
Copy link
Member

nixpanic commented Sep 5, 2023

/retest ci/centos/mini-e2e-helm/k8s-1.27

@nixpanic
Copy link
Member

nixpanic commented Sep 5, 2023

/retest ci/centos/upgrade-tests-rbd

@mergify mergify bot merged commit 80ad5b6 into ceph:devel Sep 5, 2023
24 checks passed
This was referenced Sep 5, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 left review comments

@riya-singhal31 riya-singhal31 riya-singhal31 approved these changes

@karthik-us karthik-us karthik-us approved these changes

@nixpanic nixpanic nixpanic approved these changes

Assignees
No one assigned
Labels
bug Something isn't working component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@crazytaxii @Madhu-1 @riya-singhal31 @nixpanic @ceph-csi-bot @karthik-us