From 4ecc175f1af2fe63e97b1165de2a8e9e0a4e8863 Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Wed, 3 Oct 2018 08:03:22 +0530 Subject: [PATCH] Refactor flags/filters applied to set volume options '--advanced' in the CLI and REST API during volume create doesn't intuitively and clearly indicate that it's for the volume options. A new user may read this as creation of an "advanced volume" which isn't true. The flags have now been prepended with 'allow' in the fields of request structs. The API has been renamed as 'allow--options'. Also, created 'VolOptionFlags' struct for flags of options and embeded it in all request structs. Signed-off-by: Prashanth Pai --- e2e/glustershd_test.go | 12 ++++----- e2e/quota_enable.go | 4 +-- e2e/volume_ops_test.go | 26 +++++++++---------- glustercli/cmd/volume-create.go | 21 +++++++++------ glustercli/cmd/volume-set.go | 11 +++++--- glusterd2/commands/volumes/common.go | 8 +++--- .../commands/volumes/optiongroup-create.go | 2 +- glusterd2/commands/volumes/volume-create.go | 2 +- glusterd2/commands/volumes/volume-option.go | 2 +- pkg/api/volume_req.go | 18 ++++++++----- 10 files changed, 59 insertions(+), 47 deletions(-) diff --git a/e2e/glustershd_test.go b/e2e/glustershd_test.go index b069c4f6..98c623cc 100644 --- a/e2e/glustershd_test.go +++ b/e2e/glustershd_test.go @@ -57,7 +57,7 @@ func testSelfHeal(t *testing.T, tc *testCluster) { var optionReq api.VolOptionReq optionReq.Options = map[string]string{"replicate.self-heal-daemon": "on"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(vol1.Name, optionReq)) r.True(isProcessRunning(pidpath), "glustershd is not running") @@ -69,7 +69,7 @@ func testSelfHeal(t *testing.T, tc *testCluster) { r.Nil(client.VolumeStop(vol1.Name), "Volume stop failed") optionReq.Options = map[string]string{"replicate.self-heal-daemon": "off"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(vol1.Name, optionReq)) r.False(isProcessRunning(pidpath), "glustershd is still running") @@ -126,11 +126,11 @@ func testGranularEntryHeal(t *testing.T, tc *testCluster) { var optionReq api.VolOptionReq optionReq.Options = map[string]string{"replicate.granular-entry-heal": "enable"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(volname, optionReq)) optionReq.Options = map[string]string{"replicate.self-heal-daemon": "off"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(volname, optionReq)) r.False(isProcessRunning(pidpath), "glustershd is still running") @@ -170,7 +170,7 @@ func testGranularEntryHeal(t *testing.T, tc *testCluster) { } optionReq.Options = map[string]string{"replicate.granular-entry-heal": "disable"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(volname, optionReq)) // Stop Volume @@ -178,7 +178,7 @@ func testGranularEntryHeal(t *testing.T, tc *testCluster) { r.Nil(client.VolumeStart(volname, false), "volume start failed") optionReq.Options = map[string]string{"replicate.granular-entry-heal": "enable"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.NotNil(client.VolumeSet(volname, optionReq)) err = syscall.Unmount(mntPath, 0) diff --git a/e2e/quota_enable.go b/e2e/quota_enable.go index e98717ac..65ab20be 100644 --- a/e2e/quota_enable.go +++ b/e2e/quota_enable.go @@ -79,7 +79,7 @@ func testQuotaEnable(t *testing.T, tc *testCluster) { quotaKey := "quota.enable" var optionReqOff api.VolOptionReq - optionReqOff.Advanced = true + optionReqOff.AllowAdvanced = true optionReqOff.Options = map[string]string{quotaKey: "off"} @@ -91,7 +91,7 @@ func testQuotaEnable(t *testing.T, tc *testCluster) { r.False(isProcessRunning(pidpath)) var optionReqOn api.VolOptionReq - optionReqOn.Advanced = true + optionReqOn.AllowAdvanced = true // Enable quota quotaKey = "quota.enable" diff --git a/e2e/volume_ops_test.go b/e2e/volume_ops_test.go index effbf1d4..ae74f03f 100644 --- a/e2e/volume_ops_test.go +++ b/e2e/volume_ops_test.go @@ -532,12 +532,10 @@ func TestVolumeOptions(t *testing.T) { }, }, Force: true, - // XXX: Setting advanced, as all options are advanced by default - // TODO: Remove this later if the default changes - VolOptionReq: api.VolOptionReq{ - Advanced: true, - }, } + // XXX: Setting advanced, as all options are advanced by default + // TODO: Remove this later if the default changes + createReq.AllowAdvanced = true validOpKeys := []string{"gfproxy.afr.eager-lock", "afr.eager-lock"} invalidOpKeys := []string{"..eager-lock", "a.b.afr.eager-lock", "afr.non-existent", "eager-lock"} @@ -587,7 +585,7 @@ func TestVolumeOptions(t *testing.T) { var optionReq api.VolOptionReq // XXX: Setting advanced, as all options are advanced by default // TODO: Remove this later if the default changes - optionReq.Advanced = true + optionReq.AllowAdvanced = true settableKey := "afr.use-compound-fops" optionReq.Options = map[string]string{settableKey: "on"} @@ -647,10 +645,11 @@ func TestVolumeOptions(t *testing.T) { {Name: "opt3", OnValue: "off"}}, Description: "Test profile 2", }, - // XXX: Setting advanced, as all options are advanced by default - // TODO: Remove this later if the default changes - Advanced: true, } + // XXX: Setting advanced, as all options are advanced by default + // TODO: Remove this later if the default changes + optionGroupReq.AllowAdvanced = true + err = client.OptionGroupCreate(optionGroupReq) r.NotNil(err) @@ -662,10 +661,11 @@ func TestVolumeOptions(t *testing.T) { }, Description: "Test profile 2", }, - // XXX: Setting advanced, as all options are advanced by default - // TODO: Remove this later if the default changes - Advanced: true, } + // XXX: Setting advanced, as all options are advanced by default + // TODO: Remove this later if the default changes + optionGroupReq.AllowAdvanced = true + err = client.OptionGroupCreate(optionGroupReq) r.Nil(err) @@ -820,7 +820,7 @@ func testShdOnVolumeStartAndStop(t *testing.T, tc *testCluster) { var optionReq api.VolOptionReq optionReq.Options = map[string]string{"replicate.self-heal-daemon": "on"} - optionReq.Advanced = true + optionReq.AllowAdvanced = true r.Nil(client.VolumeSet(vol1.Name, optionReq)) diff --git a/glustercli/cmd/volume-create.go b/glustercli/cmd/volume-create.go index 1344b1d8..db79250f 100644 --- a/glustercli/cmd/volume-create.go +++ b/glustercli/cmd/volume-create.go @@ -62,9 +62,12 @@ func init() { volumeCreateCmd.Flags().BoolVar(&flagCreateForce, "force", false, "Force") volumeCreateCmd.Flags().StringSliceVar(&flagCreateVolumeOptions, "options", nil, "Volume options in the format option:value,option:value") - volumeCreateCmd.Flags().BoolVar(&flagCreateAdvOpts, "advanced", false, "Allow advanced options") - volumeCreateCmd.Flags().BoolVar(&flagCreateExpOpts, "experimental", false, "Allow experimental options") - volumeCreateCmd.Flags().BoolVar(&flagCreateDepOpts, "deprecated", false, "Allow deprecated options") + + // set volume options during volume create + volumeCreateCmd.Flags().BoolVar(&flagCreateAdvOpts, "allow-advanced-options", false, "Allow setting advanced volume options") + volumeCreateCmd.Flags().BoolVar(&flagCreateExpOpts, "allow-experimental-options", false, "Allow setting experimental volume options") + volumeCreateCmd.Flags().BoolVar(&flagCreateDepOpts, "allow-deprecated-options", false, "Allow setting deprecated volume options") + volumeCreateCmd.Flags().BoolVar(&flagReuseBricks, "reuse-bricks", false, "Reuse bricks") volumeCreateCmd.Flags().BoolVar(&flagAllowRootDir, "allow-root-dir", false, "Allow root directory") volumeCreateCmd.Flags().BoolVar(&flagAllowMountAsBrick, "allow-mount-as-brick", false, "Allow mount as bricks") @@ -259,10 +262,12 @@ func volumeCreateCmdRun(cmd *cobra.Command, args []string) { Subvols: subvols, Force: flagCreateForce, VolOptionReq: api.VolOptionReq{ - Options: options, - Advanced: flagCreateAdvOpts, - Experimental: flagCreateExpOpts, - Deprecated: flagCreateDepOpts, + Options: options, + VolOptionFlags: api.VolOptionFlags{ + AllowAdvanced: flagCreateAdvOpts, + AllowExperimental: flagCreateExpOpts, + AllowDeprecated: flagCreateDepOpts, + }, }, Flags: flags, @@ -304,6 +309,6 @@ func addThinArbiter(req *api.VolCreateReq, thinArbiter string) error { req.Options = map[string]string{ "replicate.thin-arbiter": thinArbiter, } - req.Advanced = true + req.AllowAdvanced = true return nil } diff --git a/glustercli/cmd/volume-set.go b/glustercli/cmd/volume-set.go index 34e0d304..fd63eae2 100644 --- a/glustercli/cmd/volume-set.go +++ b/glustercli/cmd/volume-set.go @@ -78,10 +78,13 @@ func volumeOptionJSONHandler(cmd *cobra.Command, volname string, options []strin } err := client.VolumeSet(volname, api.VolOptionReq{ - Options: vopt, - Advanced: flagSetAdv, - Experimental: flagSetExp, - Deprecated: flagSetDep, + Options: vopt, + VolOptionFlags: api.VolOptionFlags{ + AllowAdvanced: flagSetAdv, + AllowExperimental: flagSetExp, + AllowDeprecated: flagSetDep, + }, }) + return err } diff --git a/glusterd2/commands/volumes/common.go b/glusterd2/commands/volumes/common.go index 1375f61a..36cde6fc 100644 --- a/glusterd2/commands/volumes/common.go +++ b/glusterd2/commands/volumes/common.go @@ -29,7 +29,7 @@ const volumeIDXattrKey = "trusted.glusterfs.volume-id" // validateOptions validates if the options and their values are valid and can // be set on a volume. -func validateOptions(opts map[string]string, adv, exp, dep bool) error { +func validateOptions(opts map[string]string, flags api.VolOptionFlags) error { for k, v := range opts { o, err := xlator.FindOption(k) @@ -41,13 +41,13 @@ func validateOptions(opts map[string]string, adv, exp, dep bool) error { case !o.IsSettable(): return fmt.Errorf("Option %s cannot be set", k) - case o.IsAdvanced() && !adv: + case o.IsAdvanced() && !flags.AllowAdvanced: return fmt.Errorf("Option %s is an advanced option. To set it pass the advanced flag", k) - case o.IsExperimental() && !exp: + case o.IsExperimental() && !flags.AllowExperimental: return fmt.Errorf("Option %s is an experimental option. To set it pass the experimental flag", k) - case o.IsDeprecated() && !dep: + case o.IsDeprecated() && !flags.AllowDeprecated: // TODO: Return deprecation version and alternative option if available return fmt.Errorf("Option %s will be deprecated in future releases. To set it pass the deprecated flag", k) } diff --git a/glusterd2/commands/volumes/optiongroup-create.go b/glusterd2/commands/volumes/optiongroup-create.go index d14fa0b2..9ef1ca87 100644 --- a/glusterd2/commands/volumes/optiongroup-create.go +++ b/glusterd2/commands/volumes/optiongroup-create.go @@ -16,7 +16,7 @@ func validateOptionSet(req api.OptionGroupReq) error { for _, o := range req.Options { o1[o.Name] = o.OnValue } - return validateOptions(o1, req.Advanced, req.Experimental, req.Deprecated) + return validateOptions(o1, req.VolOptionFlags) } func optionGroupCreateHandler(w http.ResponseWriter, r *http.Request) { diff --git a/glusterd2/commands/volumes/volume-create.go b/glusterd2/commands/volumes/volume-create.go index 57f04dc2..8ddd4db5 100644 --- a/glusterd2/commands/volumes/volume-create.go +++ b/glusterd2/commands/volumes/volume-create.go @@ -143,7 +143,7 @@ func volumeCreateHandler(w http.ResponseWriter, r *http.Request) { return } - if err := validateOptions(req.Options, req.Advanced, req.Experimental, req.Deprecated); err != nil { + if err := validateOptions(req.Options, req.VolOptionFlags); err != nil { restutils.SendHTTPError(ctx, w, http.StatusBadRequest, err) return } diff --git a/glusterd2/commands/volumes/volume-option.go b/glusterd2/commands/volumes/volume-option.go index c0e6d3e3..38b70c9c 100644 --- a/glusterd2/commands/volumes/volume-option.go +++ b/glusterd2/commands/volumes/volume-option.go @@ -34,7 +34,7 @@ func optionSetValidate(c transaction.TxnCtx) error { // TODO: Validate op versions of the options. Either here or inside // validateOptions. - if err := validateOptions(options, req.Advanced, req.Experimental, req.Deprecated); err != nil { + if err := validateOptions(options, req.VolOptionFlags); err != nil { return fmt.Errorf("validation failed for volume option: %s", err.Error()) } diff --git a/pkg/api/volume_req.go b/pkg/api/volume_req.go index b94e3479..4b096836 100644 --- a/pkg/api/volume_req.go +++ b/pkg/api/volume_req.go @@ -62,12 +62,18 @@ type VolCreateReq struct { VolOptionReq } +// VolOptionFlags is set of flags that allow/disallow setting certain kinds +// of volume options. +type VolOptionFlags struct { + AllowAdvanced bool `json:"allow-advanced-options,omitempty"` + AllowExperimental bool `json:"allow-experimental-options,omitempty"` + AllowDeprecated bool `json:"allow-deprecated-options,omitempty"` +} + // VolOptionReq represents an incoming request to set volume options type VolOptionReq struct { - Options map[string]string `json:"options"` - Advanced bool `json:"advanced,omitempty"` - Experimental bool `json:"experimental,omitempty"` - Deprecated bool `json:"deprecated,omitempty"` + Options map[string]string `json:"options"` + VolOptionFlags } // VolOptionResetReq represents a request to reset volume options @@ -107,9 +113,7 @@ type OptionGroup struct { // OptionGroupReq represents a request to create a new option group type OptionGroupReq struct { OptionGroup - Advanced bool `json:"advanced,omitempty"` - Experimental bool `json:"experimental,omitempty"` - Deprecated bool `json:"deprecated,omitempty"` + VolOptionFlags } // ClientStatedump uniquely identifies a client (only gfapi) connected to