Just a minor inline comment below On 06/10/2014 06:06 PM, lvroyce@linux.vnet.ibm.com wrote:

From: Royce Lv <lvroyce@linux.vnet.ibm.com>

An out of box ISO pool is created to make sure user will find a well known place to put his ISO.

Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/model.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/kimchi/model/model.py b/src/kimchi/model/model.py index a766ca5..579f2d1 100644 --- a/src/kimchi/model/model.py +++ b/src/kimchi/model/model.py @@ -24,6 +24,8 @@ import sys

import cherrypy import libvirt +import lxml.etree as ET +from lxml.builder import E

from kimchi.basemodel import BaseModel from kimchi.model.libvirtconnection import LibvirtConnection @@ -31,6 +33,9 @@ from kimchi.objectstore import ObjectStore from kimchi.utils import import_module, listPathModules

+DEFAULT_POOLS = {'default': {'path': '/var/lib/libvirt/images'}, + 'ISO': {'path': '/var/lib/libvirt/isos'}} + class Model(BaseModel): def __init__(self, libvirt_uri='qemu:///system', objstore_loc=None): self.objstore = ObjectStore(objstore_loc) @@ -38,7 +43,8 @@ class Model(BaseModel): kargs = {'objstore': self.objstore, 'conn': self.conn}

if 'qemu:///' in libvirt_uri: - self._default_pool_check() + for pool_name, pool_arg in DEFAULT_POOLS.iteritems(): + self._default_pool_check(pool_name, pool_arg)

this = os.path.basename(__file__) this_mod = os.path.splitext(this)[0] @@ -57,21 +63,18 @@ class Model(BaseModel):

return super(Model, self).__init__(models)

- def _default_pool_check(self): + def _default_pool_check(self, pool_name, pool_arg): conn = self.conn.get() - xml = """ - <pool type='dir'> - <name>default</name> - <target> - <path>/var/lib/libvirt/images</path> - </target> - </pool> - """ + pool = E.pool(E.name(pool_name), type='dir') + pool.append(E.target(E.path(pool_arg['path']))) + xml = ET.tostring(pool) try: - pool = conn.storagePoolLookupByName("default") + pool = conn.storagePoolLookupByName(pool_name) except libvirt.libvirtError: try: pool = conn.storagePoolDefineXML(xml, 0) + # Add build step to make sure target directory created + pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) pool.setAutostart(1) except libvirt.libvirtError, e: cherrypy.log.error("Fatal: Cannot create default pool because " can not create %s pool because, pool_name

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

On 06/10/2014 07:26 AM, Sheldon wrote:

Just a minor inline comment below

...

From: Royce Lv <lvroyce@linux.vnet.ibm.com>

To prevent qemu user be probed multiple times, store it in class attribute so that next time we don't need to create vm.

Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/kvmusertests.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/kimchi/kvmusertests.py b/src/kimchi/kvmusertests.py index 4884ccf..30c287a 100644 --- a/src/kimchi/kvmusertests.py +++ b/src/kimchi/kvmusertests.py @@ -37,14 +37,16 @@ class UserTests(object): <boot dev='hd'/> </os> </domain>""" + user = None + @classmethod + def probe_user(cls): + if cls.user:

On 06/10/2014 06:06 PM, lvroyce@linux.vnet.ibm.com wrote: pip8 recommends if cls.user is not None:

You should run "make check-local" to avoid those problems

...

+ return cls.user

- def __init__(self): - self.vm_uuid = uuid.uuid1() - self.vm_name = "kimchi_test_%s" % self.vm_uuid + vm_uuid = uuid.uuid1() + vm_name = "kimchi_test_%s" % vm_uuid

- def probe_user(self): - xml = self.SIMPLE_VM_XML % (self.vm_name, self.vm_uuid) - user = None + xml = cls.SIMPLE_VM_XML % (vm_name, vm_uuid) with RollbackContext() as rollback: conn = libvirt.open('qemu:///system') rollback.prependDefer(conn.close) @@ -52,7 +54,7 @@ class UserTests(object): rollback.prependDefer(dom.undefine) dom.create() rollback.prependDefer(dom.destroy) - with open('/var/run/libvirt/qemu/%s.pid' % self.vm_name) as f: + with open('/var/run/libvirt/qemu/%s.pid' % vm_name) as f: pidStr = f.read() p = psutil.Process(int(pidStr))

@@ -60,11 +62,11 @@ class UserTests(object): # in psutil 2.0 and above versions, username will be a method, # not a string if callable(p.username): - user = p.username() + cls.user = p.username() else: - user = p.username + cls.user = p.username

- return user + return cls.user

if __name__ == '__main__':

Just a minor inline comment below On 06/10/2014 06:06 PM, lvroyce@linux.vnet.ibm.com wrote:

From: Royce Lv <lvroyce@linux.vnet.ibm.com>

Permission fix needs report qemu owner and grp, add a function to report it.

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

diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 97adbf8..eb3559e 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -35,6 +35,7 @@ from cherrypy.lib.reprconf import Parser from kimchi.asynctask import AsyncTask from kimchi.config import paths, PluginPaths from kimchi.exception import InvalidParameter, TimeoutExpired +from kimchi.kvmusertests import UserTests

kimchi_log = cherrypy.log.error_log @@ -264,3 +265,10 @@ def probe_file_permission_as_user(file, user): p.start() p.join() return queue.get() + + +def get_qemu_owner(): + user = UserTests.probe_user() + uid = pwd.getpwnam(user).pw_uid + gid = pwd.getpwnam(user).pw_gid + return uid, gid you can also change the follow code to keep consistence, though they can works.

diff --git a/src/kimchi/kvmusertests.py b/src/kimchi/kvmusertests.py index 4884ccf..330525c 100644 --- a/src/kimchi/kvmusertests.py +++ b/src/kimchi/kvmusertests.py @@ -68,5 +68,4 @@ def probe_user(self): if __name__ == '__main__': - ut = UserTests() - print ut.probe_user() + print UserTests.probe_user() diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 60f4de5..8aa176e 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -43,7 +43,7 @@ def create(self, params): iso = params['cdrom'] # check search permission if iso.startswith('/') and os.path.isfile(iso): - user = UserTests().probe_user() + user = UserTests.probe_user() ret, excp = probe_file_permission_as_user(iso, user) if ret is False: raise InvalidParameter('KCHISO0008E', -- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

On 06/10/2014 06:06 PM, lvroyce@linux.vnet.ibm.com wrote:

From: Royce Lv <lvroyce@linux.vnet.ibm.com>

Add a threaded monitor to change file ownership. Also its pynotify dependecy is added.

Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/README.md | 6 ++++-- src/kimchi/isomonitor.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 src/kimchi/isomonitor.py

diff --git a/docs/README.md b/docs/README.md index 63ac760..9f059ca 100644 --- a/docs/README.md +++ b/docs/README.md @@ -53,7 +53,8 @@ Install Dependencies PyPAM m2crypto python-jsonschema rpm-build \ qemu-kvm python-psutil python-ethtool sos \ python-ipaddr python-lxml nfs-utils \ - iscsi-initiator-utils libxslt pyparted nginx + iscsi-initiator-utils libxslt pyparted nginx \ + python-inotify # If using RHEL6, install the following additional packages: $ sudo yum install python-unittest2 python-ordereddict # Restart libvirt to allow configuration changes to take effect @@ -75,7 +76,8 @@ for more information on how to configure your system to access this repository. python-pam python-m2crypto python-jsonschema \ qemu-kvm libtool python-psutil python-ethtool \ sosreport python-ipaddr python-lxml nfs-common \ - open-iscsi lvm2 xsltproc python-parted nginx + open-iscsi lvm2 xsltproc python-parted nginx \ + python-pyinotify

For openSUSE? also update spec file?

Packages version requirement: python-jsonschema >= 1.3.0 diff --git a/src/kimchi/isomonitor.py b/src/kimchi/isomonitor.py new file mode 100644 index 0000000..c20d71a --- /dev/null +++ b/src/kimchi/isomonitor.py @@ -0,0 +1,33 @@ +import os +import pyinotify + + +from kimchi.utils import get_qemu_owner + + +class EventHandler(pyinotify.ProcessEvent): + def process_IN_CREATE(self, event): + uid, gid = get_qemu_owner() + os.chown(event.pathname, uid, gid) +

also, you follow code also notify the DELETE event + def process_IN_DELETE(self, event): + uid, gid = get_qemu_owner() + os.chown(event.pathname, uid, gid)

+ +class pathMonitor(object): + def __init__(self): + self.wm = pyinotify.WatchManager() + self.notifier = pyinotify.ThreadedNotifier(self.wm, EventHandler()) + self.handlers = dict() + + self.notifier.start() + + def add_monitor_path(self, path): + mask = pyinotify.IN_DELETE | pyinotify.IN_CREATE # watched events + self.handlers[path] = self.wm.add_watch(path, mask, rec=True)

what is IN_CREATE? the start of copy a iso file? not sure it should be pyinotify.IN_CLOSE_WRITE ？

+ + def rm_monitor_path(self, path): + self.wm.rm_watch(self.handlers.pop(path).values()) + + def stop_monitor(self): + for path in self.handlers.iterkeys(): + self.rm_monitor_path(path) + self.notifier.stop() +

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