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

barumuga at redhat.com barumuga at redhat.com
Mon Feb 2 02:42:17 UTC 2015


Bala.FA has posted comments on this change.

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


Patch Set 10: Code-Review-1

(11 comments)

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

Line 36: 
Line 37: 
Line 38: _pvcreateCommandPath = utils.CommandPath("pvcreate",
Line 39:                                          "/sbin",
Line 40:                                          "/usr/sbin/pvcreate",)
if '/sbin' is allowed, why not '/usr/sbin' is not used?
Line 41: _lvconvertCommandPath = utils.CommandPath("lvconvert",
Line 42:                                           "/sbin",
Line 43:                                           "/usr/sbin/lvconvert",)
Line 44: _lvchangeCommandPath = utils.CommandPath("lvchange",


Line 58: MIN_METADATA_PERCENT = 0.005
Line 59: 
Line 60: 
Line 61: FstabRecord = namedtuple("FstabRecord", "device mountPoint fsType "
Line 62:                          "mntOpts fsDump, fsPass")
If ',' is allowed, why not have ',' separated string than space separated?
Line 63: 
Line 64: 
Line 65: class FsTab(object):
Line 66:     def __init__(self, fileName="/etc/fstab"):


Line 61: FstabRecord = namedtuple("FstabRecord", "device mountPoint fsType "
Line 62:                          "mntOpts fsDump, fsPass")
Line 63: 
Line 64: 
Line 65: class FsTab(object):
How about having fstab as separate module file?
Line 66:     def __init__(self, fileName="/etc/fstab"):
Line 67:         self.fileName = fileName
Line 68:         self.devList = []
Line 69: 


Line 64: 
Line 65: class FsTab(object):
Line 66:     def __init__(self, fileName="/etc/fstab"):
Line 67:         self.fileName = fileName
Line 68:         self.devList = []
ATM, devlist is kept constructed every time, why we need it as class variable?
Line 69: 
Line 70:     def _list(self):
Line 71:         self.devList = []
Line 72:         with open(self.fileName, "r") as f:


Line 70:     def _list(self):
Line 71:         self.devList = []
Line 72:         with open(self.fileName, "r") as f:
Line 73:             for line in f:
Line 74:                 if not (line.startswith("#") or line.strip() == ''):
If '#' starts with whitespace, this parsing breaks.  You need to take care of it too.
Line 75:                     (device, mountPoint, fsType, mntOpts,
Line 76:                      fsDump, fsPass) = line.split()[:6]
Line 77:                     mntOpts = mntOpts.split(",")
Line 78:                     fsDump = int(fsDump)


Line 78:                     fsDump = int(fsDump)
Line 79:                     fsPass = int(fsPass)
Line 80:                     self.devList.append(FstabRecord(device, mountPoint,
Line 81:                                                     fsType, mntOpts,
Line 82:                                                     fsDump, fsPass))
why not like

tokens = line.split()

devList.append(FstabRecord(tokens[0], ...)

?
Line 83:         return self.devList
Line 84: 
Line 85:     def _exists(self, device, mountPoint):
Line 86:         for dev in self._list():


Line 81:                                                     fsType, mntOpts,
Line 82:                                                     fsDump, fsPass))
Line 83:         return self.devList
Line 84: 
Line 85:     def _exists(self, device, mountPoint):
check only device, not mountpoint?
Line 86:         for dev in self._list():
Line 87:             # device already exists
Line 88:             if device == dev.device:
Line 89:                 return True


Line 89:                 return True
Line 90:             # device uuid already exists
Line 91:         if dev.device.startswith("UUID="):
Line 92:             if device == os.path.realpath(
Line 93:                     "/dev/disk/by-uuid/%s" % dev.device.split("UUID=")[-1]):
have uuid thing above for loop.
Line 94:                 return True
Line 95:             # mount already exists
Line 96:         if mountPoint == dev.mountPoint:
Line 97:             return True


Line 99: 
Line 100:     def add(self, device, mountPoint, fsType,
Line 101:             mntOpts='defaults', fsDump=0, fsPass=0):
Line 102:         if self._exists(device, mountPoint):
Line 103:             raise ge.GlusterHostStorageDeviceFsTabEntryAlreadyFound(device)
Have exception name as GlusterHostStorageDeviceFsTabFoundException(device)
Line 104:         content = open(self.fileName).read()
Line 105:         content += "UUID=%s\t%s\t%s\t%s\t%s\t%s\n" % (_getUuidByDevice(device),
Line 106:                                                       mountPoint, fsType,
Line 107:                                                       ",".join(mntOpts),


Line 101:             mntOpts='defaults', fsDump=0, fsPass=0):
Line 102:         if self._exists(device, mountPoint):
Line 103:             raise ge.GlusterHostStorageDeviceFsTabEntryAlreadyFound(device)
Line 104:         content = open(self.fileName).read()
Line 105:         content += "UUID=%s\t%s\t%s\t%s\t%s\t%s\n" % (_getUuidByDevice(device),
If no blivet, this function fails.  Have a generic private function to this class by using /dev/disk/by-uuid/* files/symlinks
Line 106:                                                       mountPoint, fsType,
Line 107:                                                       ",".join(mntOpts),
Line 108:                                                       fsDump, fsPass)
Line 109:         safeWrite(self.fileName, content)


Line 103:             raise ge.GlusterHostStorageDeviceFsTabEntryAlreadyFound(device)
Line 104:         content = open(self.fileName).read()
Line 105:         content += "UUID=%s\t%s\t%s\t%s\t%s\t%s\n" % (_getUuidByDevice(device),
Line 106:                                                       mountPoint, fsType,
Line 107:                                                       ",".join(mntOpts),
I think its good to have mountoptions as dict like mount options 

rw,seclabel,relatime,data=ordered 

needs to be as dict by
{'rw': None, 'seclablel': None, 'relatime': None, 'data': 'ordered'}

If you like mountoptions as dict, then you can change list function too
Line 108:                                                       fsDump, fsPass)
Line 109:         safeWrite(self.fileName, content)
Line 110: 
Line 111: 


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