From 4acffb5548d79936f37935be37fb6b42e5709c85 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Mon, 22 Jul 2024 16:59:07 +0200 Subject: [PATCH] rbd: make VolumeGroup Create/Delete/AddVolume/RemoveVolume idempotent Add extra error checking to make sure trying to create an existing volume group does not result in a failure. The same counts for deleting a non-existing volume group, and adding/removing volumes to/from the volume group. Signed-off-by: Niels de Vos --- internal/csi-addons/rbd/volumegroup.go | 13 ++++++------- internal/rbd/group/volume_group.go | 21 ++++++++++++++++----- internal/rbd/manager.go | 11 ++--------- internal/rbd/types/group.go | 2 +- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/internal/csi-addons/rbd/volumegroup.go b/internal/csi-addons/rbd/volumegroup.go index beda20d5b..9844c93c4 100644 --- a/internal/csi-addons/rbd/volumegroup.go +++ b/internal/csi-addons/rbd/volumegroup.go @@ -18,7 +18,6 @@ package rbd import ( "context" - "fmt" "github.com/ceph/ceph-csi/internal/rbd" "github.com/ceph/ceph-csi/internal/rbd/types" @@ -103,7 +102,7 @@ func (vs *VolumeGroupServer) CreateVolumeGroup( volumes[i] = vol } - log.DebugLog(ctx, fmt.Sprintf("all %d Volumes for VolumeGroup %q have been found", len(volumes), req.GetName())) + log.DebugLog(ctx, "all %d Volumes for VolumeGroup %q have been found", len(volumes), req.GetName()) // create a RBDVolumeGroup vg, err := mgr.CreateVolumeGroup(ctx, req.GetName()) @@ -115,7 +114,7 @@ func (vs *VolumeGroupServer) CreateVolumeGroup( err.Error()) } - log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q had been created", req.GetName())) + log.DebugLog(ctx, "VolumeGroup %q has been created: %+v", req.GetName(), vg) // add each rbd-image to the RBDVolumeGroup for _, vol := range volumes { @@ -130,7 +129,7 @@ func (vs *VolumeGroupServer) CreateVolumeGroup( } } - log.DebugLog(ctx, fmt.Sprintf("all %d Volumes have been added to for VolumeGroup %q", len(volumes), req.GetName())) + log.DebugLog(ctx, "all %d Volumes have been added to for VolumeGroup %q", len(volumes), req.GetName()) csiVG, err := vg.ToCSI(ctx) if err != nil { @@ -182,7 +181,7 @@ func (vs *VolumeGroupServer) DeleteVolumeGroup( } defer vg.Destroy(ctx) - log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q has been found", req.GetVolumeGroupId())) + log.DebugLog(ctx, "VolumeGroup %q has been found", req.GetVolumeGroupId()) // verify that the volume group is empty volumes, err := vg.ListVolumes(ctx) @@ -194,7 +193,7 @@ func (vs *VolumeGroupServer) DeleteVolumeGroup( err.Error()) } - log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q contains %d volumes", req.GetVolumeGroupId(), len(volumes))) + log.DebugLog(ctx, "VolumeGroup %q contains %d volumes", req.GetVolumeGroupId(), len(volumes)) if len(volumes) != 0 { return nil, status.Errorf( @@ -212,7 +211,7 @@ func (vs *VolumeGroupServer) DeleteVolumeGroup( err.Error()) } - log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q has been deleted", req.GetVolumeGroupId())) + log.DebugLog(ctx, "VolumeGroup %q has been deleted", req.GetVolumeGroupId()) return &volumegroup.DeleteVolumeGroupResponse{}, nil } diff --git a/internal/rbd/group/volume_group.go b/internal/rbd/group/volume_group.go index 294a05cf0..c41db02d0 100644 --- a/internal/rbd/group/volume_group.go +++ b/internal/rbd/group/volume_group.go @@ -132,7 +132,7 @@ func GetVolumeGroup( volumes = append(volumes, vol) } - return &volumeGroup{ + vg := &volumeGroup{ journal: j, credentials: creds, id: id, @@ -144,7 +144,11 @@ func GetVolumeGroup( volumes: volumes, // all allocated volumes need to be free'd at Destroy() time volumesToFree: volumes, - }, nil + } + + log.DebugLog(ctx, "GetVolumeGroup(%s) returns %+v", id, *vg) + + return vg, nil } // String makes it easy to include the volumeGroup object in log and error @@ -303,8 +307,12 @@ func (vg *volumeGroup) Destroy(ctx context.Context) { log.DebugLog(ctx, "destroyed volume group instance with id %q", vg.id) } -func (vg *volumeGroup) Create(ctx context.Context, name string) error { - // TODO: if the group already exists, resolve details and use that +func (vg *volumeGroup) Create(ctx context.Context) error { + name, err := vg.GetName(ctx) + if err != nil { + return fmt.Errorf("missing name to create volume group: %w", err) + } + ioctx, err := vg.GetIOContext(ctx) if err != nil { return err @@ -319,7 +327,6 @@ func (vg *volumeGroup) Create(ctx context.Context, name string) error { log.DebugLog(ctx, "ignoring error while creating volume group %q: %v", vg, err) } - vg.name = name log.DebugLog(ctx, "volume group %q has been created", vg) return nil @@ -401,6 +408,10 @@ func (vg *volumeGroup) RemoveVolume(ctx context.Context, vol types.Volume) error err := vol.RemoveFromGroup(ctx, vg) if err != nil { + if errors.Is(librbd.ErrNotExist, err) { + return nil + } + return fmt.Errorf("failed to remove volume %q from volume group %q: %w", vol, vg, err) } diff --git a/internal/rbd/manager.go b/internal/rbd/manager.go index e487b1c26..50e278ce6 100644 --- a/internal/rbd/manager.go +++ b/internal/rbd/manager.go @@ -237,16 +237,9 @@ func (mgr *rbdManager) CreateVolumeGroup(ctx context.Context, name string) (type } }() - // check if the volume group exists in the backend - existingName, err := vg.GetName(ctx) + err = vg.Create(ctx) if err != nil { - // the volume group does not exist yet - err = vg.Create(ctx, vgName) - if err != nil { - return nil, fmt.Errorf("failed to create volume group %q: %w", name, err) - } - } else if existingName != vgName { - return nil, fmt.Errorf("volume group id %q has a name mismatch, expected %q, not %q", name, vgName, existingName) + return nil, fmt.Errorf("failed to create volume group %q: %w", name, err) } return vg, nil diff --git a/internal/rbd/types/group.go b/internal/rbd/types/group.go index 322230952..83d0adea3 100644 --- a/internal/rbd/types/group.go +++ b/internal/rbd/types/group.go @@ -53,7 +53,7 @@ type VolumeGroup interface { ToCSI(ctx context.Context) (*volumegroup.VolumeGroup, error) // Create makes a new group in the backend storage. - Create(ctx context.Context, name string) error + Create(ctx context.Context) error // Delete removes the VolumeGroup from the backend storage. Delete(ctx context.Context) error