Mounting auto provisioned bricks on gd2 start and unified Volume create API #973

aravindavk · 2018-07-09T07:36:58Z

Commit 1 Merged Smartvol API into regular Volume Create API
Commit 2 Mounting auto provisioned bricks on glusterd2 start

Updates: #851

rafikc30 · 2018-07-09T12:46:12Z

glusterd2/volume/volume-utils.go

+		for _, b := range v.GetLocalBricks() {
+			// Mount all local Bricks if they are auto provisioned
+			if b.MountInfo.DevicePath != "" {
+				if _, exists := mounts[b.MountInfo.Mountdir]; exists {


MountInfo.Mountdir actually contains the directory after mount root, for example let's say mount point is /bricks, but brick path could be /bricks/b1 . in this case MountDir will have /b1.

The name is bit consfusing, I just took the same name from gd1.

I added without that directory. See Path and Mountdir here https://github.com/gluster/glusterd2/pull/973/files#diff-3e14b73c75a337a5a43f9719dcd68228R131

While I agree that Mountdir is not an optimal name, storing both path and actual mount dir seems redundant to me. In most case, path will be equal to mount root. And in almost all cases, directories after mount root will be less in length.
So I think we should store path and directory after mount root, and creates mount root using these two.

makes sense, we can make that optimization. That can be separate PR since that needs to be handled in many places including response. brick.Path = brick.Mountdir + brick.Dir

That's fine for me. We should store path and dir and calculate mount root like MountRoot := b.Path[:len(b.Dir)] .

One more thing As soon as this patch get's in, the snapshot functionality will break. Because, if you have any restored volume or cloned volume, then Mountdir is different in both case.

If e2e tests don't fail then I prefer submitting snapshot related changes as separate PR. is that okay?

sure. We can create an issue

rafikc30 · 2018-07-09T12:47:52Z

glusterd2/volume/volume-utils.go

@@ -199,3 +200,54 @@ func CreateVolumeInfoResp(v *Volinfo) *api.VolumeInfo {
 		SnapList:  v.SnapList,
 	}
 }
+
+// MountLocalBricks mounts bricks of auto provisioned volumes
+func MountLocalBricks() error {


Can we have this function in common utils, so that the same can be used for snapshot also. Otherwise I have to use another function during glusterd2 init.

Since it is not generic function it depends on store utils, can't move to common utils. Let me see

You could create a glusterd2/mountutils or just glusterd2/utils instead of pkg/utils

aravindavk · 2018-07-12T11:45:34Z

retest this please

aravindavk · 2018-07-12T15:31:18Z

@kshlm @prashanthpai @Madhu-1 @rafikc30 please review

Madhu-1 · 2018-07-13T04:49:35Z

glusterd2/commands/volumes/volume-create.go

@@ -18,8 +20,20 @@ import (

 const (
 	maxMetadataSizeLimit = 4096
+	minVolumeSize        = 10


question:
want to understand why minimum volume size is 10

I think we may have to increase this upto 20, this is because mkfs.xfs was not working on smaller size.

For now this is moved from plugin/smartvol code to here.

Madhu-1 · 2018-07-13T04:50:57Z

glusterd2/commands/volumes/volume-create.go

@@ -29,7 +43,11 @@ func validateVolCreateReq(req *api.VolCreateReq) error {
 		return errors.New("invalid transport. Supported values: tcp or rdma")
 	}

-	if len(req.Subvols) <= 0 {
+	if req.Size > 0 && req.Size < minVolumeSize {
+		return errors.New("invalid Volume Size, Minimum size required is " + strconv.Itoa(minVolumeSize))


suggestion: can you convert the capital case to a smaller case in the error message, wherever its required

Madhu-1 · 2018-07-13T04:53:26Z

glusterd2/commands/volumes/volume-create.go

+
+		if req.SnapshotReserveFactor < 1 {
+			restutils.SendHTTPError(ctx, w, http.StatusBadRequest,
+				errors.New("invalid Snapshot Reserve Factor"))


same here, check any changes required in error message case

Madhu-1 · 2018-07-13T04:54:56Z

glusterd2/commands/volumes/volume-smartvol-txn.go

 		c.Logger().WithError(err).WithField(
-			"key", "reqBricksPrepare").Debug("Failed to get key from store")
+			"key", "req").Debug("Failed to get key from store")


question:
This should be Debug or Error message

not changed this part of the code while moving from plugins/smartvol. Will update this as required

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

glusterd2/commands/volumes/volume-smartvol-txn.go

 			if err != nil {
 				c.Logger().WithError(err).
-					WithField("dev", dev).
-					WithField("path", brickMountDir).
+					WithField("dev", b.DevicePath).


question:

which one is best to use WithFields or WithFiled in this case

I will change this to WithFields

Madhu-1 · 2018-07-13T04:56:35Z

glusterd2/commands/volumes/volume-smartvol-txn.go

 		c.Logger().WithError(err).WithField(
-			"key", "reqBricksPrepare").Debug("Failed to get key from store")
+			"key", "req").Debug("Failed to get key from store")


same here, Debug or Error

Madhu-1 · 2018-07-13T04:57:40Z

glusterd2/volume/volume-utils.go

+	if e != nil || volumes == nil {
+		// In case cluster doesn't have any volumes configured yet,
+		// treat this as success
+		log.Debug("Failed to retrieve volumes")


This should be Error or debug log

Madhu-1 · 2018-07-13T04:58:59Z

glusterd2/volume/volume-utils.go

+				err := utils.ExecuteCommandRun("mount", "-o", b.MountInfo.MntOpts, b.MountInfo.DevicePath, b.MountInfo.Mountdir)
+				if err != nil {
+					log.WithError(err).
+						WithField("volume", v.Name).


same here, WithFields or WithField is better in this case

Madhu-1

LGTM, but I have some comments, which is not a functional changes

kshlm · 2018-07-13T11:23:53Z

glusterd2/volume/volume-utils.go

@@ -199,3 +200,54 @@ func CreateVolumeInfoResp(v *Volinfo) *api.VolumeInfo {
 		SnapList:  v.SnapList,
 	}
 }
+
+// MountLocalBricks mounts bricks of auto provisioned volumes
+func MountLocalBricks() error {


You could create a glusterd2/mountutils or just glusterd2/utils instead of pkg/utils

kshlm · 2018-07-13T11:25:55Z

glusterd2/volume/volume-utils.go

+// MountLocalBricks mounts bricks of auto provisioned volumes
+func MountLocalBricks() error {
+	volumes, e := GetVolumes()
+	if e != nil || volumes == nil {


This should be 2 unique cases. You can have an error trying to get volumes, and you can get an empty list of volumes. The error is an error, the empty list is not.

kshlm · 2018-07-13T11:43:40Z

pkg/api/volume_req.go

+	ExcludePeers            []string          `json:"exclude-peers,omitempty"`
+	ExcludeZones            []string          `json:"exclude-zones,omitempty"`
+	SubvolZonesOverlap      bool              `json:"subvolume-zones-overlap,omitempty"`
+	SubvolType              string            `json:"subvolume-type,omitempty"`


These two structs are now huge. I think it would be a good idea to group all the IVP related members in to their own structs and add those structs as members of the main structs.

I feel input API will not be intuitive if we make it as sub level.

I think otherwise. But we can finalize the request structure later.

Example: curl -XPOST http://localhost:24007/v1/volumes \ -d '{"name": "gv1", "size": 1000, "replica": 3}' Where size is in Mb. CLI usage is unchanged glustercli volume create --name gv1 --size 1G --replica 3 Signed-off-by: Aravinda VK <avishwan@redhat.com>

On `glusterd2` restart, it checks the list of auto provisioned bricks and mounts if not mounted already. Updates: #851 Signed-off-by: Aravinda VK <avishwan@redhat.com>

aravindavk · 2018-07-17T14:10:18Z

Addressed the review comments. Please review.

aravindavk requested review from kshlm and prashanthpai July 9, 2018 07:36

rafikc30 reviewed Jul 9, 2018

View reviewed changes

aravindavk mentioned this pull request Jul 12, 2018

Unified Volume Create API and mount bricks on Glusterd2 start #851

Closed

Madhu-1 reviewed Jul 13, 2018

View reviewed changes

kshlm suggested changes Jul 13, 2018

View reviewed changes

aravindavk added 2 commits July 17, 2018 16:15

Mounting auto provisioned bricks on glusterd2 start

368e802

On `glusterd2` restart, it checks the list of auto provisioned bricks and mounts if not mounted already. Updates: #851 Signed-off-by: Aravinda VK <avishwan@redhat.com>

aravindavk added the GCS-Sprint2-review-pending label Jul 18, 2018

kshlm approved these changes Jul 18, 2018

View reviewed changes

kshlm merged commit 7603e0d into gluster:master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mounting auto provisioned bricks on gd2 start and unified Volume create API #973

Mounting auto provisioned bricks on gd2 start and unified Volume create API #973

aravindavk commented Jul 9, 2018

rafikc30 Jul 9, 2018

aravindavk Jul 10, 2018

rafikc30 Jul 17, 2018

aravindavk Jul 17, 2018

rafikc30 Jul 17, 2018

aravindavk Jul 17, 2018

rafikc30 Jul 17, 2018

rafikc30 Jul 9, 2018

aravindavk Jul 10, 2018

kshlm Jul 13, 2018

aravindavk commented Jul 12, 2018

aravindavk commented Jul 12, 2018

Madhu-1 Jul 13, 2018

aravindavk Jul 13, 2018

Madhu-1 Jul 13, 2018

aravindavk Jul 13, 2018

Madhu-1 Jul 13, 2018

Madhu-1 Jul 13, 2018

aravindavk Jul 13, 2018

Madhu-1 Jul 13, 2018

aravindavk Jul 13, 2018

Madhu-1 Jul 13, 2018

Madhu-1 Jul 13, 2018 •

edited

Loading

aravindavk Jul 13, 2018

Madhu-1 Jul 13, 2018

Madhu-1 left a comment

kshlm Jul 13, 2018

kshlm Jul 13, 2018

kshlm Jul 13, 2018

aravindavk Jul 16, 2018

kshlm Jul 18, 2018

aravindavk commented Jul 17, 2018

Mounting auto provisioned bricks on gd2 start and unified Volume create API #973

Mounting auto provisioned bricks on gd2 start and unified Volume create API #973

Conversation

aravindavk commented Jul 9, 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

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 Jul 12, 2018

aravindavk commented Jul 12, 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

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

Madhu-1 Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 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

aravindavk commented Jul 17, 2018

Madhu-1 Jul 13, 2018 •

edited

Loading