Ala Hino has uploaded a new change for review.
Change subject: virt: enable libgfapi ......................................................................
virt: enable libgfapi
This change is based on Federico's changes: https://gerrit.ovirt.org/33768/
Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d Signed-off-by: Federico Simoncelli fsimonce@redhat.com Signed-off-by: Ala Hino ahino@redhat.com --- M vdsm/storage/glusterVolume.py M vdsm/storage/hsm.py M vdsm/virt/vm.py 3 files changed, 22 insertions(+), 5 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/61/44061/1
diff --git a/vdsm/storage/glusterVolume.py b/vdsm/storage/glusterVolume.py index 8f701ba..109d1c9 100644 --- a/vdsm/storage/glusterVolume.py +++ b/vdsm/storage/glusterVolume.py @@ -54,4 +54,5 @@ return {'volType': VmVolumeInfo.TYPE_NETWORK, 'path': glusterPath, 'protocol': 'gluster', 'volPort': volPort, 'volTransport': volTrans, - 'volfileServer': volfileServer} + 'volfileServer': volfileServer, + 'bricks': volInfo[volname]['bricks']} diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py index f68d3bb..109cdfa 100644 --- a/vdsm/storage/hsm.py +++ b/vdsm/storage/hsm.py @@ -3209,8 +3209,7 @@ path = os.path.join(dom.domaindir, sd.DOMAIN_IMAGES, imgUUID, volUUID) volInfo = {'domainID': sdUUID, 'imageID': imgUUID, - 'volumeID': volUUID, 'path': path, - 'volType': "path"} + 'volumeID': volUUID, 'path': path}
leasePath, leaseOffset = dom.getVolumeLease(imgUUID, volUUID)
@@ -3221,8 +3220,8 @@ })
imgVolumesInfo.append(volInfo) - if volUUID == leafUUID: - leafInfo = volInfo + + leafInfo = dom.produceVolume(imgUUID, leafUUID).getVmVolumeInfo()
return {'path': leafPath, 'info': leafInfo, 'imgVolumesInfo': imgVolumesInfo} diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 71a74b3..2976f3b 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -805,6 +805,23 @@ # A destroy request has been issued, exit early break drive['path'] = self.cif.prepareVolumePath(drive, self.id) + if drive.get('diskType') == DISK_TYPE.NETWORK: + volinfo = drive.get('volumeInfo') + drive['path'] = volinfo['path'] + drive['protocol'] = volinfo['protocol'] + if drive.get('hosts') is None: + """ + THIS CODE COMMENTED OUT DUE TO A BUG IN LIBVIRT, + CANNOT WORK WITH MULTIPLE HOSTS + """ + """ + hosts = [dict(name=brick.split(":")[0], + port='0', transport='tcp') + for brick in volinfo['bricks']] + """ + hosts = [dict(name=volinfo['bricks'][0].split(":")[0], + port='0', transport='tcp')] + drive['hosts'] = hosts
else: # Now we got all the resources we needed
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
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@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'])]
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ala Hino 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
Sure, just wanted to kick off the code review Line 11: Line 12: Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d Line 13: Signed-off-by: Federico Simoncelli fsimonce@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 th
Done
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.prepareVolum
Done 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 informa
Done Line 822: """ Line 823: hosts = [dict(name=volinfo['bricks'][0].split(":")[0], Line 824: port=volinfo['volPort'], Line 825: transport=volinfo['volTransport'])]
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
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@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).
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/4/vdsm/clientIF.py File vdsm/clientIF.py:
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 h
Correction, we should take here only the first host, since libvirt does not support mutiple hosts, so the code should be:
drive['hosts'] = [res['hosts'][0]]
And we should have a comment why we do this, and a link to the libvirt bug. Line 330: drive['hosts'] = hosts Line 331: Line 332: # GUID drive format Line 333: elif "GUID" in drive:
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ala Hino has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 4:
(4 comments)
https://gerrit.ovirt.org/#/c/44061/4/vdsm/clientIF.py File vdsm/clientIF.py:
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']
Done 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. Eithe
In ceph we get hosts from caller (engine) while in glster, caller doesn't provide hosts 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'])]
Correction, we should take here only the first host, since libvirt does no
Done 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 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 prepareVo
Done
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/4/vdsm/clientIF.py File vdsm/clientIF.py:
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:
In ceph we get hosts from caller (engine) while in glster, caller doesn't p
But ceph disk do not reach this branch, since they are not in PDIV format.
Ceph disks are handled in the "elif "path" in drive branch (line 366). Line 327: hosts = [dict(name=volinfo['hosts'][0], Line 328: port=volinfo['port'], Line 329: transport=volinfo['transport'])] Line 330: drive['hosts'] = hosts
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 5:
(3 comments)
https://gerrit.ovirt.org/#/c/44061/5/vdsm/clientIF.py File vdsm/clientIF.py:
Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 323: volinfo = res ['info'] Line 324: volPath = volinfo['path'] Line 325: drive['protocol'] = volinfo['protocol'] Line 326: if drive.get('hosts') is None: No need for this check, we are not expecting hosts from engine. Line 327: """ Line 328: currently, single host is provided due to this bug: Line 329: https://bugzilla.redhat.com/1247521 Line 330: """
Line 326: if drive.get('hosts') is None: Line 327: """ Line 328: currently, single host is provided due to this bug: Line 329: https://bugzilla.redhat.com/1247521 Line 330: """ This is not a comment, this is a docstring, which belongs only to modules, classes or functions. Comment should use # prefix. Line 331: drive['hosts'] = [volinfo['hosts'][0]] Line 332: Line 333: # GUID drive format Line 334: elif "GUID" in drive:
https://gerrit.ovirt.org/#/c/44061/5/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 50: imgFileRelPath = "/".join(imgFilePath_list[-4:]) Line 51: Line 52: glusterPath = volname + '/' + imgFileRelPath Line 53: Line 54: hosts = [dict(name=brick.split(":")[0], split only once - split(";", 1) Line 55: port=volPort, Line 56: transport=volTrans) Line 57: for brick in volInfo[volname]['bricks']] Line 58:
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ala Hino has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 5:
(3 comments)
https://gerrit.ovirt.org/#/c/44061/5/vdsm/clientIF.py File vdsm/clientIF.py:
Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 323: volinfo = res ['info'] Line 324: volPath = volinfo['path'] Line 325: drive['protocol'] = volinfo['protocol'] Line 326: if drive.get('hosts') is None:
No need for this check, we are not expecting hosts from engine.
Done Line 327: """ Line 328: currently, single host is provided due to this bug: Line 329: https://bugzilla.redhat.com/1247521 Line 330: """
Line 326: if drive.get('hosts') is None: Line 327: """ Line 328: currently, single host is provided due to this bug: Line 329: https://bugzilla.redhat.com/1247521 Line 330: """
This is not a comment, this is a docstring, which belongs only to modules,
Done Line 331: drive['hosts'] = [volinfo['hosts'][0]] Line 332: Line 333: # GUID drive format Line 334: elif "GUID" in drive:
https://gerrit.ovirt.org/#/c/44061/5/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 50: imgFileRelPath = "/".join(imgFilePath_list[-4:]) Line 51: Line 52: glusterPath = volname + '/' + imgFileRelPath Line 53: Line 54: hosts = [dict(name=brick.split(":")[0],
split only once - split(";", 1)
Done Line 55: port=volPort, Line 56: transport=volTrans) Line 57: for brick in volInfo[volname]['bricks']] Line 58:
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 6:
(4 comments)
https://gerrit.ovirt.org/#/c/44061/6//COMMIT_MSG Commit Message:
Line 4: Commit: Ala Hino ahino@redhat.com Line 5: CommitDate: 2015-07-29 17:03:43 +0300 Line 6: Line 7: virt: enable libgfapi Line 8: Please add a real commit message, see my comment in one of the older versions. 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/6/vdsm/clientIF.py File vdsm/clientIF.py:
Line 313 Line 314 Line 315 Line 316 Line 317 Lets move this to the else: part of the disk type == network check.
Line 320: drive['volumeChain'] = res['imgVolumesInfo'] Line 321: drive['volumeInfo'] = res['info'] Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 323: volinfo = res ['info'] Line 324: volPath = volinfo['path'] We are overriding what we did in line 318 - better set this explicitly:
if disk type is network: volpath = volinfo['path'] ... else: volpath = res['path']
volpath is the result of this function, so want to make it more clear when we set it. Line 325: drive['protocol'] = volinfo['protocol'] Line 326: # currently, single host is provided due to this bug: Line 327: # https://bugzilla.redhat.com/1247521 Line 328: drive['hosts'] = [volinfo['hosts'][0]]
https://gerrit.ovirt.org/#/c/44061/6/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 57: for brick in volInfo[volname]['bricks']] Line 58: Line 59: return {'path': glusterPath, Line 60: 'protocol': 'gluster', Line 61: 'volfileServer': volfileServer, any reason to keep this? see my comment in the previous version.
automation@ovirt.org has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ala Hino has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 6:
(4 comments)
https://gerrit.ovirt.org/#/c/44061/6//COMMIT_MSG Commit Message:
Line 4: Commit: Ala Hino ahino@redhat.com Line 5: CommitDate: 2015-07-29 17:03:43 +0300 Line 6: Line 7: virt: enable libgfapi Line 8:
Please add a real commit message, see my comment in one of the older versio
Done 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/6/vdsm/clientIF.py File vdsm/clientIF.py:
Line 313 Line 314 Line 315 Line 316 Line 317
Lets move this to the else: part of the disk type == network check.
Done
Line 320: drive['volumeChain'] = res['imgVolumesInfo'] Line 321: drive['volumeInfo'] = res['info'] Line 322: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 323: volinfo = res ['info'] Line 324: volPath = volinfo['path']
We are overriding what we did in line 318 - better set this explicitly:
Done Line 325: drive['protocol'] = volinfo['protocol'] Line 326: # currently, single host is provided due to this bug: Line 327: # https://bugzilla.redhat.com/1247521 Line 328: drive['hosts'] = [volinfo['hosts'][0]]
https://gerrit.ovirt.org/#/c/44061/6/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 57: for brick in volInfo[volname]['bricks']] Line 58: Line 59: return {'path': glusterPath, Line 60: 'protocol': 'gluster', Line 61: 'volfileServer': volfileServer,
any reason to keep this? see my comment in the previous version.
this is used in _disksnapshot method in vm.py
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/4/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 55: Line 56: return {'volType': VmVolumeInfo.TYPE_NETWORK, 'path': glusterPath, Line 57: 'protocol': 'gluster', 'port': volPort, Line 58: 'transport': volTrans, Line 59: 'volfileServer': volfileServer, We don't use this key in the vm side, this information is already present in the hosts list.
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/6/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 57: for brick in volInfo[volname]['bricks']] Line 58: Line 59: return {'path': glusterPath, Line 60: 'protocol': 'gluster', Line 61: 'volfileServer': volfileServer,
this is used in _disksnapshot method in vm.py
Why? it should use hosts list?
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 316 Line 317 Line 318 Line 319 Line 320 You missed my comment on an older version - you cannot must use Drive.volumeInfo.
Drive.volumeInfo was replaced by Drive.diskType, path, protocol, hosts, auth.
The best would be to remove Drive.volumeInfo in a separate patch. See Drive.__slots__
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
Please fix pep8 errors - see http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/22527/console
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
ping?
Sahina Bose has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 317: Line 318: # The order of imgVolumesInfo is not guaranteed Line 319: drive['volumeChain'] = res['imgVolumesInfo'] Line 320: drive['volumeInfo'] = res['info'] Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK: I tried applying this patch on master but drive.get('diskType') is None for gluster volume. Line 322: volinfo = res ['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 317: Line 318: # The order of imgVolumesInfo is not guaranteed Line 319: drive['volumeChain'] = res['imgVolumesInfo'] Line 320: drive['volumeInfo'] = res['info'] Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK:
I tried applying this patch on master but drive.get('diskType') is None for
We expect engine to send diskType="network" for gluster volumes, in the same way it send this value for ceph volumes.
The difference between ceph and gluster volumes is that engine sends also the protocol, hosts and path for ceph drives, but these are handled dynamically in vdsm for gluster volumes.
So this requires also a tiny engine patch. Line 322: volinfo = res ['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
Sahina Bose has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 317: Line 318: # The order of imgVolumesInfo is not guaranteed Line 319: drive['volumeChain'] = res['imgVolumesInfo'] Line 320: drive['volumeInfo'] = res['info'] Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK:
We expect engine to send diskType="network" for gluster volumes, in the sam
Ok, now it makes sense. Thanks! Line 322: volinfo = res ['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
Nir Soffer has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 317: Line 318: # The order of imgVolumesInfo is not guaranteed Line 319: drive['volumeChain'] = res['imgVolumesInfo'] Line 320: drive['volumeInfo'] = res['info'] Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK:
Ok, now it makes sense. Thanks!
We are conserned about reliability when using libgfapi, since libvirt does not support multiple hosts.
Do you know if when using libgfapi we need to specify multiple host, or myabe qemu get the replicas addresses from the server?
We filed a libvirt bug for this, hopfully Ala cam point to this bug. Line 322: volinfo = res ['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
Sahina Bose has posted comments on this change.
Change subject: virt: enable libgfapi ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/7/vdsm/clientIF.py File vdsm/clientIF.py:
Line 317: Line 318: # The order of imgVolumesInfo is not guaranteed Line 319: drive['volumeChain'] = res['imgVolumesInfo'] Line 320: drive['volumeInfo'] = res['info'] Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK:
We are conserned about reliability when using libgfapi, since libvirt does
I think u're referring to https://bugzilla.redhat.com/1247521. Prasanna from gluster team is working on this - the patch submitted is handling multiple hosts passed Line 322: volinfo = res ['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
ping - Ala, Nir - any comments for patch?
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
ping - reviews please?
Yaniv Kaul has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
I'm quite sure this patch has to depend on the cluster level being 4.1 or above (i.e, RHEL 7.3 and above), no? We don't want older hosts without this mixed with newer hosts with this functionality, or is it local to the host? Will live migration and live storage migration work between old and new host?
https://gerrit.ovirt.org/#/c/44061/8/vdsm/clientIF.py File vdsm/clientIF.py:
Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 322: volinfo = res['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug: since this patch is for 4.1, and 4.1 will be on top of 7.3, I think we need to remove this limitation. Line 326: # https://bugzilla.redhat.com/1247521 Line 327: drive['hosts'] = [volinfo['hosts'][0]] Line 328: else: Line 329: volPath = res['path']
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
I'm quite sure this patch has to depend on the cluster level being 4.1 or above (i.e, RHEL 7.3 and above), no? We don't want older hosts without this mixed with newer hosts with this functionality, or is it local to the host? Will live migration and live storage migration work between old and new host?
The gfapi access is exercised based on a parameter passed by engine and that depends on the cluster level (4.1) - see https://gerrit.ovirt.org/41899
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8/vdsm/clientIF.py File vdsm/clientIF.py:
Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 322: volinfo = res['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
since this patch is for 4.1, and 4.1 will be on top of 7.3, I think we need
I think we should take in the support for additional hosts as an addon patch. it's a separate bz - https://bugzilla.redhat.com/show_bug.cgi?id=1322852 Line 326: # https://bugzilla.redhat.com/1247521 Line 327: drive['hosts'] = [volinfo['hosts'][0]] Line 328: else: Line 329: volPath = res['path']
Nir Soffer has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 76: for brick in volInfo[volname]['bricks']] Line 77: Line 78: return {'path': glusterPath, Line 79: 'protocol': 'gluster', Line 80: 'volfileServer': volfileServer, Why do we need this?
We have list of hosts - isn't this address one of the bricks?
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8//COMMIT_MSG Commit Message:
Line 18: a url syntax which has to fit as a command line argument to qemu. Line 19: Line 20: This change is based on Federico's changes: Line 21: https://gerrit.ovirt.org/33768/ Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1022961 Line 23: Change-Id: I54b81e87b959b0b49c0f06810f88410e7c75de1d Line 24: Signed-off-by: Federico Simoncelli fsimonce@redhat.com Line 25: Signed-off-by: Ala Hino ahino@redhat.com
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 76: for brick in volInfo[volname]['bricks']] Line 77: Line 78: return {'path': glusterPath, Line 79: 'protocol': 'gluster', Line 80: 'volfileServer': volfileServer,
Why do we need this?
yes volfileserver is one of the address of bricks. However volfileserver could be the user preferred server to access the volume. Especially in case that user has setup virtual IP, they may provide that as the volfileserver. We can change to add volfileserver as the first element in hosts, as I see it is not used in prepareVolumePath - let me know if right approach
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 9:
* #1022961::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1022961::OK, public bug * Check Product::#1022961::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 10:
* #1022961::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1022961::OK, public bug * Check Product::#1022961::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8/vdsm/storage/glusterVolume.py File vdsm/storage/glusterVolume.py:
Line 76: for brick in volInfo[volname]['bricks']] Line 77: Line 78: return {'path': glusterPath, Line 79: 'protocol': 'gluster', Line 80: 'volfileServer': volfileServer,
yes volfileserver is one of the address of bricks. However volfileserver co
Yes, this sounds like a good solution.
Drive has a "hosts" and "protocol" attributes, and creating xml for network drive must use only these properties. See tests/vmStorageTests.py test_network.
If you think that the current support for network drives is not good enough for gluster, please suggest a way to add the missing info in a generic way that can work for any network drive.
I think Drive.volumeInfo is not used for anything now, and it will be removed soon.
Nir Soffer has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/44061/8/vdsm/clientIF.py File vdsm/clientIF.py:
Line 321: if drive.get('diskType') == DISK_TYPE.NETWORK: Line 322: volinfo = res['info'] Line 323: volPath = volinfo['path'] Line 324: drive['protocol'] = volinfo['protocol'] Line 325: # currently, single host is provided due to this bug:
I think we should take in the support for additional hosts as an addon patc
I agree with Sahina, this patch should make it possible to use libgfapi with current support in libvirt.
When libvirt will support more than one host we will support multiple hosts. Line 326: # https://bugzilla.redhat.com/1247521 Line 327: drive['hosts'] = [volinfo['hosts'][0]] Line 328: else: Line 329: volPath = res['path']
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 11:
* update_tracker: OK * Check Bug-Url::OK * Check Public Bug::#1022961::OK, public bug * Check Product::#1022961::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 12:
* #33768::Update tracker: OK * #1022961::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1022961::OK, public bug * Check Product::#1022961::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 13:
* #33768::Update tracker: OK * #1022961::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1022961::OK, public bug * Check Product::#1022961::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 13: Verified+1
gerrit-hooks has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 14:
* Update Tracker::IGNORE, no bug url/s found * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::#1022961::OK, public bug * Check Product::IGNORE, not relevant for branch: master * Check TM::IGNORE, not relevant for branch: master * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Sahina Bose has posted comments on this change.
Change subject: virt: enable glusterfs access through libgfapi interface ......................................................................
Patch Set 14: Verified+1
@Nir, can this patch be merged. Have verified the vm creation/migration/snapshot flows. Anything else required?
vdsm-patches@lists.fedorahosted.org