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

barumuga at redhat.com barumuga at redhat.com
Mon Feb 2 08:26:27 UTC 2015


Bala.FA has posted comments on this change.

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


Patch Set 11:

(12 comments)

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

Line 74:                                                int(tokens[5]),
Line 75:                                                int(tokens[6])))
Line 76:         return devList
Line 77: 
Line 78:     def _getDevUuidDict(self, device):
This should return fsUuid like

def _getFsUuid(self, device):
    ...
    ...
    return uuid
Line 79:         devUuidDict = {}
Line 80:         for fpath in glob.iglob("/dev/disk/by-uuid" + '/*'):
Line 81:             devUuidDict[os.path.realpath(fpath)] = fpath.split('/')[-1]
Line 82:         return devUuidDict


Line 76:         return devList
Line 77: 
Line 78:     def _getDevUuidDict(self, device):
Line 79:         devUuidDict = {}
Line 80:         for fpath in glob.iglob("/dev/disk/by-uuid" + '/*'):
you could use os.listdir() which simplifies things
Line 81:             devUuidDict[os.path.realpath(fpath)] = fpath.split('/')[-1]
Line 82:         return devUuidDict
Line 83: 
Line 84:     def _exists(self, device):


Line 91:             if dev.device.startswith("UUID="):
Line 92:                 uuid = dev.device.split("UUID=")[-1]
Line 93:                 if device in devUuidDict and uuid == devUuidDict[device]:
Line 94:                     return True
Line 95:         return False
The whole thing is simple if

uuid = "UUID=%s" % (self._getFsUuid(device))
for dev in self._list():
    if device == dev.device or uuid = dev.device:
         return True
return False
Line 96: 
Line 97:     def add(self, device, mountPoint, fsType,
Line 98:             mntOpts='defaults', fsDump=0, fsPass=0):
Line 99:         if self._exists(device):


Line 99:         if self._exists(device):
Line 100:             raise ge.GlusterHostStorageDeviceFsTabFoundException(device)
Line 101:         devUuidDict = self._getDevUuidDict(device)
Line 102:         if device not in devUuidDict:
Line 103:             raise ge.GlusterHostStorageDeviceUuidNotFoundException(device)
I think this exception is not needed.  If fsUuid is not found, you log it and continue by using device itself
Line 104:         content = open(self.fileName).read()
Line 105:         content += "UUID=%s\t%s\t%s\t%s\t%s\t%s\n" % (devUuidDict[device],
Line 106:                                                       mountPoint, fsType,
Line 107:                                                       ",".join(mntOpts),


Line 119:                 'bus': device.bus or '',
Line 120:                 'model': '',
Line 121:                 'fsType': '',
Line 122:                 'mountPoint': '',
Line 123:                 'uuid': device.format.uuid or '',
Why are you missing this fix here?  It should be a separate patch
Line 124:                 'createBrick': True}
Line 125:         if not info['bus'] and device.parents:
Line 126:             info['bus'] = device.parents[0].bus
Line 127:         if device.model:


Line 140: 
Line 141: def _canCreateBrick(device):
Line 142:     if not device or device.kids > 0 or device.format.type or \
Line 143:        hasattr(device.format, 'mountpoint') or \
Line 144:        device.type in ['cdrom', 'lvmvg', 'lvmthinpool', 'lvmlv', 'lvmthinlv']:
Have the above and this as one separate fix
Line 145:         return False
Line 146:     return True
Line 147: 
Line 148: 


Line 188:                                               '%sK' % alignment,
Line 189:                                               dev.path])
Line 190:                 if rc:
Line 191:                     raise ge.GlusterHostStorageDevicePVCreateFailedException(
Line 192:                         dev.path, "%sK" % alignment)
why can't we have alignment as is than string?
Line 193: 
Line 194:             blivetEnv.reset()
Line 195:             return _getDeviceList([dev.name for dev in deviceList])
Line 196: 


Line 260:                                       devPath])
Line 261:         if rc:
Line 262:             cmd = "%s -t xfs -f -K -i size=512 -d -sw=%sk -su=%sk -n " \
Line 263:                   "size=8192 %d" % (_mkfsCommandPath.cmd, alignment,
Line 264:                                     stripeSize, devPath)
This pattern is already logged by execCmd() itself.
Line 265:             raise ge.GlusterHostStorageDeviceMkfsFailedException(cmd)
Line 266: 
Line 267:     vgName = "vg-" + brickName
Line 268:     poolName = "pool-" + brickName


Line 261:         if rc:
Line 262:             cmd = "%s -t xfs -f -K -i size=512 -d -sw=%sk -su=%sk -n " \
Line 263:                   "size=8192 %d" % (_mkfsCommandPath.cmd, alignment,
Line 264:                                     stripeSize, devPath)
Line 265:             raise ge.GlusterHostStorageDeviceMkfsFailedException(cmd)
You would need to pass respective values to this exception and construct message there
Line 266: 
Line 267:     vgName = "vg-" + brickName
Line 268:     poolName = "pool-" + brickName
Line 269:     alignment = 0


Line 286:         set([dev.name for dev in deviceList]))
Line 287:     if notFoundList:
Line 288:         raise ge.GlusterHostStorageDeviceNotFoundException(notFoundList)
Line 289: 
Line 290:     unavailList = set(devNameList).difference(set([not _canCreateBrick(
How about inUseList?
Line 291:         dev) or dev.name for dev in deviceList]))
Line 292:     if unavailList:
Line 293:         raise ge.GlusterHostStorageDeviceInUseException(unavailList)
Line 294: 


Line 320:         m.mount(vfstype="xfs", mntOpts="inode64,noatime")
Line 321:         if not m.isMounted():
Line 322:             ge.GlusterHostStorageDeviceMountFailedException()
Line 323:     else:
Line 324:         newFmt = blivet.formats.getFormat("xfs", device=thinlv.path)
newFmt can be as format
Line 325:         blivetEnv.formatDevice(thinlv, newFmt)
Line 326:         blivetEnv.doIt()
Line 327: 
Line 328:         thinlv.format.setup(mountpoint=mountPoint)


Line 326:         blivetEnv.doIt()
Line 327: 
Line 328:         thinlv.format.setup(mountpoint=mountPoint)
Line 329:         blivetEnv.doIt()
Line 330:     FsTab("/etc/fstab").add(thinlv.path, mountPoint, "xfs")
Define DEFAULT_FS_TYPE = "xfs" and use it everywhere.  This also applicable to mountOptions.
Line 331: 
Line 332:     return {'name': thinlv.name,
Line 333:             'size': '%s' % thinlv.size,
Line 334:             'devPath': thinlv.path,


-- 
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: 11
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