Re: [Kimchi-devel] [PATCH V11 4/7] manually manage the metadata element

On 04/29/2014 06:04 AM, Aline Manera wrote: > On 04/28/2014 12:44 PM, shaohef(a)linux.vnet.ibm.com wrote: >> From: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; >> >> virDomain.metadata and virDomain.setMetadata does not work in all >> livirt >> versions used in the supported distros. >> >> So if libvirt do not support these two API. >> >> let kimchi manually manage the metadata element >> >> Signed-off-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; >> Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; >> --- >> src/kimchi/model/utils.py | 105 >> +++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 85 insertions(+), 20 deletions(-) >> >> diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py >> index 3136402..2614008 100644 >> --- a/src/kimchi/model/utils.py >> +++ b/src/kimchi/model/utils.py >> @@ -18,9 +18,10 @@ >> # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> >> from kimchi.exception import OperationFailed >> +from kimchi.model.config import CapabilitiesModel >> import libvirt >> -from lxml import etree >> -from lxml.builder import E >> +from lxml import etree, objectify >> +from lxml.builder import E, ElementMaker >> >> >> KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi" >> @@ -37,6 +38,15 @@ def get_vm_name(vm_name, t_name, name_list): >> raise OperationFailed("KCHUTILS0003E") >> >> >> +def create_metadata_node(tag): >> + if CapabilitiesModel().metadata_support: >> + return E(tag) >> + else: >> + EM = ElementMaker(namespace=KIMCHI_META_URL, >> + nsmap={KIMCHI_NAMESPACE: KIMCHI_META_URL}) >> + return EM(tag) >> + >> + >> def get_vm_config_flag(dom, mode="persistent"): >> # libvirt.VIR_DOMAIN_AFFECT_CURRENT is 0 >> # VIR_DOMAIN_AFFECT_LIVE is 1, VIR_DOMAIN_AFFECT_CONFIG is 2 >> @@ -50,6 +60,30 @@ def get_vm_config_flag(dom, mode="persistent"): >> return flag[mode] >> > >> +def _kimchi_set_metadata_node(dom, node_xml): >> + node = etree.fromstring(node_xml) >> + # some other tools will not let libvirt create a persistent >> + # configuration, raise exception. >> + if not dom.isPersistent(): >> + msg = 'The VM has not a persistent configuration' >> + raise OperationFailed("KCHVM0030E", >> + {'name': dom.name(), "err": msg}) >> + xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) >> + root = etree.fromstring(xml) >> + metadata = root.find("metadata") >> + if metadata is None: >> + metadata = E.metadata() >> + root.append(metadata) >> + kimchi = root.find("metadata/{%s}kimchi" % KIMCHI_META_URL) >> + if kimchi is None: >> + kimchi = create_metadata_node("kimchi") >> + metadata.append(kimchi) >> + old_node = kimchi.find(node.tag) >> + (kimchi.replace(old_node, node) if old_node is not None >> + else kimchi.append(node)) >> + dom.connect().defineXML(etree.tostring(root)) >> + >> + > > There is a some duplicated code here and in set_metadata_node() > We should have special function only for special matters. > See below > > >> def libvirt_get_kimchi_metadata_node(dom, mode="current"): >> try: >> xml = dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, >> @@ -61,29 +95,60 @@ def libvirt_get_kimchi_metadata_node(dom, >> mode="current"): >> >> >> def set_metadata_node(dom, node_xml, mode="all"): > > Why node_xml? Use node directly as we use ET to create the element in > model/vms.py > That way we avoid ET node -> string -> ET node > >> - element = etree.fromstring(node_xml) >> - kimchi = libvirt_get_kimchi_metadata_node(dom, mode) >> - old_node = None >> - if kimchi is not None: >> - old_node = kimchi.find(element.tag) >> - else: >> - kimchi = E.kimchi() >> - (kimchi.replace(old_node, element) if old_node is not None >> - else kimchi.append(element)) >> + if CapabilitiesModel().metadata_support: >> + element = etree.fromstring(node_xml) >> + kimchi = libvirt_get_kimchi_metadata_node(dom, mode) >> + old_node = None >> + if kimchi is not None: >> + old_node = kimchi.find(element.tag) >> + else: >> + kimchi = E.kimchi() >> + (kimchi.replace(old_node, element) if old_node is not None >> + else kimchi.append(element)) > > There is already an if/else statement above, use the same one to > append or replace the new node > > > if kimchi is None: > node = kimchi.find(element.tag) > kimchi.replace(node, element) For node maybe None. In [145]: kimchi.replace(node, element) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-145-bb05d26e1b86> in <module>() ----> 1 metadata.replace(node, element) TypeError: Argument 'old_element' has incorrect type (expected lxml.etree._Element, got NoneType)

> else: > kimchi = E.kimchi() > kimchi.append(element) > > > Joining common code in one place: > > def set_metadata_node(dom, node, mode="all") > kimchi = get_kimchi_metadata_node(dom, mode) > if kimchi is None: > kimchi = create_metadata_node("kimchi") > else: > old_node = kimchi.find(node.tag) > kimchi.replace(old_node, node) > > if CapabilitiesModel().metadata_support: > return dom.setMetadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, node_xml, > KIMCHI_NAMESPACE, KIMCHI_META_URL, > flags=get_vm_config_flag(dom, mode)) > > # Without metadata support > # some other tools will not let libvirt create a persistent > # configuration, raise exception. > if not dom.isPersistent(): > msg = 'The VM has not a persistent configuration' > raise OperationFailed("KCHVM0030E", + {'name': dom.name(), "err": msg}) > > xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) > root = etree.fromstring(xml) > metadata = root.find("metadata") > if metadtaa is None: > metadata = E.metadata() > root.append(metadata) > > old_node = root.find("metadata/{%s}kimchi" % KIMCHI_META_URL) > metadata.replace(old_node, kimchi) if old_node else > metadata.append(kimchi) > dom.connect().defineXML(etree.tostring(root)) > > def get_kimchi_metadata_node(dom, mode): > if CapabilitiesModel().metadata_support: > xml = dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, > KIMCHI_META_URL, > flags=get_vm_config_flag(dom, mode)) > return etree.fromstring(xml) > > # Without metadata support > xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) > root = etree.fromstring(xml) > metadata = root.find("metadata") > return root.find("metadata/{%s}kimchi" % KIMCHI_META_URL) > > >> >> - node_xml = etree.tostring(kimchi) >> - # From libvirt doc, Passing None for @metadata says to remove that >> - # element from the domain XML (passing the empty string leaves the >> - # element present). Do not support remove the old metadata. >> - dom.setMetadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, node_xml, >> - KIMCHI_NAMESPACE, KIMCHI_META_URL, >> - flags=get_vm_config_flag(dom, mode)) >> + node_xml = etree.tostring(kimchi) >> + # From libvirt doc, Passing None for @metadata says to remove that >> + # element from the domain XML (passing the empty string leaves the >> + # element present). Do not support remove the old metadata. >> + dom.setMetadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, node_xml, >> + KIMCHI_NAMESPACE, KIMCHI_META_URL, >> + flags=get_vm_config_flag(dom, mode)) >> + else: >> + # FIXME remove this code when all distro libvirt supports metadata >> element >> + _kimchi_set_metadata_node(dom, node_xml) >> >> >> -def get_metadata_node(dom, tag, mode="current"): >> - kimchi = libvirt_get_kimchi_metadata_node(dom, mode) >> +def _kimchi_get_metadata_node(dom, tag): >> + # some other tools will not let libvirt create a persistent >> + # configuration, just return empty >> + if not dom.isPersistent(): >> + return "" >> + xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) >> + root = etree.fromstring(xml) >> + kimchi = root.find("metadata/{%s}kimchi" % KIMCHI_META_URL) > > >> + #remove the "kimchi" prifix of xml >> if kimchi is not None: >> + for child in kimchi.getiterator(): >> + i = child.tag.find('}') >> + if i >= 0: >> + child.tag = child.tag[i+1:] >> + objectify.deannotate(kimchi, cleanup_namespaces=True) >> + kimchi_xml = etree.tostring(kimchi) >> + kimchi = etree.fromstring(kimchi_xml) >> node = kimchi.find(tag) >> if node is not None: >> return etree.tostring(node) >> return "" > > What about? > > if kimchi is None: > return "" > > kimchi_xml = etree.tostring(kimchi) > Kimchi = kimchi_xml.replace("{%s}" % KIMCHI_META_URL, "") it should be kimchi_xml.replace(xmlns:* ="https://github.com/kimchi-project/kimchi, "") also it should remove the ”kimchi:“ <kimchi:access "> <kimchi:user>user1</kimchi:user> <kimchi:user>user2</kimchi:user> <kimchi:group>group1</kimchi:group> <kimchi:group>group2</kimchi:group> </kimchi:access> or it will report: In [149]: etree.fromstring(kimchi_xml) File "<string>", line unknown XMLSyntaxError: Namespace prefix kimchi on access is not defined, line 2, column 19 I can use RE to handle the xml directly if you do not like handle the ET node directly.

> node = kimchi.find(tag) > if node is not None: > return etree.tostring(node) > > return "" > > >> + >> + >> +def get_metadata_node(dom, tag, mode="current"): >> + if CapabilitiesModel().metadata_support: >> + kimchi = libvirt_get_kimchi_metadata_node(dom, mode) >> + if kimchi is not None: >> + node = kimchi.find(tag) >> + if node is not None: >> + return etree.tostring(node) >> + return "" >> + else: >> + # FIXME remove this code when all distro libvirt supports metadata >> element >> + return _kimchi_get_metadata_node(dom, tag) > > >