Repository: ceph/go-ceph
Pull request #148 (2020-01-08 12:01:39)

rbd: add support for listing watchers on an RBD image
Checking if there are other clients interested in changes to an RBD
image is important for certain actions. For example, the Ceph-CSI
project would not want to remove an image in case it is still mounted
somewhere.

Listing the details of the watchers is not important at the moment.
Adding a watcher is only implemented for testing, and is not functional
enough to get notifications through a callback. This is something that
needs more investigation and can be added at a later point.

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


nixpanic: Right, testing fails on Luminous because librbd does not have `rbd_watchers_list` yet...
nixpanic: This PR is rebased on #139 , so it should not get merged before that.

@phlogistonjohn could you check the `Image.HasWatchers()` function? It does not have a matching librbd function, but just counts the number of watchers that `rbd_watchers_list()` could return. Not sure if I should return a list of strings instead, or a bool, or rename the function to `CountWatchers()`.
phlogistonjohn: > This PR is rebased on #139 , so it should not get merged before that.
> 

Sure. I posted a review on that.

> @phlogistonjohn could you check the `Image.HasWatchers()` function? It does not have a matching librbd function, but just counts the number of watchers that `rbd_watchers_list()` could return. Not sure if I should return a list of strings instead, or a bool, or rename the function to `CountWatchers()`.

I generally like to stick as close to the underlying apis as possible so I think we should probably define a new type, that is functionally equivalent of rbd_image_watcher_t and return an array of those from a ListWatchers() function. If  the caller only cares about the number or lack of other watchers len() of the return will suffice. If you think that's too heavyweight I'd be ok with a CountWatchers() returning an int (which int type though?).  I don't see much value in a bool-returning function or a function that returns an array of strings and loses the other struct members from rbd_image_watcher_t.


nixpanic: Replaced `HasWatchers()` with `ListWatchers()` and added Image.CleanupWatchers() so that the allocated resources can be free'd again. It required some indirection for keeping a reference to the rbd_image_watch_t array, as that type is not available in older versions of Ceph.
phlogistonjohn: I don't like the extra indirection/caching and cleanup approach.
Wouldn't it be simpler to define a new go type that has the fields from rbd_image_watch_t and then convert between the two? Rough example:
```go
type ImageWatcher struct {
	Addr string
        Id int64
        Cookie uint64
}

func (image *Image) ListWatchers() ([]ImageWatcher, error) {
	// ....

	watchers := make([]C.rbd_image_watcher_t, count)
	ret = C.rbd_watchers_list(image.image, &watchers[0], &count)
	if ret != 0 {
		return nil, GetError(ret)
	}
	
        iws := make([]ImageWatcher, count)
	for i, watcher := range watchers {
		iws[i].Addr = C.GoString(watcher.addr)
                iws[i].Id = int64(watcher.id)
                iws[i].Cookie = uint64(watcher.cookie)
	}
        C.rbd_watchers_list_cleanup(&watchers[0], count)
        // ....
        return iws, nil

}
```

There are no functions that take an rbd_image_watcher_t as "input" so I don't see value in keeping the original rbd_image_watcher_t values around. Only the contents. This strikes me as a lot simpler and potentially less error prone.
nixpanic: That approach is indeed pretty clean, thanks for the idea!

Just one more thing, should there be a fake `ListWatchers()` for versions of Ceph that do not support it? This version will then always just return an error. It will make adopting the feature a little easier, I think. Opinions, @phlogistonjohn @ansiwen?
phlogistonjohn: > That approach is indeed pretty clean, thanks for the idea!
> 
Glad it helped.

> Just one more thing, should there be a fake `ListWatchers()` for versions of Ceph that do not support it? This version will then always just return an error. It will make adopting the feature a little easier, I think. Opinions, @phlogistonjohn @ansiwen?

I'm a bit torn personally. I like the idea of failing early when possible and there's not much earlier than failing to compile. If you leave out the rbd/watchers_mimic*.go files but leave the build flags as-is I think that go will simply fail to compile with a 'no such function' error right?

I'm sympathetic to the stub approach but worry it could mislead users or lead to more subtle errors than a compile failure.

I'd also love to hear @ansiwen's opinion on this one.
nixpanic: At the moment I am also adding support for new RbdImageFeatures, as newer Ceph versions add new constants. These constants will not be available for build of go-ceph with older librbd.h versions. So, I guess this is an indication that there do not need to be backwards compatible error-not-supported functions either.

Waiting for @ansiwen his opinion on the topic before proceeding.
nixpanic: Btw, Ceph-CSI supports different versions of Ceph at the moment. It also checks for watchers of an image before deletion (and possibly other actions). If there is no backwards compatible function that returns `ErrNotSupported`, it will not be possible for Ceph-CSI to support Ceph versions older than Nautilus.

So, in the end, I think providing backwards compatible functions would be helpful. Not sure yet how feature bits can be provided. These could be copied from the headers, or set to 0, or ... But that comes later.
Madhu-1: ceph-csi supports Mimic+ for RBD and Nautilus+ for cephfs more info here   https://github.com/ceph/ceph-csi#ceph-csi-features-and-available-versions
nixpanic: Thanks @Madhu-1! I probably mixed up projects that have tests in Travic CI for a whole range of Ceph versions :factory: 
nixpanic: Dropped the backwards compatible function again. It also seems that the support for watchers was added to librbd in Ceph Mimic (thanks @phlogistonjohn for spotting that).

If projects need support for older Ceph versions, they can use build-constraints like go-ceph does. go-ceph already expects a build-tag of the Ceph version for older releases (`luminous` or `mimic`).

This PR should now be ready for serious review :tada: 
nixpanic: This is now cleaned up, and only contains the most minimal functionality. Please review again, thanks!
nixpanic: Just updated the tests a little (setup with `defer`). Also removed `rbd/watchers.go` and merged its contents in `rbd/watchers_mimic.go` as it only contained the ImageWatcher type. Keeping the `_mimic` extension to show when support was introduced.
nixpanic: @phlogistonjohn , I think your comments have been addressed now,
phlogistonjohn: Looks like a bunch of the tests are failing. I think the issue are lines like:
`defer assert.NoError(t, image.Close())`
should be:
`defer func() { assert.NoError(t, image.Close()) }()`

Only the "outer" function call is deferred, so image.Close actually gets called on that line in the first example. Wrapping everything in a function makes sure the entire "line" is deferred.

I will do a proper review once the tests are passing again.
nixpanic: On Tue, Feb 25, 2020 at 07:13:08AM -0800, John Mulligan wrote:
> Looks like a bunch of the tests are failing. I think the issue are lines like:
> `defer assert.NoError(t, image.Close())`
> should be:
> `defer func() { assert.NoError(t, image.Close()) }()`
> 
> Only the "outer" function call is deferred, so image.Close actually gets called on that line in the first example. Wrapping everything in a function makes sure the entire "line" is deferred.

Right! I did wonder a little why tests were failing in CI and working
locally... Obviously I should not pass CEPH_VERSION=luminous on the
commandline :facepalm:


nixpanic: This is fixed now, thanks for having a look!

Patch:
diff --git a/rbd/watchers_mimic.go b/rbd/watchers_mimic.go
new file mode 100644
index 0000000..6f6bfbd
--- /dev/null
+++ b/rbd/watchers_mimic.go
@@ -0,0 +1,57 @@
+// +build !luminous
+//
+// Ceph Mimic is the first version that supports watchers through librbd.
+
+package rbd
+
+// #cgo LDFLAGS: -lrbd
+// #include <errno.h>
+// #include <rbd/librbd.h>
+import "C"
+
+// ImageWatcher is a representation of the rbd_image_watcher_t from librbd.h
+type ImageWatcher struct {
+	Addr   string
+	Id     int64
+	Cookie uint64
+}
+
+// ListWatchers returns the watchers on an RBD image. In case of an error, nil
+// and an error are returned.
+//
+// Note:
+//   Only supported in Ceph Mimic and newer.
+//
+// Implements:
+//   int rbd_watchers_list(rbd_image_t image,
+//                         rbd_image_watcher_t *watchers, size_t *max_watchers)
+func (image *Image) ListWatchers() ([]ImageWatcher, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return nil, err
+	}
+
+	count := C.ulong(0)
+	ret := C.rbd_watchers_list(image.image, nil, &count)
+	if ret != 0 && ret != -C.ERANGE {
+		return nil, getError(ret)
+	}
+	if ret == 0 && count == 0 {
+		return nil, nil
+	}
+
+	watchers := make([]C.rbd_image_watcher_t, count)
+	ret = C.rbd_watchers_list(image.image, &watchers[0], &count)
+	if ret != 0 && ret != -C.ERANGE {
+		return nil, getError(ret)
+	}
+	defer C.rbd_watchers_list_cleanup(&watchers[0], count)
+
+	imageWatchers := make([]ImageWatcher, len(watchers))
+	for i, watcher := range watchers {
+		imageWatchers[i].Addr = C.GoString(watcher.addr)
+		imageWatchers[i].Id = int64(watcher.id)
+		imageWatchers[i].Cookie = uint64(watcher.cookie)
+	}
+
+	return imageWatchers, nil
+}


Test patch:
diff --git a/rbd/watchers_mimic_test.go b/rbd/watchers_mimic_test.go
new file mode 100644
index 0000000..451cfef
--- /dev/null
+++ b/rbd/watchers_mimic_test.go
@@ -0,0 +1,117 @@
+// +build !luminous
+//
+// Ceph Mimic is the first version that supports watchers through librbd.
+
+package rbd
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestListWatchers(t *testing.T) {
+	conn := radosConnect(t)
+	require.NotNil(t, conn)
+	defer conn.Shutdown()
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	require.NoError(t, err)
+	defer conn.DeletePool(poolname)
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+	defer ioctx.Destroy()
+
+	name := GetUUID()
+	options := NewRbdImageOptions()
+	err = CreateImage(ioctx, name, 1<<22, options)
+	require.NoError(t, err)
+	defer func() { assert.NoError(t, RemoveImage(ioctx, name)) }()
+
+	t.Run("imageNotOpen", func(t *testing.T) {
+		image, err := OpenImageReadOnly(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
+		require.NotNil(t, image)
+
+		err = image.Close()
+		require.NoError(t, err)
+
+		_, err = image.ListWatchers()
+		assert.Equal(t, ErrImageNotOpen, err)
+	})
+
+	t.Run("noWatchers", func(t *testing.T) {
+		// open image read-only, as OpenImage() automatically adds a watcher
+		image, err := OpenImageReadOnly(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
+		require.NotNil(t, image)
+		defer func() { assert.NoError(t, image.Close()) }()
+
+		watchers, err := image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 0, len(watchers))
+	})
+
+	t.Run("addWatchers", func(t *testing.T) {
+		// open image read-only, as OpenImage() automatically adds a watcher
+		image, err := OpenImageReadOnly(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
+		require.NotNil(t, image)
+		defer func() { assert.NoError(t, image.Close()) }()
+
+		watchers, err := image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 0, len(watchers))
+
+		// opening an image writable adds a watcher automatically
+		image2, err := OpenImage(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
+		require.NotNil(t, image2)
+
+		watchers, err = image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(watchers))
+
+		watchers, err = image2.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(watchers))
+
+		image3, err := OpenImage(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
+		require.NotNil(t, image3)
+
+		watchers, err = image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 2, len(watchers))
+
+		watchers, err = image2.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 2, len(watchers))
+
+		watchers, err = image3.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 2, len(watchers))
+
+		// closing an image removes the watchers
+		err = image3.Close()
+		require.NoError(t, err)
+
+		watchers, err = image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(watchers))
+
+		watchers, err = image2.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(watchers))
+
+		err = image2.Close()
+		require.NoError(t, err)
+
+		watchers, err = image.ListWatchers()
+		assert.NoError(t, err)
+		assert.Equal(t, 0, len(watchers))
+	})
+}