Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi
......................................................................
Patch Set 4:
(7 comments)
https://gerrit.ovirt.org/#/c/44061/4//COMMIT_MSG
Commit Message:
Line 4: Commit: Ala Hino <ahino(a)redhat.com>
Line 5: CommitDate: 2015-07-28 15:06:41 +0300
Line 6:
Line 7: virt: enable libgfapi
Line 8:
Please explain why we want to support libgafpy and how do you keep backward compatibility
with older engines.
Line 9: This change is based on Federico's changes:
Line 10:
https://gerrit.ovirt.org/33768/
Line 11:
Line 12: Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d
https://gerrit.ovirt.org/#/c/44061/4/vdsm/clientIF.py
File vdsm/clientIF.py:
Line 316
Line 317
Line 318
Line 319
Line 320
drive should not have volumeInfo now, the info is storage now in several other attributes
(diskType, path, protocol...).
You should remove this assignment, and remove this attribute from the Drive class (see
__slots__).
Line 319: # The order of imgVolumesInfo is not guaranteed
Line 320: drive['volumeChain'] = res['imgVolumesInfo']
Line 321: drive['volumeInfo'] = res['info']
Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK:
Line 323: volinfo = drive.get('volumeInfo')
Get volinfo from res['info']
Line 324: volPath = volinfo['path']
Line 325: drive['protocol'] = volinfo['protocol']
Line 326: if drive.get('hosts') is None:
Line 327: hosts = [dict(name=volinfo['hosts'][0],
Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK:
Line 323: volinfo = drive.get('volumeInfo')
Line 324: volPath = volinfo['path']
Line 325: drive['protocol'] = volinfo['protocol']
Line 326: if drive.get('hosts') is None:
Why do you allow hosts on the drive? lets simplify and be consistent. Either we get all
values from the caller, or from prepareImage().
Line 327: hosts = [dict(name=volinfo['hosts'][0],
Line 328: port=volinfo['port'],
Line 329: transport=volinfo['transport'])]
Line 330: drive['hosts'] = hosts
Line 325: drive['protocol'] = volinfo['protocol']
Line 326: if drive.get('hosts') is None:
Line 327: hosts = [dict(name=volinfo['hosts'][0],
Line 328: port=volinfo['port'],
Line 329: transport=volinfo['transport'])]
Please prepare this in glusterVolume.vmVolumeInfo(), not here, so here we have only:
drive['hosts'] = volinfo['hosts']
We need exactly the same info when creating a snapshot, and we don't want to duplicate
the code to create the hosts list. Even if we had only one place using this, we want
glusterVolume to create this.
Line 330: drive['hosts'] = hosts
Line 331:
Line 332: # GUID drive format
Line 333: elif "GUID" in drive:
https://gerrit.ovirt.org/#/c/44061/4/vdsm/storage/glusterVolume.py
File vdsm/storage/glusterVolume.py:
Line 50
Line 51
Line 52
Line 53
Line 54
- volType is not needed.
- use one item per line, eaiser to read and creates nicer diffs later.
Line 56: return {'volType': VmVolumeInfo.TYPE_NETWORK, 'path':
glusterPath,
Line 57: 'protocol': 'gluster', 'port': volPort,
Line 58: 'transport': volTrans,
Line 59: 'volfileServer': volfileServer,
Line 60: 'hosts': hosts}
Please drop the old format and return exactly what is required in prepareVolumePath.
This is the place to translate gluster format (volTrans, volfileServer, volPort, ...) to
virt format (transport, hosts, protocol, path).
--
To view, visit
https://gerrit.ovirt.org/44061
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes