Dan Kenigsberg has posted comments on this change. Change subject: caps: report if QEMU supports live snapshots ......................................................................
Patch Set 6: Code-Review-1 (6 comments) http://gerrit.ovirt.org/#/c/26149/6//COMMIT_MSG Commit Message: Line 9: Depending on QEMU version and configuration, live snapshotting Line 10: may be not supported. In that case, a request sent by engine Line 11: will of course fail. Line 12: Line 13: Libvirt until recently did not exposed any information about /me's English is bad, but I think that Until recently, libvirt did not expose is a bit better. Mentioning the libvirt version is even better. Line 14: the live snapshot support, so VDSM, and then engine, had no Line 15: option other than assume this was supported. Line 16: Line 17: Recently libvirt gained this functionality: Line 22: disable the relevant fields in the UI. Line 23: Line 24: The detection is fully backward-compatible. If the information Line 25: is missing from the domain capabilities XML, the support is Line 26: announced to be present. please consider my suggestion of reporting nothing if we know nothing. Line 27: Line 28: Change-Id: I78dd51fc72f1b6d7eadb5c18d3b768f42d8ee32b Line 29: Bug-Url: https://bugzilla.redhat.com/1009100 http://gerrit.ovirt.org/#/c/26149/6/vdsm/caps.py File vdsm/caps.py: Line 163: Line 164: return topology Line 165: Line 166: Line 167: def _findLiveSnapshotSupport(guest): The function name is a bit vague, I could use a docstring explaining when "guest" is. Line 168: features = guest.getElementsByTagName('features')[0] Line 169: for feature in features.childNodes: Line 170: if feature.nodeName == 'disksnapshot': Line 171: value = feature.getAttribute('default') Line 171: value = feature.getAttribute('default') Line 172: return value.lower() == 'on' Line 173: # libvirt < 1.2.2 does not export this information. Line 174: # we default as support present Line 175: return True instead of lying here, how about have a tri-state - True, False, and I-have-know-idea-so-I'm-not-reporting-liveSnapshot-in-caps ? This function could return None. I think that it's the honorable thing to do. Line 176: Line 177: Line 178: @utils.memoized Line 179: def _getLiveSnapshotSupport(arch, capabilities=None): Line 178: @utils.memoized Line 179: def _getLiveSnapshotSupport(arch, capabilities=None): Line 180: if capabilities is None: Line 181: capabilities = _getCapsXMLStr() Line 182: caps = minidom.parseString(capabilities) hmm, it's the third time we parse this xml; it might make sense to memoize the result. (separate patch) Line 183: Line 184: for guestTag in caps.getElementsByTagName('guest'): Line 185: archTag = guestTag.getElementsByTagName('arch')[0] Line 186: if archTag.getAttribute('name') == arch: Line 185: archTag = guestTag.getElementsByTagName('arch')[0] Line 186: if archTag.getAttribute('name') == arch: Line 187: return _findLiveSnapshotSupport(guestTag) Line 188: Line 189: logging.error('guest capabilities not found') Do we expect libvirt to ever fail to have the <guest> element for our arch, and we should go on reporting? Line 190: return False Line 191: Line 192: Line 193: @utils.memoized -- To view, visit http://gerrit.ovirt.org/26149 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78dd51fc72f1b6d7eadb5c18d3b768f42d8ee32b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Leonardo Bianconi <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Vitor de Lima <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
