Edit volume api #704

vpandey-RH · 2018-04-26T10:16:23Z

Updates #462
API to edit metadata of the specified volume.

Signed-off-by: Vishal Pandey vpandey@redhat.com

aravindavk · 2018-04-26T10:28:23Z

glusterd2/commands/volumes/commands.go

+		route.Route{
+			Name:        "VolumeEditMetadata",
+			Method:      "POST",
+			Pattern:     "/volumes/{volname}/edit-metadata",


s/edit-metadata/metadata

aravindavk · 2018-04-26T10:28:43Z

e2e/1

@@ -0,0 +1,422 @@
+package e2e


what is this file?

aravindavk · 2018-04-26T10:33:32Z

glusterd2/commands/volumes/volume-edit-metadata.go

+		restutils.SendHTTPError(ctx, w, http.StatusUnprocessableEntity, err)
+		return
+	}
+


Add one more validation to not allow Metadata set if key name starts with _. Keys starting with _ can be reserved for internal use. Refer: https://github.com/gluster/glusterd2/pull/603/files#diff-b3cf3c6dd98c4e29314e6f748f518b21R29

rishubhjain · 2018-04-26T10:36:55Z

@vpandey-RH Look at #603, I think we wont be needing a separate api rest point for editing metadata , instead we can do it in edit volume api itself.
@prashanthpai what would you suggest?

prashanthpai · 2018-04-27T09:00:17Z

@vpandey-RH Create a volume edit API as suggested by @rishubhjain.

In future, if we allow editing volume name, we can re-use it.

prashanthpai

Keep the handler simple. There is no need for transaction here. You're only updating metadata on the store. You just have to take lock on volume name.

prashanthpai · 2018-04-27T09:01:10Z

glusterd2/commands/volumes/volume-edit-metadata.go

+
+	var req api.VolEditMetadataReq
+	if err := restutils.UnmarshalRequest(r, &req); err != nil {
+		restutils.SendHTTPError(ctx, w, http.StatusUnprocessableEntity, err)


Return 400 (BadRequest)

prashanthpai · 2018-04-27T09:02:28Z

glusterd2/commands/volumes/volume-edit-metadata.go

+	// Transaction which starts self heal daemon on all nodes with atleast one brick.
+	txn := transaction.NewTxn(ctx)
+	defer txn.Cleanup()
+


This doesn't need a transaction at all. There's no need to notify clients either.

prashanthpai · 2018-05-02T10:13:48Z

glusterd2/commands/volumes/commands.go

+			Method:      "POST",
+			Pattern:     "/volumes/{volname}/edit",
+			Version:     1,
+			RequestType: utils.GetTypeString((*api.VolumeGetResp)(nil)),


Please set RequestType and ResponseType correctly.

prashanthpai · 2018-05-02T10:14:24Z

glusterd2/commands/volumes/volume-edit.go

+
+	//Lock on Volume Name
+	lock, unlock := transaction.CreateLockFuncs(volname)
+	// Taking a lock outside the txn as volinfo.Nodes() must also


This comment is not valid as there is no txn here. Remove.

prashanthpai · 2018-05-02T10:15:02Z

glusterd2/commands/volumes/volume-edit.go

+		}
+	}
+
+	v.Metadata = req.Metadata


You'll have to replace each key, value individually.

I thought we were overwriting the Metadata with every edit request?

You are over-writing individual keys only.

what I have implemented, will replace the metadata with new metadata that is sent as part of POST parameter but I get it I need to overwrite keywise.

prashanthpai · 2018-05-02T10:15:46Z

glusterd2/commands/volumes/volume-edit.go

+	txn := transaction.NewTxn(ctx)
+	defer txn.Cleanup()
+
+	txn.Nodes = v.Nodes()


Like mentioned previously, this should not be a transaction as it's only run on one node!

But I need to run a transaction step for updating the volfile ?

the volfile needs no updation as metadata isn't used for volfile generation

prashanthpai · 2018-05-03T13:08:38Z

e2e/volume_ops_test.go

+			"owner": "gd2tests",
+		},
+	}
+	_, err = client.EditVolume(volname, editMetadataReq)


You will have to check the response body. Also, test the replace key cases.

Added the check for the response as well as for the new volinfo generated after the changes.

prashanthpai · 2018-05-03T13:09:07Z

glusterd2/commands/volumes/commands.go

+			Pattern:      "/volumes/{volname}/edit",
+			Version:      1,
+			RequestType:  utils.GetTypeString((*api.VolEditReq)(nil)),
+			ResponseType: utils.GetTypeString((*api.VolumeGetResp)(nil)),


This should be api.VolumeEditResp which can be an alias.

prashanthpai · 2018-05-03T13:10:39Z

glusterd2/commands/volumes/volume-edit.go

+	}
+	if err := volume.AddOrUpdateVolumeFunc(v); err != nil {
+		logger.WithError(err).WithField(
+			"volume", v.Name).Debug("failed to store volume info")


This is a serious issue - as there is no response sent back to the client, it will hang.

Added a http error response.

prashanthpai · 2018-05-03T13:10:58Z

glusterd2/commands/volumes/volume-edit.go

+			"volume", v.Name).Debug("failed to store volume info")
+		return
+	}
+	resp := createVolumeGetResp(v)


Create a createEditVolumeResp

vpandey-RH · 2018-05-07T13:35:45Z

retest this please

prashanthpai · 2018-05-08T06:15:17Z

pkg/api/volume_resp.go

@@ -95,3 +95,6 @@ type VolumeListResp []VolumeGetResp

 // OptionGroupListResp is the response sent for a group list request.
 type OptionGroupListResp []OptionGroup
+
+// VolumeEditResp is the response sent for a edit volume request
+type VolumeEditResp VolumeGetResp


This should alias to VolumeInfo

prashanthpai · 2018-05-08T06:17:08Z

glusterd2/commands/volumes/volume-edit.go

+	defer unlock(ctx)
+
+	//validate volume name
+	v, err := volume.GetVolume(volname)


Use volinfo as variable name, to maintain consistency with rest of he codebase.

prashanthpai · 2018-05-08T06:20:40Z

glusterd2/commands/volumes/commands.go

@@ -127,6 +127,14 @@ func (c *Command) Routes() route.Routes {
 			Pattern:     "/volfiles",
 			Version:     1,
 			HandlerFunc: volfilesListHandler},
+		route.Route{


You have to regenerate the API documentation and include the changes that'll be done on doc/endpoints.md in this PR:

# Start glusterd2 $ curl -o endpoints.json -s -X GET http://127.0.0.1:24007/endpoints $ go build pkg/tools/generate-doc.go $ ./generate-doc

prashanthpai · 2018-05-08T06:22:21Z

e2e/volume_ops_test.go

+		brickPaths = append(brickPaths, brickPath)
+	}
+
+	createReq := api.VolCreateReq{


Do not create a new volume. Please re-use the volume used in other tests. See example in line 56 to line 62.

prashanthpai · 2018-05-08T06:23:27Z

Also, refer to the issue this fixes in the PR message on top. Updates #<issue-number>

prashanthpai · 2018-05-10T04:36:38Z

pkg/api/volume_req.go

+// VolEditReq represents a volume metadata edit request
+type VolEditReq struct {
+	Metadata    map[string]string `json:"metadata"`
+	MetadataDel bool              `json:"metadatadel"`


Rename this as DeleteMetadata and json string as delete-metadata

prashanthpai · 2018-05-10T05:10:44Z

e2e/volume_ops_test.go

+	volinfo, err = client.Volumes(volname)
+	r.Nil(err)
+	err = validateVolumeEdit(volinfo[0], editMetadataReq, resp)
+	r.Nil(err)


This is good work and the kind of tests I want to see in all PRs 👍

P.S: You can remove these extra lines.

@prashanthpai Which extra lines are you referring to here ?

The blank ones after this

Yeah, handled it.

prashanthpai · 2018-05-10T05:12:07Z

glustercli/cmd/volume.go

@@ -27,6 +27,7 @@ const (
 	helpVolumeListCmd   = "List all Gluster Volumes"
 	helpVolumeStatusCmd = "Get Gluster Volume Status"
 	helpVolumeExpandCmd = "Expand a Gluster Volume"
+	helpVolumeEditCmd   = "Edit Volinfo properties of a Gluster Volume"


This isn't true. Right now, this is limited to metadata only. In future, we may allow renaming volumes.

It can be - "Edit metadata (key-value pairs) of a volume. Glusterd2 will not interpret these key and value in any way"

prashanthpai · 2018-05-10T05:13:07Z

glustercli/cmd/volume.go

+	// Volume Edit
+	volumeEditCmd.Flags().StringVar(&flagCmdMetadataKey, "key", "", "Metadata Key")
+	volumeEditCmd.Flags().StringVar(&flagCmdMetadataValue, "value", "", "Metadata Value")
+	volumeEditCmd.Flags().BoolVar(&flagCmdDeleteMetadata, "delete", false, "Metadata Delete Flag")


The help string can be "Delete metadata"

prashanthpai · 2018-05-10T05:15:50Z

glustercli/cmd/volume.go

+	// Volume Edit
+	volumeEditCmd.Flags().StringVar(&flagCmdMetadataKey, "key", "", "Metadata Key")
+	volumeEditCmd.Flags().StringVar(&flagCmdMetadataValue, "value", "", "Metadata Value")
+	volumeEditCmd.Flags().BoolVar(&flagCmdDeleteMetadata, "delete", false, "Metadata Delete Flag")


As key and value flags must always be present, you can use MarkFlagRequired API provided by cobra.

prashanthpai · 2018-05-10T05:17:47Z

glustercli/cmd/volume.go

@@ -412,3 +424,28 @@ var volumeExpandCmd = &cobra.Command{
 		fmt.Printf("%s Volume expanded successfully\n", vol.Name)
 	},
 }
+
+var volumeEditCmd = &cobra.Command{
+	Use:   "edit <volname> flags",


edit-metadata <volname> --key <key> --value <value> [--delete]

keys can be edited/deleted one at a time. Signed-off-by: Vishal Pandey <vpandey@redhat.com>

vpandey-RH · 2018-05-10T10:15:29Z

retest this please

edit metadata

9471d68

aravindavk reviewed Apr 26, 2018

View reviewed changes

vpandey-RH force-pushed the edit-volume-metadata-API branch 6 times, most recently from 35523cc to 7384518 Compare April 26, 2018 13:37

prashanthpai suggested changes Apr 27, 2018

View reviewed changes

vpandey-RH force-pushed the edit-volume-metadata-API branch from 7384518 to 6dcab26 Compare April 30, 2018 09:02

Merge branch 'master' into edit-volume-metadata-API

862088d

vpandey-RH force-pushed the edit-volume-metadata-API branch from 6dcab26 to 862088d Compare April 30, 2018 09:10

vpandey-RH changed the title ~~Edit volume metadata api~~ Edit volume api May 2, 2018

prashanthpai suggested changes May 2, 2018

View reviewed changes

prashanthpai suggested changes May 3, 2018

View reviewed changes

vpandey-RH force-pushed the edit-volume-metadata-API branch 2 times, most recently from 6fae0b6 to f002609 Compare May 3, 2018 16:36

prashanthpai suggested changes May 8, 2018

View reviewed changes

Merge branch 'master' into edit-volume-metadata-API

6c3a1ef

vpandey-RH force-pushed the edit-volume-metadata-API branch from f002609 to 6c3a1ef Compare May 8, 2018 07:41

Adding support for metadata keys deletion

11fece9

vpandey-RH force-pushed the edit-volume-metadata-API branch from 59c1fd3 to 11fece9 Compare May 8, 2018 10:47

prashanthpai suggested changes May 10, 2018

View reviewed changes

Add CLI command for editing and deleting metadata keys. Currently

406542b

keys can be edited/deleted one at a time. Signed-off-by: Vishal Pandey <vpandey@redhat.com>

vpandey-RH force-pushed the edit-volume-metadata-API branch from dd54a87 to 406542b Compare May 10, 2018 07:21

Merge branch 'master' into edit-volume-metadata-API

f131aff

prashanthpai approved these changes May 10, 2018

View reviewed changes

prashanthpai merged commit f3f1029 into gluster:master May 10, 2018

Edit volume api #704

Edit volume api #704

Conversation

vpandey-RH commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishubhjain commented Apr 26, 2018

prashanthpai commented Apr 27, 2018

prashanthpai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpandey-RH May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpandey-RH commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanthpai commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpandey-RH May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpandey-RH commented May 10, 2018

vpandey-RH commented Apr 26, 2018 •

edited

Loading

vpandey-RH May 2, 2018 •

edited

Loading

vpandey-RH May 10, 2018 •

edited

Loading