go-ceph
go-ceph copied to clipboard
ceph
Method snapshot.Set has side-effects on Image state
The Set method on Snapshot in rbd calls rbd_snap_set on the Image linked to the snapshot. This has the effect of (quietly) changing the state of the ceph image handle. This is poor api design as the method is connected to the "child" object, the one not being changed, rather than object being effected, the Image.
A better approach would be to add something like SetSnapshot(...) to the *Image, making it clear the call effects the image.
@nixpanic @ansiwen @agarwal-mudit anyone interested in taking this up? I still think we ought to do this as well as other cleanups around snapshots in rbd and then deprecate the (old) snapshot type. Might be good for v0.6.0?
Certainly, I'd love to see someone take this up.
Let me know if you would like to have a more detailed discussion about the design aspects. We can take that up here, or use the github discussions feature, or you can often find me on the ceph devel irc (see our readme for details) during 9am-5pm US/Eastern.
Otherwise feel free to jump right in with a PR. If you want more incremental feedback feel welcome to start a PR in the Draft phase. I like that approach to getting feedback as you go.
@phlogistonjohn How would you solve it?
In my opinion, I think how to transfer the Set API to the rbd package.
@phlogistonjohn How would you solve it?
I would start by adding a new method to the Image. Something like func (image *Image) SetSnapshot(name string) error. Then I would replace the current Set() call on Snapshot to call snapshot.image.SetSnapshot(...). And mark Set() as deprecated.
We'd also need a Name() function as the name of the snapshot in the Snapshot type is private.
One thing I would consider, but I don't have a firm opinion on (yet) would be to have a SnapName type or not. Something like type SnapName string and work with that type rather than strings directly. It would make it more "type safe" but I am not sure its worth it yet.
In my opinion, I think how to transfer the Set API to the rbd package.
Once again, thanks for looking into this. I look forward to seeing where you go with it.
