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

[Sheldon]

On 04/29/2014 06:04 AM, Aline Manera wrote:
> On 04/28/2014 12:44 PM, shaohef at linux.vnet.ibm.com wrote:
>> From: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>
>> 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 at linux.vnet.ibm.com>
>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>> ---
>> 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
I do not think this is better.
only several line code.
for my original thought, I make the logic of virDomain api and our 
kimchi code clearly.
virDomain api can make xml parser more easily.
And I give the comments tag with FIXME.
we can easily remove the kimchi code when all distro libvirt supports 
metadata elemen.
also I have try your below code, the codes does not reduce.
And we mix the logic of virDomain api and our kimchi code in two functions.

But since we do not know how long will all distro libvirt support 
metadata elemen, I can
change as your comments.
>
>
>> 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)
>
>
>


-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef at linux.vnet.ibm.com>
IBM Linux Technology Center