PP-810: Update cgroup hook to work with systemd #626

mike0042 · 2018-04-09T03:43:53Z

Bug/feature Description

Address several issues related to the PBS Pro cgroup hook, including PP-810
PP-810

Affected Platform(s)

Linux with cgroup support

Cause / Analysis / Design

Update the cgroups hook to work in cooperation with systemd

Solution Description

Refactored fixes from multiple sources

Checklist:

I have joined the pbspro community forum.
My pull request contains a single, signed commit. See setting up gpg signature.
My code follows the coding style of this project.
My change requires project documentation. See required documentation checklist for details.
- I have added documentation in the project documentation area.
I have added new PTL test(s) to my commit. (See using PTL for testing) (or)
- I have added manual test(s) to this pull request and explained why PTL is not appropriate for this case.
All new and existing automated tests have passed. (See running automated PTL tests).
I have attached test logs to this pull request as evidence of testing/verification.

For further information please visit the Developer Guide Home.

vstumpf

Hey mike, I know it's still a WIP, but pylint picked up some issues.

vstumpf · 2018-04-09T20:26:22Z

src/hooks/cgroups/pbs_cgroups.PY

+        if not result:
+            continue
+        key, val, extra = result.groups()
+        if not key or not value:


value should probably be val

Fixed in next commit.

vstumpf · 2018-04-09T20:27:41Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -1770,10 +1969,13 @@ class CgroupUtils(object):
                 repr(self.subsystems),
                 repr(self.paths),
                 repr(self.vntype),
-                 repr(self.assigned_resources)))
+                 repr(self.assigned_resources),
+                 repr(self.systemd_version)))


There's too many arguments to the string, you need to add another %s to the format string.

Fixed in next commit.

vstumpf · 2018-04-09T20:31:35Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -1177,11 +1249,48 @@ class NodeConfig(object):
                            convert_size(entries[3] + entries[4], 'kb')
                    elif entries[2] == "HugePages_Total:":
                        numa_nodes[num]['HugePages_Total'] = entries[3]
+        # Spread memory resources across NUMA nodes
+        if self.cfg['vnode_per_numa_node'] and len(numa_nodes) > 0:


Since numa_nodes is a list, you can just check if numa_nodes instead of its length.

Since we use len(numa_nodes) as a denominator we must ensure it is not zero. There was no sense recalculating this value in the block that followed, so assigned len(num_nodes) to a variable.

minghui-liu · 2018-04-09T21:25:12Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -347,6 +356,8 @@ def expand_list(old):
    2,5-7,10
    """
    new = []
+    if isinstance(old, list):
+        old = ','.join(str(x) for x in old)


map(str, old) does the same thing but is more concise.

Will be restored to map() in the next commit.

minghui-liu · 2018-04-09T21:32:29Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -360,20 +371,37 @@ def expand_list(old):
    return new


-def find_files(path, pattern='*', kind=''):
+def find_files(path, pattern='*', kind='',
+               follow_links=False, follow_mounts=True):
    """
    Return a list of files similar to the find command
    """
    if isinstance(pattern, str):
        pattern = [pattern]


pattern can be ['']. Is this okay?

Yes, it is safe. Just tried it out to make sure.

minghui-liu · 2018-04-09T21:34:34Z

src/hooks/cgroups/pbs_cgroups.PY

+    info = {}
+    jobfile = os.path.join(PBS_MOM_JOBS, '%s.JB' % jobid)
+    if not os.path.isfile(jobfile):
+        pbs.logmsg(pbs.EVENT_DEBUG4, "File not found: %s" % (jobfile))


Why parentheses around jobfile?

Not strictly necessary in this case, but if we were doing multiple substitutions it would be required.

And PEP8 doesn't complain, so it must be "acceptable" Python syntax.

minghui-liu · 2018-04-09T21:37:57Z

src/hooks/cgroups/pbs_cgroups.PY

-        return substate in ['0x2b', '0x2d', 'unknown']
+    except KeyboardInterrupt:
+        raise
+    except Exception, exc:


I don't understand the comma here. Do you mean
except Exception as exc ?

All instances have been updated to "except Exception as exc:" which is compatible with Python 2.6 through 3.x.

minghui-liu · 2018-04-09T22:07:06Z

src/hooks/cgroups/pbs_cgroups.PY

-                numa_nodes[num]['cpus'] = expand_list(desc.readline())
+                numa_nodes[num]['cpus'] = \
+                    [x for x in expand_list(desc.readline()) if x not in
+                     expand_list(self.cfg['cgroup']['cpuset']['exclude_cpus'])]


Another way of writing this is

a = expand_list(self.cfg['cgroup']['cpuset']['exclude_cpus']) b = expand_list(desc.readline()) numa_nodes[num]['cpus'] = filter(lambda x : x not in a, b)

a and b are just placeholder variable names that you can replace with more meaningful ones.

I personally this is more readable but you are free to choose which one you like better.

I tend to agree with you, so I updated it.

minghui-liu · 2018-04-10T17:58:35Z

src/hooks/cgroups/pbs_cgroups.PY

+    def _systemd_unescape(self, buf):
+        """
+        Unescape strings encoded for usage in system unit names
+        Some distros don't provide the systemd-escape commad


Same here. Missing a 'n'.

minghui-liu · 2018-04-10T18:03:11Z

src/hooks/cgroups/pbs_cgroups.PY

+        while i < length:
+            if (length - i) > 3 and buf[i] == '\\' and buf[i + 1] == 'x' and \
+                buf[i + 2] in '0123456789abcdef' and \
+                    buf[i + 3] in '0123456789abcdef':


What about capital 'A' to 'F'? Do they need to be handled?

No, because the actual systemd-escape command doesn't handles lower case.

minghui-liu · 2018-04-10T18:05:15Z

src/hooks/cgroups/pbs_cgroups.PY

+        if os.path.isdir(os.path.join(os.sep, 'proc', 'self', 'task')):
+            check_tasks = 1
+        else:
+            check_tasks = 0


Why not use a boolean type?

Converted to boolean type.

minghui-liu · 2018-04-10T20:31:06Z

src/hooks/cgroups/pbs_cgroups.PY


    def remove_jobid_from_cgroup_jobs(self, jobid):
+        """
+        Remove a job idea from the file where local jobs are maintained


typo: job id

minghui-liu · 2018-04-10T20:31:18Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -3773,66 +4383,82 @@ class CgroupUtils(object):
        return self.assigned_resources is not None

    def add_jobid_to_cgroup_jobs(self, jobid):
+        """
+        Add a job idea to the file where local jobs are maintained


Typo: job id

bhroam · 2018-04-09T20:51:50Z

src/hooks/cgroups/pbs_cgroups.PY

+    result = re.match(r'([-+]?\d+)\s*([a-zA-Z]+)',
+                      str(value).lower())
+    if not result:
+        raise ValueError('Unrecognized value.')


I'd drop the period from this error message

Will be removed in the next commit.

bhroam · 2018-04-09T20:55:13Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -347,6 +356,8 @@ def expand_list(old):
    2,5-7,10
    """
    new = []
+    if isinstance(old, list):
+        old = ','.join(str(x) for x in old)


IIRC you previously used map() to call str() on each element in a list. This is fine though. Up to you on how you want to do it.

Switched it back to map().

bhroam · 2018-04-09T21:29:09Z

src/hooks/cgroups/pbs_cgroups.PY

-        return substate in ['0x2b', '0x2d', 'unknown']
+    except KeyboardInterrupt:
+        raise
+    except Exception, exc:


Use the newer except Exception as esc style

That won't work with some older versions of PBS Pro. There are still users with Python 2.5 that want to make use of the cgroup hook. We'll have to use the "as" syntax when we migrate to Python 3.

That's the thing. You are checking this hook into 18.1 OSS. It uses the system python which will be at least 2.6. You really should be using the as syntax.

I'm going to have to defer to @scc138 for a decision. There was an issue related to this recently.

Maybe @subhasisb should weigh in on the decision as well?

While it would be convenient if this hook would work with PBS Pro 13.x where Python 2.5 is present it is not a requirement. Doing the right thing for the targeted Python versions (2.6 and up, less than 3.0) and being future proof are more important.

I'll update all of the "Exception, exc:" to "Exception as exc:" in the next push.

bhroam · 2018-04-09T21:30:32Z

src/hooks/cgroups/pbs_cgroups.PY

+        pbs.logmsg(pbs.EVENT_DEBUG2, "Error message: %s" % err)
+        pbs.logmsg(pbs.EVENT_DEBUG2, "Exception: %s" % exc)
+        return info
+    pattern = re.compile(r'^(\w.*):\s*(\S+)\s*(\S*).*$')


You shouldn't need the .*$ since you're just throwing it away. The regex should apply to a substring.

Fixed in next commit.

bhroam · 2018-04-09T21:32:12Z

src/hooks/cgroups/pbs_cgroups.PY

+        result = re.match(pattern, line)
+        if not result:
+            continue
+        key, val, extra = result.groups()


Why group extra if you don't use it?

Fixed in next commit.

bhroam · 2018-04-10T20:58:47Z

src/hooks/cgroups/pbs_cgroups.PY

+                        if int(entries[0]) not in pids:
+                            pids.append(int(entries[0]))
+            except:
+                raise


This try except block doesn't do anything. You have a blanket except where all you do is raise the exception.

I added a comment and changed the raise to a pass. We're inspecting proc, which could change while we're reading files. That should not cause a fatal error since it is expected (occasionally). When I pasted the method I had unit tested into the hook I didn't update that piece.

bhroam · 2018-04-10T21:42:04Z

src/hooks/cgroups/pbs_cgroups.PY

+                    elif vtype is pbs.int:
+                        dest[key] = pbs.int(0)
+                    elif vtype is pbs.float:
+                        dest[key] = pbs.float(0.0)
                    else:
                        raise ValueError('Unrecognized resource type.')


I'd remove the period from the error message

bhroam · 2018-04-10T21:59:33Z

src/hooks/cgroups/pbs_cgroups.PY

        pbs.logmsg(pbs.EVENT_DEBUG4, "%s: Method called" % caller_name())
        if not os.path.isdir(path):
            pbs.logmsg(pbs.EVENT_DEBUG4, "%s: No such directory: %s" %
                       (caller_name(), path))
            return False
-        tasks_file = os.path.join(path, 'tasks')
+        if not jobid:
+            parent = path


If you can pass None in for jobid, make it a default value for jobid so it doesn't have to be passed in.

Done. Also corrected some log messages associated with the jobid.

bhroam · 2018-04-10T22:04:29Z

src/hooks/cgroups/pbs_cgroups.PY

+                except KeyboardInterrupt:
+                    raise
+                except:
+                    pass


Don't use blanket except statements like this. Narrow the scope.

This is another case where we don't care what the error was, or even that it occurred. We just keep trying to identify and cleanup orphans. I added a log message in the case where os.rename() fails.

Using blanket except pass statements is a bad habit to get into. I understand you don't care about the exceptions, but I'd still suggest you narrow the scope. You know what os.rename() will throw.

bhroam · 2018-04-10T22:11:41Z

src/hooks/cgroups/pbs_cgroups.PY

        try:
-            with open(self.cgroup_jobs_file, 'w') as f:
+            with open(self.cgroup_jobs_file, 'w+') as f:


Why w+? You're not reading the file.

Changed to 'w'. Thanks for all of your feedback!

bhroam · 2018-04-19T00:19:04Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -2738,14 +2739,16 @@ class CgroupUtils(object):
                        if int(entries[0]) not in pids:
                            pids.append(int(entries[0]))
            except:
-                raise
+                # PIDs may come and go as we read /proc so the glob data can


Shouldn't this except be an except OSError? The comment makes it sound like we're trying to be accepting of pids that go away. That means the open will fail and raise an OSError.

Added OSError.

bhroam · 2018-04-19T00:20:49Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -3166,8 +3170,8 @@ class CgroupUtils(object):
            avail_mem = available['memory']
            if req_mem > avail_mem:
                pbs.logmsg(pbs.EVENT_DEBUG4,
-                           "Insufficient memory on socket(s) " +
-                           "%s: requested:%s, assigned:%s" %
+                           'Insufficient memory on socket(s) ' +


I'd replace this + with a '\'. This way the string doesn't need to be regenerated.

Placed both strings within parens and removed the + operator.

bhroam · 2018-04-19T00:23:48Z

src/hooks/cgroups/pbs_cgroups.PY

-            msg += "has the offline file."
-            pbs.logmsg(pbs.EVENT_DEBUG2, msg)
+            pbs.logmsg(pbs.EVENT_DEBUG2,
+                       'Cgroup(s) not cleaning up but the node already ' +


Remove this +. It isn't needed and will cause the string to be regenerated.

Fixed several instances, including those you flagged.

bhroam · 2018-04-19T00:24:02Z

src/hooks/cgroups/pbs_cgroups.PY

-                msg += "already offline."
-                pbs.logmsg(pbs.EVENT_DEBUG2, msg)
+                pbs.logmsg(pbs.EVENT_DEBUG2,
+                           'Cgroup(s) not cleaning up but the node is ' +


Same as above, remove the +

bhroam · 2018-04-19T00:24:12Z

src/hooks/cgroups/pbs_cgroups.PY

-                msg += "cleaning up"
-                pbs.logmsg(pbs.EVENT_DEBUG2, msg)
+                pbs.logmsg(pbs.EVENT_DEBUG2,
+                           'Offlining node since cgroup(s) are not ' +


Same as above, remove the +

bhroam · 2018-04-20T21:55:01Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -1252,20 +1252,23 @@ class NodeConfig(object):
            # Huge page memory
            host_hpmem = self.get_hpmem_on_node(ignore_reserved=True)
            host_resv_hpmem = host_hpmem - self.get_hpmem_on_node()
-            host_resv_hpmem = 0 if (host_resv_hpmem < 0) else host_resv_hpmem
+            if (host_resv_hpmem < 0):


Python style doesn't use () in ifs unless necessary. Please remove them (and for the other cases in this function)

bhroam · 2018-04-20T21:57:02Z

src/hooks/cgroups/pbs_cgroups.PY

+                            'Description=%s\n'
+                            '[Slice]\n'
+                            'Delegate=yes\n'
+                            'TasksMax=infinity\n') % description)


No need for the extra set of (). You're in a set of () by being a function call. (and for other similar strings in function calls)

bhroam · 2018-04-20T21:59:29Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -4156,8 +4150,6 @@ class CgroupUtils(object):
                raise CgroupLimitError('Invalid limit value: %s' % (value))
            else:
                raise
-        except KeyboardInterrupt:
-            raise
        except:
            raise


There is no real purpose to an except: raise. If you don't have this statement, all uncaught exceptions will be just that, uncaught. This just makes you catch them and reraise them.

vstumpf · 2018-04-27T17:35:27Z

src/hooks/cgroups/pbs_cgroups.PY

+            if os.path.isfile(tasks_file):
+                self._kill_tasks(tasks_file)
+            pbs.logmsg(pbs.EVENT_DEBUG2, '%s: Removing directory %s' %
+                       (caller_name(), parent))


Parent is undefined here

Good catch. Thanks!

mike0042 · 2018-05-01T17:36:04Z

Conflict resolution did not occur smoothly. Git tried to replay each commit in order, and it was a waste of time re-applying changes one by one to get the branch back on track. I ended up copying the latest hook and tests over the instances with conflicts until everything was resolved. Git then noticed that several patches had already been applied and without my knowledge discarded some previous commits.

anamikau · 2018-05-01T17:39:25Z

oh no and I see Vincent's commit is gone ..

mike0042 · 2018-05-01T17:40:11Z

But his changes are all still there. Nothing was lost in the process, aside from a bit of history which is eventually going to be squashed out.

anamikau · 2018-05-01T17:43:29Z

oh okay .. phew!!

anamikau · 2018-05-02T19:16:56Z

test/tests/functional/pbs_cgroups_hook.py

-        if not os.path.isdir(fdir):
+        fdir = self.get_cgroup_job_dir('freezer', '123.foo', self.momA)
+        fdir = os.path.dirname(fdir)
+        fdir = os.path.dirname(fdir)


duplicate lines

That was intentional.

can you add a note why you are assigning fdir twice?

Can you turn these two comments into one that says "Get the parent's parent"

anamikau · 2018-05-02T19:27:03Z

test/tests/functional/pbs_cgroups_hook.py

+            'python - 100 4 <<EOF\n' + \
+            self.eatmem_script + 'EOF\n' + \
+            'let i=0; while [ $i -lt 400000 ]; do let i+=1 ; done\n' + \
+            'sleep 5\n'


The reason this job script was updated to allocate less memory as sometime VMs were running out of memory or causing a hang. Why do we need this job script?

It allocates less memory than it used to, and is needed to accumulate CPU time so that the measurements get larger as the job runs.

Thanks Mike. Please update the script to run explicitly with bash. As these syntax are failing with csh.

anamikau · 2018-05-02T20:41:12Z

test/tests/functional/pbs_cgroups_hook.py

@@ -1166,7 +1167,7 @@ def test_cgroup_enforce_memory(self):
                             max_attempts=20)
        self.wait_and_remove_file(filename=o.split(':')[1], mom=self.momA)

-    def Xtest_cgroup_enforce_memsw(self):
+    def test_cgroup_enforce_memsw(self):


Please skip this test if resources_available.vmem is not present as jobs will not run in that case.

I've done completely clean installs and immediately after ran the cgroup tests, but have not seen this fail. Do you have an example where it fails?

Here is the example,

pbsroot@testdev-09-s12:~/cgroup_dev/tests/functional> qsub -l select=1:ncpus=1:mem=300mb:vmem=320mb:host=testdev-09-s12 -- /bin/sleep 10
63.testdev-09-s12

comment = Can Never Run: Insufficient amount of resource: vmem (R: 320mb A:
0kb T: 0kb)

pbsroot@testdev-09-s12:~/cgroup_dev/tests/functional> pbsnodes -a | grep vmem
resources_available.vmem = 0kb
resources_assigned.vmem = 0kb

I updated the code with following and added a skip
if have_swap() == 0 or not (self.server.status(NODE,'resources_available.vmem',id=self.momA)):

You must have manually added "vmem" to the sched_config "resources:" line. The test harness doesn't do that, so the scheduler doesn't consider vmem when scheduling jobs.

OK, now I understand why. I'll update it.

anamikau · 2018-05-03T19:59:45Z

test/tests/functional/pbs_cgroups_hook.py

@@ -84,7 +107,6 @@ def setUp(self):
        self.vntypenameA = "no_cgroups"
        self.vntypenameB = self.vntypenameA
        self.iscray = 0
-        self.tempfile = []


Please add this piece of code back with related changes at other places too. This was added to cleanup the temporary files created by create_temp_file function.

anamikau · 2018-05-03T20:02:18Z

test/tests/functional/pbs_cgroups_hook.py

@@ -321,7 +373,7 @@ def setUp(self):
 }
 """
        self.cfg2 = """{
-    "cgroup_prefix"         : "pbs",
+    "cgroup_prefix"         : "propbs",


other palces it is pbspro. is "propbs" intentional?

Yes, it is intentional. It is needed by test_cgroup_prefix_and_devices.

anamikau · 2018-05-03T20:02:47Z

test/tests/functional/pbs_cgroups_hook.py

        self.logger.info("vntype set for %s is %s" %
                         (self.momA, self.vntypenameA))
        self.logger.info("vntype set for %s is %s" %
-                         (self.momB, self.vntypenameB))
+                         (self.momB, self.vntypenameA))


this should be self.vntypenameB

anamikau · 2018-05-03T20:03:55Z

test/tests/functional/pbs_cgroups_hook.py

@@ -571,7 +637,6 @@ def load_config(self, cfg):
        Create a hook configuration file with the provided contents.
        """
        fn = self.du.create_temp_file(hostname=self.serverA, body=cfg)
-        self.tempfile.append(fn)


Add all self.tempfile related code back

anamikau · 2018-05-03T20:05:51Z

test/tests/functional/pbs_cgroups_hook.py

@@ -580,9 +645,11 @@ def load_config(self, cfg):
                             'file request received',
                             starttime=self.server.ctime)
        self.logger.info("Current config: %s" % cfg)
+        self.du.rm(hostname=self.serverA, path=fn, force=True)


with self.tempfile we do not need to remove files here but in tearDown.

OK, removed the line.

anamikau · 2018-05-03T20:14:50Z

test/tests/functional/pbs_cgroups_hook.py

-                "%s;update_job_usage: CPU usage: [0-9.]+ secs" % jid,
-                regexp=True)
+                "%s;update_job_usage: CPU usage: [0-9].[0-9]+ secs" % jid,
+                regexp=True, max_attempts=5)


remove max_attempts or keep something more than 15-20s.

Removed max_attempts.

anamikau · 2018-05-03T20:15:21Z

test/tests/functional/pbs_cgroups_hook.py

-            self.wait_and_remove_file(filename=o.split(':')[1], mom=self.momA)
+            lines = self.hostA.log_match(
+                "%s;update_job_usage: Memory usage: mem=" % jid,
+                allmatch=True, max_attempts=5)


remove max_attempts or keep something more than 15-20

Removed max_attempts.

anamikau · 2018-05-03T20:18:37Z

test/tests/functional/pbs_cgroups_hook.py

-        if not os.path.isdir(fdir):
+        fdir = self.get_cgroup_job_dir('freezer', '123.foo', self.momA)
+        fdir = os.path.dirname(fdir)
+        fdir = os.path.dirname(fdir)


can you add a note why you are assigning fdir twice?

anamikau · 2018-05-03T20:30:37Z

test/tests/functional/pbs_cgroups_hook.py

@@ -1598,24 +1676,24 @@ def tearDown(self):
                                             dest=freezer_file, sudo=True,
                                             uid='root', gid='root',
                                             mode=0644)
-                            self.du.rm(hostname=self.momA, path=fn)
-                            cmd = ["rmdir", jpath]
+                            self.du.rm(hostname=self.momA)


I believe some of the changes were not merged from master. for example, above rm do not have file specified which was fixed in master

I must have missed one of your commits, but I thought I got them all. Fixed this as well.

I've already added the comments to the fdir lines for my next commit, so not adding them here. I realize this line doesn't have anything to do with fdir, but GitHub wouldn't let me respond to your earlier comment.

bhroam · 2018-05-07T18:40:06Z

src/hooks/cgroups/pbs_cgroups.PY

+                    if entries[2] != 'cgroup':
+                        continue
+                    flags = entries[3].split(',')
+                    if 'systemd' in flags or 'name=systemd' in flags:


If name=systemd is in flags, then the first part would have caught it first (systemd is in flags).

I don't believe that is correct because 'systemd' will not match 'name=systemd' and vice-versa. We need to explicitly check for both. On my system the line we start with is...
cgroup /sys/fs/cgroup/systemd cgroup rw,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd 0 0

Oh, it's a list. I thought it was a string. If it was a string, in would work. Since it is a list of strings, you do need the exact string.

Yes. Will respond to more comments tomorrow.

bhroam · 2018-05-07T19:57:30Z

src/hooks/cgroups/pbs_cgroups.PY

+                if curval == 0:
+                    self.write_value(path, '1')
+            else:
+                if curval == 1:


Should this else and if be turned into an elif?

Definitely not. We're testing a boolean, so there are only two possible values.

bhroam · 2018-05-07T20:25:44Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -4297,6 +4367,8 @@ class CgroupUtils(object):
        # Check to see if the job id is found in dmesg output
        for line in out:
            start = line.find('Killed process ')
+            if start < 0:
+                start = line.find('Task in /%s' % self.cfg['cgroup_prefix'])
            if start < 0:


Should this be inside the above if?

No, it is correct. The dmesg output differs on newer kernels. First, we look for the old line, and only look for the new line if we didn't find the old one.

bhroam · 2018-05-07T20:28:59Z

test/tests/functional/pbs_cgroups_hook.py

+        raise ValueError('Not a basetype string')
+    ret = ''
+    for i, char in enumerate(buf):
+        if i < 1 and char == '.':


enumerate() will start at 0. The only value this can be is 0. Consider checking for only that.

You are correct, but it's effectively the same test whether we check for (i < 1) or (i == 0). It's a shame this function even has to be present, IMHO. I'll change it if you insist.

bhroam · 2018-05-07T20:31:21Z

test/tests/functional/pbs_cgroups_hook.py

@@ -136,92 +160,150 @@ def setUp(self):
 totalSizeMb = chunkSizeMb * iterations
 print('Allocating %d chunk(s) of size %dMB. (%dMB total)' %
      (iterations, chunkSizeMb, totalSizeMb))
-buf = ""
+buf = ''
 for i in range(0, iterations):


ranges start at 0 by default. You can just say range(iterations)

That's been there for a long time. Fixed.

bhroam · 2018-05-07T22:42:38Z

test/tests/functional/pbs_cgroups_hook.py

+        result = self.du.cat(hostname=self.momA, filename=fn, sudo=True)
+        self.assertTrue(result['rc'] == 0)
+        self.logger.info('output: %s' % result['out'][0])
+        self.assertTrue(result['out'][0] == '1')


use assertEqual()

bhroam · 2018-05-07T22:43:11Z

test/tests/functional/pbs_cgroups_hook.py

+        fn = os.path.join(fn, 'cpuset.memory_spread_page')
+        result = self.du.cat(hostname=self.momA, filename=fn, sudo=True)
+        self.assertTrue(result['rc'] == 0)
+        self.assertTrue(result['out'][0] == '0')


use assertEqual()

bhroam · 2018-05-07T22:43:52Z

test/tests/functional/pbs_cgroups_hook.py

+        fn = os.path.join(fn, 'cpuset.memory_spread_page')
+        result = self.du.cat(hostname=self.momA, filename=fn, sudo=True)
+        self.assertTrue(result['rc'] == 0)
+        self.assertTrue(result['out'][0] == '1')


use assertEqual()

bhroam · 2018-05-07T22:44:51Z

test/tests/functional/pbs_cgroups_hook.py

-                            cmd = ["rmdir", jpath]
-                            self.logger.info("deleting jobdir %s" % cmd)
+                            cmd = 'rmdir ' + jpath
+                            self.logger.info('deleting jobdir %s' % cmd)
                            self.du.run_cmd(cmd=cmd, sudo=True)


Why use du.run_cmd() instead of du.rm()?

That's more of a question for Anamika. At first glance, we're running the command "rmdir" and not "rm".

bhroam · 2018-05-07T22:46:14Z

test/tests/functional/pbs_cgroups_hook.py

-                                self.du.run_cmd(cmd=cmd2, sudo=True)
+                        for jdir in glob.glob(os.path.join(cpath, '*', '')):
+                            if not os.path.isdir(cpath):
+                                continue


You know cpath is a dir, you're in a if os.path.isdir(cpath)

That should have been jdir and not cpath. Fixed.

minghui-liu · 2018-05-07T17:08:50Z

src/hooks/cgroups/pbs_cgroups.PY

        val *= 2 ** factor
-        slop = val - int(val)
+        if (val - int(val)) > 0.0:
+            val += 1.0
        val = int(val)


Are you trying to do a ceiling() operation here? If so I recommend using the library call math.ceil under math module.

Valid point, but I didn't want to pull in the math module for a single operation that can be done without it. Leaving it as-is unless you insist.

minghui-liu · 2018-05-07T18:06:16Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -237,7 +236,7 @@ def size_as_int(value):
 #
 # FUNCTION convert_time
 #
-def convert_time(value, return_unit='s'):
+def convert_time(value, units='s'):


Why use plural instead of unit?

Because one would normally say "express the time in units" as opposed to "express the time in unit".

minghui-liu · 2018-05-07T18:32:52Z

src/hooks/cgroups/pbs_cgroups.PY

+                        undermount = True
+                        break
+                if undermount:
+                    continue


Can you please explain what this part is doing? What is undermount?

The os.walk() method takes a parameter called followlinks to determine whether symbolic links are to be avoided. It does not offer a similar parameter (like followmounts) to avoid mount points it encounters. We maintain a list of mount points encountered in the mounts variable. When we encounter a mount, it gets add it to the list. As the search progresses we check to see if the current item is under a mount point. If it is, we skip it when follow_mounts is false.

minghui-liu · 2018-05-07T20:55:08Z

src/hooks/cgroups/pbs_cgroups.PY

+                return size_as_int(total)
+            pbs.logmsg(pbs.EVENT_DEBUG4,
+                       '%s: Failed to obtain memory using NUMA node method' %
+                       caller_name())


Can the three methods get_mem_on_node , get_vmem_on_node and get_hpmem_on_node be combined into one? They have a lot of common code.

Great idea, but I'm apprehensive about refactoring so close to release. Filed a ticket so we can address it later. https://pbspro.atlassian.net/browse/PP-1276

minghui-liu · 2018-05-07T22:14:19Z

src/hooks/cgroups/pbs_cgroups.PY

+                elif key == 'hpmem':
+                    # Used for vnodes per NUMA socket
+                    if vnodes:
+                        vnode_resc_avail['hpmem'] = pbs.size(val)


Three cases above can be combined into

elif key in ['mem', 'vmem', 'hpmem'] and vnodes: vnode_resc_avail[key] = pbs.size(val)

Good idea. Done.

minghui-liu · 2018-05-07T22:28:09Z

src/hooks/cgroups/pbs_cgroups.PY

-        else:
-            self.assigned_resources = self._get_assigned_cgroup_resources()
+        self.offline_msg = 'Hook %s: ' % pbs.event().hook_name
+        self.offline_msg += 'Unable to clean up one or more cgroups'


You can use a single string assignment and save a string concat operation.

minghui-liu · 2018-05-07T22:39:46Z

src/hooks/cgroups/pbs_cgroups.PY

@@ -1902,13 +2138,21 @@ class CgroupUtils(object):
                    # but use a different prefix
                    paths['memsw'] = \
                        self._assemble_path('memsw', entries[1], flags)
+                if 'pids' in flags:
+                    paths['pids'] = \
+                        self._assemble_path('pids', entries[1], flags)


A lot of cases in this if statement do the same thing so why not

for subsys in ['blkio', 'cpu', 'cpuacct', 'cpuset', 'devices', 'freezer', 'hugetlb', 'pids']: if subsys in flags: paths[subsys] = self._assemble_path(subsys, entries[1], flags)

And then handle memory, memsw, systemd in seperate cases.

Good idea. Done.

bhroam · 2018-05-09T17:47:33Z

test/tests/functional/pbs_cgroups_hook.py

+        match = re.match(r'(\d+)kb', mem)
+        self.assertFalse(match is None)
+        usage = int(match.groups()[0])
+        self.assertTrue(usage < 20000)


use assertLesser()

FYI... there is no assertLesser. I used assertGreater and reversed the arguments.

bhroam · 2018-05-09T17:47:55Z

test/tests/functional/pbs_cgroups_hook.py

-                self.assertFalse(match is None)
-                usage = int(match.groups()[0])
-                self.assertTrue(usage < 400000)
+            self.assertTrue(usage < 20000)


Same here, use assertLesser()

bhroam · 2018-05-09T17:48:27Z

test/tests/functional/pbs_cgroups_hook.py

        # Allow some time to pass for values to be updated
        self.logger.info('Waiting for periodic hook to update usage data.')
        time.sleep(5)
+        begin = time.time()


If you are going to use this to match logs, you need to int() this. time.time() returns a float.

Already addressed with latest commit.

bhroam · 2018-05-09T17:49:03Z

test/tests/functional/pbs_cgroups_hook.py

+                usage = float(match.groups()[0])
+                if usage > 1.0:
+                    break
+            self.assertTrue(usage > 1.0)


use assertGreater()

bhroam · 2018-05-09T17:53:23Z

test/tests/functional/pbs_cgroups_hook.py

@@ -1010,14 +1012,14 @@ def test_cgroup_periodic_update(self):
                usage = int(match.groups()[0])
                if usage > 400000:
                    break
-            self.assertTrue(usage > 400000)
+            self.assertTrue(usage > 400000, 'Max memory usage: %dkb' % usage)


use assertGreater()

mike0042 · 2018-05-09T19:54:58Z

Pull request is too large and becoming unresponsive. I will open a new one.

mike0042 · 2018-05-09T20:02:36Z

New PR available here: #678

vstumpf reviewed Apr 9, 2018

View reviewed changes

mike0042 mentioned this pull request Apr 10, 2018

Fix _assign_resources fail to assign mem only vnode #623

Merged

9 tasks

minghui-liu reviewed Apr 10, 2018

View reviewed changes

bhroam requested changes Apr 10, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 2 times, most recently from 04356e8 to f0e29da Compare April 11, 2018 16:44

mike0042 force-pushed the pp810 branch 2 times, most recently from 71310be to 56eec1e Compare April 18, 2018 21:28

bhroam requested changes Apr 19, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 4 times, most recently from 3363b3e to 5641d4b Compare April 20, 2018 21:18

bhroam requested changes Apr 20, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 11 times, most recently from 2ac5837 to 5367b96 Compare April 27, 2018 15:49

vstumpf reviewed Apr 27, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 4 times, most recently from ce29cd8 to 35aeceb Compare April 30, 2018 21:36

mike0042 force-pushed the pp810 branch 2 times, most recently from 9ccf0c1 to 3300370 Compare May 1, 2018 17:31

anamikau requested changes May 2, 2018

View reviewed changes

anamikau reviewed May 2, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch from 8bf151e to 810e4d9 Compare May 3, 2018 15:30

anamikau requested changes May 3, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 2 times, most recently from 93ee5ef to 003b82b Compare May 5, 2018 22:44

mike0042 changed the title ~~[WIP] PP-810: Update cgroup hook to work with systemd~~ PP-810: Update cgroup hook to work with systemd May 5, 2018

mike0042 force-pushed the pp810 branch from 10a35c5 to 960af93 Compare May 7, 2018 17:24

bhroam requested changes May 7, 2018

View reviewed changes

minghui-liu reviewed May 8, 2018

View reviewed changes

mike0042 force-pushed the pp810 branch 3 times, most recently from 7296a54 to 265da8a Compare May 9, 2018 03:20

mike0042 added 3 commits May 9, 2018 13:25

[WIP] PP-810: Update cgroup hook to work with systemd

84a75f9

Address review comments and fix most tests

cee3047

All tests passing

482bc30

mike0042 force-pushed the pp810 branch from 0cf88d4 to 482bc30 Compare May 9, 2018 17:25

bhroam requested changes May 9, 2018

View reviewed changes

mike0042 closed this May 9, 2018

mike0042 mentioned this pull request May 9, 2018

PP-810: Update cgroup hook to work with systemd #678

Merged

9 tasks

PP-810: Update cgroup hook to work with systemd #626

PP-810: Update cgroup hook to work with systemd #626

Conversation

mike0042 commented Apr 9, 2018 • edited

Bug/feature Description

Affected Platform(s)

Cause / Analysis / Design

Solution Description

Checklist:

vstumpf left a comment

Choose a reason for hiding this comment

vstumpf Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike0042 commented Apr 9, 2018 •

edited

vstumpf Apr 9, 2018 •

edited