Simone Tiraboschi has posted comments on this change. Change subject: vdsm: hook for booting from an ISO image gathered via https ......................................................................
Patch Set 7: (11 comments) https://gerrit.ovirt.org/#/c/41076/7/vdsm_hooks/httpsisoboot/README File vdsm_hooks/httpsisoboot/README: Line 7: Line 8: The https source won't work reliably unless you can guarantee a low latency Line 9: between the VM and the web server. If ever a packet is lost of takes longer Line 10: than quite a short period to arrive, the guest will see EIO errors Line 11: and fail. > doesn't that depend on the policy of the disk drive? I don't know: Richard W.M. Jones reported it here and I'm simply trusting him cause he has more experience than me on this topic. https://bugzilla.redhat.com/show_bug.cgi?id=917026#c13 Line 12: So it could be adopted with a local web server with plenty of spare capacity Line 13: and a fast LAN link to RHEV-H, but using it with remotely available ISO Line 14: images is not recommended. Line 15: Line 13: and a fast LAN link to RHEV-H, but using it with remotely available ISO Line 14: images is not recommended. Line 15: Line 16: The lack of support for plain http is due to a limit of qemu-kvm-rhev, see BZ1132569, Line 17: while the qemu rpm in the Fedora Virtualization Preview Repository supports it. > I think it's just upstream qemu which supports it, while the -rhev and -ev Done Line 18: Please see https://fedoraproject.org/wiki/Virtualization_Preview_Repository Line 19: for info about it. Line 20: Line 21: syntax: Line 15: Line 16: The lack of support for plain http is due to a limit of qemu-kvm-rhev, see BZ1132569, Line 17: while the qemu rpm in the Fedora Virtualization Preview Repository supports it. Line 18: Please see https://fedoraproject.org/wiki/Virtualization_Preview_Repository Line 19: for info about it. > trailing whitespace Done Line 20: Line 21: syntax: Line 22: httpsisoboot=https://server/path/to/disk.iso Line 23: Line 31: <source protocol="https" name="/path/to/disk.iso"> Line 32: <host name="server" port="8080"/> Line 33: </source> Line 34: <target dev='hdc' bus='ide' tray='closed'/> Line 35: <backingStore/> > I think you don't need to produce the <backingStore> element. Done Line 36: <readonly/> Line 37: <boot order='1'/> Line 38: </disk> Line 39: https://gerrit.ovirt.org/#/c/41076/7/vdsm_hooks/httpsisoboot/before_vm_start.py File vdsm_hooks/httpsisoboot/before_vm_start.py: Line 17: httpsisoboot=https://server/path/to/disk.iso Line 18: ''' Line 19: Line 20: Line 21: disk_by_index = { > if you keep this, please name it as constant: Done Line 22: 0: 'hda', Line 23: 1: 'hdb', Line 24: 2: 'hdc', Line 25: 3: 'hdd' Line 39: ] Line 40: ) Line 41: Line 42: Line 43: def increaseDevicesBootOrder(domxml): > we try to keep new code pep8-compliant. Done Line 44: xmldevices = [ Line 45: e for e in domxml.getElementsByTagName('devices')[0].childNodes Line 46: if e.nodeType == e.ELEMENT_NODE Line 47: ] Line 58: )+1 Line 59: ) Line 60: ) Line 61: except ValueError: Line 62: hooking.exit_hook( > I don't see how this try block is of assistance beyond the catch-all one ar Done Line 63: 'httpsisoboot: unable to manipulate the boot order\n' Line 64: ) Line 65: Line 66: Line 98: disk.appendChild(source) Line 99: Line 100: readonly = domxml.createElement('readonly') Line 101: disk.appendChild(readonly) Line 102: backingStore = domxml.createElement('backingStore') > as wrote elsewhere, you probably don't need to produce this element. Done Line 103: disk.appendChild(backingStore) Line 104: Line 105: # find a name for hdX Line 106: target = domxml.createElement('target') Line 149: Line 150: port = None Line 151: if parsed.port is not None: Line 152: port = parsed.port Line 153: elif protocol == 'http': > http was excluded above. just set Done Line 154: port = 80 Line 155: elif protocol == 'https': Line 156: port = 443 Line 157: if not port >= 1 and port <= 65535: Line 240: print( Line 241: "\n Devices definition before increaseDevicesBootOrder \n %s" Line 242: % pretty_print(devices) Line 243: ) Line 244: increaseDevicesBootOrder(domxml) > you could pass the devices here, instead of re-iterating them in the functi Done Line 245: devices = domxml.getElementsByTagName('devices')[0] Line 246: print( Line 247: "\n Devices definition after increaseDevicesBootOrder \n %s" Line 248: % pretty_print(devices) Line 283: else: Line 284: main() Line 285: except: Line 286: hooking.exit_hook( Line 287: 'spiceoptions: [unexpected error]: %s\n' % traceback.format_exc() > it's not spiceoptions ;-) Done -- To view, visit https://gerrit.ovirt.org/41076 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I22b190b5d3d43cab7bdbfa81bdf0904b2988b2bc Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
