gd2 plugin: added a plugin for block volume management by oshankkumar · Pull Request #1357 · gluster/glusterd2

oshankkumar · 2018-11-30T09:56:14Z

added APIs for creation,deleting and listing block volumes.
added pluggable interface for block volume providers.

Refer Design Doc: #1319

Signed-off-by: Oshank Kumar okumar@redhat.com

centos-ci · 2018-11-30T09:56:15Z

Can one of the admins verify this patch?

Madhu-1 · 2018-11-30T11:19:09Z

add to whitelist

Madhu-1 · 2018-12-05T15:25:49Z

retest this please

Madhu-1 · 2018-12-05T15:31:27Z

CI failure

20:58:20 Building glusterd2 v6.0-dev.54.gita525bc5
20:58:21 plugins/blockvolume/blockprovider/gluster-block/glusterblock.go:10:2: cannot find package "github.com/gluster/gluster-block-restapi/client" in any of:
20:58:21 	/root/go/src/github.com/gluster/glusterd2/vendor/github.com/gluster/gluster-block-restapi/client (vendor tree)
20:58:21 	/usr/local/go/src/github.com/gluster/gluster-block-restapi/client (from $GOROOT)
20:58:21 	/root/go/src/github.com/gluster/gluster-block-restapi/client (from $GOPATH)
20:58:21 plugins/blockvolume/blockprovider/gluster-block/glusterblock.go:11:2: cannot find package "github.com/gluster/gluster-block-restapi/pkg/api" in any of:
20:58:21 	/root/go/src/github.com/gluster/glusterd2/vendor/github.com/gluster/gluster-block-restapi/pkg/api (vendor tree)
20:58:21 	/usr/local/go/src/github.com/gluster/gluster-block-restapi/pkg/api (from $GOROOT)
20:58:21 	/root/go/src/github.com/gluster/gluster-block-restapi/pkg/api (from $GOPATH)

nigelbabu · 2018-12-06T09:06:15Z

retest this please

amarts · 2018-12-06T11:26:32Z

pkg/size/size_test.go

@@ -0,0 +1,166 @@
+package size


looks like 'package size' can get into code just like that. It can be used by other services too.

amarts · 2018-12-06T11:30:03Z

plugins/blockvolume/blockprovider/gluster-block/glusterblock.go

+		Hosts:              hosts,
+	}
+
+	resp, err := g.client.CreateBlockVolume(volInfo.Name, name, req)


We need to make sure we have the BlockHostingVolume is up and running. Because, if it is not present, then this will fail for sure. If it is already taken care by the events above, then all good.

Line 149 is checking that Hosting Volume is in Started State

aravindavk · 2018-12-12T03:31:45Z

I don't see Gopkg.toml changes which mentions the new dependency

oshankkumar · 2018-12-12T06:52:17Z

I don't see Gopkg.toml changes which mentions the new dependency

I am getting error like this while updating the dependencies using dep ensure

Solving failure: No versions of github.com/gluster/gluster-block-restapi met constraints:
	v0.2: Could not introduce github.com/gluster/gluster-block-restapi@v0.2, as it has a dependency on github.com/sirupsen/logrus with constraint ^1.2.0, which has no overlap with existing constraint ~1.0.3 from (root)

@aravindavk @Madhu-1 can you please suggest how to resolve this

Madhu-1 · 2018-12-12T07:00:20Z

@oshankkumar update github.com/sirupsen/logrus version to "~1.2.0" in toml file to fix this issue

aravindavk

Did a initial review. Mostly looks good, a few comments. I will test the PR once.

aravindavk · 2018-12-13T05:43:50Z

plugins/blockvolume/blockprovider/gluster-block/glusterblock.go

+
+	for _, blockVol := range availableBlockVolumes {
+		if blockVol.Name() == name {
+			blockVolume = blockVol


aravindavk · 2018-12-13T05:46:18Z

plugins/blockvolume/blockprovider/gluster-block/volume.go

+}
+
+// StartBlockHostingVolume starts a gluster volume
+func StartBlockHostingVolume(name string) error {


Is this different from regular Volume start. If not can we reuse the code

aravindavk · 2018-12-13T05:51:37Z

plugins/blockvolume/blockprovider/gluster-block/volume.go

+		return nil, err
+	}
+
+	vInfo.Metadata["_block-hosting-volume-auto-created"] = "yes"


Using _ prefix will make it impossible to manually create block hosting volume and use that for block file creation.(Since Metadata keys starts with _ are restricted for API users). We can skip using prefix or we can expose BlockHosting=Yes/No flag in Volume Edit API

aravindavk · 2018-12-13T05:53:42Z

glusterd2/commands/volumes/volume-create.go

-	}
+		return http.StatusBadRequest, err

-	// Include default Volume Options profile


this new change is lost in Rebase?

Madhu-1 · 2018-12-13T06:02:37Z

glusterd2/commands/volumes/volume-create.go

 		return
 	}

-	if containsReservedGroupProfile(req.Options) {


this check also lost

Madhu-1

i would like to do one more round of review

Madhu-1 · 2018-12-13T06:05:56Z

plugins/blockvolume/api/types.go

+// BlockVolumeInfo represents block volume info
+type BlockVolumeInfo struct {
+	// HostingVolume name is optional
+	HostingVolume string `json:"hosting_volume"`


don't use underscore in json name

can this be omitempty?

am i missing anything, dont we need to consider size for blockhosting volume.

Madhu-1 · 2018-12-13T06:08:53Z

plugins/blockvolume/api/types.go

+
+// BlockVolumeCreateResp represents resp body for a Block Vol Create req
+type BlockVolumeCreateResp struct {
+	*BlockVolumeInfo


as you are using BlockVolumeInfo size cannot be omitempty for this volume response

Since we are getting only list of blockvolume name from gluster-block list command.
So for a blockvolume list response, we are only showing blockvolume name and hosting volume name, so if I remove omitempty then size will be displayed as 0 in response.

for a BlockVolumeList response size is not a mandatory field

Madhu-1 · 2018-12-13T06:09:19Z

plugins/blockvolume/api/types.go

+type BlockVolumeGetResp struct {
+	*BlockVolumeInfo
+	Hosts    []string `json:"hosts"`
+	Password string   `json:"password,omitempty"`


dont we need username here?

we don't get username details from gluster-block info command

Madhu-1 · 2018-12-13T06:10:51Z

plugins/blockvolume/blockprovider/blockvolume_provider.go

+
+// RegisterBlockProvider will register a block provider
+func RegisterBlockProvider(name string, f ProviderFunc) {
+	providersMutex.Lock()


can you explain whats the advantage of taking lock here and in below functions.

just to make sure providerFactory map is not accessed parallelly by some other goroutine

poornimag · 2018-12-18T09:22:56Z

As discussed, could you please move the block host volume management to block provider instead of gluster-block, so that the block host volume management code can be reused by all providers?

oshankkumar · 2018-12-18T09:26:56Z

As discussed, could you please move the block host volume management to block provider instead of gluster-block, so that the block host volume management code can be reused by all providers?

moved the common code to a utils package

aravindavk · 2018-12-18T10:14:35Z

Block provider details can be handled in URL to support different providers. POST /v1/subvolumes/{provider}. (Note: Name "subvolumes" here may add confusion wrt subvol in gluster. Any suggestions for the name?)

Create

POST /subvolumes/gluster-block         To provide block volumes using gluster-block
POST /subvolumes/loop-block             To provide block volumes using loop devices
POST /subvolumes/dir                         To provide volumes using subdir approach

Similarly for other URLS

Same ReST handler receives all the requests and host volume management part will remain same. Instead of calling g.client.CreateBlockVolume(volInfo.Name, name, req) in Line 164(https://github.com/gluster/glusterd2/pull/1357/files#diff-3a743ffc1638946e95edba8bc6e26c50R164) it will call provider.CreateBlockVolume(args...)

In handler,

providerName := mux.Vars(r)["provider"]
provider := getProvider(providerName)  // This gets the interface implementation of respective provider

@oshankkumar @poornimag @Madhu-1 @amarts thoughts?

atinmu · 2018-12-24T13:54:00Z

@aravindavk @Madhu-1 can you folks take a look at the latest version? I believe all comments are being incorporated now?

oshankkumar · 2018-12-24T15:12:54Z

@aravindavk @Madhu-1 can you folks take a look at the latest version? I believe all comments are being incorporated now?

yes, all review comments has been addressed.

amarts · 2018-12-28T03:18:18Z

what more is pending on this ?

aravindavk · 2019-01-03T06:02:13Z

glusterd2.toml.example

+#[gluster-block-hosting-vol-options]
+block-hosting-volume-size = 20971520
+auto-create-block-hosting-volumes = true
+block-hosting-volume-replica-count = 1


Change this to 3

aravindavk · 2019-01-03T06:02:28Z

glusterd2.toml.example

+block-hosting-volume-size = 20971520
+auto-create-block-hosting-volumes = true
+block-hosting-volume-replica-count = 1
+#block-hosting-volume-type = "Distribute"


Change this to Replicate

Madhu-1 · 2019-01-03T06:13:00Z

@oshankkumar please resolve merge conflicts

Madhu-1 · 2019-01-03T06:13:53Z

glusterd2.toml.example

 #restauth = true
+
+#[gluster-block-hosting-vol-options]
+block-hosting-volume-size = 20971520


add a comment what is this size in Gib for more readability

Madhu-1 · 2019-01-03T06:14:31Z

glusterd2.toml.example

+gluster-block-hostaddr = "192.168.122.16:8081"
+#gluster-block-cacert = "/path/to/ca.crt"
+#gluster-block-user = "username_for_rest_authentication"
+#gluster-block-secret = "secret_for_rest_authentication"


add a new line

Madhu-1 · 2019-01-03T06:23:12Z

pkg/size/size.go

@@ -0,0 +1,187 @@
+package size


but glusterd2 doeent make a call to the gluster block cli, even the input request and the response recevied from the block rest server is interger, but am not sure how useful it is? it will be only useful if we use gluster block cli with gd2 directly

Madhu-1 · 2019-01-03T06:41:19Z

@oshankkumar regenerate endpoints.md file

Madhu-1 · 2019-01-03T06:47:32Z

plugins/blockvolume/blockprovider/gluster-block/glusterblock.go

+	}
+	defer clusterLocks.UnLock(context.Background())
+
+	volInfo, err := volume.GetVolume(hostVolume)


can we move this logic to some other function, so this can be reused for checking block hosting volume by other providers?

moved updating available hosting volume size to a common function,

Madhu-1 · 2019-01-03T16:28:18Z

glusterd2.toml.example

+block-hosting-volume-size = 5368709120 #5GiB
+auto-create-block-hosting-volumes = true
+block-hosting-volume-replica-count = 3
+#block-hosting-volume-type = "Replicate"


this needs to be commented?

Madhu-1 · 2019-01-03T16:39:01Z

@aravindavk PTAL.

- added APIs for creation,deleting and listing block volumes. - added pluggable interface for block volume providers. Refer Design Doc: gluster#1319 Signed-off-by: Oshank Kumar <okumar@redhat.com>

Signed-off-by: Oshank Kumar <okumar@redhat.com>

- added block volume provider name in path parameter of url - block provider will not be responsible for managing host volumes. Signed-off-by: Oshank Kumar <okumar@redhat.com>

Signed-off-by: Oshank Kumar <okumar@redhat.com>

…volume size - moved hosts parameter from mandatory to optional field in CreateBlockVolume method of BlockProvider interface,since hosts field may not required for other block providers like loopback. - a common function for updating available hosting volume size will prevent from duplicate code Signed-off-by: Oshank Kumar <okumar@redhat.com>

ghost assigned oshankkumar Nov 30, 2018

ghost added the in progress label Nov 30, 2018

oshankkumar force-pushed the gluster-block branch from 161550f to 23f11fe Compare November 30, 2018 14:15

ghost assigned Madhu-1 Dec 5, 2018

amarts reviewed Dec 6, 2018

View reviewed changes

ghost assigned aravindavk Dec 6, 2018

oshankkumar force-pushed the gluster-block branch from cc48db2 to 199161b Compare December 11, 2018 09:25

oshankkumar force-pushed the gluster-block branch from 199161b to 6fb1dac Compare December 12, 2018 06:44

aravindavk suggested changes Dec 13, 2018

View reviewed changes

Madhu-1 requested review from Madhu-1 and raghavendra-talur December 13, 2018 06:00

Madhu-1 reviewed Dec 13, 2018

View reviewed changes

Madhu-1 suggested changes Dec 13, 2018

View reviewed changes

oshankkumar force-pushed the gluster-block branch 4 times, most recently from 9b02076 to cc4c519 Compare December 17, 2018 10:38

oshankkumar force-pushed the gluster-block branch from cc4c519 to 79e14b6 Compare December 18, 2018 09:24

poornimag mentioned this pull request Dec 27, 2018

Add loopback block provider #1439

Merged

oshankkumar force-pushed the gluster-block branch 2 times, most recently from c08a0ee to bd64a95 Compare December 31, 2018 11:07

aravindavk reviewed Jan 3, 2019

View reviewed changes

Madhu-1 reviewed Jan 3, 2019

View reviewed changes

oshankkumar force-pushed the gluster-block branch 4 times, most recently from 7b02a7e to 56f9240 Compare January 3, 2019 12:35

Madhu-1 reviewed Jan 3, 2019

View reviewed changes

Madhu-1 approved these changes Jan 3, 2019

View reviewed changes

Oshank Kumar added 7 commits January 4, 2019 20:52

gd2 plugin: added a plugin for block volume management

fe42d9e

- added APIs for creation,deleting and listing block volumes. - added pluggable interface for block volume providers. Refer Design Doc: gluster#1319 Signed-off-by: Oshank Kumar <okumar@redhat.com>

block-volume: moved common code to a utils package

4dabeb1

Signed-off-by: Oshank Kumar <okumar@redhat.com>

block-volume: added a interface for host volume management.

9391a6c

- added block volume provider name in path parameter of url - block provider will not be responsible for managing host volumes. Signed-off-by: Oshank Kumar <okumar@redhat.com>

block-volume: change size type from int64 to uint64

ffadf9e

Signed-off-by: Oshank Kumar <okumar@redhat.com>

update dependencies using dep ensure

c60180a

Signed-off-by: Oshank Kumar <okumar@redhat.com>

block volume: Fix Merge conflicts in volume-start.go

827f2f0

Signed-off-by: Oshank Kumar <okumar@redhat.com>

oshankkumar force-pushed the gluster-block branch from 56f9240 to 0561c0e Compare January 4, 2019 15:23

aravindavk approved these changes Jan 7, 2019

View reviewed changes

aravindavk merged commit 8ef2e18 into gluster:master Jan 7, 2019

ghost removed the in progress label Jan 7, 2019

Conversation

oshankkumar commented Nov 30, 2018

Uh oh!

centos-ci commented Nov 30, 2018

Uh oh!

Madhu-1 commented Nov 30, 2018

Uh oh!

Madhu-1 commented Dec 5, 2018

Uh oh!

Madhu-1 commented Dec 5, 2018

Uh oh!

nigelbabu commented Dec 6, 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!

aravindavk commented Dec 12, 2018

Uh oh!

oshankkumar commented Dec 12, 2018

Uh oh!

Madhu-1 commented Dec 12, 2018

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!

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!

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!

poornimag commented Dec 18, 2018

Uh oh!

oshankkumar commented Dec 18, 2018

Uh oh!

aravindavk commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atinmu commented Dec 24, 2018

Uh oh!

oshankkumar commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amarts commented Dec 28, 2018

aravindavk commented Dec 18, 2018 •

edited

Loading

oshankkumar commented Dec 24, 2018 •

edited

Loading

Madhu-1 Jan 3, 2019 •

edited

Loading

oshankkumar Jan 3, 2019 •

edited

Loading