Bala.FA has posted comments on this change. Change subject: gluster: add createBrick verb ......................................................................
Patch Set 12: (7 comments) http://gerrit.ovirt.org/#/c/35498/12/vdsm/gluster/storagedev.py File vdsm/gluster/storagedev.py: Line 20: Line 21: import blivet Line 22: import blivet.formats Line 23: Line 24: from . import makePublic Move this import below in local imports Line 25: from blivet.devices import LVMVolumeGroupDevice Line 26: from blivet.devices import LVMThinPoolDevice Line 27: from blivet.devices import LVMLogicalVolumeDevice Line 28: from blivet.devices import LVMThinLogicalVolumeDevice Line 26: from blivet.devices import LVMThinPoolDevice Line 27: from blivet.devices import LVMLogicalVolumeDevice Line 28: from blivet.devices import LVMThinLogicalVolumeDevice Line 29: Line 30: import fstab fstab module is missing in this patch Line 31: import exception as ge Line 32: from vdsm import utils Line 33: import storage.mount as mount Line 34: Line 29: Line 30: import fstab Line 31: import exception as ge Line 32: from vdsm import utils Line 33: import storage.mount as mount Have vdsm/storage imports above and after blivet Line 34: Line 35: Line 36: _pvcreateCommandPath = utils.CommandPath("pvcreate", Line 37: "/sbin/pvcreate", Line 102: return _parseDevices(blivetEnv.devices) Line 103: Line 104: Line 105: @makePublic Line 106: def createBrick(brickName, mountPoint, devNameList, raidParams={}): You should have fsType=DEFAULT_FS_TYPE param here Line 107: def _getDeviceList(devNameList): Line 108: return [blivetEnv.devicetree.getDeviceByName(devName.split("/")[-1]) Line 109: for devName in devNameList] Line 110: Line 205: '-K', '-i', 'size=512', '-d', Line 206: '-sw=%sk' % alignment, Line 207: '-su=%sk' % stripeSize, Line 208: '-n', 'size=8192', Line 209: devPath]) 1. I think this function needs to be generic enough to work with any filesystem and options. Time being have it as _formatDeviceXfs() and have a wrapper function like _formatDevice() which calls _formatDeviceXfs() if 'xfs' is passed else log and fail 2. You should consume DEFAULT_FS_TYPE 3. I see alignment and stripesize are optional. You should do util.execCmd() accordingly. Line 210: if rc: Line 211: raise ge.GlusterHostStorageDeviceMkfsFailedException(devPath, Line 212: alignment, Line 213: stripeSize) Line 209: devPath]) Line 210: if rc: Line 211: raise ge.GlusterHostStorageDeviceMkfsFailedException(devPath, Line 212: alignment, Line 213: stripeSize) You should pass DEFAULT_FS_TYPE too Line 214: Line 215: vgName = "vg-" + brickName Line 216: poolName = "pool-" + brickName Line 217: alignment = 0 Line 285: 'model': thinlv.model, Line 286: 'fsType': DEFAULT_FS_TYPE, Line 287: 'mountPoint': mountPoint, Line 288: 'uuid': thinlv.format.uuid or '', Line 289: 'createBrick': False} I think it should be better to have this in a separate function separately which would be common for devicelist and createbrick -- 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: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Timothy Asir <tjeya...@redhat.com> Gerrit-Reviewer: Bala.FA <barum...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Darshan N <dnara...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
