Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi
......................................................................
Patch Set 2:
(4 comments)
https://gerrit.ovirt.org/#/c/44061/2//COMMIT_MSG
Commit Message:
Line 6:
Line 7: virt: enable libgfapi
Line 8:
Line 9: This change is based on Federico's changes:
Line 10:
https://gerrit.ovirt.org/33768/
What about the snapshot support in federico patch? Are you planning this to the next
patch?
Line 11:
Line 12: Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d
Line 13: Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com>
https://gerrit.ovirt.org/#/c/44061/2/vdsm/storage/glusterVolume.py
File vdsm/storage/glusterVolume.py:
Line 54: return {'volType': VmVolumeInfo.TYPE_NETWORK, 'path':
glusterPath,
Line 55: 'protocol': 'gluster', 'volPort':
volPort,
Line 56: 'volTransport': volTrans,
Line 57: 'volfileServer': volfileServer,
Line 58: 'bricks': volInfo[volname]['bricks']}
Please change this to return the format we expect on the other side. See the comment in
vm.py.
https://gerrit.ovirt.org/#/c/44061/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 804: if self._destroyed:
Line 805: # A destroy request has been issued, exit early
Line 806: break
Line 807: drive['path'] = self.cif.prepareVolumePath(drive,
self.id)
Line 808: if drive.get('diskType') == DISK_TYPE.NETWORK:
This does not belong here, I think you should handle it in cif.prepareVolumePath, where
other types of drives are manipulated.
This code is only about setting drive["path"], other drive modifications are
handled in prepareVolumePath.
As a general rule, minimize change to vm.py - this module is much too big and hard to
test. We should not add here any storage code.
Line 809: volinfo = drive.get('volumeInfo')
Line 810: drive['path'] = volinfo['path']
Line 811: drive['protocol'] = volinfo['protocol']
Line 812: if drive.get('hosts') is None:
Line 817: """
Line 818: hosts = [dict(name=brick.split(":")[0],
Line 819: port=volinfo['volPort'],
Line 820:
transport=volinfo['volTransport'])
Line 821: for brick in volinfo['bricks']]
volumeInfo should return the output we expect, not gluster specific information (such as
bricks, volPort and volTransport).
We have generic api - vmVolumeInfo(), that happen to be implemented only by gluster. It
should return generic network disk parameters.
Since libvirt supports only one host for gluster volumes you check check the protocol
("gluster") and use only one host from the hosts list in this case. Add a
comments explaining this.
Don't add commented code, we don't do that in vdsm. Non working code should be
deleted.
Don't add comments about BUG before you talk with libvirt guys about that.
If you have a real bug to comment about, don't use UPPERCASE, instead explain the
nature of the bug and add a bug url.
Line 822: """
Line 823: hosts =
[dict(name=volinfo['bricks'][0].split(":")[0],
Line 824: port=volinfo['volPort'],
Line 825: transport=volinfo['volTransport'])]
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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