Repository: ceph/go-ceph
Pull request #906 (2023-07-31 15:14:58)

rbd: include details for parent image in the trash
When a parent image has been removed, it will linger in the trash until all siblings are gone. The image is not accessible through it's name anymore, only through its ID.

The ImageSpec that is returned by Image.GetParent() now contains the Trash boolean and the ImageID to identify if the image is in the trash, and use OpenImageById() to access the removed parent image.

Related-to: ceph/ceph-csi#4013

## Checklist
- [x] Added tests for features and functional changes
- [ ] Public functions and types are documented
- [x] Standard formatting is applied to Go code
- [ ] Is this a new API? Added a new file that begins with `//go:build ceph_preview`
- [ ] Ran `make api-update` to record new APIs


phlogistonjohn: Marked as an API change for review purposes. Change itself extends an existing type with new fields.
phlogistonjohn: Change generally looks good to me and I'm willing to approve as-is, but first I want to ask. Would you mind completing the object and putting all fields present in rbd_linked_image_spec_t into the Go equivalent?

As of nautilus the fields are:
```C
typedef struct {
  int64_t pool_id;
  char *pool_name;
  char *pool_namespace;
  char *image_id;
  char *image_name;
  bool trash;
} rbd_linked_image_spec_t;

```

After your change we still lack `pool_id` and `pool_namespace`. I think it would be nice to proactively complete the set so that the next person who needs one of those doesn't need to file a PR :-)

What do you think?
phlogistonjohn: Also, we can ignore failures in the CI against ceph main. This is due to a general infra issues on the ceph side related to nfs-ganesha builds.
nixpanic: > Should we add an additional check to ensure that parent is no longer accessible by name but via ID(using `OpenImageById()`) as mentioned in the commit message?

That was taken from the discussion in ceph/ceph-csi#4013. I do not think we need to validate this particular RBD behavior. Possibly it could be added to a test that verifies the `Trash()` function.

Patch:
diff --git a/rbd/snapshot_nautilus.go b/rbd/snapshot_nautilus.go
index dc28db2..f81e8d9 100644
--- a/rbd/snapshot_nautilus.go
+++ b/rbd/snapshot_nautilus.go
@@ -67,8 +67,12 @@ func (image *Image) GetParentInfo(pool, name, snapname []byte) error {
 
 // ImageSpec represents the image information.
 type ImageSpec struct {
-	ImageName string
-	PoolName  string
+	ImageName     string
+	ImageID       string
+	PoolName      string
+	PoolNamespace string
+	PoolID        uint64
+	Trash         bool
 }
 
 // SnapSpec represents the snapshot infomation.
@@ -104,8 +108,12 @@ func (image *Image) GetParent() (*ParentInfo, error) {
 	defer C.rbd_snap_spec_cleanup(&parentSnap)
 
 	imageSpec := ImageSpec{
-		ImageName: C.GoString(parentImage.image_name),
-		PoolName:  C.GoString(parentImage.pool_name),
+		ImageName:     C.GoString(parentImage.image_name),
+		ImageID:       C.GoString(parentImage.image_id),
+		PoolName:      C.GoString(parentImage.pool_name),
+		PoolNamespace: C.GoString(parentImage.pool_namespace),
+		PoolID:        uint64(parentImage.pool_id),
+		Trash:         bool(parentImage.trash),
 	}
 
 	snapSpec := SnapSpec{


Test patch:
diff --git a/rbd/rbd_nautilus_test.go b/rbd/rbd_nautilus_test.go
index da6d9ee..2ae4306 100644
--- a/rbd/rbd_nautilus_test.go
+++ b/rbd/rbd_nautilus_test.go
@@ -2,6 +2,7 @@ package rbd
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -185,6 +186,55 @@ func TestGetParent(t *testing.T) {
 		assert.Equal(t, parentInfo.Image.ImageName, imgName)
 		assert.Equal(t, parentInfo.Snap.SnapName, snapName)
 		assert.Equal(t, parentInfo.Image.PoolName, poolName)
+		assert.False(t, parentInfo.Image.Trash)
+		// TODO: add a comaprison for snap ID
+	})
+
+	t.Run("ParentInTrash", func(t *testing.T) {
+		trashName := "trashed"
+		_, err := Create(ioctx, trashName, testImageSize, testImageOrder, 1)
+		assert.NoError(t, err)
+
+		imgTrash, err := OpenImage(ioctx, trashName, NoSnapshot)
+		assert.NoError(t, err)
+		defer func() { assert.NoError(t, imgTrash.Close()) }()
+		imgTrashID, err := imgTrash.GetId()
+		assert.NoError(t, err)
+
+		snapNameTrash := "snapTrash"
+		snapTrash, err := imgTrash.CreateSnapshot(snapNameTrash)
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, snapTrash.Remove())
+		}()
+
+		cloneName := "childWithTrash"
+		optionsClone := NewRbdImageOptions()
+		defer optionsClone.Destroy()
+		err = optionsClone.SetUint64(ImageOptionCloneFormat, 2)
+		assert.NoError(t, err)
+
+		// Create a clone of the image.
+		err = CloneImage(ioctx, trashName, snapNameTrash, ioctx, cloneName, optionsClone)
+		assert.NoError(t, err)
+		defer func() { assert.NoError(t, RemoveImage(ioctx, cloneName)) }()
+
+		// Move the parent image to the trash, won't be deleted until the clone is removed.
+		assert.NoError(t, imgTrash.Trash(15*time.Second))
+
+		imgNew, err := OpenImage(ioctx, cloneName, NoSnapshot)
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, imgNew.Close())
+		}()
+
+		parentInfo, err := imgNew.GetParent()
+		assert.NoError(t, err)
+		assert.Equal(t, parentInfo.Image.ImageName, trashName)
+		assert.Equal(t, parentInfo.Image.ImageID, imgTrashID)
+		assert.Equal(t, parentInfo.Image.PoolName, poolName)
+		assert.True(t, parentInfo.Image.Trash)
+		assert.Equal(t, parentInfo.Snap.SnapName, snapNameTrash)
 		// TODO: add a comaprison for snap ID
 	})