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

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) 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, "") 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)