Project

Profile

Help

Issue #2047

closed

directory creation race condition during publish

Added by mhrivnak over 7 years ago. Updated almost 5 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
2.8.5
Platform Release:
2.9.3
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Easy Fix, Pulp 2
Sprint:
Quarter:

Description

If two repos are published concurrently and share a parent directory, there is a chance they will try to create the parent directory at the same time, and the loser will fail with a traceback like this:

traceback: ! "Traceback (most recent call last):\n  File \"/usr/lib/python2.6/site-packages/celery/app/trace.py\",
    line 240, in trace_task\n    R = retval = fun(*args, **kwargs)\n  File \"/usr/lib/python2.6/site-packages/pulp/server/async/tasks.py\",
    line 328, in __call__\n    return super(Task, self).__call__(*args, **kwargs)\n
    \ File \"/usr/lib/python2.6/site-packages/celery/app/trace.py\", line 437, in
    __protected_call__\n    return self.run(*args, **kwargs)\n  File \"/usr/lib/python2.6/site-packages/pulp/server/managers/repo/publish.py\",
    line 99, in publish\n    transfer_repo, conduit, call_config)\n  File \"/usr/lib/python2.6/site-packages/pulp/server/managers/repo/publish.py\",
    line 127, in _do_publish\n    publish_report = publish_repo(transfer_repo, conduit,
    call_config)\n  File \"/usr/lib/python2.6/site-packages/pulp/server/async/tasks.py\",
    line 495, in wrap_f\n    return f(*args, **kwargs)\n  File \"/usr/lib/python2.6/site-packages/pulp_rpm/plugins/distributors/yum/distributor.py\",
    line 143, in publish_repo\n    return self._publisher.publish()\n  File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 558, in publish\n    return self.process_lifecycle()\n  File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 503, in process_lifecycle\n    super(PluginStep, self).process_lifecycle()\n
    \ File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 127, in process_lifecycle\n    step.process()\n  File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 204, in process\n    self._process_block()\n  File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 246, in _process_block\n    self.process_main()\n  File \"/usr/lib/python2.6/site-packages/pulp/plugins/util/publish_step.py\",
    line 766, in process_main\n    os.makedirs(publish_dir_parent, 0750)\n  File \"/usr/lib64/python2.6/os.py\",
    line 157, in makedirs\n    mkdir(name, mode)\nOSError: [Errno 17] File exists:
    '/var/lib/pulp/published/yum/https/repos/Default_Organization/Library/content_view_name/custom/somename'\n"

Instead of checking for existence, then creating, the code should try to create and handle the exception if it exists.

Here is one such occurrence: https://github.com/pulp/pulp/blob/pulp-2.8.5-1/server/pulp/plugins/util/publish_step.py#L904

And there are two others in the same file.

Actions #1

Updated by amacdona@redhat.com over 7 years ago

  • Triaged changed from No to Yes
Actions #2

Updated by amacdona@redhat.com over 7 years ago

This is a little tricky, there are edge cases all over the place.

http://stackoverflow.com/questions/273192/how-to-check-if-a-directory-exists-and-create-it-if-necessary/

Be sure to read the comments, the "correct" answer has race conditions. I just encountered this in the Python plugin, and I have opted for this solution (it also has a race condition, but it is very unlikely:

try:
    os.makedirs(path)
except:
    except OSError:
        if not os.path.isdir(path)
            raise

The best solution (exists_ok=True) is only available in Python 3.

Actions #3

Updated by mhrivnak over 7 years ago

I think the new race condition you're referring to relates to the secondary stat that happens in the isdir() call. That can be eliminated by just checking the errno attribute of the exception to see if the error is due to the dir already existing. It looks something like this:

try:
    os.makedirs(path)
except OSError as e:
    if e.errno != errno.EEXIST:
        raise
Actions #4

Updated by amacdona@redhat.com over 7 years ago

That is correct, but that ignores a different problem. See the comments: http://stackoverflow.com/a/5032238/3154930

It may be that the an existing path that is not a directory is not actually possible in this case though.

Actions #5

Updated by pmoravec@redhat.com over 7 years ago

FYI pulp-katello hit an identical issue and they fixed it by https://github.com/Katello/pulp-katello/pull/21/files

Actions #6

Updated by bizhang over 7 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to bizhang
Actions #7

Updated by bizhang over 7 years ago

  • Status changed from ASSIGNED to POST

Added by bizhang over 7 years ago

Revision 90b64986 | View on GitHub

handles mkdirs race condition

closes #2047 https://pulp.plan.io/issues/2047

Added by bizhang over 7 years ago

Revision 90b64986 | View on GitHub

handles mkdirs race condition

closes #2047 https://pulp.plan.io/issues/2047

Actions #8

Updated by bizhang over 7 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100
Actions #9

Updated by semyers over 7 years ago

  • Platform Release set to 2.9.3
Actions #10

Updated by semyers over 7 years ago

  • Status changed from MODIFIED to 5
Actions #11

Updated by pthomas@redhat.com over 7 years ago

  • Status changed from 5 to 6

verified

1. Created 2 repos with --relative-url sharing a the same parent directory
2. Synced & publish the repo at the same time
3. Both repos synced/published successfully

Actions #12

Updated by semyers over 7 years ago

  • Status changed from 6 to CLOSED - CURRENTRELEASE
Actions #15

Updated by bmbouter almost 5 years ago

  • Tags Pulp 2 added

Also available in: Atom PDF