-
Notifications
You must be signed in to change notification settings - Fork 82
Edit volume api #704
Edit volume api #704
Changes from 2 commits
9471d68
862088d
6c3a1ef
11fece9
406542b
f131aff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,9 @@ func TestVolume(t *testing.T) { | |
|
||
// Disperse volume test | ||
t.Run("Disperse", testDisperse) | ||
|
||
// Edit Volume Metadata test | ||
t.Run("Edit", testEditVolume) | ||
} | ||
|
||
func testVolumeCreate(t *testing.T) { | ||
|
@@ -409,3 +412,41 @@ func testDisperse(t *testing.T) { | |
r.Nil(client.VolumeStop(disperseVolName), "disperse volume stop failed") | ||
r.Nil(client.VolumeDelete(disperseVolName), "disperse volume delete failed") | ||
} | ||
|
||
func testEditVolume(t *testing.T) { | ||
r := require.New(t) | ||
|
||
var brickPaths []string | ||
|
||
for i := 1; i <= 4; i++ { | ||
brickPath, err := ioutil.TempDir(tmpDir, "brick") | ||
r.Nil(err) | ||
brickPaths = append(brickPaths, brickPath) | ||
} | ||
|
||
createReq := api.VolCreateReq{ | ||
Name: volname, | ||
Subvols: []api.SubvolReq{ | ||
{ | ||
ReplicaCount: 2, | ||
Type: "replicate", | ||
Bricks: []api.BrickReq{ | ||
{PeerID: gds[0].PeerID(), Path: brickPaths[0]}, | ||
{PeerID: gds[1].PeerID(), Path: brickPaths[1]}, | ||
}, | ||
}, | ||
}, | ||
Force: true, | ||
} | ||
_, err := client.VolumeCreate(createReq) | ||
|
||
editMetadataReq := api.VolEditReq{ | ||
Metadata: map[string]string{ | ||
"owner": "gd2tests", | ||
}, | ||
} | ||
_, err = client.EditVolume(volname, editMetadataReq) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You will have to check the response body. Also, test the replace key cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the check for the response as well as for the new volinfo generated after the changes. |
||
r.Nil(err) | ||
err = client.VolumeDelete(volname) | ||
r.Nil(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good work and the kind of tests I want to see in all PRs 👍 P.S: You can remove these extra lines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prashanthpai Which extra lines are you referring to here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The blank ones after this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, handled it. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,13 @@ func (c *Command) Routes() route.Routes { | |
Pattern: "/volfiles", | ||
Version: 1, | ||
HandlerFunc: volfilesListHandler}, | ||
route.Route{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have to regenerate the API documentation and include the changes that'll be done on # 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 |
||
Name: "EditVolume", | ||
Method: "POST", | ||
Pattern: "/volumes/{volname}/edit", | ||
Version: 1, | ||
RequestType: utils.GetTypeString((*api.VolumeGetResp)(nil)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please set |
||
HandlerFunc: volumeEditHandler}, | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package volumecommands | ||
|
||
import ( | ||
"net/http" | ||
"strings" | ||
|
||
"github.com/gluster/glusterd2/glusterd2/gdctx" | ||
restutils "github.com/gluster/glusterd2/glusterd2/servers/rest/utils" | ||
"github.com/gluster/glusterd2/glusterd2/transaction" | ||
"github.com/gluster/glusterd2/glusterd2/volume" | ||
"github.com/gluster/glusterd2/pkg/api" | ||
"github.com/gluster/glusterd2/pkg/errors" | ||
|
||
"github.com/gorilla/mux" | ||
"github.com/pborman/uuid" | ||
) | ||
|
||
func volumeEditHandler(w http.ResponseWriter, r *http.Request) { | ||
|
||
p := mux.Vars(r) | ||
volname := p["volname"] | ||
|
||
ctx := r.Context() | ||
logger := gdctx.GetReqLogger(ctx) | ||
|
||
var req api.VolEditReq | ||
if err := restutils.UnmarshalRequest(r, &req); err != nil { | ||
restutils.SendHTTPError(ctx, w, http.StatusBadRequest, err) | ||
return | ||
} | ||
|
||
//Lock on Volume Name | ||
lock, unlock := transaction.CreateLockFuncs(volname) | ||
// Taking a lock outside the txn as volinfo.Nodes() must also | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is not valid as there is no txn here. Remove. |
||
// be populated holding the lock. | ||
if err := lock(ctx); err != nil { | ||
if err == transaction.ErrLockTimeout { | ||
restutils.SendHTTPError(ctx, w, http.StatusConflict, err) | ||
} else { | ||
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err) | ||
} | ||
return | ||
} | ||
defer unlock(ctx) | ||
|
||
//validate volume name | ||
v, err := volume.GetVolume(volname) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
if err != nil { | ||
if err == errors.ErrVolNotFound { | ||
restutils.SendHTTPError(ctx, w, http.StatusNotFound, errors.ErrVolNotFound) | ||
} else { | ||
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err) | ||
} | ||
return | ||
} | ||
|
||
for key := range req.Metadata { | ||
if strings.HasPrefix(key, "_") { | ||
logger.WithField("key", key).Error("Key names starting with '_' are restricted in metadata field") | ||
restutils.SendHTTPError(ctx, w, http.StatusBadRequest, "Key names starting with '_' are restricted in metadata field") | ||
return | ||
} | ||
} | ||
|
||
v.Metadata = req.Metadata | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll have to replace each key, value individually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we were overwriting the Metadata with every edit request? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are over-writing individual keys only. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// Transaction which starts self heal daemon on all nodes with atleast one brick. | ||
txn := transaction.NewTxn(ctx) | ||
defer txn.Cleanup() | ||
|
||
txn.Nodes = v.Nodes() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like mentioned previously, this should not be a transaction as it's only run on one node! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I need to run a transaction step for updating the volfile ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the volfile needs no updation as metadata isn't used for volfile generation |
||
txn.Steps = []*transaction.Step{ | ||
{ | ||
DoFunc: "vol-option.UpdateVolinfo", | ||
Nodes: []uuid.UUID{gdctx.MyUUID}, | ||
}, | ||
} | ||
|
||
if err := txn.Ctx.Set("volinfo", v); err != nil { | ||
logger.WithError(err).WithField("key", "volinfo").Error("failed to set key in transaction context") | ||
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err) | ||
return | ||
} | ||
|
||
err = txn.Do() | ||
if err != nil { | ||
logger.WithField("volname", volname).Error("failed to edit metadata") | ||
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err) | ||
return | ||
} | ||
|
||
resp := createVolumeGetResp(v) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create a |
||
restutils.SendHTTPResponse(ctx, w, http.StatusOK, resp) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not create a new volume. Please re-use the volume used in other tests. See example in line 56 to line 62.