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

tjeyasin at redhat.com tjeyasin at redhat.com
Fri Feb 13 08:54:54 UTC 2015


Timothy Asir has posted comments on this change.

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


Patch Set 17:

(14 comments)

http://gerrit.ovirt.org/#/c/35498/17/vdsm/gluster/Makefile.am
File vdsm/gluster/Makefile.am:

Line 31: 	api.py \
Line 32: 	apiwrapper.py \
Line 33: 	cli.py \
Line 34: 	exception.py \
Line 35: 	fstab.py \
> You have to add fstab.py to vdsm.spec.in
Added into vdsm.spec.in
Line 36: 	gfapi.py \
Line 37: 	hooks.py \
Line 38: 	services.py \
Line 39: 	storagedev.py \


http://gerrit.ovirt.org/#/c/35498/17/vdsm/gluster/api.py
File vdsm/gluster/api.py:

Line 391:                                                     mountPoint,
Line 392:                                                     devList,
Line 393:                                                     fsType,
Line 394:                                                     raidParams)
Line 395:         return {'deviceInfo': status}
> how about 'device' instead of 'deviceInfo'?
Done
Line 396: 
Line 397: 
Line 398: def getGlusterMethods(gluster):
Line 399:     l = []


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

Line 416: class GlusterHostStorageDeviceMountFailedException(GlusterHostException):
Line 417:     code = 4411
Line 418: 
Line 419:     def __init__(self, device):
Line 420:         self.message = "Device %s mount failed" % device
> You should have message like
Done
Line 421: 
Line 422: 
Line 423: class GlusterHostStorageDeviceMkfsFailedException(GlusterHostException):
Line 424:     code = 4412


Line 428:         self.out = out
Line 429:         self.err = err
Line 430:         self.message = "Device format failed for device %s with params " + \
Line 431:                        "alignment: %s, stripe size: %s" % (device, alignment,
Line 432:                                                            stripeSize)
> You can have clear message like
Done
Line 433: 
Line 434: 
Line 435: class GlusterHostStorageDeviceFsTabFoundException(GlusterHostException):
Line 436:     code = 4413


Line 435: class GlusterHostStorageDeviceFsTabFoundException(GlusterHostException):
Line 436:     code = 4413
Line 437: 
Line 438:     def __init__(self, device):
Line 439:         self.message = "Device entry %s already found in fstab" % device
> "fstab entry for device %s already exists"
Done
Line 440: 
Line 441: 
Line 442: class GlusterHostStorageDevicePVCreateFailedException(GlusterHostException):
Line 443:     code = 4414


Line 446:         self.rc = rc
Line 447:         self.out = out
Line 448:         self.err = err
Line 449:         self.message = "PV Create failed for the device %s " + \
Line 450:                        "with dataalignment %s" % (device, alignment)
> "Failed to create LVM PV for device %s with data alignment %s"
Done
Line 451: 
Line 452: 
Line 453: class GlusterHostStorageDeviceUuidNotFoundException(GlusterHostException):
Line 454:     code = 4415


Line 449:         self.message = "PV Create failed for the device %s " + \
Line 450:                        "with dataalignment %s" % (device, alignment)
Line 451: 
Line 452: 
Line 453: class GlusterHostStorageDeviceUuidNotFoundException(GlusterHostException):
> This exception is not consumed anywhere.  Please remove it
Done
Line 454:     code = 4415
Line 455: 
Line 456:     def __init__(self, device):
Line 457:         self.message = "Device %s UUID not found" % device


Line 456:     def __init__(self, device):
Line 457:         self.message = "Device %s UUID not found" % device
Line 458: 
Line 459: 
Line 460: class GlusterHostStorageDeviceLvConvertFailedException(GlusterHostException):
> rename to GlusterHostStorageDeviceLVConvertFailedException
Done
Line 461:     code = 4416
Line 462: 
Line 463:     def __init__(self, devicePath, alignment, rc=0, out=(), err=()):
Line 464:         self.rc = rc


Line 464:         self.rc = rc
Line 465:         self.out = out
Line 466:         self.err = err
Line 467:         self.message = "LV Convert failed for the device: " + \
Line 468:                        "%s with alignment: %s" % (devicePath, alignment)
> 1. "Failed to run lvconvert for device %s with data alignment %s"
Done
Line 469: 
Line 470: 
Line 471: # Hook
Line 472: class GlusterHookException(GlusterException):


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

Line 78:     else:
Line 79:         info['model'] = device.type
Line 80:     if device.format:
Line 81:         info['uuid'] = device.format.uuid or ''
Line 82:         info['fsType'] = device.format.type or fsType or ''
> 1. I think 'device.format.type or fsType' itself is ok.  Please check it ou
I have remove fsType already as per the previous comment in the new separate patch

Yes i have already sent a separate patch and made that as a dependency to this patch.
Line 83:     if hasattr(device.format, 'mountpoint'):
Line 84:         info['mountPoint'] = device.format.mountpoint or ''
Line 85:     return info
Line 86: 


Line 199: 
Line 200:             rc, out, err = utils.execCmd([_lvchangeCommandPath.cmd,
Line 201:                                           '--zero', 'n', vgPoolName])
Line 202:             if rc:
Line 203:                 raise ge.GlusterHostStorageDeviceLvChangeFailedException(
> 1. This exception is missing in exception.py
Yes, I have added the exception and passed vgPoolName to it
Line 204:                     rc, out, err)
Line 205: 
Line 206:             blivetEnv.reset()
Line 207:             return blivetEnv.devicetree.getDeviceByName(poolLv.name)


Line 230: 
Line 231:         if fsType == DEFAULT_FS_TYPE:
Line 232:             return _formatDeviceXfs()
Line 233:         else:
Line 234:             log.error("%s is currently unsupported" % fsType)
> How about 'fstype %s is currently unsupported' ?
Done
Line 235:             raise ge.GlusterHostStorageDeviceMkfsFailedException(
Line 236:                 devPath, alignment, stripeSize, fsType)
Line 237: 
Line 238:     vgName = "vg-" + brickName


Line 290:                       raidParams.get('stripeSize'), fsType)
Line 291:         m = mount.Mount(thinlv.path, mountPoint)
Line 292:         m.mount(vfstype=DEFAULT_FS_TYPE, mntOpts=DEFAULT_MOUNT_OPTIONS)
Line 293:         if not m.isMounted():
Line 294:             ge.GlusterHostStorageDeviceMountFailedException()
> You should pass device, mount-point, fstype, mount options to this exceptio
Done
Line 295:     else:
Line 296:         format = blivet.formats.getFormat(DEFAULT_FS_TYPE, device=thinlv.path)
Line 297:         blivetEnv.formatDevice(thinlv, format)
Line 298:         blivetEnv.doIt()


Line 298:         blivetEnv.doIt()
Line 299: 
Line 300:         thinlv.format.setup(mountpoint=mountPoint)
Line 301:         blivetEnv.doIt()
Line 302:     fstab.FsTab("/etc/fstab").add(thinlv.path, mountPoint, DEFAULT_FS_TYPE)
> Why pass '/etc/fstab' as its default in FsTab class
Done
Line 303: 


-- 
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: 17
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: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbonazzo at redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtripat at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin 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