Bug 3531

Created attachment 4147 [details]
SLURM configuration file

Hi Sam -

Thanks for submitting this, it looks pretty good.

I know it's a bit of work, but would it be possible to separate out the change to the memory calculation from the task directory code? We usually try to structure each logical change as a separate commit to make review and later troubleshooting simpler.

I'm marking up this bug as an enhancement, targeting 17.11 as well. I can't make significant changes to how the memory calculations are done on the existing releases, but this does look like something we should explore before the next major release.

thanks,
- Tim

Hi Tim,

I'll do as you say and split out the memory calculation change.  Once done I'll attach both patches to this bugzilla.

thanks,
Sam

Created attachment 4185 [details]
Patch to remove references to task directory in jobacct_gather/cgroup

Created attachment 4186 [details]
Change to memory calculation in jobacct_gather/cgroup

Hi,

I've split the original patch out into two separate patches.  The attachments are ...

jobacct_gather_cgroup_part1.patch - Patch to remove references to task directory in jobacct_gather/cgroup
jobacct_gather_cgroup_part2.patch - Change to memory calculation in jobacct_gather/cgroup

Please let me know if you need anything further from me.

thanks,
Sam

Hi Tim,

I agree that attachment 4186 [details] should be delayed until 17.11 as it changes the way that consumed memory is calculated, but I think attachment 4185 [details] should be considered a bugfix and added to 17.02.2 .

We currently plan to switch from jobacct_gather/linux to jobacct_gather/cgroup, but for some kind of jobs, it leads to AveRSS = 0 and MaxRSS = 0 in the current implementation. I think this is a bug and should not be considered as "working as intended in this release". Attachment 4185 [details] seems to provide correct results for these jobs.

Just for the sake of completeness: As our own monitoring makes use of MaxRSS, we cannot switch to jobacct_gather/cgroup until this is fixed.

Of course, I would like to thank Sam for providing these improvements.

Best regards,
Thomas

Hi,

    Will the patch in attachment 4185 [details] make it into 17.02? We are seeing the same issue with missing data.  This patch fixes the issue.  Thanks Sam for the fix!

Thanks

Martins

Sam, this patch breaks task/rank related accounting.  The jobacct_gather plugin is designed to track by task for each step so you can get outliers.  This patch removes that functionality.

This also gives bogus memory by multiplying usage by the number of tasks on the node in the step (meaning in most cases you will get way more memory than you would normally get, same for cpu time).  If all your jobs are only 1 task then you would not notice this, but any job having more than 1 task would get exaggerated stats.

I can easily see the task_* directories.  I am not sure why you aren't.

I have found if the tasks end really quickly they could be missed from the final poll meaning no accounting is had.  Perhaps this is what you are seeing?

In any case could you see if adding --acctg-freq=1 to your srun line causes you to get memory stats?  At the moment I believe this is a race condition in the cgroup system more than anything else.  I am looking into other ways to fix this, but I don't believe this is the correct way at the moment.  If you find a magic bullet to always get the task cgroup to be up to date let me know.

In my testing setting --acctg-freq=1 seems to always give good results, but I understand that isn't a real fix.

There seems to be a bug/race condition when task/cgroup and jobacct_gather/cgroup are both used.

When submitting a job with just 1 cpu, the process is attached to the step_0 memory cgroup and not the task_0. However, when requesting more than 1 cpus, it is attached to the right cgroup, see below:

# grep cgroup /etc/slurm/slurm.conf
ProctrackType=proctrack/cgroup
TaskPlugin=task/cgroup
JobAcctGatherType=jobacct_gather/cgroup

$ srun -c 1 cat /proc/self/cgroup
20:net_cls:/
19:perf_event:/
18:devices:/system.slice/slurm.service
17:cpuacct,cpu:/slurm/uid_82718/job_1349/step_0/task_0
16:cpuset:/slurm/uid_82718/job_1349/step_0
15:memory:/slurm/uid_82718/job_1349/step_0
14:freezer:/slurm/uid_82718/job_1349/step_0
13:blkio:/system.slice/slurm.service
12:hugetlb:/
1:name=systemd:/system.slice/slurm.service

$ srun -c 2 cat /proc/self/cgroup
20:net_cls:/
19:perf_event:/
18:devices:/system.slice/slurm.service
17:cpuacct,cpu:/slurm/uid_82718/job_1348/step_0/task_0
16:cpuset:/slurm/uid_82718/job_1348/step_0
15:memory:/slurm/uid_82718/job_1348/step_0/task_0
14:freezer:/slurm/uid_82718/job_1348/step_0
13:blkio:/system.slice/slurm.service
12:hugetlb:/
1:name=systemd:/system.slice/slurm.service

This is fixed in commit e5c0554965712.  We were able to find the race condition and the fix turned out to be fairly straightforward.

As you can only have a pid in one cgroup there was a race from the task plugin putting the pid in the step level cgroup where the jobacct_gather needed it in the task level cgroup.  Since the task plugin was doing it in a forked process it created a window where it could happen after the jobacct_gather already place it in the task level cgroup thus moving it erroneously to the step level cgroup.  The task plugin call to do this didn't need to happen in the fork and it was moved to right before we do the jobacct_gather removing the race all together.

*** Bug 3895 has been marked as a duplicate of this bug. ***