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 <ndevos@ibm.com>

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
 }

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)
 	}
 

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

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