Make the Shredder ignore empty lines #1088

kcgthb · 2019-10-02T22:40:51Z

When the job accounting file contains empty lines, the shredder fails.

xdmod-slurm-helper fails when a job name contains a carriage-return character. The generated sacct output contains a blank line that makes xdmod-slurm-helper fail.

Here's an example:

$ sacct -j 51180322 -p -o jobid,jobname%32
JobID|JobName|
51180322|f20__1__e3__d55__t6__p1__rs0.5__rr165__c0.25
|
51180322.batch|batch|
51180322.extern|extern|

Note how the job name sends the last pipe delimiter to the next line. Because of this, the temporary file that sacct output is sent to contains a blank line. And xdmod-slurm-helper fails with the following error:

2019-10-02 11:58:14 [debug] Shredding line ''
2019-10-02 11:58:14 [info] Rolling back database transaction
2019-10-02 11:58:18 [critical] Failed to shred line 165993 of file
/tmp/sacct-log-sherlock-8syyVH "": Malformed Slurm sacct line: ''
(stacktrace: #0 /usr/bin/xdmod-slurm-helper(177):
OpenXdmod\Shredder->shredFile('/tmp/sacct-log-...')
#1 /usr/bin/xdmod-slurm-helper(21): main()
#2 {main})
2019-10-02 11:58:18 [critical] Malformed Slurm sacct line: ''
(stacktrace: #0 /usr/share/xdmod/classes/OpenXdmod/Shredder.php(405):
OpenXdmod\Shredder\Slurm->shredLine('')
#1 /usr/bin/xdmod-slurm-helper(177):
OpenXdmod\Shredder->shredFile('/tmp/sacct-log-...')
#2 /usr/bin/xdmod-slurm-helper(21): main()
#3 {main})

Description

Check that the line is not empty before processing it. Skip empty lines

Motivation and Context

https://help.xdmod.org/support/tickets/22127

Tests performed

Run xdmod-slurm-helper on job records with job names containing carriage returns. The job records are correctly processed.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project as found in the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

bin/xdmod-update-resource-specs

plessbd · 2019-10-02T23:59:03Z

In the mean time you might be able to skip using xdmod-slurm-helper and use the documentation located at https://open.xdmod.org/8.1/resource-manager-slurm.html to export to a file and use xdmod-shredder.

plessbd · 2019-10-03T00:14:56Z

I said might ;) I am not the main developer of the shredders, but I thought we had some code in there that would at least catch this, but I could be wrong...

I also might have been thinking of an incident we had where I had to fix old files that had this issue... https://github.com/ubccr/xdmod/blob/xdmod8.5/tools/dev/format-slurm/

jpwhite4 · 2019-10-03T13:24:31Z

classes/OpenXdmod/Shredder.php

-                    } else {
-                        throw $e;
+                // Skip empty lines
+                if ($line !== '') {


if the line is skipped then the record count should not be updated.

jtpalmer · 2019-10-03T14:43:53Z

classes/OpenXdmod/Shredder.php

-                try {
-                    $this->shredLine($line);
-                } catch (PDOException $e) {
-
-                    // Ignore duplicate key errors.
-                    if ($e->getCode() == 23000) {
-                        $msg = 'Skipping duplicate data: ' . $e->getMessage();
-                        $this->logger->debug(array(
-                            'message'     => $msg,
-                            'file'        => $file,
-                            'line_number' => $lineNumber,
-                            'line'        => $line,
-                        ));
-                        $duplicateCount++;
-                        continue;
-                    } else {
-                        throw $e;
+                // Skip empty lines
+                if ($line !== '') {
+
+                    try {
+                        $this->shredLine($line);
+                    } catch (PDOException $e) {
+
+                        // Ignore duplicate key errors.
+                        if ($e->getCode() == 23000) {
+                            $msg = 'Skipping duplicate data: ' . $e->getMessage();
+                            $this->logger->debug(array(
+                                'message'     => $msg,
+                                'file'        => $file,
+                                'line_number' => $lineNumber,
+                                'line'        => $line,
+                            ));
+                            $duplicateCount++;
+                            continue;
+                        } else {
+                            throw $e;
+                        }


I'd also like to see some debug logging in this change. You can also simplify the change and address @jpwhite4 's comment with something like this:

- $recordCount++; + if ($line === '') { + $this->logger->debug([ + 'message' => 'Skipping blank line', + 'file' => $file, + 'line_number' => $lineNumber + )); + continue; + } + + $recordCount++;

Ah yes, good suggestions! Let me try this.

kcgthb · 2019-10-03T16:31:46Z

Implemented @jtpalmer's and @jpwhite4's suggestions.

plessbd · 2019-10-03T17:08:34Z

switched back to the 8.5 branch so tests can hopefully pass since we dont have the new branch completely upgraded

kcgthb · 2019-10-04T17:21:25Z

Well, thinking about it a little more, this will only solve the case where the job name contains \n but nothing after.
If a user submits a job like this:

$ srun -J "$(echo test$'\n'break)" sleep 1

the job name will be split over 2 lines in the sacct output, and the shredder will try to parse a line that contains just break and fail.

So I guess special characters should be escaped earlier, directly at the sacct level.

plessbd requested a review from jtpalmer October 2, 2019 23:17

plessbd reviewed Oct 2, 2019

View reviewed changes

bin/xdmod-update-resource-specs Outdated Show resolved Hide resolved

plessbd changed the base branch from xdmod8.5 to xdmod8.6 October 2, 2019 23:52

jpwhite4 reviewed Oct 3, 2019

View reviewed changes

jtpalmer reviewed Oct 3, 2019

View reviewed changes

jtpalmer added bug Bugfixes Category:ETL Extract Transform Load labels Oct 3, 2019

Shredder: avoid failing on empty lines

183941c

jtpalmer changed the base branch from xdmod8.6 to xdmod9.0 October 24, 2019 12:54

Merge branch 'xdmod9.0' into shredder

c3e7729

jpwhite4 added this to the 9.0.0 milestone Jun 25, 2020

jpwhite4 approved these changes Jun 25, 2020

View reviewed changes

jpwhite4 merged commit eaa64e1 into ubccr:xdmod9.0 Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the Shredder ignore empty lines #1088

Make the Shredder ignore empty lines #1088

kcgthb commented Oct 2, 2019

plessbd commented Oct 2, 2019 •

edited

Loading

plessbd commented Oct 3, 2019 •

edited

Loading

jpwhite4 Oct 3, 2019

jtpalmer Oct 3, 2019

kcgthb Oct 3, 2019

kcgthb commented Oct 3, 2019

plessbd commented Oct 3, 2019

kcgthb commented Oct 4, 2019

Make the Shredder ignore empty lines #1088

Make the Shredder ignore empty lines #1088

Conversation

kcgthb commented Oct 2, 2019

Description

Motivation and Context

Tests performed

Types of changes

Checklist:

plessbd commented Oct 2, 2019 • edited Loading

plessbd commented Oct 3, 2019 • edited Loading

jpwhite4 Oct 3, 2019

Choose a reason for hiding this comment

jtpalmer Oct 3, 2019

Choose a reason for hiding this comment

kcgthb Oct 3, 2019

Choose a reason for hiding this comment

kcgthb commented Oct 3, 2019

plessbd commented Oct 3, 2019

kcgthb commented Oct 4, 2019

plessbd commented Oct 2, 2019 •

edited

Loading

plessbd commented Oct 3, 2019 •

edited

Loading