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

Tue Apr 29 12:59:37 UTC 2014

On 04/28/2014 09:13 PM, Sheldon wrote:
> 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
>>
>>
>>> 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)
>

OK. I thought all "node" would be valid

>
>> 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.

Ok. Keep it as you have done.

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