Change in vdsm[master]: gluster: add createBrick verb

barumuga at redhat.com barumuga at redhat.com
Tue Jan 27 09:10:40 UTC 2015


Bala.FA has posted comments on this change.

Change subject: gluster: add createBrick verb
......................................................................


Patch Set 8: Code-Review-1

(12 comments)

http://gerrit.ovirt.org/#/c/35498/8/vdsm.spec.in
File vdsm.spec.in:

Line 121: BuildRequires: python-ordereddict
Line 122: %endif
Line 123: %if 0%{?rhel} > 6 || 0%{?fedora}
Line 124: BuildRequires: python-blivet
Line 125: BuildRequires: xfsprogs
xfsprogs is available in rhel 6 too.  This dependency needs to be placed in gluster sub-package.
Line 126: %endif
Line 127: 
Line 128: # Autotools BuildRequires
Line 129: %if 0%{?enable_autotools}


Line 701: Requires: glusterfs-server
Line 702: Requires: python-magic
Line 703: %if 0%{?rhel} > 6 || 0%{?fedora}
Line 704: Requires: python-blivet
Line 705: Requires: xfsprogs
same as above
Line 706: %endif
Line 707: 
Line 708: %description gluster
Line 709: Gluster plugin enables VDSM to serve Gluster functionalities.


http://gerrit.ovirt.org/#/c/35498/8/vdsm/gluster/exception.py
File vdsm/gluster/exception.py:

Line 403:     code = 4409
Line 404: 
Line 405:     def __init__(self, deviceList):
Line 406:         self.message = "Device(s) %s not found" % [
Line 407:             name for name in deviceList]
Why is device list read and converted to list again?
Line 408: 
Line 409: 
Line 410: class GlusterHostStorageDeviceInUseException(GlusterHostException):
Line 411:     code = 4410


Line 411:     code = 4410
Line 412: 
Line 413:     def __init__(self, deviceList):
Line 414:         self.message = "Device(s) %s already in use" % [
Line 415:             name for name in deviceList]
same as above
Line 416: 
Line 417: 
Line 418: class GlusterHostStorageDeviceMountFailedException(GlusterHostException):
Line 419:     code = 4411


Line 416: 
Line 417: 
Line 418: class GlusterHostStorageDeviceMountFailedException(GlusterHostException):
Line 419:     code = 4411
Line 420:     message = "Device mount failed"
Why is deviceName taken out?
Line 421: 
Line 422: 
Line 423: class GlusterHostStorageDeviceMkfsFailedException(GlusterHostException):
Line 424:     code = 4412


Line 421: 
Line 422: 
Line 423: class GlusterHostStorageDeviceMkfsFailedException(GlusterHostException):
Line 424:     code = 4412
Line 425:     message = "Device format failed"
1. Why is deviceName taken out?

2. Why aren't file system type/options added in the exception?
Line 426: 
Line 427: 
Line 428: class GlusterHostStorageDeviceAlreadyMounted(GlusterHostException):
Line 429:     code = 4413


http://gerrit.ovirt.org/#/c/35498/8/vdsm/gluster/storagedev.py
File vdsm/gluster/storagedev.py:

Line 44: _mountCommandPath = utils.CommandPath("mount",
Line 45:                                       "/sbin",
Line 46:                                       "/usr/sbin/mount",)
Line 47: _mkfsCommandPath = utils.CommandPath("mkfs",
Line 48:                                      "/sbin",
Have you tested this?
Line 49:                                      "/usr/sbin/mkfs",)
Line 50: 
Line 51: # All size are in MiB unless otherwise specified
Line 52: DEFAULT_CHUNK_SIZE = 0.25


Line 106:                 for devName in devNameList]
Line 107: 
Line 108:     def _makePartition(deviceList):
Line 109:         pvDeviceList = []
Line 110:         requirePart = False
How about doPartitioning than requirePart?
Line 111:         for dev in deviceList:
Line 112:             if dev.type in ['disk', 'dm-multipath']:
Line 113:                 blivetEnv.initializeDisk(dev)
Line 114:                 part = blivetEnv.newPartition(fmt_type="lvmpv", grow=True,


Line 128:             for dev in deviceList:
Line 129:                 rc, out, err = utils.execCmd([_pvcreateCommandPath.cmd,
Line 130:                                               '--dataalignment',
Line 131:                                               '%sK' % alignment,
Line 132:                                               dev.path])
You haven't done

1. Please mention this workaround along with blivet bz# and why you are doing.

2. Handle if the command fails.
Line 133:             blivetEnv.reset()
Line 134:             return _getDeviceList([dev.name for dev in deviceList])
Line 135: 
Line 136:         if alignment:


Line 203:     def _updateFstab(device, path):
Line 204:         if mount.isMounted(path):
Line 205:             raise ge.GlusterHostStorageDeviceAlreadyMounted(device)
Line 206:         with open("/etc/fstab", "a") as fstab:
Line 207:             fstab.write("%s  %s xfs rw,inode64,noatime,nouuid  1 2\n" % (
below is not yet addressed

Write proper fstab management. I believe vdsm has this capabilities. Please check and write fstab management accordingly.
Line 208:                 device,  path))
Line 209: 
Line 210:     vgName = "vg-" + brickName
Line 211:     poolName = "pool-" + brickName


Line 217:         alignment = raidParams['stripeSize'] * (raidParams['pdCount'] - 2)
Line 218:         chunkSize = alignment
Line 219:     elif raidParams.get('type') == '10':
Line 220:         alignment = raidParams['stripeSize'] * (raidParams['pdCount'] / 2)
Line 221:         chunkSize = DEFAULT_CHUNK_SIZE
Below is not yet addressed

Add pointer as comment to admin guide how/why you are doing this calculation.
Line 222: 
Line 223:     blivetEnv = blivet.Blivet()
Line 224:     blivetEnv.reset()
Line 225: 


Line 254:     blivetEnv.doIt()
Line 255: 
Line 256:     if alignment:
Line 257:         _formatDevice(thinlv.path, alignment, raidParams.get('stripeSize'))
Line 258:         mpath = "/bricks/%s" % brickName
Below is not yet addressed

The mount prefix should be configurable. For eg, in RHS its '/rhs' and in upstream its /bricks
Line 259:         m = mount.Mount(thinlv.path, mpath)
Line 260:         m.mount(vfstype="xfs", mntOpts="inode64,noatime")
Line 261:         if not m.isMounted():
Line 262:             ge.GlusterHostStorageDeviceMountFailedException()


-- 
To view, visit http://gerrit.ovirt.org/35498
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic47c4c56834deb457ae9d038f77bcf69c7b39ba5
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtripat at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list