API to remove device by rishubhjain · Pull Request #1120 · gluster/glusterd2

rishubhjain · 2018-08-02T21:07:09Z

Madhu-1 · 2018-08-07T04:03:06Z

plugins/device/init.go

+		route.Route{
+			Name:        "DeviceDelete",
+			Method:      "DELETE",
+			Pattern:     "/devices/{peerid}",


can we add a comment saying the device ID is a query param?

Madhu-1 · 2018-08-07T04:06:43Z

plugins/device/rest.go

+	}
+	defer txn.Done()
+
+	err = deviceutils.DeleteDevice(peerID, deviceName)


question: don't we need to lock device before deleting it?

#998, as @aravindavk said we need to lock device for smart volume creation, the same way we should consider locking device before deleting it.

Use lock as PeerID + deviceName

Madhu-1 · 2018-08-07T04:08:58Z

plugins/device/rest.go

+		return
+	}
+
+	restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)


the response should be 204 for a delete operation

Madhu-1

instead of adding review comments by mistake I have added it as just comments.

Madhu-1 · 2018-08-07T04:10:27Z

plugins/device/rest.go

+	}
+	defer txn.Done()
+
+	err = deviceutils.DeleteDevice(peerID, deviceName)


#998, as @aravindavk said we need to lock device for smart volume creation, the same way we should consider locking device before deleting it.

Madhu-1 · 2018-08-07T04:12:23Z

@rishubhjain I would suggest disabling the device first so it won't be considered for any volume/brick creation, later we can delete the device. just a question, how are we planning to move bricks from the device to another device before deleting it?

@aravindavk @phlogistonjohn @raghavendra-talur @obnoxxx need input from your side on this

Madhu-1 · 2018-08-07T04:13:30Z

TODO list

CLI
rest API
e2e test cases
endpoint update

aravindavk · 2018-08-07T04:21:52Z

@rishubhjain I would suggest disabling the device first so it won't be considered for any volume/brick creation, later we can delete the device. just a question, how are we planning to move bricks from the device to another device before deleting it?

disabling before delete is nice. +1

just a question, how are we planning to move bricks from the device to another device before deleting it?

For now only allow device delete if no LVs in it.

atinmu · 2018-11-10T01:43:00Z

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

aravindavk · 2018-11-28T04:55:11Z

plugins/device/rest.go

+	}
+	defer txn.Done()
+
+	err = deviceutils.DeleteDevice(peerID, deviceName)


Use lock as PeerID + deviceName

aravindavk · 2018-11-28T04:55:52Z

plugins/device/rest.go

+		restutils.SendHTTPError(ctx, w, http.StatusBadRequest, "invalid peer-id passed in url")
+		return
+	}
+	deviceName := r.URL.Query().Get("device")


Do not use query use in url itself

aravindavk · 2018-11-28T04:57:25Z

plugins/device/init.go

+		route.Route{
+			Name:        "DeviceDelete",
+			Method:      "DELETE",
+			Pattern:     "/devices/{peerid}",


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

aravindavk · 2018-11-28T04:59:36Z

plugins/device/deviceutils/store-utils.go

+		return errors.New("device does not exist in the given peer")
+	}
+
+	if devices[index].Used {


I don't think this flag is updated on volume delete. Please open new issue to rename this field to NumberOfBricks.

Remove this check from here(add TODO note). While addressing the new issue, check need to be added again(if devices[index].NumberOfBricks == 0)

aravindavk

Please add a restclient function and tests

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

plugins/device/init.go

 			Version:      1,
 			ResponseType: utils.GetTypeString((*deviceapi.ListDeviceResp)(nil)),
 			HandlerFunc:  listAllDevicesHandler},
+		// device name is passed as query param in url.


remove comment

aravindavk

Looks good. one minor comment about the test

aravindavk · 2018-11-29T14:25:21Z

e2e/smartvol_ops_test.go

+
+	newDeviceList, err := client.DeviceList(tc.gds[0].PeerID())
+
+	r.NotEqual(len(deviceList), len(newDeviceList))


check using Equal. r.Equal(len(deviceList) - 1, len(newDeviceList))

aravindavk · 2018-12-03T06:54:57Z

pkg/restclient/device.go

+}
+
+// DeviceList lists the devices
+func (c *Client) DeviceList(peerid string) ([]deviceapi.Info, error) {


This is repeated in PR #1118

Yes I required this for the e2e test cases, will rebase once that PR is merged.

aravindavk · 2018-12-03T06:57:43Z

plugins/device/rest.go

 	restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)
 }

+func deviceDeleteHandler(w http.ResponseWriter, r *http.Request) {


sorry, I missed it earlier. We need to remove the device using vgremove and pvremove as a transaction step. Deleting device from etcd should be last step.

rishubhjain · 2018-12-12T22:18:59Z

retest this please

Madhu-1 · 2018-12-13T05:37:07Z

e2e/device_test.go

+	t.Run("Delete device", testDeviceDelete)
+
+	// // Device Cleanup
+	r.Nil(loopDevicesCleanup(t))


can we move this after device add? even if the device remove or device list fails, these block fo code won't get hit. it's good to do cleanup in defer().
@aravindavk let me know your thoughts.

Should we move all the clean up task to defer? Like deleting and stopping volume etc.

this is also called in main_test.go, so should be fine

Madhu-1 · 2018-12-13T05:44:21Z

plugins/device/rest.go

+
+	err = txn.Do()
+	if err != nil {
+		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, "transaction to prepare device failed")


even the device is not present in the peer is considered as internal server error, need to handle not found error also

Madhu-1 · 2018-12-13T05:45:51Z

plugins/device/transaction.go

+	}
+
+	//Remove PV
+	if err := lvmutils.RemovePV(deviceName); err != nil {


make use of err throughout the function. don't create a new variable for each block.

Madhu-1 · 2018-12-13T05:47:41Z

plugins/device/transaction.go

+	}
+
+	if vgName == "" {
+		return errors.New("No device found with given device name")


an error message should start with the small case. and move this error message to error pkg. it will be helpful for the consumer.

Madhu-1 · 2018-12-13T05:47:49Z

plugins/device/transaction.go

+	}
+
+	if len(devices) == 0 {
+		return errors.New("No devices added in the given peer")


same here as well

Madhu-1 · 2018-12-13T05:49:56Z

plugins/device/rest.go

+
+	txn.Nodes = []uuid.UUID{peerInfo.ID}
+	txn.Steps = []*transaction.Step{
+		{


@aravindavk don't we need to check any volume present on the device before removing it?

Vgremove will fail if the device is being used. We don't wipe the device

no harm in checking any volume present on the device, so that user will get a meaning full message something like bricks present on the device, cannot delete the device,

Madhu-1 · 2018-12-13T05:50:54Z

plugins/device/rest.go

+}
+
+func listAllDevicesHandler(w http.ResponseWriter, r *http.Request) {
+


remove this extra line

…lete_device

Madhu-1 · 2018-12-13T12:42:18Z

plugins/device/transaction.go

+	}
+
+	// Remove VG
+	if err = lvmutils.RemoveVG(vgName); err != nil {


what happens when the VG is already deleted in the last execution and something failed in PV deletion below?
before deleting the resource don't we need to check if the resource is available or not?

@Madhu-1 Can you elaborate more on whether resource is available or not. Do you mean whether the device exist or in use or not?

@rishubhjain during the device removal if the VG removal was successful and it got failed in PV remove stage, and if the user again tries to delete the device, it will fail with VG not found error right, how are we solving this problem?
dont we need to handle VG not found scenario and PV not found scenario?

@aravindavk Can you help me with this? the basic way would be to check pvs and vgs before removing to avoid issues. But it's not a clean solution, can you suggest a cleaner solution to handle this scenerio?

try to delete VG and continue if it is not exists error. But if Vg is not found make sure no Vg exists before removing PV.(If user manually deleted Vg and Created Vg with different name then gd2 may end up deleting, If PV fails when Vg exists then that is also Ok)

Also check PV not exists. If Pv not exists then remove only from store and return success

This is only possible if user manually deletes PV right?

Madhu-1 · 2018-12-13T12:43:46Z

@rishubhjain please update the commit message with description

aravindavk · 2018-12-18T11:20:55Z

rebase required

…lete_device Conflicts: e2e/smartvol_ops_test.go

Madhu-1 reviewed Aug 7, 2018

View reviewed changes

Madhu-1 suggested changes Aug 7, 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 delete_device branch from 585b080 to adc34f2 Compare November 29, 2018 14:27

aravindavk reviewed Nov 29, 2018

View reviewed changes

rishubhjain force-pushed the delete_device branch 7 times, most recently from cafd783 to 91f0e56 Compare December 1, 2018 10:31

aravindavk suggested changes Dec 3, 2018

View reviewed changes

rishubhjain force-pushed the delete_device branch 3 times, most recently from 2d5a7e7 to 98967b9 Compare December 11, 2018 11:07

API to remove device

17496a8

rishubhjain force-pushed the delete_device branch from 98967b9 to 17496a8 Compare December 12, 2018 22:09

Madhu-1 reviewed Dec 13, 2018

View reviewed changes

Madhu-1 suggested changes Dec 13, 2018

View reviewed changes

rishubhjain force-pushed the delete_device branch from a6cd208 to 7740aff Compare December 13, 2018 07:40

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

fb3f69a

…lete_device

rishubhjain force-pushed the delete_device branch from 7740aff to fb3f69a Compare December 13, 2018 12:31

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

11efa13

…lete_device

Madhu-1 reviewed Dec 13, 2018

View reviewed changes

rishubhjain mentioned this pull request Dec 18, 2018

Check whether peer has devices before removing peer #1421

Open

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

c4d3db0

…lete_device Conflicts: e2e/smartvol_ops_test.go


		newDeviceList, err := client.DeviceList(tc.gds[0].PeerID())

		r.NotEqual(len(deviceList), len(newDeviceList))

		}

		func listAllDevicesHandler(w http.ResponseWriter, r *http.Request) {

Conversation

rishubhjain commented Aug 2, 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!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Madhu-1 commented Aug 7, 2018

Uh oh!

Madhu-1 commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aravindavk commented Aug 7, 2018

Uh oh!

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

aravindavk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishubhjain commented Dec 12, 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

Madhu-1 commented Aug 7, 2018 •

edited

Loading