plugins/geo-rep: Geo-replication Session Create #385

aravindavk · 2017-10-10T14:48:56Z

REST API to create Geo-replication session create

POST /v1/geo-replication/:mastervolid/:slavevolid

Request body,

{
    "mastervol": MASTER_VOL_NAME,
    "slavevol": SLAVE_VOL_NAME,
    "slavehosts": LIST_OF_SLAVE_HOSTS,
    "slaveuser": SLAVE_USER
}

Updates: #271
Signed-off-by: Aravinda VK avishwan@redhat.com

prashanthpai · 2017-10-12T05:59:00Z

plugins/georeplication/api/req.go

+package georeplication
+
+// GeorepCreateRequest represents REST API request to create Geo-rep session
+type GeorepCreateRequest struct {


Question: Are all the fields required for a valid request ?

SlaveUser is optional, if not provided defaults to "root"

In that case, you can set it as omitempty

omitempty is required for response struct right? In this case if SlaveUser is not passed then it will be empty string.

prashanthpai · 2017-10-12T06:02:38Z

plugins/georeplication/api/resp.go

+	"github.com/pborman/uuid"
+)
+
+const (


Just a FYI: These can be like enums instead. There's stringer tool which can make it strings. I got to know about this while reviewing the volgen PR. You can take a look at it if it makes things simpler/easier for you.

Let me explore that, thanks. Is that libray already part of glusterd2?

Is that libray already part of glusterd2?

It's part of standard go tools and isn't an importable library. The volgen PR has example on how to use it. The stringer tool will generate code. All you have to do it include a one line comment in the go source file where you have your types (enum-like consts) defined. Example:

//go:generate stringer -type=GeorepStatus

I will include this in another patch later.

prashanthpai · 2017-10-12T06:04:29Z

plugins/georeplication/api/resp.go

+
+// GeorepSession represents Geo-replication session
+type GeorepSession struct {
+	MasterID   uuid.UUID         `json:"master_volume_id"`


Check if the string representation of UUID is what that is seen in response.

Yes. Sample output

HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
X-Request-Id: 52e4ac7a-a470-4a9c-952f-687bebe442f0
Date: Thu, 12 Oct 2017 07:26:23 GMT
Content-Length: 255

{"master_volume_id":"254218fe-37ac-40e7-9b76-980700217862","slave_volume_id":"bc8834b4-c710-4fd6-b07e-f25380595a62","master_volume":"gv1","slave_user":"root","slave_hosts":["f26"],"slave_volume":"gv3","monitor_status":"Created","workers":[],"options":{}}

prashanthpai · 2017-10-12T06:08:48Z

plugins/georeplication/init.go

+	return route.Routes{
+		route.Route{
+			Name:        "GeoreplicationCreate",
+			Method:      "PUT",


Personally, I really prefer PUT over POST for resource creation and POST for resource modification.

But it's been POST for all requests in REST API so far. We should open up this for discussion. We will need the APIs to be uniform i.e creation of volumes, snapshot, georep to be all PUT or all POST.

Also, mastervolid and slaveid can also be part of request body. Any reason for it being part of URL ?

Also, mastervolid and slaveid can also be part of request body. Any reason for it being part of URL ?

Since it is unique combination for a georep session and required always. It also makes more sense for other commands start/stop/status/config (Ex: POST /v1/geo-replication/:masterid/:slaveid/start)

I will change this to POST

prashanthpai · 2017-10-12T06:14:58Z

plugins/georeplication/rest.go

+	}
+
+	// Validate UUID format of Master and Slave Volume ID
+	masterid, slaveid, err := validateMasterAndSlaveIDFormat(w, masteridRaw, slaveidRaw)


To be consistent with rest of the code, you can return SendHTTPError() from the handler itself.

if uuid.Parse(masteridRaw) == nil || uuid.Parse(slaveidRaw) == nil { restutils.SendHTTPError(w, http.StatusBadRequest, "Invalid Master/Slave Volume ID") return }

Or you can put all of the below validations (MasterVol, SlaveHosts, SlaveVol) in a single validation function. This will also make the handler function smaller and more readable.

Initially I had that in handler itself, but observed that I have to repeat the stuff almost for all Georep API calls. (MasterVol, SlaveHosts and SlaveVol) validation is required only for Create/Update. For all the other apis Start/Stop/Delete/Status/Config we don't need to validate these fields except Master ID and Slave ID

prashanthpai · 2017-10-12T06:30:23Z

plugins/georeplication/rest.go

+			return
+		}
+
+		geoSession = new(georepapi.GeorepSession)


Consider populating this struct instance (including members filled below) in it's own function, may be something like:

func newGeorepSession(req *georepapi.GeorepCreateRequest) *georepapi.GeorepSession

prashanthpai · 2017-10-12T06:35:05Z

plugins/georeplication/rest.go

+		lock,
+		{
+			DoFunc: "georeplication-create.Commit",
+			Nodes:  []uuid.UUID{gdctx.MyUUID},


As this run only one node, is this made a transaction only for the locking ?

There are some todos for Geo-rep create steps. I think I need to create a working directory in all Volume nodes before starting. I will work on those steps later.

prashanthpai · 2017-10-12T06:35:52Z

plugins/georeplication/rest.go

+
+	geoSession.Status = georepapi.GeorepStatusCreated
+
+	e = addOrUpdateSession(geoSession)


Can't this part of txnGeorepCreate ?

Ok, I will move this to txn

aravindavk

Thanks @prashanthpai for review comments, I will address the comments and refresh the patch.

aravindavk · 2017-10-12T07:24:31Z

plugins/georeplication/api/req.go

+package georeplication
+
+// GeorepCreateRequest represents REST API request to create Geo-rep session
+type GeorepCreateRequest struct {


SlaveUser is optional, if not provided defaults to "root"

aravindavk · 2017-10-12T07:26:02Z

plugins/georeplication/api/resp.go

+	"github.com/pborman/uuid"
+)
+
+const (


Let me explore that, thanks. Is that libray already part of glusterd2?

aravindavk · 2017-10-12T07:27:09Z

plugins/georeplication/api/resp.go

+
+// GeorepSession represents Geo-replication session
+type GeorepSession struct {
+	MasterID   uuid.UUID         `json:"master_volume_id"`


Yes. Sample output

HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
X-Request-Id: 52e4ac7a-a470-4a9c-952f-687bebe442f0
Date: Thu, 12 Oct 2017 07:26:23 GMT
Content-Length: 255

{"master_volume_id":"254218fe-37ac-40e7-9b76-980700217862","slave_volume_id":"bc8834b4-c710-4fd6-b07e-f25380595a62","master_volume":"gv1","slave_user":"root","slave_hosts":["f26"],"slave_volume":"gv3","monitor_status":"Created","workers":[],"options":{}}

aravindavk · 2017-10-12T07:28:42Z

plugins/georeplication/init.go

+	return route.Routes{
+		route.Route{
+			Name:        "GeoreplicationCreate",
+			Method:      "PUT",


Also, mastervolid and slaveid can also be part of request body. Any reason for it being part of URL ?

Since it is unique combination for a georep session and required always. It also makes more sense for other commands start/stop/status/config (Ex: POST /v1/geo-replication/:masterid/:slaveid/start)

aravindavk · 2017-10-12T07:29:37Z

plugins/georeplication/init.go

+	return route.Routes{
+		route.Route{
+			Name:        "GeoreplicationCreate",
+			Method:      "PUT",


I will change this to POST

aravindavk · 2017-10-12T07:32:23Z

plugins/georeplication/rest.go

+	}
+
+	// Validate UUID format of Master and Slave Volume ID
+	masterid, slaveid, err := validateMasterAndSlaveIDFormat(w, masteridRaw, slaveidRaw)


Initially I had that in handler itself, but observed that I have to repeat the stuff almost for all Georep API calls. (MasterVol, SlaveHosts and SlaveVol) validation is required only for Create/Update. For all the other apis Start/Stop/Delete/Status/Config we don't need to validate these fields except Master ID and Slave ID

aravindavk · 2017-10-12T07:34:11Z

plugins/georeplication/rest.go

+			return
+		}
+
+		geoSession = new(georepapi.GeorepSession)


aravindavk · 2017-10-12T07:36:00Z

plugins/georeplication/rest.go

+		lock,
+		{
+			DoFunc: "georeplication-create.Commit",
+			Nodes:  []uuid.UUID{gdctx.MyUUID},


There are some todos for Geo-rep create steps. I think I need to create a working directory in all Volume nodes before starting. I will work on those steps later.

aravindavk · 2017-10-12T07:37:05Z

plugins/georeplication/rest.go

+
+	geoSession.Status = georepapi.GeorepStatusCreated
+
+	e = addOrUpdateSession(geoSession)


Ok, I will move this to txn

prashanthpai

LGTM for now. I'd prefer you move a function from api package though.

prashanthpai · 2017-10-13T04:03:49Z

plugins/georeplication/api/resp.go

@@ -54,3 +54,22 @@ type GeorepSession struct {
 	Workers    []GeorepWorker    `json:"workers"`
 	Options    map[string]string `json:"options"`
 }
+
+// NewGeorepSession creates new instance of GeorepSession
+func NewGeorepSession(mastervolid uuid.UUID, slavevolid uuid.UUID, req GeorepCreateReq) *GeorepSession {


This shouldn't be part of api package though which is supposed to contain only API requests and responses. In future, I hope we will be able to generate API doc from the package or generate package code from the API spec.

You can place this function near the handler. And if it's only invoked there, you can as well choose not export it.

REST API to create Geo-replication session create PUT /v1/geo-replication/:mastervolid/:slavevolid Request body, { "mastervol": MASTER_VOL_NAME, "slavevol": SLAVE_VOL_NAME, "slavehosts": LIST_OF_SLAVE_HOSTS, "slaveuser": SLAVE_USER } REST API to update Geo-replication session update POST /v1/geo-replication/:mastervolid/:slavevolid Request body, { "mastervol": MASTER_VOL_NAME, "slavevol": SLAVE_VOL_NAME, "slavehosts": LIST_OF_SLAVE_HOSTS, "slaveuser": SLAVE_USER } Updates: #271 Signed-off-by: Aravinda VK <avishwan@redhat.com>

Added a basic test for Georep create, also added restclient entry. Georep apis are added to main restclient package itself since main restclient can't be extended if it is separate package. Updates: #271 Signed-off-by: Aravinda VK <avishwan@redhat.com>

Updates: #271 Signed-off-by: Aravinda VK <mail@aravindavk.in>

aravindavk · 2017-10-13T05:48:42Z

@prashanthpai updated the patch, please check

aravindavk requested review from kshlm, prashanthpai and atinmu October 10, 2017 14:50

prashanthpai reviewed Oct 12, 2017

View reviewed changes

aravindavk commented Oct 12, 2017

View reviewed changes

prashanthpai suggested changes Oct 13, 2017

View reviewed changes

aravindavk added 3 commits October 13, 2017 10:10

plugins/georep: Moved NewGeorepSession to handler

228f23d

Updates: #271 Signed-off-by: Aravinda VK <mail@aravindavk.in>

prashanthpai approved these changes Oct 13, 2017

View reviewed changes

aravindavk merged commit 9e96491 into gluster:master Oct 13, 2017

aravindavk deleted the georep_create branch October 13, 2017 09:52

prashanthpai mentioned this pull request Nov 23, 2017

plugins/snapshot: REST API placeholder for Snapshot #383

Closed


		geoSession.Status = georepapi.GeorepStatusCreated

		e = addOrUpdateSession(geoSession)

plugins/geo-rep: Geo-replication Session Create #385

plugins/geo-rep: Geo-replication Session Create #385

Conversation

aravindavk commented Oct 10, 2017 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Oct 13, 2017

aravindavk commented Oct 10, 2017 •

edited

Loading