Thanks for the review. On Wed, 2012-10-17 at 14:27 +0200, Lukáš Doktor wrote: > Hi Sateesh, > > yes I guess now it's the time to create cgroup library and put all > related functions in one place. Would you like to do it, Satheesh, or > should I? > Hi Lukáš,
Sure, I can start doing this and we can make modification based on the review. I have in my mind following things: 1) To pull out the functionality available in client/tests/cgroup/cgroup_common.py asis and place it in client/base_utils.py. 2) Change cgroup.py accordingly to make it work. (Or) 1) Move the client/tests/cgroup/cgroup_common.py to client/cgroup_common.py 2) Change cgroup.py accordingly to make it work. 3) Modify and add the below patch(function) there. Please provide your comments here. > Anyway your code has one small issue, please find it bellow... > > On 10/12/2012 07:34 AM, Satheesh Rajendran wrote: > > On Thu, 2012-10-11 at 09:55 -0300, Lucas Meneghel Rodrigues wrote: > >> Sateesh, > >> > >> I wonder if you ever saw the Cgroup class that was developed by > >> ldoktor. Currently it is on a test module > >> > > Yes, I had gone through them, this particular functionality is not > > available AFIK. > > > >> https://github.com/autotest/autotest-client-tests/blob/master/cgroup/cgroup_common.py > >> https://github.com/autotest/autotest-client-tests/blob/master/cgroup/cgroup_client.py > >> > >> But we could move them into the client (or shared) library. Lukas' > >> implementation takes care of of a good deal of what you want to do > >> with cgroups. > > Agree, As you said these files have many other useful functions which > > will aid for any cgroup related tests, please consider moving them in to > > the shared library. > > > >> Please let me know what you think, > >> > > This particular patch mainly intended to resolve the controller path of > > a given task id, where in there is already a created controller group, > > which is the case for VM's created by libvirt, and this patch will aid > > the future virsh patchs like memtune, blkiotune etc... > > > > Let me know your view, if fine, please consider pushing it after > > review. > > > > Regards, > > -Satheesh. > >> Lucas > >> > >> On Thu, Oct 11, 2012 at 6:47 AM, <[email protected]> wrote: > >>> From: Satheesh Rajendran <[email protected]> > >>> > >>> This function would provide a support for resolving a cgroup path for a > >>> specific controller and a given task id. > >>> > >>> > >>> > >>> Signed-off-by: Satheesh Rajendran <[email protected]> > >>> --- > >>> client/base_utils.py | 36 ++++++++++++++++++++++++++++++++++++ > >>> 1 files changed, 36 insertions(+), 0 deletions(-) > >>> > >>> diff --git a/client/base_utils.py b/client/base_utils.py > >>> index 5e45efd..8485142 100644 > >>> --- a/client/base_utils.py > >>> +++ b/client/base_utils.py > >>> @@ -825,3 +825,39 @@ def suspend_to_disk(): > >>> Suspend the system to disk (S4) > >>> """ > >>> set_power_state('disk') > >>> + > >>> + > >>> +def resolve_task_cgroup_path(pid, controller): > >>> + """ > >>> + Resolving cgroup mount path of a particular task > >>> + > >>> + @params: pid : process id of a task for which the cgroup path > >>> required > >>> + @params: controller: takes one of the controller names in controller > >>> list > >>> + > >>> + @return: resolved path for cgroup controllers of a given pid > >>> + """ > >>> + > >>> + # Initialise cgroup controller list > >>> + controller_list = [ 'cpuacct', 'cpu', 'memory', 'cpuset', > >>> + 'devices', 'freezer', 'blkio', 'netcls' ] > >>> + > >>> + if controller not in controller_list: > >>> + raise error.TestError("Doesn't support controller <%s>" % > >>> controller) > >>> + > >>> + root_path = get_cgroup_mountpoint(controller) > >>> + > >>> + proc_cgroup = "/proc/%d/cgroup" % pid > >>> + if not os.path.isfile(proc_cgroup): > >>> + raise NameError('File %s does not exist\n Check whether cgroup \ > >>> + installed in the system' % > >>> proc_cgroup) > >>> + > >>> + f = open(proc_cgroup, 'r') > >>> + proc_cgroup_txt = f.read() > >>> + f.close > >>> + > >>> + mount_path = re.findall(r":%s:(\S*)\n" % controller, proc_cgroup_txt) > This regext won't work for multiple cgroups (fedora uses this by default > for cpuacct,cpu) and users are allowed to do so. One solution comes to > my mind: > re.findall(r":([^:]*,)*%s(,[^:]*)*:(\S*)\n" % controller, > proc_cgroup_txt)[0][2] > Thanks, I would consider this scenario, modify the patch and send next version accordingly. Regards, -Satheesh. > Than you have to pass only the third argument. I don't know how to > specify groups without putting them into the output. > > Tested on proc_cgroup_txt = """9:perf_event:/ > 8:blkio:/ > 7:net_cls:/ > 6:freezer:/ > 5:devices:/ > 4:memory:/ > 3:cpuacct,cpu:/system/dbus.service > 2:cpuset:/ > 1:name=systemd:/system/dbus.service > """ > > Kind regards, > Lukáš > >>> + logging.info(mount_path[0]) > >>> + > >>> + path = root_path + mount_path[0] > >>> + logging.info(path) > >>> + return path > >>> -- > >>> 1.7.1 > >>> > >>> _______________________________________________ > >>> Autotest-kernel mailing list > >>> [email protected] > >>> https://www.redhat.com/mailman/listinfo/autotest-kernel > >> > >> > > > > _______________________________________________ > > Autotest-kernel mailing list > > [email protected] > > https://www.redhat.com/mailman/listinfo/autotest-kernel > _______________________________________________ Autotest-kernel mailing list [email protected] https://www.redhat.com/mailman/listinfo/autotest-kernel
