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