Plugin architecture for brick Provisioners #1256

aravindavk · 2018-09-29T13:27:56Z

Code is refactored to create Provisioner interface and lvm
provisioner is now a plugin

To create a new Provisioner, implement the following interface
and add it to glusterd2/provisioners/provisioner.go

type Provisioner interface {
	// Register will be called when user registers a device or directory.
	// PV and VG will be created in case of lvm plugin
	Register(devpath string) error
	// AvailableSize returns available size of the device
	AvailableSize(devpath string) (uint64, uint64, error)
	// Unregister will be called when device/directory needs to be removed
	Unregister(devpath string) error
	// CreateBrick creates the brick volume
	CreateBrick(devpath, brickid string, size uint64, bufferFactor float64) error
	// CreateBrickFs will create the brick filesystem
	CreateBrickFS(devpath, brickid, fstype string) error
	// CreateBrickDir will create the brick directory
	CreateBrickDir(brickPath string) error
	// MountBrick will mount the brick
	MountBrick(devpath, brickid, brickPath string) error
	// UnmountBrick will unmount the brick
	UnmountBrick(devpath, brickid string) error
	// RemoveBrick will remove the brick
	RemoveBrick(devpath, brickid string) error
}

Signed-off-by: Aravinda VK [email protected]

amarts

This is very very critical! With this, technically, we are opening up GD2 (and IVP too) many other projects, who wants to use something other than xfs/lvm combination!

Would be great if this can make it in soon!

amarts · 2018-09-29T18:22:29Z

plugins/lvmprovisioner/provisioner.go

+	"github.com/gluster/glusterd2/pkg/lvmutils"
+)
+
+var mountOpts = "rw,inode64,noatime,nouuid"


We also need 'discard' as a mount option? So that actual backend gets free'd up ?

Is this option for general purpose or only for loopmount bricks?

On LVM it returns space to the TP, and on loop, it'll return space to the outer file system... both are desirable.

amarts · 2018-09-29T18:25:26Z

pkg/api/volume_req.go

+	Path       string `json:"path"`
+	Name       string `json:"name,omitempty"`
+	Size       uint64 `json:"size,omitempty"`
+	VgID       string `json:"vg-id,omitempty"`


do we need this any more?

Code is refactored to create Provisioner interface and lvm provisioner is now a plugin To create a new Provisioner, implement the following interface and add it to `glusterd2/provisioners/provisioner.go` ``` type Provisioner interface { // Register will be called when user registers a device or directory. // PV and VG will be created in case of lvm plugin Register(devpath string) error // AvailableSize returns available size of the device AvailableSize(devpath string) (uint64, uint64, error) // Unregister will be called when device/directory needs to be removed Unregister(devpath string) error // CreateBrick creates the brick volume CreateBrick(devpath, brickid string, size uint64, bufferFactor float64) error // CreateBrickFs will create the brick filesystem CreateBrickFS(devpath, brickid, fstype string) error // CreateBrickDir will create the brick directory CreateBrickDir(brickPath string) error // MountBrick will mount the brick MountBrick(devpath, brickid, brickPath string) error // UnmountBrick will unmount the brick UnmountBrick(devpath, brickid string) error // RemoveBrick will remove the brick RemoveBrick(devpath, brickid string) error } ``` Signed-off-by: Aravinda VK <[email protected]>

aravindavk · 2018-10-08T15:08:31Z

Snapshot related code is not refactored in this PR, I will add Snapshot related changes in separate PR.
This PR also includes device API refactor

Please review

Madhu-1 · 2018-10-09T06:48:19Z

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

@@ -21,74 +20,76 @@ func txnPrepareBricks(c transaction.TxnCtx) error {
 		return err
 	}

+	var provisioner provisioners.Provisioner


can you make a seperate function to avoid code duplication
like

func validateProvisioner(req *api.VolCreateReq)(error,provisioners.Provisioner){ var provisioner provisioners.Provisioner var err error if req.Provisioner == "" { provisioner = provisioners.GetDefault() } else { provisioner, err = provisioners.Get(req.Provisioner) if err != nil { c.Logger().WithError(err).WithField("name", req.Provisioner).Error("invalid provisioner") return err,nil } } return nil,provisioner }

Madhu-1 · 2018-10-09T06:51:51Z

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

 			if err != nil {
 				c.Logger().WithError(err).WithField(
 					"path", b.Path).Error("failed to create brick directory in mount")
 				return err
 			}

-			// Update current Vg free size
-			err = deviceutils.UpdateDeviceFreeSize(gdctx.MyUUID.String(), b.VgName)
+			availableSize, extentSize, err := provisioner.AvailableSize(b.Device)


same for AvailableSize and UpdateDeviceFreeSize
I think we can create a separate function which does the functionality for getting the Available size and updating free size

Madhu-1 · 2018-10-09T07:01:18Z

plugins/device/deviceutils/errors.go

@@ -0,0 +1,10 @@
+package deviceutils


instead of having a separate file, can we make use of pkg/error/error.go?

not added in pkg/error since this is in plugins right now

Madhu-1 · 2018-10-09T07:02:49Z

plugins/device/deviceutils/store-utils.go

+}
+
+// UpdateDeviceFreeSize updates the actual available size of VG
+func UpdateDeviceFreeSize(peerid, device string, size uint64, extentSize uint64) error {


size uint64, extentSize uint64 can be made as size , extentSize uint64

Madhu-1 · 2018-10-09T07:07:12Z

plugins/device/transaction.go

-		if errPV != nil {
-			c.Logger().WithError(err).WithField("device", device).Error("Failed to remove physical volume")
+
+	var provisioner provisioners.Provisioner


this also can be avoided if we have this code in separate function

Madhu-1 · 2018-10-09T07:10:18Z

@ansiwen as you are integrating heketi with glusterd2, can you check this PR once as this changes device structure.

prashanthpai · 2018-10-11T06:20:13Z

pkg/lvmutils/utils.go

@@ -16,6 +15,16 @@ const (
 	chunkSize         = "1280k"
 )

+// MbToKb converts Value from Mb to Kb


A better place for these two functions would be pkg/utils as they are used outside of lvmutils as well.

I left it since this will go away once we convert everything to bytes.

prashanthpai · 2018-10-11T06:24:19Z

pkg/fsutils/utils.go

+	"github.com/gluster/glusterd2/pkg/utils"
+)
+
+// MakeXfs creates XFS filesystem


nit: Can be better named MakeXFS ?

prashanthpai · 2018-10-11T06:25:22Z

pkg/fsutils/utils.go

+
+// Mount mounts the brick LV
+func Mount(dev, mountdir, options string) error {
+	return utils.ExecuteCommandRun("mount",


If https://golang.org/pkg/syscall/#Mount supports mount options, you can use that.

prashanthpai · 2018-10-11T06:28:38Z

plugins/device/rest.go

+		return
+	}
+
+	if err != deviceutils.ErrDeviceNotFound {


You can return 404 on ErrDeviceNotFound

prashanthpai · 2018-10-11T06:29:39Z

plugins/device/rest.go

 		return
 	}

 	// FIXME: Change this to http.StatusCreated when we are able to set
 	// location header with a unique URL that points to created device.
-	restutils.SendHTTPResponse(ctx, w, http.StatusOK, peerInfo)
+	restutils.SendHTTPResponse(ctx, w, http.StatusOK, deviceInfo)


Please check if the above FIXME can now be handled. Feel free to address in a later PR or commit.

prashanthpai · 2018-10-11T06:51:45Z

pkg/api/volume_req.go

+	Name       string `json:"name,omitempty"`
+	Size       uint64 `json:"size,omitempty"`
+	Mountdir   string `json:"mount-dir,omitempty"`
+	Device     string `json:"device,omitempty"`


What's the difference between device and device-path ?

I need to think about different naming for this. Device is main device where VG is created in case of lvm. DevicePath is path of LV where brick needs to be mounted. Is RootDevice or ParentDevice sounds good instead of just Device?

prashanthpai · 2018-10-11T06:54:26Z

glusterd2/utils/mount.go

@@ -46,6 +47,28 @@ func MountLocalBricks() error {
 					continue
 				}

+				if v.Provisioner != "" {
+					provisioner, err := provisioners.Get(v.Provisioner)


This will and should not happen, right ?
May be return error instead of continuing ?

Currently Snapshot Provisioned volumes are not included in this Provisioner interface. Mounting Snapshot provisioned volume or restored volume support needs to be handled.

prashanthpai · 2018-10-11T06:55:17Z

glusterd2/utils/mount.go

+						continue
+					}
+
+					err = provisioner.MountBrick(b.Device, b.Name, b.Path)


I see a mount command just below. What case does that handle ?

that is to temporarily support mounting snapshot provisioned bricks.

prashanthpai · 2018-10-11T06:56:38Z

glusterd2/provisioners/provisioners.go

+	// Unregister will be called when device/directory needs to be removed
+	Unregister(devpath string) error
+	// CreateBrick creates the brick volume
+	CreateBrick(devpath, brickid string, size uint64, bufferFactor float64) error


I haven't dived too deep but I think there's an opportunity to merge some of these brick related methods, especially the ones that are always called in order.

For example, can the following for methods be just one instead, say named as CreateBrick ? .

CreateBrick CreateBrickFS MountBrick CreateBrickDir

Do we need that much granularity ? Having less methods will make the provisioner interface more flexible in what it can choose to do internally when creating a brick

makes sense, I will merge the functions into one. MountBrick is also required as separate since bricks needs to be mounted on glusterd start/node reboot.

Register AvailableSize Unregister CreateBrick MountBrick UnmountBrick RemoveBrick

CreateBrick also mounts the brick. but exported MountBrick can be used separately when required.

Not completely related to this changes. Is it possible to delay the mounting until the brick is actually started? Since the bricks are provisioned by glusterd, we have complete ownership of it. The problem with keeping the mount as soon as the volume creation is that, some application like an antivirus scan or a find, etc could be accessing the mount point when we try to unmount.

yeah possible to move bricks mount to volume start. With this change, we should only mount the bricks if volume.State=="Started" during glusterd2 start/restart

prashanthpai · 2018-10-11T07:00:36Z

glusterd2/brick/types.go

@@ -33,6 +33,7 @@ type MountInfo struct {
 // Brickinfo is the static information about the brick
 type Brickinfo struct {
 	ID             uuid.UUID
+	Name           string


What is stored in this variable ?

<volname>_s<subvol-number>_b<brick-number> similar to subvolume name

prashanthpai · 2018-10-11T07:06:15Z

@rafikc30 Can you please review the LVM related changes ?
@rishubhjain Can you please review device related changes ?

rafikc30 · 2018-10-11T07:08:43Z

@prashanthpai I will review by eod

rafikc30 · 2018-10-11T13:33:43Z

glusterd2/provisioners/provisioners.go

+	// Unregister will be called when device/directory needs to be removed
+	Unregister(devpath string) error
+	// CreateBrick creates the brick volume
+	CreateBrick(devpath, brickid string, size uint64, bufferFactor float64) error


Not completely related to this changes. Is it possible to delay the mounting until the brick is actually started? Since the bricks are provisioned by glusterd, we have complete ownership of it. The problem with keeping the mount as soon as the volume creation is that, some application like an antivirus scan or a find, etc could be accessing the mount point when we try to unmount.

rafikc30 · 2018-10-12T04:40:04Z

pkg/lvmutils/utils.go

-// GetPoolMetadataSize calculates the thin pool metadata size based on the given thin pool size
-func GetPoolMetadataSize(poolsize uint64) uint64 {
+// GetTpMetadataSize calculates the thin pool metadata size based on the given thin pool size
+func GetTpMetadataSize(poolsize uint64) uint64 {


we should have a minimum value for metadata size, for example, if a user creates with a pool size of 100MB, then metadata will be 0.5MB. We had a recent issue in a user setup where metadata became full because of small size

I will look into that. But this is calculated based on given pool size.

Refer https://access.redhat.com/documentation/en-us/red_hat_gluster_storage/3.3/html-single/administration_guide/#Brick_Configuration

Minimum metadata size required is 0.5% and Max upto 16GB

please provide more details about the issue. This function is moved from plugins/deviceutils to pkg/lvmutils

rafikc30 · 2018-10-12T04:52:36Z

plugins/lvmprovisioner/provisioner.go

+	if err != nil {
+		return err
+	}
+	return fsutils.Mount(brickdev, mountdir, mountOpts)


I think mount can be variable. Different provisioners of the same type may need to mount with different mount options. For example, in the case of a snapshot, we have to mount using the same options that the parent bricks are used. In this case, the default value won't help

Snapshot mount should be different interface function in my opinion. Bricks of Cloned volume will use this function but not snapshot bricks.

Any particular reason why snapshot should have separate mount interface. Snapshot mount has nothing special comared to the normal mount. IMHO, we can take brickinfo.MountInfo as an argument, because it is a strcture contains xclusive information about mount. And that is common to any provisoner. Something like glusterd2/volume/fs_utils.go:MountDirectory

rafikc30 · 2018-10-12T04:56:26Z

plugins/lvmprovisioner/provisioner.go

+// Provisioner represents lvm provisioner plugin
+type Provisioner struct{}
+
+func getVgName(devpath string) string {


can we store the values like vgname, lvname, device name etc. As of now it return constant values, which is not the case with snapshot.

Or is it possible to move the Mountinfo variable in the brickinfo to provisioner, and return this values based on the values stored

I am trying to keep lvm specific things outside the device info. Non LVM use case will not have Vg. For example, loopmount devices provisioner

rafikc30 · 2018-10-12T04:59:10Z

plugins/lvmprovisioner/provisioner.go

+
+// UnmountBrick unmounts the brick
+func (p Provisioner) UnmountBrick(brickPath string) error {
+	mountdir := path.Dir(brickPath)


This hold true only for smartvol, to extend to the other provisioners, we need to take the mount root from brickpath

Some provisioners may not have a directory after mount or it may not be mount. what other possible things after mount root? multiple directory levels?

yea, we can have multiple levels. Totally depends on how users provisioned it. So we use this function volume.GetBrickMountRoot to find that

atinmu · 2018-10-25T11:46:09Z

@rafikc30 have you managed to find time to review this code?

rafikc30 · 2018-10-26T07:13:36Z

plugins/lvmprovisioner/provisioner.go

+	if err != nil {
+		return err
+	}
+	return fsutils.Mount(brickdev, mountdir, mountOpts)


Any particular reason why snapshot should have separate mount interface. Snapshot mount has nothing special comared to the normal mount. IMHO, we can take brickinfo.MountInfo as an argument, because it is a strcture contains xclusive information about mount. And that is common to any provisoner. Something like glusterd2/volume/fs_utils.go:MountDirectory

rafikc30 · 2018-10-26T07:16:19Z

plugins/lvmprovisioner/provisioner.go

+
+// UnmountBrick unmounts the brick
+func (p Provisioner) UnmountBrick(brickPath string) error {
+	mountdir := path.Dir(brickPath)


yea, we can have multiple levels. Totally depends on how users provisioned it. So we use this function volume.GetBrickMountRoot to find that

aravindavk · 2018-10-26T10:13:47Z

Removing GCS tracking label from pull request

rishubhjain · 2018-11-02T05:53:13Z

glusterd2/brick/types.go

@@ -33,6 +33,7 @@ type MountInfo struct {
 // Brickinfo is the static information about the brick
 type Brickinfo struct {
 	ID             uuid.UUID
+	Name           string
 	Hostname       string
 	PeerID         uuid.UUID
 	Path           string


Does this path represents the path where the brick is mounted?

rishubhjain · 2018-11-02T06:05:21Z

pkg/api/volume_req.go

+	Type       string `json:"type"`
+	PeerID     string `json:"peerid"`
+	Path       string `json:"path"`
+	Name       string `json:"name,omitempty"`


I guess we do not store the name of brick in the BrickInfo. Do we really require this?

rishubhjain · 2018-11-02T06:16:19Z

plugins/device/api/resp.go

-	ExtentSize    uint64 `json:"extent-size"`
-	Used          bool   `json:"used"`
-	PeerID        string `json:"peer-id"`
+	Device        string    `json:"device"`


@aravindavk I would like to know why Vgname was removed, it would be useful to know the VG of device via the device struct itself.

VgName is plugin specific, doesn't make sense if it is non LVM plugin

rishubhjain · 2018-11-02T06:40:06Z

plugins/device/rest.go

 }

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

 	ctx := r.Context()
 	logger := gdctx.GetReqLogger(ctx)
 	peerID := mux.Vars(r)["peerid"]
-	if uuid.Parse(peerID) == nil {
+	if peerID != "" && uuid.Parse(peerID) == nil {


If the peerID is empty then uuid.Parse() will return nil, so IMO we don't have to put an additional check.

aravindavk · 2018-12-17T09:08:12Z

Updates: #1418

ghost assigned aravindavk Sep 29, 2018

ghost added the in progress label Sep 29, 2018

aravindavk requested a review from amarts September 29, 2018 13:29

amarts reviewed Sep 29, 2018

View reviewed changes

aravindavk requested review from prashanthpai, Madhu-1 and kshlm October 8, 2018 15:08

Madhu-1 reviewed Oct 9, 2018

View reviewed changes

prashanthpai added the GCS/0.2 GCS 0.2 release label Oct 11, 2018

prashanthpai reviewed Oct 11, 2018

View reviewed changes

prashanthpai requested review from rishubhjain and rafikc30 October 11, 2018 07:04

rafikc30 suggested changes Oct 12, 2018

View reviewed changes

rafikc30 suggested changes Oct 26, 2018

View reviewed changes

aravindavk removed the GCS/0.2 GCS 0.2 release label Oct 26, 2018

rishubhjain reviewed Nov 2, 2018

View reviewed changes

Plugin architecture for brick Provisioners #1256

Are you sure you want to change the base?

Plugin architecture for brick Provisioners #1256

Conversation

aravindavk commented Sep 29, 2018 • edited Loading

amarts 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

aravindavk commented Oct 8, 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

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

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 Oct 11, 2018 • 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

prashanthpai commented Oct 11, 2018

rafikc30 commented Oct 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atinmu commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Oct 26, 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

aravindavk commented Dec 17, 2018

aravindavk commented Sep 29, 2018 •

edited

Loading

prashanthpai Oct 11, 2018 •

edited

Loading