Repository: ceph/go-ceph
Pull request #328 (2020-07-14 06:39:41)

rados: implement rados pool snapshot functionality
Implemented following rados API components:

- rados_ioctx_snap_create
- rados_ioctx_snap_get_name
- rados_ioctx_snap_get_stamp
- rados_ioctx_snap_list
- rados_ioctx_snap_lookup
- rados_ioctx_snap_remove
- rados_ioctx_snap_rollback
- rados_ioctx_snap_set_read

Signed-off-by: Mudit Agarwal <muagarwa@redhat.com>

Fixes: #269 

## Checklist
- [ ] Added tests for features and functional changes
- [ ] Public functions and types are documented
- [ ] Standard formatting is applied to Go code


agarwal-mudit: @nixpanic @phlogistonjohn PTAL
agarwal-mudit: I guess let's discuss it before I make further changes so that we all are on same page.

Currently snapshot object is like this:  (similar to rbd snapshot)

```
type Snapshot struct {                                                                                                                            
    ioctx *IOContext                                                                                                                              
    name  string                                                                                                                                  
}        

```
Suggestion from @nixpanic is to make it like this:

```
type Snapshot struct {                                                                                                                            
    ioctx *IOContext                                                                                                                              
    name  string
    SnapID snapID                                                                                                                                  
} 
```

Should we go ahead with this? 
If yes, should we get the ID and fill it during create snapshot itself?

If we do that, we can make following functions of Snapshot type because all of these use ID

```
rados_ioctx_snap_get_name
rados_ioctx_snap_get_stamp
rados_ioctx_snap_set_read
```

We can as well write GetName() and GetID() methods for Snapshot type.
       
phlogistonjohn: > I guess let's discuss it before I make further changes so that we all are on same page.
> 
> Currently snapshot object is like this: (similar to rbd snapshot)
> 
> ```
> type Snapshot struct {                                                                                                                            
>     ioctx *IOContext                                                                                                                              
>     name  string                                                                                                                                  
> }        
> ```
> 
> Suggestion from @nixpanic is to make it like this:
> 
> ```
> type Snapshot struct {                                                                                                                            
>     ioctx *IOContext                                                                                                                              
>     name  string
>     SnapID snapID                                                                                                                                  
> } 
> ```
> 
> Should we go ahead with this?
> If yes, should we get the ID and fill it during create snapshot itself?
> 
> If we do that, we can make following functions of Snapshot type because all of these use ID
> 
> ```
> rados_ioctx_snap_get_name
> rados_ioctx_snap_get_stamp
> rados_ioctx_snap_set_read
> ```
> 
> We can as well write GetName() and GetID() methods for Snapshot type.

As I note in my review above I don't either are appropriate. The SnapID type itself is fine. However there probably shouldn't be an extra level of indirection in go-ceph just to "smuggle" the snapshot name string around. It will be far simplier to make these methods on ioctx that take the appropriate arguments.
agarwal-mudit: Yeah, I was mimicking rbd snapshot pattern here which I also didn't like much.
So, lets not have a Snapshot object and base all functions on icotx.
Will send the PR with the changes.
agarwal-mudit: @nixpanic @phlogistonjohn PTAL, completely changed from last time.
agarwal-mudit: > Looks good to me. I'd prefer to have names start woth `Snap` though, that matches more what librados offers, and automatically sorts the functions together in the [documentation](https://pkg.go.dev/github.com/ceph/go-ceph/rados?tab=doc).

We can do that but else where in the code we have followed the pattern to put verb in front.
phlogistonjohn: Regarding the naming: I don't have strong opinions on this. I usually prefer names in more typical order for english prose "PostMesage" "GetBlock" rather than the "backwards" noun-verb style. However, because the iocontext type has a lot of methods and we're wrapping code with the noun-verb style I'm willing to give here, but I won't argue strongly either way for this particular case.
agarwal-mudit: > What you've got looks good to me. However, I checked the coverage report and there's a bunch of missing coverage for the cases where validate() returns an error. I don't need you to fix this to merge but I would like it get added. So we can either do it in a follow up PR or here. I'm going to leave this as a comment to hear back from you on what approach you'd prefer.

Updated test cases for validate() in the same PR, PTAL.
agarwal-mudit: @nixpanic PTAL

Patch:
diff --git a/rados/errors.go b/rados/errors.go
index 6bf98e6..2f643f3 100644
--- a/rados/errors.go
+++ b/rados/errors.go
@@ -48,11 +48,15 @@ func getErrorIfNegative(ret C.int) error {
 // Public go errors:
 
 var (
-	// ErrNotConnected is returned when functions are called without a RADOS connection
+	// ErrNotConnected is returned when functions are called
+	// without a RADOS connection.
 	ErrNotConnected = errors.New("RADOS not connected")
 	// ErrEmptyArgument may be returned if a function argument is passed
 	// a zero-length slice or map.
 	ErrEmptyArgument = errors.New("Argument must contain at least one item")
+	// ErrInvalidIOContext may be returned if an api call requires an IOContext
+	// but IOContext is not ready for use.
+	ErrInvalidIOContext = errors.New("IOContext is not ready for use")
 )
 
 // Public radosErrors:
diff --git a/rados/ioctx.go b/rados/ioctx.go
index 598976d..26519d6 100644
--- a/rados/ioctx.go
+++ b/rados/ioctx.go
@@ -96,7 +96,7 @@ type IOContext struct {
 // with ceph C calls.
 func (ioctx *IOContext) validate() error {
 	if ioctx.ioctx == nil {
-		return ErrNotConnected
+		return ErrInvalidIOContext
 	}
 	return nil
 }
diff --git a/rados/snapshot.go b/rados/snapshot.go
new file mode 100644
index 0000000..db4d145
--- /dev/null
+++ b/rados/snapshot.go
@@ -0,0 +1,189 @@
+package rados
+
+// #cgo LDFLAGS: -lrados
+// #include <stdlib.h>
+// #include <rados/librados.h>
+import "C"
+
+import (
+	"time"
+	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/retry"
+)
+
+// CreateSnap creates a pool-wide snapshot.
+//
+// Implements:
+// int rados_ioctx_snap_create(rados_ioctx_t io, const char *snapname)
+func (ioctx *IOContext) CreateSnap(snapName string) error {
+	if err := ioctx.validate(); err != nil {
+		return err
+	}
+
+	cSnapName := C.CString(snapName)
+	defer C.free(unsafe.Pointer(cSnapName))
+
+	ret := C.rados_ioctx_snap_create(ioctx.ioctx, cSnapName)
+	return getError(ret)
+}
+
+// RemoveSnap deletes the pool snapshot.
+//
+// Implements:
+//  int rados_ioctx_snap_remove(rados_ioctx_t io, const char *snapname)
+func (ioctx *IOContext) RemoveSnap(snapName string) error {
+	if err := ioctx.validate(); err != nil {
+		return err
+	}
+
+	cSnapName := C.CString(snapName)
+	defer C.free(unsafe.Pointer(cSnapName))
+
+	ret := C.rados_ioctx_snap_remove(ioctx.ioctx, cSnapName)
+	return getError(ret)
+}
+
+// SnapID represents the ID of a rados snapshot.
+type SnapID C.rados_snap_t
+
+// LookupSnap returns the ID of a pool snapshot.
+//
+// Implements:
+//  int rados_ioctx_snap_lookup(rados_ioctx_t io, const char *name, rados_snap_t *id)
+func (ioctx *IOContext) LookupSnap(snapName string) (SnapID, error) {
+	var snapID SnapID
+
+	if err := ioctx.validate(); err != nil {
+		return snapID, err
+	}
+
+	cSnapName := C.CString(snapName)
+	defer C.free(unsafe.Pointer(cSnapName))
+
+	ret := C.rados_ioctx_snap_lookup(
+		ioctx.ioctx,
+		cSnapName,
+		(*C.rados_snap_t)(&snapID))
+	return snapID, getError(ret)
+}
+
+// GetSnapName returns the name of a pool snapshot with the given snapshot ID.
+//
+// Implements:
+//  int rados_ioctx_snap_get_name(rados_ioctx_t io, rados_snap_t id, char *name, int maxlen)
+func (ioctx *IOContext) GetSnapName(snapID SnapID) (string, error) {
+	if err := ioctx.validate(); err != nil {
+		return "", err
+	}
+
+	var (
+		buf []byte
+		err error
+	)
+	// range from 1k to 64KiB
+	retry.WithSizes(1024, 1<<16, func(len int) retry.Hint {
+		cLen := C.int(len)
+		buf = make([]byte, cLen)
+		ret := C.rados_ioctx_snap_get_name(
+			ioctx.ioctx,
+			(C.rados_snap_t)(snapID),
+			(*C.char)(unsafe.Pointer(&buf[0])),
+			cLen)
+		err = getError(ret)
+		return retry.Size(int(cLen)).If(err == errRange)
+	})
+
+	if err != nil {
+		return "", err
+	}
+	return C.GoString((*C.char)(unsafe.Pointer(&buf[0]))), nil
+}
+
+// GetSnapStamp returns the time of the pool snapshot creation.
+//
+// Implements:
+//  int rados_ioctx_snap_get_stamp(rados_ioctx_t io, rados_snap_t id, time_t *t)
+func (ioctx *IOContext) GetSnapStamp(snapID SnapID) (time.Time, error) {
+	var cTime C.time_t
+
+	if err := ioctx.validate(); err != nil {
+		return time.Unix(int64(cTime), 0), err
+	}
+
+	ret := C.rados_ioctx_snap_get_stamp(
+		ioctx.ioctx,
+		(C.rados_snap_t)(snapID),
+		&cTime)
+	return time.Unix(int64(cTime), 0), getError(ret)
+}
+
+// ListSnaps returns a slice containing the SnapIDs of existing pool snapshots.
+//
+// Implements:
+//  int rados_ioctx_snap_list(rados_ioctx_t io, rados_snap_t *snaps, int maxlen)
+func (ioctx *IOContext) ListSnaps() ([]SnapID, error) {
+	if err := ioctx.validate(); err != nil {
+		return nil, err
+	}
+
+	var (
+		snapList []SnapID
+		cLen     C.int
+		err      error
+		ret      C.int
+	)
+	retry.WithSizes(100, 1000, func(maxlen int) retry.Hint {
+		cLen = C.int(maxlen)
+		snapList = make([]SnapID, cLen)
+		ret = C.rados_ioctx_snap_list(
+			ioctx.ioctx,
+			(*C.rados_snap_t)(unsafe.Pointer(&snapList[0])),
+			cLen)
+		err = getErrorIfNegative(ret)
+		return retry.Size(int(cLen)).If(err == errRange)
+	})
+
+	if err != nil {
+		return nil, err
+	}
+	return snapList[:ret], nil
+}
+
+// RollbackSnap rollbacks the object with key oID to the pool snapshot.
+// The contents of the object will be the same as when the snapshot was taken.
+//
+// Implements:
+//  int rados_ioctx_snap_rollback(rados_ioctx_t io, const char *oid, const char *snapname);
+func (ioctx *IOContext) RollbackSnap(oid, snapName string) error {
+	if err := ioctx.validate(); err != nil {
+		return err
+	}
+
+	coid := C.CString(oid)
+	defer C.free(unsafe.Pointer(coid))
+	cSnapName := C.CString(snapName)
+	defer C.free(unsafe.Pointer(cSnapName))
+
+	ret := C.rados_ioctx_snap_rollback(ioctx.ioctx, coid, cSnapName)
+	return getError(ret)
+}
+
+// SnapHead is the representation of LIBRADOS_SNAP_HEAD from librados.
+// SnapHead can be used to reset the IOContext to stop reading from a snapshot.
+const SnapHead = SnapID(C.LIBRADOS_SNAP_HEAD)
+
+// SetReadSnap sets the snapshot from which reads are performed.
+// Subsequent reads will return data as it was at the time of that snapshot.
+// Pass SnapHead for no snapshot (i.e. normal operation).
+//
+// Implements:
+//  void rados_ioctx_snap_set_read(rados_ioctx_t io, rados_snap_t snap);
+func (ioctx *IOContext) SetReadSnap(snapID SnapID) error {
+	if err := ioctx.validate(); err != nil {
+		return err
+	}
+
+	C.rados_ioctx_snap_set_read(ioctx.ioctx, (C.rados_snap_t)(snapID))
+	return nil
+}


Test patch:
diff --git a/rados/snapshot_test.go b/rados/snapshot_test.go
new file mode 100644
index 0000000..4c73786
--- /dev/null
+++ b/rados/snapshot_test.go
@@ -0,0 +1,312 @@
+package rados
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func (suite *RadosTestSuite) TestCreateRemoveSnapshot() {
+	suite.SetupConnection()
+
+	suite.T().Run("invalidIOCtx", func(t *testing.T) {
+		ioctx := &IOContext{}
+		err := ioctx.CreateSnap("someSnap")
+		assert.Error(t, err)
+		err = ioctx.RemoveSnap("someSnap")
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+	})
+
+	suite.T().Run("NewSnap", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		snapName := "mySnap"
+		err = ioctx.CreateSnap(snapName)
+		assert.NoError(t, err)
+		err = ioctx.RemoveSnap(snapName)
+		assert.NoError(t, err)
+	})
+
+	suite.T().Run("ExistingSnap", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		snapName := "mySnap"
+		err = ioctx.CreateSnap(snapName)
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, ioctx.RemoveSnap(snapName))
+		}()
+		err = ioctx.CreateSnap(snapName)
+		assert.Error(t, err)
+	})
+
+	suite.T().Run("NonExistingSnap", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		err = ioctx.RemoveSnap("someSnapName")
+		assert.Error(t, err)
+	})
+
+	// Strangely, this works!!
+	suite.T().Run("EmptySnapNameString", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		err = ioctx.CreateSnap("")
+		assert.NoError(t, err)
+
+		err = ioctx.RemoveSnap("")
+		assert.NoError(t, err)
+	})
+}
+
+func (suite *RadosTestSuite) TestSnapshotIDFunctions() {
+	suite.SetupConnection()
+
+	suite.T().Run("invalidIOCtx", func(t *testing.T) {
+		ioctx := &IOContext{}
+		_, err := ioctx.LookupSnap("")
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+
+		var snapID SnapID
+		snapID = 22 // some random number
+		_, err = ioctx.GetSnapName(snapID)
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+
+		_, err = ioctx.GetSnapStamp(snapID)
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+	})
+
+	// Invalid args
+	suite.T().Run("InvalidArgs", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		err = ioctx.CreateSnap("")
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, ioctx.RemoveSnap(""))
+		}()
+
+		// Again, this works!!
+		_, err = ioctx.LookupSnap("")
+		assert.NoError(t, err)
+
+		// Non-existing Snap
+		_, err = ioctx.LookupSnap("someSnapName")
+		assert.Error(t, err)
+
+		var snapID SnapID
+		snapID = 22 // some random number
+		_, err = ioctx.GetSnapName(snapID)
+		assert.Error(t, err)
+
+		_, err = ioctx.GetSnapStamp(snapID)
+		assert.Error(t, err)
+	})
+
+	// Valid SnapID operations.
+	suite.T().Run("ValidSnapIDOps", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+
+		snapName := "mySnap"
+		err = ioctx.CreateSnap(snapName)
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, ioctx.RemoveSnap(snapName))
+		}()
+
+		snapID, err := ioctx.LookupSnap(snapName)
+		assert.NoError(t, err)
+
+		retName, err := ioctx.GetSnapName(snapID)
+		assert.NoError(t, err)
+		assert.Equal(t, snapName, retName)
+
+		_, err = ioctx.GetSnapStamp(snapID)
+		assert.NoError(t, err)
+	})
+}
+
+func (suite *RadosTestSuite) TestListSnapshot() {
+	suite.SetupConnection()
+	ioctx, err := suite.conn.OpenIOContext(suite.pool)
+	require.NoError(suite.T(), err)
+
+	snapName := []string{"snap1", "snap2", "snap3"}
+	err = ioctx.CreateSnap(snapName[0])
+	assert.NoError(suite.T(), err)
+	defer func() {
+		assert.NoError(suite.T(), ioctx.RemoveSnap(snapName[0]))
+	}()
+
+	err = ioctx.CreateSnap(snapName[1])
+	assert.NoError(suite.T(), err)
+	defer func() {
+		assert.NoError(suite.T(), ioctx.RemoveSnap(snapName[1]))
+	}()
+
+	err = ioctx.CreateSnap(snapName[2])
+	assert.NoError(suite.T(), err)
+	defer func() {
+		assert.NoError(suite.T(), ioctx.RemoveSnap(snapName[2]))
+	}()
+
+	suite.T().Run("invalidIOContext", func(t *testing.T) {
+		ioctx := &IOContext{}
+		_, err := ioctx.ListSnaps()
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+	})
+
+	snapList, err := ioctx.ListSnaps()
+	assert.NoError(suite.T(), err)
+	assert.NotNil(suite.T(), snapList)
+
+	listLen := len(snapList)
+
+	suite.T().Run("NumberOfSnapshots", func(t *testing.T) {
+		assert.Equal(t, 3, listLen)
+	})
+
+	suite.T().Run("MatchSnapNamesWithID", func(t *testing.T) {
+		for _, id := range snapList[0 : listLen-1] {
+			retName, err := ioctx.GetSnapName(id)
+			assert.NoError(t, err)
+			assert.NotNil(t, retName)
+			assert.Contains(t, snapName, retName)
+		}
+	})
+}
+
+func (suite *RadosTestSuite) TestRollbackSnapshot() {
+	suite.SetupConnection()
+
+	suite.T().Run("InvalidArgs", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+		oid := suite.GenObjectName()
+		defer suite.ioctx.Delete(oid)
+
+		err = ioctx.RollbackSnap(oid, "someName")
+		assert.Error(t, err)
+
+		err = ioctx.CreateSnap("")
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, ioctx.RemoveSnap(""))
+		}()
+		err = ioctx.RollbackSnap("someObj", "")
+		assert.NoError(t, err)
+	})
+
+	suite.T().Run("invalidIOContext", func(t *testing.T) {
+		ioctx := &IOContext{}
+		err := ioctx.RollbackSnap("someObj", "someName")
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+	})
+
+	suite.T().Run("WriteAndRollback", func(t *testing.T) {
+		ioctx, err := suite.conn.OpenIOContext(suite.pool)
+		require.NoError(suite.T(), err)
+		oid := suite.GenObjectName()
+		defer suite.ioctx.Delete(oid)
+
+		bytesIn := []byte("Harry Potter")
+		err = suite.ioctx.Write(oid, bytesIn, 0)
+		assert.NoError(t, err)
+		// Take snap.
+		err = ioctx.CreateSnap("mySnap")
+		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, ioctx.RemoveSnap("mySnap"))
+		}()
+		// Overwrite.
+		bytesOver := []byte("Potter Harry")
+		err = suite.ioctx.Write(oid, bytesOver, 0)
+		assert.NoError(t, err)
+
+		// Before rollback.
+		bytesOut := make([]byte, len(bytesOver))
+		nOut, err := suite.ioctx.Read(oid, bytesOut, 0)
+		assert.NoError(t, err)
+		assert.Equal(t, nOut, len(bytesOver))
+		assert.Equal(t, bytesOver, bytesOut)
+
+		// Rollback.
+		err = ioctx.RollbackSnap(oid, "mySnap")
+		assert.NoError(t, err)
+
+		// After Rollback.
+		nOut, err = suite.ioctx.Read(oid, bytesOut, 0)
+		assert.NoError(t, err)
+		assert.Equal(t, nOut, len(bytesIn))
+		assert.Equal(t, bytesOut, bytesIn)
+	})
+}
+
+func (suite *RadosTestSuite) TestSetReadSnapshot() {
+	suite.SetupConnection()
+	ioctx, err := suite.conn.OpenIOContext(suite.pool)
+	require.NoError(suite.T(), err)
+
+	bytesIn := []byte("The Order of the Phoenix")
+	err = ioctx.Write("obj", bytesIn, 0)
+	assert.NoError(suite.T(), err)
+
+	// Take snap.
+	err = ioctx.CreateSnap("mySnap")
+	assert.NoError(suite.T(), err)
+	defer func() {
+		assert.NoError(suite.T(), ioctx.RemoveSnap("mySnap"))
+	}()
+
+	// Get Snap ID.
+	snapID, err := ioctx.LookupSnap("mySnap")
+	assert.NoError(suite.T(), err)
+	assert.NotNil(suite.T(), snapID)
+
+	// Overwrite the object.
+	bytesOver := []byte("The Half blood Prince")
+	err = ioctx.Write("obj", bytesOver, 0)
+	assert.NoError(suite.T(), err)
+
+	// Set read to mySnap.
+	err = ioctx.SetReadSnap(snapID)
+	assert.NoError(suite.T(), err)
+
+	// Read the object.
+	bytesOut := make([]byte, len(bytesIn))
+	nOut, err := ioctx.Read("obj", bytesOut, 0)
+	assert.NoError(suite.T(), err)
+	assert.Equal(suite.T(), nOut, len(bytesIn))
+	assert.Equal(suite.T(), bytesOut, bytesIn)
+
+	// Set read to SnapHead (back to normal).
+	err = ioctx.SetReadSnap(SnapHead)
+	assert.NoError(suite.T(), err)
+
+	// Read the same object.
+	bytesOut = make([]byte, len(bytesOver))
+	nOut, err = ioctx.Read("obj", bytesOut, 0)
+	assert.NoError(suite.T(), err)
+	assert.Equal(suite.T(), nOut, len(bytesOver))
+	assert.Equal(suite.T(), bytesOut, bytesOver)
+
+	suite.T().Run("invalidIOContext", func(t *testing.T) {
+		ioctx := &IOContext{}
+		err := ioctx.SetReadSnap(SnapHead)
+		assert.Error(t, err)
+		assert.Equal(t, err, ErrInvalidIOContext)
+	})
+}