Repository: ceph/go-ceph
Pull request #499 (2021-05-10 05:50:25)

 Method snapshot.Set has side-effects on Image state
This is a change to what is described in #176.

Transfer of the Set function in the Snapshot.go file to the SetSnapshot() function in rbd.go
 - rbd: add SetSnapshot re-implementing rbd_snap_set on image type
 - rbd: add deprecation comment to snapshot.Set method

## 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: > @phlogistonjohn Is there anything missing?

Hi! Thanks for the reminder ping. There are still a few issues I've previously mentioned that need to be resolved:

* The test code still adds net new lines. I suggested, "It would probably better if we replaced the use of snapshot.Set at around line number 97." This is important because we want to ensure the new method is a functionally drop-in replacement for the old `Set` call.
* I previously suggested we should replace the implementation of the old `Set` method with a call to `SetSnapshot`. I won't demand this one, but I'd like it if you could respond to this point and either defer it (and I'll create a new issue to track the work item) or make it part of this PR.
* The commit messages and the commits need cleaning up.

As for the last point, I realize that not everyone is accustomed to rebasing/squashing/re-writing git history. If you don't know how to fix up your commits just let us know and I will either try to help you out with instructions or I will ask your permission to alter the git history myself - keeping your authorship of course!

Let me know if you have any question, comments, or concerns about these items. Thanks!
oct28-yjkim: @phlogistonjohn I am new to changing commit using git

I would appreciate if you could give me the guide instructions.
phlogistonjohn: > @phlogistonjohn I am new to changing commit using git
> 
> I would appreciate if you could give me the guide instructions.

Sure! Because this is a reasonably small PR in terms of lines of code changed, I am going to recommend we squash everything into a single commit. He's how I would do it:
* Ensure you're already on the branch containing your changes.
* Run git rebase against the base branch for your changes, this may vary based on your set up but I think it should be something like: `git rebase -i origin/master`
* This will bring up an editor window that looks something like:
```
pick 567fed0 Update snapshot.go
pick ce9fd36 #176
pick 0279dcd #176
pick 7db5c28 Update rbd/snapshot.go
pick 503db18 Update snapshot.go

```

* On every line except the first, change `pick` to `squash`. Example:
```
pick 567fed0 Update snapshot.go
squash ce9fd36 #176
squash 0279dcd #176
squash 7db5c28 Update rbd/snapshot.go
squash 503db18 Update snapshot.go
````

* git will now start a new editor window with the combined commit messages. Delete the existing messages and replace it with something like the one below. And since we're editing commits add a signed off by (DCO) line, similar to the _example_ below but with *your* name and email:
```
rbd: add SetSnapshot (re)implementing rbd_snap_set

Signed-off-by: Your Preferred Name <youremail@example.com>

```

* Once you've saved and quit the editor git should create a single new commit from the previous commits.
* Now, you must force push `git push --force ...` to update your fork+branch

That should be all. Once done we should see the new commit here. Let me know if you have any additional questions about this. 

oct28-yjkim: @phlogistonjohn I combined the commit msg. Is this okay?
phlogistonjohn: > @phlogistonjohn I combined the commit msg. Is this okay?

Commit structure and commit message text look good, thanks! However, the signed off by line should not contain the literal string "Your Preferred Name" but rather *your name*. 

Here's [a reference ](https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for) that you can use as a starting point to read up on it more if you want to. You can also look at the recent git history of go-ceph to see other examples.


The change I mentioned about replacing Set rather than adding new lines still applies as well. I think it's the cause of the test failure in the CI.

In addition, because you now only have 1 commit you can use the (simpler, IMO) `git commit --amend` command to make changes to the commit that exists rather than adding new patches.
oct28-yjkim: @phlogistonjohn `The change I mentioned about replacing Set rather than adding new lines still applies as well. I think it's the cause of the test failure in the CI.`

* I changed the contents of `Set` API to `rbd.Setsnapshot` 
* changed the Error Case of snapshot_test.go to `ErrImageNotOpen`


phlogistonjohn: > @phlogistonjohn `The change I mentioned about replacing Set rather than adding new lines still applies as well. I think it's the cause of the test failure in the CI.`
> 
>     * I changed the contents of `Set` API to `rbd.Setsnapshot`
> 

Great! This means we only have 1 implementation of the binding now. Plus, every test that exercised `Set` is now also exercising  `rbd.Setsnapshot`!

>     * changed the Error Case of snapshot_test.go to `ErrImageNotOpen`

OK, thanks.

oct28-yjkim: @phlogistonjohn  All checklists have been passed.

What should I do?
phlogistonjohn: @mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

phlogistonjohn: @nixpanic or @ansiwen - need a 2nd review on this, so PTAL when you have a moment. Thanks!

Patch:
diff --git a/rbd/rbd.go b/rbd/rbd.go
index 066e009..f827bd2 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -916,6 +916,22 @@ func (image *Image) GetId() (string, error) {
 
 }
 
+// SetSnapshot updates the rbd image (not the Snapshot) such that the snapshot
+// is the source of readable data.
+//
+// Implements:
+//  int rbd_snap_set(rbd_image_t image, const char *snapname);
+func (image *Image) SetSnapshot(snapname string) error {
+	if err := image.validate(imageIsOpen); err != nil {
+		return err
+	}
+
+	c_snapname := C.CString(snapname)
+	defer C.free(unsafe.Pointer(c_snapname))
+
+	return getError(C.rbd_snap_set(image.image, c_snapname))
+}
+
 // GetTrashList returns a slice of TrashInfo structs, containing information about all RBD images
 // currently residing in the trash.
 func GetTrashList(ioctx *rados.IOContext) ([]TrashInfo, error) {
diff --git a/rbd/snapshot.go b/rbd/snapshot.go
index 9f10c63..97277d7 100644
--- a/rbd/snapshot.go
+++ b/rbd/snapshot.go
@@ -149,6 +149,7 @@ func (snapshot *Snapshot) IsProtected() (bool, error) {
 
 // Set updates the rbd image (not the Snapshot) such that the snapshot
 // is the source of readable data.
+// This method is deprecated. Refer the SetSnapshot method of the Image type instead.
 //
 // Implements:
 //  int rbd_snap_set(rbd_image_t image, const char *snapname);
@@ -157,10 +158,7 @@ func (snapshot *Snapshot) Set() error {
 		return err
 	}
 
-	c_snapname := C.CString(snapshot.name)
-	defer C.free(unsafe.Pointer(c_snapname))
-
-	return getError(C.rbd_snap_set(snapshot.image.image, c_snapname))
+	return snapshot.image.SetSnapshot(snapshot.name)
 }
 
 // GetSnapTimestamp returns the timestamp of a snapshot for an image.


Test patch:
diff --git a/rbd/snapshot_test.go b/rbd/snapshot_test.go
index be8e4b4..6cc7442 100644
--- a/rbd/snapshot_test.go
+++ b/rbd/snapshot_test.go
@@ -94,6 +94,9 @@ func TestErrorSnapshotNoName(t *testing.T) {
 	_, err = snapshot.IsProtected()
 	assert.Equal(t, err, ErrSnapshotNoName)
 
+	err = img.SetSnapshot(snapshot.name)
+	assert.Equal(t, err, ErrImageNotOpen)
+
 	err = snapshot.Set()
 	assert.Equal(t, err, ErrSnapshotNoName)