Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote:

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>

it should be: FeatureTests.enable_screen_error_logging() instead of: FeatureTests.enable_screen_error_logging

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/featuretests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/kimchi/featuretests.py b/src/kimchi/featuretests.py index 07d5676..b1001ea 100644 --- a/src/kimchi/featuretests.py +++ b/src/kimchi/featuretests.py @@ -171,7 +171,7 @@ class FeatureTests(object): # Libvirt requires adapter name, not needed when supports to FC return False finally: - FeatureTests.enable_screen_error_logging + FeatureTests.enable_screen_error_logging() pool is None or pool.undefine() conn is None or conn.close() return True

Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote:

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>

same mechanism with other feature tests.

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/featuretests.py | 31 +++++++++++++++++++++++++++++++ src/kimchi/model/config.py | 2 ++ 2 files changed, 33 insertions(+)

diff --git a/src/kimchi/featuretests.py b/src/kimchi/featuretests.py index b1001ea..5192361 100644 --- a/src/kimchi/featuretests.py +++ b/src/kimchi/featuretests.py @@ -28,6 +28,7 @@ import threading from lxml.builder import E

+from kimchi.rollbackcontext import RollbackContext from kimchi.utils import kimchi_log

@@ -53,6 +54,16 @@ ISO_STREAM_XML = """ </devices> </domain>"""

+SIMPLE_VM_XML = """ +<domain type='kvm'> + <name>A_SIMPLE_VM</name> + <memory unit='KiB'>10240</memory> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> +</domain>""" + SCSI_FC_XML = """ <pool type='scsi'> <name>TEST_SCSI_FC_POOL</name> @@ -175,3 +186,23 @@ class FeatureTests(object): pool is None or pool.undefine() conn is None or conn.close() return True + + @staticmethod + def has_metadata_support(): + KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi/" + KIMCHI_NAMESPACE = "kimchi" + with RollbackContext() as rollback: + FeatureTests.disable_screen_error_logging() + rollback.prependDefer(FeatureTests.enable_screen_error_logging) + conn = libvirt.open('qemu:///system') + rollback.prependDefer(conn.close) + dom = conn.defineXML(SIMPLE_VM_XML) + rollback.prependDefer(dom.undefine) + try: + dom.setMetadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, + "<metatest/>", KIMCHI_NAMESPACE, + KIMCHI_META_URL, + flags=libvirt.VIR_DOMAIN_AFFECT_CURRENT) + return True + except libvirt.libvirtError: + return False diff --git a/src/kimchi/model/config.py b/src/kimchi/model/config.py index c9e3e9d..0ef0855 100644 --- a/src/kimchi/model/config.py +++ b/src/kimchi/model/config.py @@ -53,6 +53,7 @@ class CapabilitiesModel(object): self.qemu_stream_dns = False self.libvirt_stream_protocols = [] self.fc_host_support = False + self.metadata_support = False

# Subscribe function to set host capabilities to be run when cherrypy # server is up @@ -65,6 +66,7 @@ class CapabilitiesModel(object): self.qemu_stream_dns = FeatureTests.qemu_iso_stream_dns() self.nfs_target_probe = FeatureTests.libvirt_support_nfs_probe() self.fc_host_support = FeatureTests.libvirt_support_fc_host() + self.metadata_support = FeatureTests.has_metadata_support()

self.libvirt_stream_protocols = [] for p in ['http', 'https', 'ftp', 'ftps', 'tftp']:

As we discussed, metadata xml will be changed to structural rather than flat right? See little comments below. On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote:

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>

Libvirt support two API to set and get domain xml metadata. These two function wrap them. Put them in model/utils.py, so vm_storage and vm_iface can make use of them.

For set domain xml metadata, we set live and config xml by default. For get domain xml metadata, we get the current xml by default.

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/model/utils.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)

diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index a1af1f7..e28f689 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -87,6 +87,7 @@ messages = { "KCHVM0027E": _("User(s) '%(users)s' do not exist"), "KCHVM0028E": _("Group(s) '%(groups)s' do not exist"), "KCHVM0029E": _("Unable to shutdown virtual machine %(name)s. Details: %(err)s"), + "KCHVM0030E": _("Unable to get access metadata of virtual machine %(name)s. Details: %(err)s"),

"KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py index 80e1801..9b94a06 100644 --- a/src/kimchi/model/utils.py +++ b/src/kimchi/model/utils.py @@ -18,6 +18,12 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

from kimchi.exception import OperationFailed +import libvirt +from lxml import etree + + +KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi/metadata/" +KIMCHI_NAMESPACE = "kimchi"

def get_vm_name(vm_name, t_name, name_list): @@ -28,3 +34,35 @@ def get_vm_name(vm_name, t_name, name_list): if vm_name not in name_list: return vm_name raise OperationFailed("KCHUTILS0003E") + + +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 + flag = {"live": libvirt.VIR_DOMAIN_AFFECT_LIVE, + "persistent": libvirt.VIR_DOMAIN_AFFECT_CONFIG, + "current": libvirt.VIR_DOMAIN_AFFECT_CURRENT, Why do we need 'current'? 'current' refers to either 'config' or 'live' to me. + "all": libvirt.VIR_DOMAIN_AFFECT_CONFIG + + libvirt.VIR_DOMAIN_AFFECT_LIVE if dom.isActive() and + dom.isPersistent() else libvirt.VIR_DOMAIN_AFFECT_CURRENT} + + return flag[mode] + + +def set_metadata_node(dom, meta_xml, mode="all"): + element = etree.fromstring(meta_xml) + # 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, meta_xml, + KIMCHI_NAMESPACE, KIMCHI_META_URL + element.tag, + flags=get_vm_config_flag(dom, mode)) + + +def get_metadata_node(dom, element, mode="current"): + try: + return dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, + KIMCHI_META_URL + element, + flags=get_vm_config_flag(dom, mode)) + except libvirt.libvirtError: + return ""

As we discussed, metadata xml will be changed to structural rather than flat right? See little comments below. On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote:

...

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>

Libvirt support two API to set and get domain xml metadata. These two function wrap them. Put them in model/utils.py, so vm_storage and vm_iface can make use of them.

For set domain xml metadata, we set live and config xml by default. For get domain xml metadata, we get the current xml by default.

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/model/utils.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)

diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index a1af1f7..e28f689 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -87,6 +87,7 @@ messages = { "KCHVM0027E": _("User(s) '%(users)s' do not exist"), "KCHVM0028E": _("Group(s) '%(groups)s' do not exist"), "KCHVM0029E": _("Unable to shutdown virtual machine %(name)s. Details: %(err)s"), + "KCHVM0030E": _("Unable to get access metadata of virtual machine %(name)s. Details: %(err)s"),

"KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py index 80e1801..9b94a06 100644 --- a/src/kimchi/model/utils.py +++ b/src/kimchi/model/utils.py @@ -18,6 +18,12 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

from kimchi.exception import OperationFailed +import libvirt +from lxml import etree + + +KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi/metadata/" +KIMCHI_NAMESPACE = "kimchi"

def get_vm_name(vm_name, t_name, name_list): @@ -28,3 +34,35 @@ def get_vm_name(vm_name, t_name, name_list): if vm_name not in name_list: return vm_name raise OperationFailed("KCHUTILS0003E") + + +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 + flag = {"live": libvirt.VIR_DOMAIN_AFFECT_LIVE, + "persistent": libvirt.VIR_DOMAIN_AFFECT_CONFIG, + "current": libvirt.VIR_DOMAIN_AFFECT_CURRENT, Why do we need 'current'? 'current' refers to either 'config' or 'live' to me.

On 04/28/2014 02:32 PM, Royce Lv wrote: libvirt support 'current', so let kimchi support it. if other developers do not want to check the status of VM, then he can easily to call 'current'.

...

+ "all": libvirt.VIR_DOMAIN_AFFECT_CONFIG + + libvirt.VIR_DOMAIN_AFFECT_LIVE if dom.isActive() and + dom.isPersistent() else libvirt.VIR_DOMAIN_AFFECT_CURRENT} + + return flag[mode] + + +def set_metadata_node(dom, meta_xml, mode="all"): + element = etree.fromstring(meta_xml) + # 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, meta_xml, + KIMCHI_NAMESPACE, KIMCHI_META_URL + element.tag, + flags=get_vm_config_flag(dom, mode)) + + +def get_metadata_node(dom, element, mode="current"): + try: + return dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, + KIMCHI_META_URL + element, + flags=get_vm_config_flag(dom, mode)) + except libvirt.libvirtError: + return ""

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote:

From: ShaoHe Feng <shaohef@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@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/utils.py | 93 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 14 deletions(-)

diff --git a/src/kimchi/model/utils.py b/src/kimchi/model/utils.py index 9b94a06..257b9e7 100644 --- a/src/kimchi/model/utils.py +++ b/src/kimchi/model/utils.py @@ -18,8 +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 import etree, objectify +from lxml.builder import E, ElementMaker

KIMCHI_META_URL = "https://github.com/kimchi-project/kimchi/metadata/" @@ -36,6 +38,16 @@ def get_vm_name(vm_name, t_name, name_list): raise OperationFailed("KCHUTILS0003E")

+def create_metadata_node(element): + # create the xml using "element" + if CapabilitiesModel().metadata_support: + return E + else: + return ElementMaker(namespace=KIMCHI_META_URL + element, + nsmap={KIMCHI_NAMESPACE: + KIMCHI_META_URL + element}) + + 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 @@ -49,20 +61,73 @@ def get_vm_config_flag(dom, mode="persistent"): return flag[mode]

-def set_metadata_node(dom, meta_xml, mode="all"): +def _kimchi_set_metadata_node(dom, meta_xml): element = etree.fromstring(meta_xml) - # 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, meta_xml, - KIMCHI_NAMESPACE, KIMCHI_META_URL + element.tag, - flags=get_vm_config_flag(dom, mode)) + # 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) + old_elem = metadata.find(element.tag) + (metadata.replace(old_elem, element) if old_elem is not None + else metadata.append(element))

+ dom.connect().defineXML(etree.tostring(root))

-def get_metadata_node(dom, element, mode="current"): - try: - return dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, - KIMCHI_META_URL + element, - flags=get_vm_config_flag(dom, mode)) - except libvirt.libvirtError: + +def set_metadata_node(dom, meta_xml, mode="all"): + if CapabilitiesModel().metadata_support: + element = etree.fromstring(meta_xml) + # 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, meta_xml, + KIMCHI_NAMESPACE, KIMCHI_META_URL + element.tag, + flags=get_vm_config_flag(dom, mode)) + else: + # FIXME remove this code when all distro libvirt supports metadata element + _kimchi_set_metadata_node(dom, meta_xml) + + +def _kimchi_get_metadata_node(dom, element): + # 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) + metadata = root.find("metadata") + if metadata is None: return "" + tag = "{%s%s}%s" % (KIMCHI_META_URL, element, element) + elem = metadata.find(tag) + #remove the "kimchi" prifix of xml + if elem is not None: + for child in elem.getiterator(): + i = child.tag.find('}') + if i >= 0: + child.tag = child.tag[i+1:] + objectify.deannotate(elem, cleanup_namespaces=True) + return etree.tostring(elem) + + return "" + + +def get_metadata_node(dom, element, mode="current"): + if CapabilitiesModel().metadata_support: + try: + return dom.metadata(libvirt.VIR_DOMAIN_METADATA_ELEMENT, + KIMCHI_META_URL + element, + flags=get_vm_config_flag(dom, mode)) + except libvirt.libvirtError: + return "" + else: + # FIXME remove this code when all distro libvirt supports metadata element + return _kimchi_get_metadata_node(dom, element)

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>

define a domain just edit the persistent xml. So we should get user and group from persistent xml instead of live xml when domain is living. Refacor the code by libvirt metadata API and fix the bug.

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 97 ++++++++++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 8a79f0f..8def756 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -25,7 +25,6 @@ from xml.etree import ElementTree

import libvirt from cherrypy.process.plugins import BackgroundTask -from lxml.builder import E

from kimchi import vnc from kimchi import xmlutils @@ -35,6 +34,9 @@ from kimchi.exception import NotFoundError, OperationFailed from kimchi.model.config import CapabilitiesModel from kimchi.model.templates import TemplateModel from kimchi.model.utils import get_vm_name +from kimchi.model.utils import get_metadata_node +from kimchi.model.utils import set_metadata_node +from kimchi.model.utils import create_metadata_node from kimchi.screenshot import VMScreenshot from kimchi.utils import import_class, kimchi_log, run_setfacl_set_attr from kimchi.utils import template_name_from_uri @@ -247,48 +249,59 @@ class VMModel(object): self._live_vm_update(dom, params) return dom.name().decode('utf-8')

- def _get_metadata_node(self, users, groups): - access = E.access() + def _get_metadata_access_node(self, users, groups): + node = create_metadata_node("access") + access = node.access() for user in users: - access.append(E.user(user)) + access.append(node.user(user))

for group in groups: - access.append(E.group(group)) - - return E.metadata(E.kimchi(access)) + access.append(node.group(group)) + + return access I would say the name is a little bit confusing with 'get_metadata', this

+ + def _vm_updata_access_metadata(self, dom, params): typo _vm_update_access_metadata + users = groups = None + if "users" in params: + users = params["users"]

On 2014年04月25日 22:14, shaohef@linux.vnet.ibm.com wrote: part deals with Element and Etree, right? What about 'build_access_elem'? little advice: users = params.get('users', [])

+ invalid_users = set(users) - set(self.users.get_list()) + if len(invalid_users) != 0: + raise InvalidParameter("KCHVM0027E", + {'users': ", ".join(invalid_users)}) + if "groups" in params: + groups = params["groups"] + invalid_groups = set(groups) - set(self.groups.get_list()) + if len(invalid_groups) != 0: + raise InvalidParameter("KCHVM0028E", + {'groups': ", ".join(invalid_groups)}) + + if users is None and groups is None: + return + + access_xml = (get_metadata_node(dom, "access") or + """<access></access>""") + old_users = xpath_get_text(access_xml, "/access/user") + old_groups = xpath_get_text(access_xml, "/access/group") + users = old_users if users is None else users + groups = old_groups if groups is None else groups + + node = self._get_metadata_access_node(users, groups) + set_metadata_node(dom, ET.tostring(node))

def _static_vm_update(self, dom, params): state = DOM_STATE_MAP[dom.info()[0]] old_xml = new_xml = dom.XMLDesc(0)

- metadata_xpath = "/domain/metadata/kimchi/access/%s" - users = xpath_get_text(old_xml, metadata_xpath % "user") - groups = xpath_get_text(old_xml, metadata_xpath % "group") - for key, val in params.items(): - if key == 'users': - invalid_users = set(val) - set(self.users.get_list()) - if len(invalid_users) != 0: - raise InvalidParameter("KCHVM0027E", - {'users': ", ".join(invalid_users)}) - users = val - elif key == 'groups': - invalid_groups = set(val) - set(self.groups.get_list()) - if len(invalid_groups) != 0: - raise InvalidParameter("KCHVM0028E", - {'groups': - ", ".join(invalid_groups)}) - groups = val - else: - if key in VM_STATIC_UPDATE_PARAMS: - if key == 'memory': - # Libvirt saves memory in KiB. Retrieved xml has memory - # in KiB too, so new valeu must be in KiB here - val = val * 1024 - if type(val) == int: - val = str(val) - xpath = VM_STATIC_UPDATE_PARAMS[key] - new_xml = xmlutils.xml_item_update(new_xml, xpath, val) + if key in VM_STATIC_UPDATE_PARAMS: + if key == 'memory': + # Libvirt saves memory in KiB. Retrieved xml has memory + # in KiB too, so new valeu must be in KiB here + val = val * 1024 + if type(val) == int: + val = str(val) + xpath = VM_STATIC_UPDATE_PARAMS[key] + new_xml = xmlutils.xml_item_update(new_xml, xpath, val)

conn = self.conn.get() try: @@ -302,13 +315,6 @@ class VMModel(object):

root = ET.fromstring(new_xml) root.remove(root.find('.currentMemory')) - # Update metadata element - current_metadata = root.find('metadata') - new_metadata = self._get_metadata_node(users, groups) - if current_metadata is not None: - root.replace(current_metadata, new_metadata) - else: - root.append(new_metadata) dom = conn.defineXML(ET.tostring(root, encoding="utf-8")) except libvirt.libvirtError as e: dom = conn.defineXML(old_xml) @@ -317,7 +323,7 @@ class VMModel(object): return dom

def _live_vm_update(self, dom, params): - pass + self._vm_updata_access_metadata(dom, params)

def _has_video(self, dom): dom = ElementTree.fromstring(dom.XMLDesc(0)) @@ -356,9 +362,10 @@ class VMModel(object): res['io_throughput'] = vm_stats.get('disk_io', 0) res['io_throughput_peak'] = vm_stats.get('max_disk_io', 100)

- xml = dom.XMLDesc(0) - users = xpath_get_text(xml, "/domain/metadata/kimchi/access/user") - groups = xpath_get_text(xml, "/domain/metadata/kimchi/access/group") + access_xml = (get_metadata_node(dom, "access") or + """<access></access>""") + users = xpath_get_text(access_xml, "/access/user") + groups = xpath_get_text(access_xml, "/access/group")

return {'name': name, 'state': state,