Repository: ceph/go-ceph
Pull request #310 (2020-06-17 14:41:13)

Add support for rbd_get_{create,access,modify}_timestamp() functions
Ceph-CSI needs functions to fetch the timestamps from RBD images, these functions have not been implemented in go-ceph/rbd yet.

CephFS already returns a `Timespec` type, which is `golang.org/x/sys/unix.Timespec` compatible. Re-using that in thr rbd package with `cephfs.Timespec` is a little ugly, so the 'proxy type' has been moved under `internal/timespec` package. This might be a little controversial, but I could not think of a different way without duplicating the code too much. This approach does not require the consumers of go-ceph/cephfs to make any changes.

Closes: #309

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


phlogistonjohn: > CephFS already returns a `Timespec` type, which is `golang.org/x/sys/unix.Timespec` compatible. Re-using that in thr rbd package with `cephfs.Timespec` is a little ugly, so the 'proxy type' has been moved under `internal/timespec` package. This might be a little controversial, but I could not think of a different way without duplicating the code too much. 


I think that'll be fine. I should have looked a bit wider to see if that type was used in the other Ceph C libs before just dropping it in cephfs.


>This approach does not require the consumers of go-ceph/cephfs to make any changes.
> 

Good, as long as its invisible to the callers I'm OK with this.
phlogistonjohn: Mimic test failed to compile:
> # github.com/ceph/go-ceph/rbd
> rbd/rbd.go:457:12: could not determine kind of name for C.rbd_get_access_timestamp
> rbd/rbd.go:475:12: could not determine kind of name for C.rbd_get_modify_timestamp

Might these be only in nautiuls + (note I haven't check myself yet, just speculating)
phlogistonjohn: Just peeked at the ceph git tree, and it does appear to be new in nautilus.
nixpanic: > Just peeked at the ceph git tree, and it does appear to be new in nautilus.

Thanks! Updated and should be ready for review.
nixpanic: Thanks for the prompt review, updates and taken care of your remarks.

Patch:
diff --git a/cephfs/statx.go b/cephfs/statx.go
index f822a8d..00f4fa1 100644
--- a/cephfs/statx.go
+++ b/cephfs/statx.go
@@ -7,6 +7,13 @@ package cephfs
 */
 import "C"
 
+import (
+	ts "github.com/ceph/go-ceph/internal/timespec"
+)
+
+// Timespec is a public type for the internal C 'struct timespec'
+type Timespec ts.Timespec
+
 // StatxMask values contain bit-flags indicating what data should be
 // populated by a statx-type call.
 type StatxMask uint32
@@ -109,10 +116,10 @@ func cStructToCephStatx(s C.struct_ceph_statx) *CephStatx {
 		Blocks:  uint64(s.stx_blocks),
 		Dev:     uint64(s.stx_dev),
 		Rdev:    uint64(s.stx_rdev),
-		Atime:   cStructToTimespec(s.stx_atime),
-		Ctime:   cStructToTimespec(s.stx_ctime),
-		Mtime:   cStructToTimespec(s.stx_mtime),
-		Btime:   cStructToTimespec(s.stx_btime),
+		Atime:   Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&s.stx_atime))),
+		Ctime:   Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&s.stx_ctime))),
+		Mtime:   Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&s.stx_mtime))),
+		Btime:   Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&s.stx_btime))),
 		Version: uint64(s.stx_version),
 	}
 }
diff --git a/cephfs/timespec.go b/internal/timespec/timespec.go
similarity index 55%
rename from cephfs/timespec.go
rename to internal/timespec/timespec.go
index b10b36b..54076e3 100644
--- a/cephfs/timespec.go
+++ b/internal/timespec/timespec.go
@@ -1,4 +1,4 @@
-package cephfs
+package timespec
 
 /*
 #include <time.h>
@@ -6,6 +6,8 @@ package cephfs
 import "C"
 
 import (
+	"unsafe"
+
 	"golang.org/x/sys/unix"
 )
 
@@ -14,7 +16,13 @@ import (
 // apis that could be lossy with the use of Go time types.
 type Timespec unix.Timespec
 
-func cStructToTimespec(t C.struct_timespec) Timespec {
+// CTimespecPtr is an unsafe pointer wrapping C's `struct timespec`.
+type CTimespecPtr unsafe.Pointer
+
+// CStructToTimespec creates a new Timespec for the C 'struct timespec'.
+func CStructToTimespec(cts CTimespecPtr) Timespec {
+	t := (*C.struct_timespec)(cts)
+
 	return Timespec{
 		Sec:  int64(t.tv_sec),
 		Nsec: int64(t.tv_nsec),
diff --git a/rbd/rbd.go b/rbd/rbd.go
index 76cc5be..767ba07 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -18,6 +18,7 @@ import (
 	"unsafe"
 
 	"github.com/ceph/go-ceph/internal/retry"
+	ts "github.com/ceph/go-ceph/internal/timespec"
 	"github.com/ceph/go-ceph/rados"
 )
 
@@ -45,6 +46,9 @@ const (
 	NoSnapshot = ""
 )
 
+// Timespec is a public type for the internal C 'struct timespec'
+type Timespec ts.Timespec
+
 // ImageInfo represents the status information for an image.
 type ImageInfo struct {
 	Size              uint64
diff --git a/rbd/rbd_nautilus.go b/rbd/rbd_nautilus.go
index cf479a0..3ae2a48 100644
--- a/rbd/rbd_nautilus.go
+++ b/rbd/rbd_nautilus.go
@@ -1,6 +1,7 @@
 // +build !luminous,!mimic
 //
-// Ceph Nautilus is the first release that includes rbd_list2().
+// Ceph Nautilus is the first release that includes rbd_list2() and
+// rbd_get_create_timestamp().
 
 package rbd
 
@@ -14,6 +15,7 @@ import (
 	"unsafe"
 
 	"github.com/ceph/go-ceph/internal/retry"
+	ts "github.com/ceph/go-ceph/internal/timespec"
 	"github.com/ceph/go-ceph/rados"
 )
 
@@ -45,3 +47,57 @@ func GetImageNames(ioctx *rados.IOContext) ([]string, error) {
 	}
 	return names, nil
 }
+
+// GetCreateTimestamp returns the time the rbd image was created.
+//
+// Implements:
+//  int rbd_get_create_timestamp(rbd_image_t image, struct timespec *timestamp);
+func (image *Image) GetCreateTimestamp() (Timespec, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return Timespec{}, err
+	}
+
+	var cts C.struct_timespec
+
+	if ret := C.rbd_get_create_timestamp(image.image, &cts); ret < 0 {
+		return Timespec{}, getError(ret)
+	}
+
+	return Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&cts))), nil
+}
+
+// GetAccessTimestamp returns the time the rbd image was last accessed.
+//
+// Implements:
+//  int rbd_get_access_timestamp(rbd_image_t image, struct timespec *timestamp);
+func (image *Image) GetAccessTimestamp() (Timespec, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return Timespec{}, err
+	}
+
+	var cts C.struct_timespec
+
+	if ret := C.rbd_get_access_timestamp(image.image, &cts); ret < 0 {
+		return Timespec{}, getError(ret)
+	}
+
+	return Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&cts))), nil
+}
+
+// GetModifyTimestamp returns the time the rbd image was last modified.
+//
+// Implements:
+//  int rbd_get_modify_timestamp(rbd_image_t image, struct timespec *timestamp);
+func (image *Image) GetModifyTimestamp() (Timespec, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return Timespec{}, err
+	}
+
+	var cts C.struct_timespec
+
+	if ret := C.rbd_get_modify_timestamp(image.image, &cts); ret < 0 {
+		return Timespec{}, getError(ret)
+	}
+
+	return Timespec(ts.CStructToTimespec(ts.CTimespecPtr(&cts))), nil
+}


Test patch:
diff --git a/rbd/rbd_nautilus_test.go b/rbd/rbd_nautilus_test.go
new file mode 100644
index 0000000..4d0d43f
--- /dev/null
+++ b/rbd/rbd_nautilus_test.go
@@ -0,0 +1,79 @@
+// +build !luminous,!mimic
+
+package rbd
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestImagePropertiesNautilus(t *testing.T) {
+	conn := radosConnect(t)
+	defer conn.Shutdown()
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	require.NoError(t, err)
+	defer func() { assert.NoError(t, conn.DeletePool(poolname)) }()
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+	defer ioctx.Destroy()
+
+	name := GetUUID()
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
+	require.NoError(t, err)
+
+	img, err := OpenImage(ioctx, name, NoSnapshot)
+	require.NoError(t, err)
+	defer func() { assert.NoError(t, img.Remove()) }()
+	defer func() { assert.NoError(t, img.Close()) }()
+
+	_, err = img.GetCreateTimestamp()
+	assert.NoError(t, err)
+
+	_, err = img.GetAccessTimestamp()
+	assert.NoError(t, err)
+
+	_, err = img.GetModifyTimestamp()
+	assert.NoError(t, err)
+}
+
+func TestClosedImageNautilus(t *testing.T) {
+	conn := radosConnect(t)
+	defer conn.Shutdown()
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	assert.NoError(t, err)
+	defer func() { assert.NoError(t, conn.DeletePool(poolname)) }()
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+	defer ioctx.Destroy()
+
+	name := GetUUID()
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
+	assert.NoError(t, err)
+
+	image, err := OpenImage(ioctx, name, NoSnapshot)
+	assert.NoError(t, err)
+	defer func() { assert.NoError(t, image.Remove()) }()
+
+	// close the image
+	err = image.Close()
+	assert.NoError(t, err)
+
+	// functions should now fail with an RBDError
+
+	_, err = image.GetCreateTimestamp()
+	assert.Error(t, err)
+
+	_, err = image.GetAccessTimestamp()
+	assert.Error(t, err)
+
+	_, err = image.GetModifyTimestamp()
+	assert.Error(t, err)
+}