Updates #456: Add Volume Reset Rest API by devyanikota · Pull Request #545 · gluster/glusterd2

devyanikota · 2018-01-31T19:25:19Z

Volume reset replaces the existing value of an option key with
the new value.

Signed-off-by: Devyani Kota devyanikota@gmail.com

devyanikota · 2018-01-31T19:28:18Z

WIP PR, wasn't getting time to work on it.
Haven't tested, not up for review yet.

aravindavk

Volume Reset logic is very simple. No validations required except checking in volinfo.Options

For example, if "changelog.changelog" is set to "on" then it will be available in volinfo.Options["changelog.changelog"]. So if Reset comes to "changelog.changelog" then just delete from volinfo.Options, storeVolume(which regenerates the volfiles) and then Notify.

If reset comes to a key which is not set, log and ignore it.(log message, Option trying to reset is not set or invalid option")

if _, ok := volinfo.Options[k]; ok {
// Delete from volinfo
}

Volfile generation is not incremental, full volfile will be regenerated. If option key is reset then it is removed from volinfo.Options so volgen will automatically add default value and generate volfile.

aravindavk · 2018-02-01T09:06:18Z

glusterd2/volgen/volgen.go

 }

+// DeleteClientVoloption deletes the client vol option
+func DeleteClientVoloption(vol *volume.Volinfo) error {


Not required with volgen2

@aravindavk worked on the suggested changes. PR is ready for review.
thanks :)

atinmu · 2018-02-01T09:54:40Z

There's one more catch which I highlighted in one of our stand up calls. Take a look at the volopt flags like VOLOPT_FLAG_FORCE , VOLOPT_FLAG_NEVER_RESET . We need to have special handling for those options and hence a force option in reset API is also required.

…

On Thu, Feb 1, 2018 at 2:46 PM, Aravinda VK ***@***.***> wrote: ***@***.**** requested changes on this pull request. Volume Reset logic is very simple. No validations required except checking in volinfo.Options For example, if "changelog.changelog" is set to "on" then it will be available in volinfo.Options["changelog.changelog"]. So if Reset comes to "changelog.changelog" then just delete from volinfo.Options, storeVolume(which regenerates the volfiles) and then Notify. If reset comes to a key which is not set, log and ignore it.(log message, Option trying to reset is not set or invalid option") if _, ok := volinfo.Options[k]; ok { // Delete from volinfo } Volfile generation is not incremental, full volfile will be regenerated. If option key is reset then it is removed from volinfo.Options so volgen will automatically add default value and generate volfile. ------------------------------ In glusterd2/volgen/volgen.go <#545 (comment)>: > @@ -80,6 +80,21 @@ func DeleteClientVolfile(vol *volume.Volinfo) error { return nil } +// DeleteClientVoloption deletes the client vol option +func DeleteClientVoloption(vol *volume.Volinfo) error { Not required with volgen2 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#545 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGp7mKlXZsHAVRpovac0N9agFW-8TWIcks5tQYDTgaJpZM4R0g1K> .

atinmu · 2018-02-18T04:08:33Z

@devyanikota Checking back on the progress on this PR. Could you update with the status?

aravindavk · 2018-03-14T10:45:33Z

glusterd2/commands/volumes/volume-reset.go

+	"github.com/pborman/uuid"
+)
+
+func registerVolResetStepFuncs() {


Not required, same name can be used as used in other places

aravindavk · 2018-03-14T10:47:03Z

glusterd2/commands/volumes/volume-reset.go

+		unlock,
+	}
+
+	for k, v := range req.Options {


why this is required?

This part is where we Re- set the options with new values?

aravindavk · 2018-03-14T10:47:47Z

glusterd2/commands/volumes/volume-reset.go

+	txn := transaction.NewTxn(ctx)
+	for _, k := range req.Options {
+		if _, ok := volinfo.Options[k]; ok {
+			err = txn.Ctx.Delete(k)


why delete from txn.Ctx? should be deleted from volinfo.Options

ack! thanks :)

aravindavk · 2018-03-14T10:50:23Z

glusterd2/commands/volumes/volume-reset.go

+			}
+		}
+	}
+	defer txn.Cleanup()


Maintain a flag saying minimum one option reset. Return from here if no option is reset.

aravindavk · 2018-03-14T10:52:35Z

glusterd2/commands/volumes/volume-reset.go

+	txn := transaction.NewTxn(ctx)
+	for _, k := range req.Options {
+		if _, ok := volinfo.Options[k]; ok {
+			err = txn.Ctx.Delete(k)


Before deleting from volinfo, check is required for VOLOPT_FLAG_FORCE and VOLOPT_FLAG_NEVER_RESET #545 (comment)

Error if VOLOPT_FLAG_NEVER_RESET is set and option is trying to reset.

Only allow Reset if force is used in API request in case of VOLOPT_FLAG_FORCE

@aravindavk am not quite sure how to get these volopt flags' values. I intend to ask if those flags happen to be part of the volinfo or ..?
Checking glusterfs code, I found them listed as follows:

{ .key = VKEY_FEATURES_QUOTA, .voltype = "features/marker", .option = "quota", .value = "off", .type = NO_DOC, .flags = VOLOPT_FLAG_NEVER_RESET, .op_version = 1 },

aravindavk · 2018-03-14T10:55:42Z

glusterd2/volume/store-utils.go

 	return e
 }

+// //DeleteVolOption passes the option name to store to delete the option key


commented lines

remove commented lines

aravindavk · 2018-03-14T10:57:58Z

glusterd2/commands/volumes/volume-reset.go

+		return
+	}
+
+	var req api.VolOptionReq


This is different from Volume Set. Value will not be sent in request.

type VolOptionResetReq struct {
Options []string json:"options"
Force bool json:"force"
}

atinmu · 2018-04-05T05:00:34Z

@devyanikota @aravindavk Has all the comments been addressed? Can we try to get this PR in by end of this week?

aravindavk · 2018-04-16T03:57:48Z

glusterd2/commands/volumes/volume-reset.go

+	success := false
+	for _, k := range req.Options {
+		if _, ok := volinfo.Options[k]; ok {
+			if !volinfo.Options[k].VOLOPT_FLAG_NEVER_RESET {


volinfo.Options is map[string]string. This usage is wrong. You have to do xlator.FindOption(k) and then check for these flag. Expose function IsNeverReset similar to IsSettable in $SRC/glusterd2/xlator/options/options.go.

aravindavk · 2018-04-16T04:01:14Z

glusterd2/commands/volumes/volume-reset.go

+	for _, k := range req.Options {
+		if _, ok := volinfo.Options[k]; ok {
+			if !volinfo.Options[k].VOLOPT_FLAG_NEVER_RESET {
+				if !volinfo.Options[k].VOLOPT_FLAG_FORCE {


Same as above, expose function as IsForceRequired in options.go

aravindavk · 2018-04-25T05:24:25Z

@devyanikota any blockers for this patch?

devyanikota · 2018-05-04T10:12:45Z

retest this please

aravindavk · 2018-05-04T10:04:25Z

glusterd2/commands/volumes/volume-reset.go

+	}
+	// Check if an option was reset, else return.
+	if !success {
+		return


Returning without any http response is bad, Add success response and return

aravindavk · 2018-05-04T10:08:31Z

glusterd2/commands/volumes/volume-reset.go

+			if err != nil {
+				logger.WithError(err)
+			} else if op.IsNeverReset() {
+				logger.WithError(err).Error("Option %s needs NEVER_RESET flag to be set", k)


Http error required here. Return from here in case of failure. Message can be like "Reserved option, can't be reset"

aravindavk · 2018-05-04T10:08:59Z

glusterd2/commands/volumes/volume-reset.go

+					delete(volinfo.Options, k)
+					success = true
+				} else {
+					logger.WithError(err).Error("Option %s needs a force flag to be set", k)


Http error required. return after the error

aravindavk · 2018-05-04T10:09:30Z

glusterd2/commands/volumes/volume-reset.go

+
+	txn := transaction.NewTxn(ctx)
+	// Delete the option after checking for volopt flags
+	success := false


variable name "success" is very confusing here

aravindavk · 2018-05-04T10:09:55Z

glusterd2/commands/volumes/volume-reset.go

+				}
+			}
+		} else {
+			logger.WithError(err).Error("Option %s trying to reset is not set or invalid option", k)


http error and return

aravindavk · 2018-05-04T10:12:36Z

glusterd2/commands/volumes/volume-reset.go

+				} else {
+					logger.WithError(err).Error("Option %s needs a force flag to be set", k)
+				}
+			}


else is required here. Valid case, add else {delete(volinfo.Options, k)}

aravindavk · 2018-05-04T10:14:49Z

glusterd2/commands/volumes/volume-reset.go

+	}
+
+	// Reset the Options with new values
+	for key, value := range req.Options {


This step is not required. reseted options already removed above.

aravindavk · 2018-05-04T10:15:46Z

glusterd2/volume/store-utils.go

 	return e
 }

+// //DeleteVolOption passes the option name to store to delete the option key


remove commented lines

aravindavk · 2018-05-04T10:17:06Z

glusterd2/commands/volumes/volume-reset.go

+	txn.Steps = []*transaction.Step{
+		lock,
+		{
+			DoFunc: "vol-reset.Store",


Reuse the name from other files for "storeVolume", and remove register function

aravindavk · 2018-05-04T10:17:14Z

glusterd2/commands/volumes/volume-reset.go

+			Nodes:  []uuid.UUID{gdctx.MyUUID},
+		},
+		{
+			DoFunc: "vol-reset.NotifyVolfileChange",


Same as above

Volume reset replaces the existing value of an option key with the new value. Signed-off-by: Devyani Kota <devyanikota@gmail.com>

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

aravindavk · 2018-05-04T12:14:00Z

glusterd2/commands/volumes/volume-reset.go

+			op, err := xlator.FindOption(k)
+			// If key exists, check for NEVER_RESET and FORCE flags
+			if err != nil {
+				restutils.SendHTTPError(ctx, http.StatusBadRequest, err)


missing return after this line

aravindavk · 2018-05-04T12:15:23Z

glusterd2/commands/volumes/volume-reset.go

+
+	txn := transaction.NewTxn(ctx)
+	// Delete the option after checking for volopt flags
+	op_reset := false


s/o_reset/opReset

aravindavk · 2018-05-04T12:18:21Z

glusterd2/commands/volumes/volume-reset.go

+			op, err := xlator.FindOption(k)
+			// If key exists, check for NEVER_RESET and FORCE flags
+			if err != nil {
+				restutils.SendHTTPError(ctx, w, http.StatusBadRequest, err)


missing return after this line

aravindavk · 2018-05-04T12:19:09Z

glusterd2/commands/volumes/volume-reset.go

+			// If key exists, check for NEVER_RESET and FORCE flags
+			if err != nil {
+				restutils.SendHTTPError(ctx, w, http.StatusBadRequest, err)
+			} else if op.IsNeverReset() {


Since all these conditions contains return statement. Skip else and just use if op.IsNeverReset in new line

aravindavk · 2018-05-04T12:19:17Z

glusterd2/commands/volumes/volume-reset.go

+				errMsg := "Reserved option, can't be reset"
+				restutils.SendHTTPError(ctx, w, http.StatusBadRequest, errMsg)
+				return
+			} else if op.IsForceRequired() {


aravindavk · 2018-05-04T12:21:04Z

glusterd2/commands/volumes/volume-reset.go

+					restutils.SendHTTPError(ctx, w, http.StatusBadRequest, errMsg)
+					return
+				}
+			} else {


skip else statement and add this in next line

aravindavk · 2018-05-04T12:21:39Z

glusterd2/commands/volumes/volume-reset.go

+			}
+		} else {
+			errMsg := "Option trying to reset is not set or invalid option"
+			restutils.SendHTTPError(ctx, w, http.StatusBadRequest, errMsg)


missing return after this line

aravindavk · 2018-05-04T12:22:37Z

glusterd2/commands/volumes/volume-reset.go

+	}
+	// Check if an option was reset, else return.
+	if !op_reset {
+		restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)


send volinfo.Options instead of nil as in the last line.

devyanikota · 2018-05-04T12:28:08Z

retest this please

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

aravindavk

Looks good to me.

devyanikota force-pushed the vreset branch from aa56fc4 to c095922 Compare January 31, 2018 19:26

devyanikota changed the title ~~Updates #456: Add Volume Reset Rest API~~ WIP: Updates #456: Add Volume Reset Rest API Jan 31, 2018

devyanikota force-pushed the vreset branch from c095922 to 0ce7251 Compare February 1, 2018 07:22

aravindavk suggested changes Feb 1, 2018

View reviewed changes

atinmu assigned devyanikota Feb 6, 2018

atinmu requested a review from prashanthpai February 6, 2018 02:27

devyanikota force-pushed the vreset branch 2 times, most recently from aa6f4f9 to 80b3c08 Compare February 8, 2018 18:18

devyanikota force-pushed the vreset branch 3 times, most recently from 5b91287 to 4ddbbd5 Compare February 26, 2018 08:12

devyanikota force-pushed the vreset branch 10 times, most recently from 0d0eed4 to 8ec85c2 Compare March 13, 2018 14:30

devyanikota changed the title ~~WIP: Updates #456: Add Volume Reset Rest API~~ Updates #456: Add Volume Reset Rest API Mar 14, 2018

aravindavk suggested changes Mar 14, 2018

View reviewed changes

devyanikota force-pushed the vreset branch from 8ec85c2 to e9c1ba2 Compare March 20, 2018 16:56

devyanikota force-pushed the vreset branch 2 times, most recently from b0bd68d to 73071bf Compare April 5, 2018 20:44

devyanikota force-pushed the vreset branch from 73071bf to 0b3f3cf Compare April 5, 2018 21:18

aravindavk suggested changes Apr 16, 2018

View reviewed changes

devyanikota force-pushed the vreset branch 4 times, most recently from 6a4a6ab to 3ed9871 Compare May 4, 2018 10:06

aravindavk suggested changes May 4, 2018

View reviewed changes

devyanikota force-pushed the vreset branch 2 times, most recently from d692b48 to b466f3a Compare May 4, 2018 12:15

devyanikota added 5 commits May 4, 2018 17:48

Updates gluster#456: Add Volume Reset Rest API

bd81166

Volume reset replaces the existing value of an option key with the new value. Signed-off-by: Devyani Kota <devyanikota@gmail.com>

Add VolumeOptionResetReq struct to represent a reset option request

d7ba58b

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

Add checks for volopt flags

35b1ec1

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

Options: Add checks for NEVER_RESET and FORCE flags for options

dd67053

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

Add checks for NEVER_RESET and FORCE flags for options to be reset

a8cf201

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

devyanikota force-pushed the vreset branch from b466f3a to d7eb3dd Compare May 4, 2018 12:19

aravindavk suggested changes May 4, 2018

View reviewed changes

devyanikota force-pushed the vreset branch 2 times, most recently from 5e55d53 to e50cf11 Compare May 4, 2018 13:22

Add HTTP response and other typo fixes

575d3c7

Signed-off-by: Devyani Kota <devyanikota@gmail.com>

devyanikota force-pushed the vreset branch from e50cf11 to 575d3c7 Compare May 4, 2018 13:27

Merge branch 'master' into vreset

15a3d7c

aravindavk approved these changes May 8, 2018

View reviewed changes

Merge branch 'master' into vreset

5111ee2

aravindavk merged commit 0feaad9 into gluster:master May 9, 2018

devyanikota deleted the vreset branch May 9, 2018 08:45

Conversation

devyanikota commented Jan 31, 2018

Uh oh!

devyanikota commented Jan 31, 2018

Uh oh!

aravindavk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atinmu commented Feb 1, 2018 via email

Uh oh!

atinmu commented Feb 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atinmu commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aravindavk commented Apr 25, 2018

Uh oh!

devyanikota commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!