Repository: ceph/go-ceph (go)

Issue:
cephfs admin: Add support for retain-snapshots flag when removing subvolumes
Future versions of ceph are expected to have a boolean (in the json) equivalent to a cli option --retain-snapshots. It is expcted that future versions of ceph-csi will make use of this flag. Once this flag is in at least one released version of ceph, and it can be tested in the CI, we should add support for it.

Pull request:
cephfs admin: add extended set of flags for subvolume remove function(s)
Recently, the subvolume remove json for the mgr's cephfs command grew a retain snapshots flag. Because we don't really want a zillion functions in go-ceph for all possible flags, the subvolume remove feature now comes with:
* RemoveSubVolume
* ForceRemoveSubVolume
* RemoveSubVolumeWithFlags

The latter is new and takes a new type SubVolRmFlags. It is expected that the caller create a new SubVolRmFlags and set "Force" or "RetainSnapshots" as needed. All other variants that only take a force flag continue to only have a Remove<Foo> and ForceRemove<Foo> functions in order to avoid boolean control arguments.

Fixes #385 

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


Patch:
diff --git a/cephfs/admin/flags.go b/cephfs/admin/flags.go
new file mode 100644
index 0000000..393ebe3
--- /dev/null
+++ b/cephfs/admin/flags.go
@@ -0,0 +1,60 @@
+// +build !luminous,!mimic
+
+package admin
+
+// For APIs that accept extra sets of "boolean" flags we may end up wanting
+// multiple different sets of supported flags. Example: most rm functions
+// accept a force flag, but only subvolume delete has retain snapshots.
+// To make this somewhat uniform in the admin package we define a utility
+// interface and helper function to merge flags with naming options.
+
+type flagSet interface {
+	flags() map[string]bool
+}
+
+type commonRmFlags struct {
+	force bool
+}
+
+func (f commonRmFlags) flags() map[string]bool {
+	o := make(map[string]bool)
+	if f.force {
+		o["force"] = true
+	}
+	return o
+}
+
+// SubVolRmFlags does not embed other types to simplify and keep the
+// interface with the type flat and simple. At the cost of some code
+// duplication we get a nicer UX for those using the library.
+
+// SubVolRmFlags may be used to specify behavior modifying flags when
+// removing sub volumes.
+type SubVolRmFlags struct {
+	Force           bool
+	RetainSnapshots bool
+}
+
+func (f SubVolRmFlags) flags() map[string]bool {
+	o := make(map[string]bool)
+	if f.Force {
+		o["force"] = true
+	}
+	if f.RetainSnapshots {
+		o["retain-snapshots"] = true
+	}
+	return o
+}
+
+// mergeFlags combines a set of key-value settings with any type implementing
+// the flagSet interface.
+func mergeFlags(m map[string]string, f flagSet) map[string]interface{} {
+	o := make(map[string]interface{})
+	for k, v := range m {
+		o[k] = v
+	}
+	for k, v := range f.flags() {
+		o[k] = v
+	}
+	return o
+}
diff --git a/cephfs/admin/fsadmin.go b/cephfs/admin/fsadmin.go
index 2b49980..e7bed2d 100644
--- a/cephfs/admin/fsadmin.go
+++ b/cephfs/admin/fsadmin.go
@@ -143,18 +143,3 @@ func modeString(m int, force bool) string {
 func uint64String(v uint64) string {
 	return strconv.FormatUint(uint64(v), 10)
 }
-
-type rmFlags struct {
-	force bool
-}
-
-func (f rmFlags) Update(m map[string]string) map[string]interface{} {
-	o := make(map[string]interface{})
-	for k, v := range m {
-		o[k] = v
-	}
-	if f.force {
-		o["force"] = true
-	}
-	return o
-}
diff --git a/cephfs/admin/subvolume.go b/cephfs/admin/subvolume.go
index b0d6935..ac18983 100644
--- a/cephfs/admin/subvolume.go
+++ b/cephfs/admin/subvolume.go
@@ -87,7 +87,7 @@ func (fsa *FSAdmin) ListSubVolumes(volume, group string) ([]string, error) {
 // Similar To:
 //  ceph fs subvolume rm <volume> --group-name=<group> <name>
 func (fsa *FSAdmin) RemoveSubVolume(volume, group, name string) error {
-	return fsa.rmSubVolume(volume, group, name, rmFlags{})
+	return fsa.RemoveSubVolumeWithFlags(volume, group, name, SubVolRmFlags{})
 }
 
 // ForceRemoveSubVolume will delete a CephFS subvolume in a volume and optional
@@ -96,10 +96,18 @@ func (fsa *FSAdmin) RemoveSubVolume(volume, group, name string) error {
 // Similar To:
 //  ceph fs subvolume rm <volume> --group-name=<group> <name> --force
 func (fsa *FSAdmin) ForceRemoveSubVolume(volume, group, name string) error {
-	return fsa.rmSubVolume(volume, group, name, rmFlags{force: true})
+	return fsa.RemoveSubVolumeWithFlags(volume, group, name, SubVolRmFlags{Force: true})
 }
 
-func (fsa *FSAdmin) rmSubVolume(volume, group, name string, o rmFlags) error {
+// RemoveSubVolumeWithFlags will delete a CephFS subvolume in a volume and
+// optional subvolume group. This function accepts a SubVolRmFlags type that
+// can be used to specify flags that modify the operations behavior.
+// Equivalent to RemoveSubVolume with no flags set.
+// Equivalent to ForceRemoveSubVolume if only the "Force" flag is set.
+//
+// Similar To:
+//  ceph fs subvolume rm <volume> --group-name=<group> <name> [...flags...]
+func (fsa *FSAdmin) RemoveSubVolumeWithFlags(volume, group, name string, o SubVolRmFlags) error {
 	m := map[string]string{
 		"prefix":   "fs subvolume rm",
 		"vol_name": volume,
@@ -109,7 +117,7 @@ func (fsa *FSAdmin) rmSubVolume(volume, group, name string, o rmFlags) error {
 	if group != NoGroup {
 		m["group_name"] = group
 	}
-	return fsa.marshalMgrCommand(o.Update(m)).noData().End()
+	return fsa.marshalMgrCommand(mergeFlags(m, o)).noData().End()
 }
 
 type subVolumeResizeFields struct {
@@ -264,7 +272,7 @@ func (fsa *FSAdmin) CreateSubVolumeSnapshot(volume, group, source, name string)
 // Similar To:
 //  ceph fs subvolume snapshot rm <volume> --group-name=<group> <subvolume> <name>
 func (fsa *FSAdmin) RemoveSubVolumeSnapshot(volume, group, subvolume, name string) error {
-	return fsa.rmSubVolumeSnapshot(volume, group, subvolume, name, rmFlags{})
+	return fsa.rmSubVolumeSnapshot(volume, group, subvolume, name, commonRmFlags{})
 }
 
 // ForceRemoveSubVolumeSnapshot removes the specified snapshot from the subvolume.
@@ -272,10 +280,10 @@ func (fsa *FSAdmin) RemoveSubVolumeSnapshot(volume, group, subvolume, name strin
 // Similar To:
 //  ceph fs subvolume snapshot rm <volume> --group-name=<group> <subvolume> <name> --force
 func (fsa *FSAdmin) ForceRemoveSubVolumeSnapshot(volume, group, subvolume, name string) error {
-	return fsa.rmSubVolumeSnapshot(volume, group, subvolume, name, rmFlags{force: true})
+	return fsa.rmSubVolumeSnapshot(volume, group, subvolume, name, commonRmFlags{force: true})
 }
 
-func (fsa *FSAdmin) rmSubVolumeSnapshot(volume, group, subvolume, name string, o rmFlags) error {
+func (fsa *FSAdmin) rmSubVolumeSnapshot(volume, group, subvolume, name string, o commonRmFlags) error {
 
 	m := map[string]string{
 		"prefix":    "fs subvolume snapshot rm",
@@ -287,7 +295,7 @@ func (fsa *FSAdmin) rmSubVolumeSnapshot(volume, group, subvolume, name string, o
 	if group != NoGroup {
 		m["group_name"] = group
 	}
-	return fsa.marshalMgrCommand(o.Update(m)).noData().End()
+	return fsa.marshalMgrCommand(mergeFlags(m, o)).noData().End()
 }
 
 // ListSubVolumeSnapshots returns a listing of snapshots for a given subvolume.
diff --git a/cephfs/admin/subvolumegroup.go b/cephfs/admin/subvolumegroup.go
index 0e2caf9..573c8c1 100644
--- a/cephfs/admin/subvolumegroup.go
+++ b/cephfs/admin/subvolumegroup.go
@@ -69,23 +69,23 @@ func (fsa *FSAdmin) ListSubVolumeGroups(volume string) ([]string, error) {
 // Similar To:
 //  ceph fs subvolumegroup rm <volume> <group_name>
 func (fsa *FSAdmin) RemoveSubVolumeGroup(volume, name string) error {
-	return fsa.rmSubVolumeGroup(volume, name, rmFlags{})
+	return fsa.rmSubVolumeGroup(volume, name, commonRmFlags{})
 }
 
 // ForceRemoveSubVolumeGroup will delete a subvolume group in a volume.
 // Similar To:
 //  ceph fs subvolumegroup rm <volume> <group_name> --force
 func (fsa *FSAdmin) ForceRemoveSubVolumeGroup(volume, name string) error {
-	return fsa.rmSubVolumeGroup(volume, name, rmFlags{force: true})
+	return fsa.rmSubVolumeGroup(volume, name, commonRmFlags{force: true})
 }
 
-func (fsa *FSAdmin) rmSubVolumeGroup(volume, name string, o rmFlags) error {
-	res := fsa.marshalMgrCommand(o.Update(map[string]string{
+func (fsa *FSAdmin) rmSubVolumeGroup(volume, name string, o commonRmFlags) error {
+	res := fsa.marshalMgrCommand(mergeFlags(map[string]string{
 		"prefix":     "fs subvolumegroup rm",
 		"vol_name":   volume,
 		"group_name": name,
 		"format":     "json",
-	}))
+	}, o))
 	return res.noData().End()
 }
 


Test patch:
diff --git a/cephfs/admin/subvolume_test.go b/cephfs/admin/subvolume_test.go
index b214329..31a5326 100644
--- a/cephfs/admin/subvolume_test.go
+++ b/cephfs/admin/subvolume_test.go
@@ -141,6 +141,21 @@ func TestRemoveSubVolume(t *testing.T) {
 	t.Run("force", func(t *testing.T) {
 		removeTest(t, fsa.ForceRemoveSubVolume)
 	})
+	t.Run("withFlagsEmpty", func(t *testing.T) {
+		removeTest(t, func(v, g, n string) error {
+			return fsa.RemoveSubVolumeWithFlags(v, g, n, SubVolRmFlags{})
+		})
+	})
+	t.Run("withFlagsForce", func(t *testing.T) {
+		removeTest(t, func(v, g, n string) error {
+			return fsa.RemoveSubVolumeWithFlags(v, g, n, SubVolRmFlags{Force: true})
+		})
+	})
+	t.Run("withFlagsRetainSnaps", func(t *testing.T) {
+		removeTest(t, func(v, g, n string) error {
+			return fsa.RemoveSubVolumeWithFlags(v, g, n, SubVolRmFlags{RetainSnapshots: true})
+		})
+	})
 }
 
 func TestResizeSubVolume(t *testing.T) {