Edit device #1118

rishubhjain · 2018-08-02T19:39:15Z

Getting device name from user in URL instead of request body during device edit.

Madhu-1 · 2018-08-03T05:05:02Z

@rishubhjain I have some questions related to device EDIT,
if the current device state and the requested device state doesn't match then we need to update device state

IMO if device state is already disabled we don't need to go and disable it again?

I think we are missing this check in device EDIT API can you confirm on this?

rishubhjain · 2018-08-03T05:54:20Z

@prashanthpai @aravindavk If the current state of the device is same as the requested state, then I am returning without updating etcd instead of throwing an error.

aravindavk · 2018-08-06T06:02:09Z

What is the usecase for editing device name(what is device name?)?

aravindavk · 2018-08-06T06:02:58Z

plugins/device/rest.go

@@ -148,6 +148,12 @@ func deviceEditHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	deviceName := r.URL.Query().Get("device")


any issues while using requestBody? why query param?

Madhu-1 · 2018-08-06T06:26:39Z

why can't we return an error if device state is already set, currently if the volume is already started we are returning an error saying already started, why can't we follow the same approach here?
@rishubhjain @prashanthpai @aravindavk need your thought on this.

prashanthpai · 2018-08-06T06:30:02Z

What is the usecase for editing device name(what is device name?)?

+1

Madhu-1 · 2018-08-07T04:15:31Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusBadRequest, "Device name not provided in URL")
+		return
+	}
+
 	txn, err := transaction.NewTxnWithLocks(ctx, peerID)


lock peer + device before editing it, so it won't be considered for brick/volume creation during device edit.

this will be taken up after #998

lock peer+device similar to delete device

Madhu-1 · 2018-08-07T04:17:19Z

TODO list

CLI
rest API
e2e test cases
endpoint update

rishubhjain · 2018-08-16T05:33:21Z

@aravindavk @prashanthpai I have changed it to device instead of device name.

aravindavk · 2018-08-20T14:18:56Z

plugins/device/rest.go

@@ -151,6 +151,12 @@ func deviceEditHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	device := r.URL.Query().Get("device")


Query parameters are mostly used for optional parameters. Please add device to URL itself or request body.

Use as part of url itself

rishubhjain · 2018-10-15T14:57:34Z

Fixes: #1283

atinmu · 2018-11-10T01:42:18Z

@rishubhjain Ping! What's pending on this PR?

aravindavk

Also add Metadata adding support

aravindavk · 2018-11-28T05:06:43Z

plugins/device/init.go

@@ -37,7 +37,7 @@ func (p *Plugin) RestRoutes() route.Routes {
 		route.Route{
 			Name:        "DeviceEdit",
 			Method:      "POST",
-			Pattern:     "/devices/{peerid}",
+			Pattern:     "/devices/{peerid}/edit",


/devices/edit/{peerid}/{device:.*}

aravindavk · 2018-11-28T05:07:28Z

plugins/device/rest.go

@@ -151,6 +151,12 @@ func deviceEditHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	device := r.URL.Query().Get("device")


Use as part of url itself

aravindavk

Please add restclient function and add tests

aravindavk · 2018-11-29T12:17:21Z

plugins/device/rest.go

@@ -151,6 +151,12 @@ func deviceEditHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	device := mux.Vars(r)["device"]


Move this before Unmarshal

aravindavk · 2018-11-29T12:17:58Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusBadRequest, "Device name not provided in URL")
+		return
+	}
+
 	txn, err := transaction.NewTxnWithLocks(ctx, peerID)


lock peer+device similar to delete device

aravindavk · 2018-11-30T03:14:40Z

e2e/smartvol_ops_test.go

+		}
+	}
+
+	if len(deviceList) > 0 {


testing with this condition is not good idea. Tests will pass if for some failure reason len(deviceList) is zero.

as this is part of smart-volume test cases, we already have devices attached to the nodes, why can't we fail the test if there are not devices in attached to peers?

Madhu-1 · 2018-11-30T05:16:39Z

e2e/smartvol_ops_test.go

+			err = client.DeviceEdit(peerID, device.Name, "disabled")
+			r.Nil(err)
+		} else if device.State == "disabled" {
+			err = client.DeviceEdit(peerID, device.Name, "enabled")


I would like to see below-listed tests in this PR

Disable device and check devices state is disabled

when all devices are disabled, the volume creation should fail

Enable all the devices, and check devices is enabled

when all the devices are enabled, the volume creation should pass.

@aravindavk ^^

Madhu-1 · 2018-11-30T05:17:57Z

pkg/restclient/device.go

+}
+
+// DeviceEdit edits devices
+func (c *Client) DeviceEdit(peerid string, device string, state string) error {


(peerid string, device string, state string) to (peerid, device, state string)

Madhu-1 · 2018-11-30T05:20:08Z

pkg/restclient/device.go

+	return deviceList, err
+}
+
+// DeviceEdit edits devices


/devices to /device

Madhu-1 · 2018-11-30T05:24:14Z

plugins/device/rest.go

+		return
+	}
+
+	if !strings.HasPrefix(device, "/") {


do we need this check? as part of URL, I don't think we will get devices with /.

there is a possibility that user provides extra slashes /v1/devices/{peerid}//dev/vdb/, to be on the safer side we can check for a slash before adding another slash

please test this scenario, I think // will be converted into / by HTTP framework,

aravindavk · 2018-12-03T07:05:29Z

Please squash all commits into one.

Madhu-1 · 2018-12-03T08:35:09Z

e2e/smartvol_ops_test.go

+func editDevice(t *testing.T) {
+	r := require.New(t)
+	peerList, err := client.Peers()
+	r.Nil(err)


if peerList len is zero fail the test case.

I think this should be done in the test case of peerList and not in the edit-device.

as this is dependent on the peer list, no harm in having one more higher level check

Madhu-1 · 2018-12-03T08:37:51Z

e2e/smartvol_ops_test.go

+		SubvolZonesOverlap: true,
+	}
+	_, err = client.VolumeCreate(createReq)
+	r.NotNil(err)


enable the devices and create the volume, the volume creation should be successful.

Madhu-1 · 2018-12-03T09:11:35Z

09:02:32 --- FAIL: TestSmartVolume/Edit_device (0.07s)
09:02:32 require.go:765:
09:02:32 Error Trace: smartvol_ops_test.go:374
09:02:32 Error: Expected nil, but got: &errors.errorString{s:"volume not found"}
09:02:32 Test: TestSmartVolume/Edit_device
@rishubhjain CI failure

Madhu-1 · 2018-12-04T09:30:54Z

@rishubhjain PR is having conflicts, please resolve.

Madhu-1

resolve conflicts

…it_device

rishubhjain · 2018-12-04T18:14:06Z

retest this please

aravindavk · 2018-12-05T03:28:24Z

retest this please

rishubhjain force-pushed the edit_device branch from 4e15ee9 to 62ac746 Compare August 2, 2018 19:47

prashanthpai requested review from aravindavk, prashanthpai and Madhu-1 August 3, 2018 04:24

rishubhjain force-pushed the edit_device branch 2 times, most recently from 0a9af25 to 980b933 Compare August 3, 2018 05:49

aravindavk reviewed Aug 6, 2018

View reviewed changes

Madhu-1 suggested changes Aug 7, 2018

View reviewed changes

rishubhjain force-pushed the edit_device branch from eb5c3d5 to 1249790 Compare August 14, 2018 06:17

aravindavk reviewed Aug 20, 2018

View reviewed changes

aravindavk reviewed Nov 28, 2018

View reviewed changes

ghost assigned rishubhjain Nov 29, 2018

ghost added the in progress label Nov 29, 2018

aravindavk reviewed Nov 29, 2018

View reviewed changes

rishubhjain force-pushed the edit_device branch 2 times, most recently from 174ecbe to cfabd20 Compare November 29, 2018 15:47

aravindavk reviewed Nov 30, 2018

View reviewed changes

Madhu-1 reviewed Nov 30, 2018

View reviewed changes

pkg/restclient/device.go Outdated

return deviceList, err

}

// DeviceEdit edits devices

Copy link

Member

Madhu-1 Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/devices to /device

Madhu-1 reviewed Nov 30, 2018

View reviewed changes

aravindavk mentioned this pull request Nov 30, 2018

Refactor Device management #1354

Merged

rishubhjain force-pushed the edit_device branch 3 times, most recently from 7b13d3c to 922f434 Compare November 30, 2018 11:47

aravindavk approved these changes Dec 3, 2018

View reviewed changes

aravindavk mentioned this pull request Dec 3, 2018

API to remove device #1120

Open

Madhu-1 reviewed Dec 3, 2018

View reviewed changes

rishubhjain force-pushed the edit_device branch from 922f434 to bfa74cc Compare December 3, 2018 08:55

rishubhjain force-pushed the edit_device branch 2 times, most recently from 8cf80da to 849d943 Compare December 4, 2018 08:27

Madhu-1 approved these changes Dec 4, 2018

View reviewed changes

Madhu-1 suggested changes Dec 4, 2018

View reviewed changes

Madhu-1 approved these changes Dec 4, 2018

View reviewed changes

ghost assigned aravindavk Dec 4, 2018

Edit device API

6bba9f0

rishubhjain force-pushed the edit_device branch from 63219db to 6bba9f0 Compare December 4, 2018 17:22

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

14846a5

…it_device

aravindavk merged commit 3d9707a into gluster:master Dec 5, 2018

ghost removed the in progress label Dec 5, 2018

Edit device #1118

Edit device #1118

Conversation

rishubhjain commented Aug 2, 2018

Madhu-1 commented Aug 3, 2018

rishubhjain commented Aug 3, 2018

aravindavk commented Aug 6, 2018

Choose a reason for hiding this comment

Madhu-1 commented Aug 6, 2018

prashanthpai commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Aug 7, 2018 • edited Loading

rishubhjain commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishubhjain commented Oct 15, 2018

atinmu commented Nov 10, 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

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 commented Dec 3, 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

Madhu-1 commented Dec 3, 2018

Madhu-1 commented Dec 4, 2018

Madhu-1 left a comment

Choose a reason for hiding this comment

rishubhjain commented Dec 4, 2018

aravindavk commented Dec 5, 2018

Madhu-1 commented Aug 7, 2018 •

edited

Loading

rishubhjain commented Aug 16, 2018 •

edited

Loading