fix when rbdVol is nil, panic occurred #4002

huangzynn · 2023-07-18T11:09:35Z

When rbdVol is nil, panic occurred
add if rbdVol != nil

nixpanic · 2023-07-18T11:37:45Z

Do you have steps to reproduce this problem? It would be good to have a test case, or at least be able to validate the fix.

nixpanic · 2023-07-18T11:38:50Z

internal/rbd/controllerserver.go

@@ -1078,7 +1078,9 @@ func (cs *ControllerServer) CreateSnapshot(

 	// Fetch source volume information
 	rbdVol, err := GenVolFromVolID(ctx, req.GetSourceVolumeId(), cr, req.GetSecrets())
-	defer rbdVol.Destroy()
+	if rbdVol != nil {
+		defer rbdVol.Destroy()


this could be moved after if err != nil, no need to check if rbdVol != nil in that case.

Rakshith-R

Thanks for the contribution,
Please fix your commit message by referring to https://github.com/ceph/ceph-csi/blob/b1d1d3bf75b8becb427ec494dd6449f7b5bce115/docs/development-guide.md#commit-messages .

huangzynn · 2023-07-19T07:18:32Z

Such as:

nixpanic · 2023-07-19T08:07:50Z

Hi @huangzynn , in order to fix the commitlint issue, please make these changes:

squash the commits into one
change the subject of the commit to rbd: move CreateSnapshot defer rbdVol.Destroy() after err != nil
force push the single commit to your GitHub branch

Many thanks!

nixpanic · 2023-07-19T08:30:40Z

Do you have steps to reproduce this problem? It would be good to have a test case, or at least be able to validate the fix.

So, by reading the screenshots, is it correct to say that the problem can be triggered when creating a VolumeSnapshot of a non-existing PVC?

huangzynn · 2023-07-19T08:34:45Z

Do you have steps to reproduce this problem? It would be good to have a test case, or at least be able to validate the fix.

So, by reading the screenshots, is it correct to say that the problem can be triggered when creating a VolumeSnapshot of a non-existing PVC?
yes

Madhu-1 · 2023-07-19T08:35:01Z

internal/rbd/controllerserver.go

@@ -1092,6 +1091,7 @@ func (cs *ControllerServer) CreateSnapshot(

 		return nil, err
 	}
+	defer rbdVol.Destroy()


This can cause a cleanup problem as rbdVol is returned even in case of error. IMO moving this to line 1081 with a check for nil is okay.

That's what I thought from the beginning

rbdVol should never be returned in a case of an error. Either err != nil or rbdVol != nil. The additional if-statement should be useless, and in effect is the same as having the defer below the error handling. It adds a bit of complexity, and makes me wonder if rbdVol == nil should not be handled further down in the function as well?

Madhu-1 · 2023-07-19T08:50:47Z

@huangzynn as suggested here https://github.com/ceph/ceph-csi/blob/b1d1d3bf75b8becb427ec494dd6449f7b5bce115/docs/development-guide.md#commit-messages you need to update the commit to fix the commitlint CI problem. @nixpanic posted a sample for you above. you are missing rbd: prefix in commit message. job output here https://github.com/ceph/ceph-csi/actions/runs/5596968399/jobs/10234777959?pr=4002

add if rbdVol != nil defer rbdVol.Destroy() Signed-off-by: huangzy <[email protected]>

nixpanic · 2023-07-19T11:00:54Z

It seems not trivial to reproduce this, when I try, I get the following

Status:
  Error:
    Message:     Failed to create snapshot content with error snapshot controller failed to update invalid-pvc-pr-4002 on API server: cannot get claim from snapshot
    Time:        2023-07-19T10:55:43Z
  Ready To Use:  false
Events:
  Type     Reason                         Age    From                 Message
  ----     ------                         ----   ----                 -------
  Warning  SnapshotContentCreationFailed  2m39s  snapshot-controller  Failed to create snapshot content with error snapshot controller failed to update invalid-pvc-pr-4002 on API server: cannot get claim from snapshot

Maybe the PCV needs to exist to pass the snapshot-controller check, and get deleted before Ceph-CSI acts on it?

huangzynn · 2023-07-20T02:39:31Z

It seems not trivial to reproduce this, when I try, I get the following

Status:
  Error:
    Message:     Failed to create snapshot content with error snapshot controller failed to update invalid-pvc-pr-4002 on API server: cannot get claim from snapshot
    Time:        2023-07-19T10:55:43Z
  Ready To Use:  false
Events:
  Type     Reason                         Age    From                 Message
  ----     ------                         ----   ----                 -------
  Warning  SnapshotContentCreationFailed  2m39s  snapshot-controller  Failed to create snapshot content with error snapshot controller failed to update invalid-pvc-pr-4002 on API server: cannot get claim from snapshot

Maybe the PCV needs to exist to pass the snapshot-controller check, and get deleted before Ceph-CSI acts on it?

What is your snapshot controller version？
My snapshot-controller is v6.2.2

nixpanic · 2023-07-20T13:37:55Z

My snapshot-controller is v6.2.2

My cluster has v6.2.1... not sure if there is a big difference between those.

humblec · 2023-07-20T17:40:05Z

@huangzynn can you please provide the below details from this cluster.

The exact yaml of volume snapshot object and #kubectl get pvc output from the namespace
The snapshot controller and external snapshotter version
Kubernetes cluster and Ceph CSI driver version

huangzynn · 2023-07-21T01:40:25Z

kubectl get pvc

csi-snapshotter:v6.2.2
snapshot-controller:v6.2.2
cephcsi:v3.6.2
Kubernetes:v1.21.5

github-actions · 2023-08-20T21:01:14Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

github-actions · 2023-09-04T21:01:18Z

This pull request has been automatically closed due to inactivity. Please re-open if these changes are still required.

mergify bot added the bug Something isn't working label Jul 18, 2023

nixpanic reviewed Jul 18, 2023

View reviewed changes

Rakshith-R requested changes Jul 19, 2023

View reviewed changes

nixpanic added the component/rbd Issues related to RBD label Jul 19, 2023

huangzynn force-pushed the devel branch from bc77aa3 to 5f84a22 Compare July 19, 2023 07:15

Madhu-1 reviewed Jul 19, 2023

View reviewed changes

huangzynn force-pushed the devel branch 2 times, most recently from 4bfae38 to f61add6 Compare July 19, 2023 08:43

rbd: fix CreateSnapshot panic

8a57f3d

add if rbdVol != nil defer rbdVol.Destroy() Signed-off-by: huangzy <[email protected]>

huangzynn force-pushed the devel branch from f61add6 to 8a57f3d Compare July 19, 2023 09:29

github-actions bot added the stale label Aug 20, 2023

Madhu-1 mentioned this pull request Sep 4, 2023

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

Merged

github-actions bot closed this Sep 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix when rbdVol is nil, panic occurred #4002

fix when rbdVol is nil, panic occurred #4002

huangzynn commented Jul 18, 2023

nixpanic commented Jul 18, 2023

nixpanic Jul 18, 2023

Rakshith-R left a comment

huangzynn commented Jul 19, 2023

nixpanic commented Jul 19, 2023

nixpanic commented Jul 19, 2023

huangzynn commented Jul 19, 2023

Madhu-1 Jul 19, 2023

huangzynn Jul 19, 2023

nixpanic Jul 19, 2023

Madhu-1 commented Jul 19, 2023

nixpanic commented Jul 19, 2023

huangzynn commented Jul 20, 2023 •

edited

Loading

nixpanic commented Jul 20, 2023

humblec commented Jul 20, 2023 •

edited

Loading

huangzynn commented Jul 21, 2023

github-actions bot commented Aug 20, 2023

github-actions bot commented Sep 4, 2023

fix when rbdVol is nil, panic occurred #4002

fix when rbdVol is nil, panic occurred #4002

Conversation

huangzynn commented Jul 18, 2023

nixpanic commented Jul 18, 2023

nixpanic Jul 18, 2023

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

huangzynn commented Jul 19, 2023

nixpanic commented Jul 19, 2023

nixpanic commented Jul 19, 2023

huangzynn commented Jul 19, 2023

Madhu-1 Jul 19, 2023

Choose a reason for hiding this comment

huangzynn Jul 19, 2023

Choose a reason for hiding this comment

nixpanic Jul 19, 2023

Choose a reason for hiding this comment

Madhu-1 commented Jul 19, 2023

nixpanic commented Jul 19, 2023

huangzynn commented Jul 20, 2023 • edited Loading

nixpanic commented Jul 20, 2023

humblec commented Jul 20, 2023 • edited Loading

huangzynn commented Jul 21, 2023

github-actions bot commented Aug 20, 2023

github-actions bot commented Sep 4, 2023

huangzynn commented Jul 20, 2023 •

edited

Loading

humblec commented Jul 20, 2023 •

edited

Loading