From 6dec5e0e3b3e3fe1ab5ccb9e0469d4ad06fd6cf2 Mon Sep 17 00:00:00 2001
From: Niels de Vos <ndevos@ibm.com>
Date: Mon, 22 Jul 2024 15:02:20 +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 <ndevos@ibm.com>
---
 internal/csi-addons/rbd/volumegroup.go |  2 +-
 internal/rbd/group/volume_group.go     | 27 +++++++++++++++++++++-----
 internal/rbd/manager.go                | 11 ++---------
 internal/rbd/types/group.go            |  2 +-
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/internal/csi-addons/rbd/volumegroup.go b/internal/csi-addons/rbd/volumegroup.go
index beda20d5bf1..969f2301036 100644
--- a/internal/csi-addons/rbd/volumegroup.go
+++ b/internal/csi-addons/rbd/volumegroup.go
@@ -115,7 +115,7 @@ func (vs *VolumeGroupServer) CreateVolumeGroup(
 			err.Error())
 	}
 
-	log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q had been created", req.GetName()))
+	log.DebugLog(ctx, fmt.Sprintf("VolumeGroup %q had been created: %+v", req.GetName(), vg))
 
 	// add each rbd-image to the RBDVolumeGroup
 	for _, vol := range volumes {
diff --git a/internal/rbd/group/volume_group.go b/internal/rbd/group/volume_group.go
index f8404e37888..b3696f597c9 100644
--- a/internal/rbd/group/volume_group.go
+++ b/internal/rbd/group/volume_group.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/ceph/go-ceph/rados"
 	librbd "github.com/ceph/go-ceph/rbd"
@@ -131,7 +132,7 @@ func GetVolumeGroup(
 		volumes = append(volumes, vol)
 	}
 
-	return &volumeGroup{
+	vg := &volumeGroup{
 		journal:     j,
 		credentials: creds,
 		id:          id,
@@ -143,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
@@ -302,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
@@ -314,7 +323,6 @@ func (vg *volumeGroup) Create(ctx context.Context, name string) error {
 		return fmt.Errorf("failed to create volume group %q: %w", name, err)
 	}
 
-	vg.name = name
 	log.DebugLog(ctx, "volume group %q has been created", vg)
 
 	return nil
@@ -344,6 +352,11 @@ func (vg *volumeGroup) Delete(ctx context.Context) error {
 func (vg *volumeGroup) AddVolume(ctx context.Context, vol types.Volume) error {
 	err := vol.AddToGroup(ctx, vg)
 	if err != nil {
+		// rados.ErrObjectExists does not match the rbd error (there is no EEXISTS error)
+		if errors.Is(rados.ErrObjectExists, err) || strings.Contains(err.Error(), "ret=-17") {
+			return nil
+		}
+
 		return fmt.Errorf("failed to add volume %q to volume group %q: %w", vol, vg, err)
 	}
 
@@ -390,6 +403,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 4e9f385e129..9f04e947683 100644
--- a/internal/rbd/manager.go
+++ b/internal/rbd/manager.go
@@ -223,16 +223,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 322230952bc..83d0adea376 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