Edit Zone API #603

rishubhjain · 2018-03-15T16:26:51Z

No description provided.

aravindavk

Looks good. A few minor comments

aravindavk · 2018-03-15T16:37:53Z

plugins/device/api/req.go

@@ -15,3 +15,7 @@ const (
 type AddDeviceReq struct {
 	Devices []string `json:"devices"`
 }
+
+type EditGroupReq struct {


Not required. group is already part of URL

aravindavk · 2018-03-15T16:38:24Z

plugins/device/init.go

+			Method:       "POST",
+			Pattern:      "/peers/{peerid}/group/{group_id}",
+			Version:      1,
+			RequestType:  utils.GetTypeString((*deviceapi.EditGroupReq)(nil)),


aravindavk · 2018-03-15T16:39:13Z

plugins/device/rest.go

+	ctx := r.Context()
+	logger := gdctx.GetReqLogger(ctx)
+
+	req := new(deviceapi.EditGroupReq)


Not required

aravindavk · 2018-03-15T16:41:27Z

plugins/device/rest.go

+	}
+	err = txn.Ctx.Set("peerid", peerID)
+	if err != nil {
+		logger.WithError(err).Error("Failed to set data for transaction")


Also use WithField for peerID

aravindavk · 2018-03-15T16:41:42Z

plugins/device/rest.go

+	}
+	err = txn.Ctx.Set("groupid", groupID)
+	if err != nil {
+		logger.WithError(err).Error("Failed to set data for transaction")


Also use WithField for groupID

aravindavk · 2018-03-15T16:43:24Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, "Transaction to edit group failed", api.ErrCodeDefault)
+		return
+	}
+	peerInfo, err := peer.GetPeer(peerID)


PeerInfo response may not be necessory. Just return success with a message

@rishubhjain

Add peer and edit peer currently returns peerinfo. However, as this API is under devices, as suggested by Aravinda, it's okay to just return http.StatusOK without a body 👍

aravindavk · 2018-03-15T16:43:51Z

plugins/device/transaction.go

+func txnEditGroup(c transaction.TxnCtx) error {
+	var peerID string
+	var groupID string
+	var req deviceapi.EditGroupReq


req not required

aravindavk · 2018-03-15T16:44:11Z

plugins/device/transaction.go

+		c.Logger().WithError(err).Error("Failed transaction, cannot find group-id")
+		return err
+	}
+	if err := c.Get("req", req); err != nil {


not required

aravindavk · 2018-03-15T16:44:44Z

plugins/device/transaction.go

+		c.Logger().WithError(err).WithField("peerid", peerID).Error("Peer ID not found in store")
+		return err
+	}
+	peerInfo.MetaData["_group"] = req.Group


Use peerInfo.MetaData["_group"] = groupID

aravindavk · 2018-03-15T16:45:13Z

plugins/device/transaction.go

+	peerInfo.MetaData["_group"] = req.Group
+	err = peer.AddOrUpdatePeer(peerInfo)
+	if err != nil {
+		c.Logger().WithError(err).WithField("peerid", peerID).Error("Failed to update peer Info")


also add WithField groupID

aravindavk

Looks good

prashanthpai

I'd prefer the edit peer PR to get in first.

prashanthpai · 2018-03-16T04:47:34Z

plugins/device/init.go

+			Pattern:      "/peers/{peerid}/group/{group_id}",
+			Version:      1,
+			ResponseType: utils.GetTypeString((*deviceapi.EditGroupResp)(nil)),
+			HandlerFunc:  groupEditHandler,


peerGroupEditHandler

Same comment elsewhere. Prefix peer wherever applicable. Without that, someone new might mistake the group to be related to device and not peer.

prashanthpai · 2018-03-16T04:49:50Z

plugins/device/transaction.go

+		c.Logger().WithError(err).WithField("peerid", peerID).Error("Peer ID not found in store")
+		return err
+	}
+	peerInfo.MetaData["_group"] = groupID


This is duplicate work. See if you can re-use the edit metadata functionality implemented in the other PR.

Edit Peer functionality can't be used. Because any group key starting with "_" is restricted with that API so that user configured Metadata will not override internal components Metadata.

What do you think of this ? Restrict edit peer handler from editing metadata keys that begin with _. We can re-use the transaction step function.

Makes sense.

@prashanthpai I have made changes in the the peer api PR,once that is merged I will update this code. Please review #600

prashanthpai · 2018-03-16T04:51:26Z

plugins/device/init.go

+		route.Route{
+			Name:         "EditGroup",
+			Method:       "POST",
+			Pattern:      "/peers/{peerid}/group/{group_id}",


We've never done this in other APIs - the request data being part of the URL. To maintain uniformity, I'd prefer if this was in the body.

aravindavk · 2018-03-16T04:59:29Z

I'd prefer the edit peer PR to get in first.

Edit PR is unrelated to this

prashanthpai · 2018-03-21T04:00:53Z

plugins/device/rest.go

+	req := new(deviceapi.PeerEditGroupReq)
+	if err := restutils.UnmarshalRequest(r, req); err != nil {
+		logger.WithError(err).Error("Failed to Unmarshal request")
+		restutils.SendHTTPError(ctx, w, http.StatusBadRequest, "Unable to marshal request", api.ErrCodeDefault)


Unmarshal, not marshal

prashanthpai · 2018-03-21T04:01:17Z

plugins/device/rest.go

+		return
+	}
+
+	peerID := mux.Vars(r)["peerid"]


Use uuid.Parse to validate.

prashanthpai · 2018-03-21T04:01:59Z

plugins/device/rest.go

+	}
+	err = txn.Ctx.Set("peerid", peerID)
+	if err != nil {
+		logger.WithError(err).WithField("PeerID", peerID).Error("Failed to set data for transaction")


End this error statement with in transaction context. Same comment for this usage elsewhere too.

prashanthpai · 2018-03-21T04:05:19Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, "Transaction to edit group failed", api.ErrCodeDefault)
+		return
+	}
+	peerInfo, err := peer.GetPeer(peerID)


@rishubhjain

Add peer and edit peer currently returns peerinfo. However, as this API is under devices, as suggested by Aravinda, it's okay to just return http.StatusOK without a body 👍

prashanthpai · 2018-03-21T04:06:20Z

plugins/device/init.go

 	}
 }

 // RegisterStepFuncs registers transaction step functions with
 // Glusterd Transaction framework
 func (p *Plugin) RegisterStepFuncs() {
 	transaction.RegisterStepFunc(txnPrepareDevice, "prepare-device")
+	transaction.RegisterStepFunc(txnPeerEditGroup, "peer-edit-group")


Wait until the edit peer metadata PR gets in. You can re-use code (step function) from that PR here.

aravindavk · 2018-03-27T05:34:17Z

glusterd2/commands/peers/addpeer.go

@@ -81,6 +90,14 @@ func addPeerHandler(w http.ResponseWriter, r *http.Request) {
 	}

 	newpeer.MetaData = req.MetaData
+	if req.Zone != "" {
+		if newpeer.MetaData != nil {


nit: can be simplified

if newpeer.MetaData == nil { newpeer.MetaData = make(map[string]string) } newpeer.MetaData["_zone"] = req.Zone

aravindavk · 2018-03-27T05:35:57Z

glusterd2/peer/self.go

+		p.MetaData = make(map[string]string)
+	}
+	if p.MetaData["_zone"] == "" {
+		p.MetaData["_zone"] = "1"


Do not default it to "1". If zone is not added for any peers then all peers will belong to same zone(that is "1"). Peer.ID can be used as default.

p.MetaData["_zone"] = p.ID

aravindavk · 2018-03-27T05:50:08Z

plugins/device/rest.go

@@ -49,13 +54,13 @@ func deviceAddHandler(w http.ResponseWriter, r *http.Request) {
 	}
 	err = txn.Ctx.Set("peerid", peerID)
 	if err != nil {
-		logger.WithError(err).Error("Failed to set data for transaction")
+		logger.WithError(err).WithField("key", peerID).Error("Failed to set key in transaction context")


To understand structured logging better. Above message will be logged as

Failed to set key in transaction context key=53503353-0ba8-4fd3-a8cf-aba2798ab8e5

Note: Peer ID used is just for example.

Advantage of structured logging is that, message remains constant even if failure is for different Peer ID and all the dynamic fields are easily parsable by script. Traditionally logs are written as free hand format, but it makes very difficult to parse the logs through scripts for making sense out of log messages. Structured logging will help to address this issue. WithField accepts two fields, one is key and other one is value. Key name should be self explanatory and should not consist spaces. With this understanding, change the above log message as,

logger.WithError(err).WithField("key", "peerid").WithField("value", peerID).Error("Failed to set key in transaction context")

aravindavk

Looks good to me

aravindavk · 2018-03-29T09:28:43Z

retest this please

Conflicts: plugins/device/rest.go

aravindavk

A few minor comments

aravindavk · 2018-04-25T05:03:38Z

glusterd2/commands/peers/addpeer.go

@@ -80,7 +89,16 @@ func addPeerHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	newpeer.MetaData = req.MetaData
+	if newpeer.Metadata == nil {


Since this is Peer Add, it is safe to assume no Metadata exists. if newpeer.Metadata == nil is not required.

aravindavk · 2018-04-25T05:07:20Z

glusterd2/peer/self.go

 	p.ClientAddresses, err = normalizeAddrs()
 	if err != nil {
 		return err
 	}

+	peerInfo, err := GetPeer(gdctx.MyUUID.String())


Why these changes required?

In case the glusterd2 service was restarted and the data (metadata ) already existed then it is important to go and get back the data .

aravindavk · 2018-04-25T05:10:55Z

plugins/device/rest.go

+		return
+	}
+	if !CheckIfDeviceExist(req.Devices, devices) {
+		logger.WithError(err).WithField("device", req.Devices).Error("One or more  already exists")


nit: error message can be changed to "One or more devices already exists"

aravindavk · 2018-04-25T05:15:13Z

plugins/device/store-utils.go

@@ -13,16 +13,29 @@ func GetDevices(peerID string) ([]deviceapi.Info, error) {
 	if err != nil {
 		return nil, err
 	}
-	if len(peerInfo.MetaData["devices"]) > 0 {
+	if len(peerInfo.Metadata["devices"]) > 0 {


aravindavk · 2018-04-25T05:15:38Z

plugins/device/store-utils.go

 		var deviceInfo []deviceapi.Info
-		if err := json.Unmarshal([]byte(peerInfo.MetaData["devices"]), &deviceInfo); err != nil {
+		if err := json.Unmarshal([]byte(peerInfo.Metadata["devices"]), &deviceInfo); err != nil {


Conflicts: plugins/device/rest.go

aravindavk · 2018-04-30T14:32:17Z

glusterd2/commands/peers/addpeer.go

@@ -25,6 +26,14 @@ func addPeerHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	for key := range req.Metadata {
+		if strings.HasPrefix(key, "_") {
+			logger.WithField("metadata-key", key).Error("Key names starting with '_' are restricted in metadata field")


This error can be added to generic errors list. Used this in multiple places and also Volume metadata will also requires this error msg.

aravindavk · 2018-04-30T14:41:56Z

plugins/device/cmdexec/device.go

@@ -0,0 +1,51 @@
+package cmdexec


Move only device commands to plugins/device/deviceutils/utils.go

package deviceutils func CreatePV func CreateVG func RemovePV func RemoveVG

Move other parts of code back to transactions.go.

plugins/device/deviceutils/utils.go can be moved to pkg/deviceutils later if required.

Why is there a need to move commands to device utils?, won't it be useful if a separate library is made for commands

cmdexec is not a good name for library, doesn't give any clue what that cmdexec is. Another problem with current approach is that this library can't be used without having Transaction context. Library should be independent of Transaction/etcd so that it can be reused in other places like Tests.

Based on the discussion with you, I grouped the device related functions in separate file here 2adafaa#diff-388b0ce2a0b41316c76c470557c4cbd1

aravindavk · 2018-05-21T08:28:41Z

plugins/device/store-utils.go

+func CheckIfDeviceExist(reqDevice string, devices []deviceapi.Info) bool {
+	for _, key := range devices {
+		if reqDevice == key.Name {
+			return false


return true?

aravindavk · 2018-05-21T08:51:05Z

plugins/device/rest.go

 		}
 		return
 	}
+
+	var devices []deviceapi.Info
+	err = json.Unmarshal([]byte(peerInfo.Metadata["_"]), &devices)


peerInfo.Metadata["_devices"]?

aravindavk · 2018-05-21T08:51:31Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
+		return
+	}
+	if !CheckIfDeviceExist(req.Device, devices) {


flip condition. see comment below

aravindavk · 2018-05-21T08:53:13Z

plugins/device/transaction.go

 	}
-	err := AddDevices(deviceList, peerID.String())
+	c.Logger().WithField("device", device).Info("Device setup successful, setting device status to 'DeviceEnabled'")


s/DeviceEnabled/Enabled

prashanthpai

Have suggested some minor changes.

Further, the contents/logic present in txnPeerEdit() need not be a transaction step but can be handled in the handler itself. It simplifies the code. Considering that this PR has been on review for a long time, I'm okay if you can address that refactoring in a separate PR.

prashanthpai · 2018-05-22T07:12:16Z

plugins/device/deviceutils/utils.go

+//CreatePV is used to create physical volume.
+func CreatePV(device string) error {
+	pvcreateCmd := exec.Command("pvcreate", "--metadatasize=128M", "--dataalignment=256K", device)
+	if err := pvcreateCmd.Run(); err != nil {


The Go report service will likely flag this as redundant. You can directly return.

return pvcreateCmd.Run()

Please do the same for all the functions in this file.

prashanthpai · 2018-05-22T07:16:14Z

plugins/device/store-utils.go

 			return nil, err
 		}
 		return deviceInfo, nil
 	}
 	return nil, nil
 }

-// AddDevices adds device to specific peer
-func AddDevices(devices []deviceapi.Info, peerID string) error {
+//CheckIfDeviceExist returns error if all devices already exist or returns list of devices to be added


Incorrect comment: This function returns true if any of the devices exist.

Also, do not export this function until you have an external consumer out of this package.

prashanthpai · 2018-05-22T07:19:21Z

plugins/device/store-utils.go

+}
+
+// AddDevice adds device of specific peer
+func AddDevice(device deviceapi.Info, peerID string) error {


Do not export this if it's not currently used outside this package.

prashanthpai · 2018-05-22T07:21:48Z

plugins/device/transaction.go

 		return err
 	}
-	if err := c.Get("req", req); err != nil {
-		c.Logger().WithError(err).Error("Failed transaction, cannot find device-details")
+


Suggestion for improvement (not in this PR but later):

Every call to c.Get translates to a etcd client call. You can create a local structure containing both peer ID and device name and then set it in the transaction context.

prashanthpai · 2018-05-22T07:23:49Z

plugins/device/transaction.go

 	}
-	err := AddDevices(deviceList, peerID.String())
+	c.Logger().WithField("device", device).Info("Device setup successful, setting device status to 'Enabled'")
+	deviceInfo.State = deviceapi.DeviceEnabled


You can create the entire deviceapi.Info structure here itself for better readability:

deviceInfo = deviceapi.Info { Name : device, State: deviceapi.DeviceEnabled, }

Conflicts: pkg/errors/error.go

rishubhjain · 2018-05-23T04:19:00Z

@prashanthpai Addressed the comments, please review

Edit Group API

3caf951

aravindavk suggested changes Mar 15, 2018

View reviewed changes

aravindavk approved these changes Mar 16, 2018

View reviewed changes

prashanthpai suggested changes Mar 16, 2018

View reviewed changes

Updating code

76324c9

rishubhjain force-pushed the group branch from ae88792 to 76324c9 Compare March 19, 2018 09:40

prashanthpai suggested changes Mar 21, 2018

View reviewed changes

rishubhjain force-pushed the group branch from 5f7db38 to c54c2f9 Compare March 21, 2018 11:24

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

fe15576

rishubhjain force-pushed the group branch 4 times, most recently from f41a387 to ba23334 Compare March 26, 2018 09:11

rishubhjain changed the title ~~Edit Group API~~ Edit Zone API Mar 26, 2018

rishubhjain force-pushed the group branch 5 times, most recently from de27fcf to f0e1922 Compare March 26, 2018 12:14

aravindavk reviewed Mar 27, 2018

View reviewed changes

rishubhjain force-pushed the group branch 2 times, most recently from 7e7e851 to 79e5e6b Compare March 28, 2018 04:34

aravindavk approved these changes Mar 29, 2018

View reviewed changes

rishubhjain force-pushed the group branch 2 times, most recently from 814dcf2 to ec8c72c Compare March 30, 2018 15:04

rishubhjain added 2 commits March 31, 2018 02:24

Updating code

9d1d4bf

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

8ec64a7

rishubhjain force-pushed the group branch from ec8c72c to 8ec64a7 Compare March 31, 2018 06:25

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

1a6c001

Conflicts: plugins/device/rest.go

aravindavk suggested changes Apr 25, 2018

View reviewed changes

rishubhjain mentioned this pull request Apr 26, 2018

Edit volume api #704

Merged

rishubhjain added 2 commits April 26, 2018 08:51

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

8282ce4

Conflicts: plugins/device/rest.go

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

d589c4d

aravindavk suggested changes Apr 30, 2018

View reviewed changes

rishubhjain added 4 commits May 2, 2018 07:04

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

7d905c7

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

552fbb4

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

034fa48

Renaming commands folder to utils

3ed7136

rishubhjain force-pushed the group branch 8 times, most recently from f03e833 to ba5ae60 Compare May 21, 2018 07:26

aravindavk reviewed May 21, 2018

View reviewed changes

rishubhjain force-pushed the group branch from ba5ae60 to cebe505 Compare May 21, 2018 09:06

aravindavk approved these changes May 21, 2018

View reviewed changes

rishubhjain mentioned this pull request May 21, 2018

Using global error message #774

Merged

prashanthpai suggested changes May 22, 2018

View reviewed changes

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

62273e4

Conflicts: pkg/errors/error.go

rishubhjain force-pushed the group branch from cebe505 to 62273e4 Compare May 23, 2018 04:16

Merge branch 'master' of https://github.com/gluster/glusterd2 into group

8ad368d

prashanthpai approved these changes May 23, 2018

View reviewed changes

prashanthpai merged commit 09f1358 into gluster:master May 23, 2018

prashanthpai mentioned this pull request May 23, 2018

Store peer address provided during peer add #761

Merged

Edit Zone API #603

Edit Zone API #603

Conversation

rishubhjain commented Mar 15, 2018

aravindavk 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

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

aravindavk left a comment

Choose a reason for hiding this comment

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

aravindavk commented Mar 16, 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

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

aravindavk left a comment

Choose a reason for hiding this comment

aravindavk commented Mar 29, 2018

aravindavk 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

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

prashanthpai left a comment • 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

rishubhjain commented May 23, 2018

prashanthpai left a comment •

edited

Loading