rbd: don't delete volume/snapshot if metadata creation fails #201
Conversation
| @@ -115,6 +115,15 @@ func parseVolCreateRequest(req *csi.CreateVolumeRequest) (*rbdVolume, error) { | |||
| return rbdVol, nil | |||
| } | |||
|
|
|||
| func storeVolumeMetadata(vol *rbdVolume, cp util.CachePersister) error { | |||
| if err := cp.Create(vol.VolID, vol); err != nil { | |||
There was a problem hiding this comment.
any special handling if the err is already exist?
There was a problem hiding this comment.
Already taken care of :)
ceph-csi/pkg/util/k8scmcache.go
Lines 145 to 152 in 8223ae3
|
ha, no that nolint can help |
| @@ -136,6 +145,11 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol | |||
| // request | |||
| if exVol.VolSize >= req.GetCapacityRange().GetRequiredBytes() { | |||
| // existing volume is compatible with new request and should be reused. | |||
|
|
|||
| if err = storeVolumeMetadata(exVol, cs.MetadataStore); err != nil { | |||
There was a problem hiding this comment.
@gman0 I have a question here, as this exVol is retrieved from the metadataStore why we need to store it back?
There was a problem hiding this comment.
Well yes and no... rbdVolumes (and snapshots) is initialized from the metadata cache - but after that, it's populated by the CreateVolume function itself https://github.com/gman0/ceph-csi/blob/d9f71d3887f4eaed3cfa34de8bbd1686163d0a32/pkg/rbd/controllerserver.go#L178
There was a problem hiding this comment.
But my question is why we have store it back. I think we can update the local cache once we store the Metadata on configmap is successful.
By doing this we can avoid extra step of storing if volume is already present, isn't it?
There was a problem hiding this comment.
Oops, you're right! For whatever reason I've completely missed this bit https://github.com/gman0/ceph-csi/blob/d9f71d3887f4eaed3cfa34de8bbd1686163d0a32/pkg/rbd/controllerserver.go#L172-L176
which checks for the existence of the image too...nice catch!
There was a problem hiding this comment.
@gman0 even with the current design, suppose CSI pod restarts after creating volume in the backend and before storing metadata on config map, we may end up in having stale volumes in the backend. do we have a CLI command in cephfs to list the volumes (by checking at the backend we can assure that there won't be any stale volumes even if CSI restarts) (note: PVC create/delete may become a slow process)
There was a problem hiding this comment.
reading more through the code, the only issue we are having with restart (as explained above) is volumeID and snapshotID because we are generating new UUID for each request for the same volume we may generate two volume ID if restart happens.
#205 fixes the issue as of now. but as suggested by @ShyamsundarR in #135 (comment) will fix the issue in long run.
| @@ -311,6 +323,10 @@ func (cs *ControllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS | |||
| // check for the requested source volume id and already allocated source volume id | |||
| if exSnap, err := getRBDSnapshotByName(req.GetName()); err == nil { | |||
| if req.SourceVolumeId == exSnap.SourceVolumeID { | |||
| if err = storeSnapshotMetadata(exSnap, cs.MetadataStore); err != nil { | |||
There was a problem hiding this comment.
same here as well for snapshot
When
MetadataStore.Create()failed in eitherCreateVolumeorCreateSnapshot, the rbd volume/snapshot would get deleted and the whole creation process would start over (volume created successfully -> failed to store metadata for volume -> delete volume -> create volume again -> try to store metadata again -> ...). This is quite expensive error handling.This PR relies on idempotency of both creation of rbd volumes/snapshots and metadata. If the volume/snapshot was created successfully but metadata storage failed, reuse the volume/snapshot and in next attempt only try to store metadata:
CreateVolumefirst attempt:CreateVolumesecond attempt:The same goes for snapshots.