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

fix empty volume group creation through cli tool #767

Merged
merged 1 commit into from
Feb 13, 2025
Merged

fix empty volume group creation through cli tool #767

merged 1 commit into from
Feb 13, 2025

Conversation

Nikhil-Ladha
Copy link
Contributor

when creating empty volume group through cli, the strings.Split() function returns a [""] value, i.e, a slice of length 1 having the value as 1 ([""]) empty string. As a result, the csi panics when it tries to find the volumeID with the respective value.

@Nikhil-Ladha Nikhil-Ladha force-pushed the fix-empty-group-creation branch from 088819c to 4409e17 Compare February 13, 2025 12:56
nixpanic
nixpanic approved these changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@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.

Thanks @Nikhil-Ladha, the change looks good to me.

Can you explain a little more about the csi panics? Is there a particular CSI driver that has a problem with an empty volume-id? If so, please report that as a bug against the driver too.

@Nikhil-Ladha
Copy link
Contributor Author

Can you explain a little more about the csi panics? Is there a particular CSI driver that has a problem with an empty volume-id? If so, please report that as a bug against the driver too.

The rbd ceph-csi driver panics when I try to create the VolumeGroup though CLI with an empty volumeid list.

The following is the error on CLI:

[nladha@dhcp53-204 ~]$ kubectl exec -it deployments/csi-rbdplugin-provisioner -c csi-addons -n rook-ceph -- csi-addons  --endpoint unix:///csi/csi-addons.sock --operation CreateVolumeGroup  -volumegroupname grp3 -parameters clusterID=rook-ceph,pool=replicapool -secret rook-ceph/rook-csi-rbd-provisioner
E0213 15:24:06.668477      13 volumegroup.go:75] Failed to create volume group: rpc error: code = Internal desc = panic runtime error: invalid memory address or nil pointer dereference
ERROR: failed to execute "CreateVolumeGroup": rpc error: code = Internal desc = panic runtime error: invalid memory address or nil pointer dereference
command terminated with exit code 1

And, the below is the csi-rbdplugin container logs:

I0213 15:24:06.656330       1 utils.go:266] ID: 24 GRPC call: /volumegroup.Controller/CreateVolumeGroup
I0213 15:24:06.659070       1 utils.go:267] ID: 24 GRPC request: {"name":"grp3","parameters":{"clusterID":"rook-ceph","pool":"replicapool"},"secrets":"***stripped***","volume_ids":[""]}
I0213 15:24:06.663248       1 volumegroup.go:98] ID: 24 Volume not found
E0213 15:24:06.663939       1 utils.go:335] panic occurred: runtime error: invalid memory address or nil pointer dereference
goroutine 157 [running]:
runtime/debug.Stack()
	/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.5.linux-amd64/src/runtime/debug/stack.go:26 +0x5e
runtime/debug.PrintStack()
	/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.5.linux-amd64/src/runtime/debug/stack.go:18 +0x13
github.com/ceph/ceph-csi/internal/csi-common.panicHandler.func1()
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:336 +0xca
panic({0x22ecac0?, 0x40086a0?})
	/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.5.linux-amd64/src/runtime/panic.go:785 +0x132
github.com/ceph/ceph-csi/internal/csi-addons/rbd.destoryVolumes(...)
	/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/replication.go:1006
github.com/ceph/ceph-csi/internal/csi-addons/rbd.(*VolumeGroupServer).CreateVolumeGroup(0x4017ff0?, {0x2b858d0, 0xc000461200}, 0xc00066c7e0)
	/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/volumegroup.go:99 +0x4b6
github.com/csi-addons/spec/lib/go/volumegroup._Controller_CreateVolumeGroup_Handler.func1({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?})
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/csi-addons/spec/lib/go/volumegroup/volumegroup_grpc.pb.go:162 +0xcb
github.com/ceph/ceph-csi/internal/csi-common.panicHandler({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?}, 0xc0005048c0?, 0x1e778ab?)
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:341 +0x71
github.com/ceph/ceph-csi/internal/csi-common.NewMiddlewareServerOption.ChainUnaryServer.func3.1({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?})
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/grpc-ecosystem/go-grpc-middleware/chain.go:48 +0x45
github.com/ceph/ceph-csi/internal/csi-common.logSlowGRPC(0x6fc23ac00, {0x2b858d0, 0xc000461200}, {0x2509780, 0xc00066c7e0}, 0xc0006202c0, 0xc000888880)
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:320 +0x1db
github.com/ceph/ceph-csi/internal/csi-common.NewMiddlewareServerOption.func1({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?}, 0xc000132cf0?, 0xc00084a6f0?)
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:134 +0x45
github.com/ceph/ceph-csi/internal/csi-common.NewMiddlewareServerOption.ChainUnaryServer.func3.1({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?})
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/grpc-ecosystem/go-grpc-middleware/chain.go:48 +0x45
github.com/ceph/ceph-csi/internal/csi-common.logGRPC({0x2b858d0, 0xc000461200}, {0x2509780, 0xc00066c7e0}, 0x2188e00?, 0xc0008888c0)
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:269 +0x165
github.com/ceph/ceph-csi/internal/csi-common.NewMiddlewareServerOption.ChainUnaryServer.func3.1({0x2b858d0?, 0xc000461200?}, {0x2509780?, 0xc00066c7e0?})
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/grpc-ecosystem/go-grpc-middleware/chain.go:48 +0x45
github.com/ceph/ceph-csi/internal/csi-common.contextIDInjector({0x2b858d0, 0xc0004607e0}, {0x2509780, 0xc00066c7e0}, 0x0?, 0xc000888900)
	/go/src/github.com/ceph/ceph-csi/internal/csi-common/utils.go:257 +0x132
github.com/ceph/ceph-csi/internal/csi-common.NewMiddlewareServerOption.ChainUnaryServer.func3({0x2b858d0, 0xc0004607e0}, {0x2509780, 0xc00066c7e0}, 0xc0006202c0, 0x80?)
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/grpc-ecosystem/go-grpc-middleware/chain.go:53 +0x123
github.com/csi-addons/spec/lib/go/volumegroup._Controller_CreateVolumeGroup_Handler({0x249c580, 0xc0000134a0}, {0x2b858d0, 0xc0004607e0}, 0xc000a28400, 0xc00082c990)
	/go/src/github.com/ceph/ceph-csi/vendor/github.com/csi-addons/spec/lib/go/volumegroup/volumegroup_grpc.pb.go:164 +0x143
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0000bb200, {0x2b858d0, 0xc000460750}, 0xc00066c780, 0xc00082cd20, 0x402e9a0, 0x0)
	/go/src/github.com/ceph/ceph-csi/vendor/google.golang.org/grpc/server.go:1400 +0x103b
google.golang.org/grpc.(*Server).handleStream(0xc0000bb200, {0x2b86568, 0xc00084c000}, 0xc00066c780)
	/go/src/github.com/ceph/ceph-csi/vendor/google.golang.org/grpc/server.go:1810 +0xbaa
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	/go/src/github.com/ceph/ceph-csi/vendor/google.golang.org/grpc/server.go:1030 +0x7f
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 156
	/go/src/github.com/ceph/ceph-csi/vendor/google.golang.org/grpc/server.go:1041 +0x125
E0213 15:24:06.665498       1 utils.go:271] ID: 24 GRPC error: rpc error: code = Internal desc = panic runtime error: invalid memory address or nil pointer dereference

Notice the volume_ids: [""] in the GRPC request. I don't think we need a fix in the csi driver per se, even if we do not sure how exactly we could safeguard a situation like this 🤔
The error is happening here

@Nikhil-Ladha
fix empty volume group creation through cli tool
db71f1b 
when creating empty volume group through cli, the strings.Split()
function returns a [""] value, i.e, a slice of length 1 having
the value as 1 `[""]` empty string. As a result, csi panics
when it tries to find the volumeID with the respective value.

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha Nikhil-Ladha force-pushed the fix-empty-group-creation branch from 4409e17 to db71f1b Compare February 13, 2025 17:04
@mergify mergify bot merged commit f8e615a into csi-addons:main Feb 13, 2025
15 checks passed
@nixpanic
Copy link
Collaborator

github.com/ceph/ceph-csi/internal/csi-addons/rbd.(*VolumeGroupServer).CreateVolumeGroup(0x4017ff0?, {0x2b858d0, 0xc000461200}, 0xc00066c7e0)
/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/volumegroup.go:99 +0x4b6

This code seems to return an error when the volume-id can not be found. I do not think there is a bug in the current Ceph-CSI implementation for this. You seem to have a different/modified version (line numbers do not match or exist (/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/replication.go:1006).

It should be safe to pass an empty volume-id to any Ceph-CSI gRPC procedures, and Ceph-CSI should definitely not crash. If you hit such a problem, please create an issue against Ceph-CSI for that. Thank!

@Nikhil-Ladha
Copy link
Contributor Author

Nikhil-Ladha commented Feb 19, 2025 •
edited
Loading

github.com/ceph/ceph-csi/internal/csi-addons/rbd.(*VolumeGroupServer).CreateVolumeGroup(0x4017ff0?, {0x2b858d0, 0xc000461200}, 0xc00066c7e0)
/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/volumegroup.go:99 +0x4b6

This code seems to return an error when the volume-id can not be found. I do not think there is a bug in the current Ceph-CSI implementation for this. You seem to have a different/modified version (line numbers do not match or exist (/go/src/github.com/ceph/ceph-csi/internal/csi-addons/rbd/replication.go:1006).

Yeah, I have few changes included for VGR testing.

It should be safe to pass an empty volume-id to any Ceph-CSI gRPC procedures, and Ceph-CSI should definitely not crash. If you hit such a problem, please create an issue against Ceph-CSI for that. Thank!

It is safe to pass an empty id, and with this change that is verified.

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

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@black-dragon74 black-dragon74 Awaiting requested review from black-dragon74

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

@yati1998 yati1998 Awaiting requested review from yati1998

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nikhil-Ladha @nixpanic @Madhu-1