Pulp with multiple celeryBeat instances #1940
Conversation
|
||
On some Pulp system, configure, start and enable the Celerybeat process. This process performs a | ||
job similar to a cron daemon for Pulp. Edit ``/etc/default/pulp_celerybeat`` to your liking, and | ||
then enable and start it. Again, do not enable this on more than one host. For Upstart:: | ||
then enable and start it. Multiple instances of this may be enabled, which will make the system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/this/pulp_celerybeat/
s/enabled/run concurrently/
s/the system/the Pulp installation/
Everywhere that you refer in the docs to pulp_celerybeat, ensure to wrap it in two backticks to rst escape the term.
|
Refer to this link for build results (access rights to CI server needed): |
@@ -30,7 +30,7 @@ For a recap of Pulp components and the work they are responsible for, read :ref: | |||
available workers. If a worker with the same name is started again after being missing, it is | |||
added into the pool of workers as any worker starting up normally would. | |||
|
|||
* If ``pulp_celerybeat`` dies, and in case then new workers start, they won't be given work. If | |||
* If all instances of ``pulp_celerybeat`` dies, and in case then new workers start, they won't be given work. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorten this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/dies/die/
s/ in case then//
A lot of these files have had their permissions changed and show |
collection.remove({'lock_timestamp': {'$lte': old_timestamp}}) | ||
try: | ||
# Insert new lock entry | ||
collection.insert({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the CeleryBeatLock model definition and use save()
instead.
@shubham90 I'm done for now I'll re-review after the changes are made. |
@@ -8,6 +8,8 @@ Pulp master | |||
New Features | |||
------------ | |||
|
|||
* Multiple instances of ``pulp_celerybeat`` can now run simultaneously. | |||
If one of them goes down, another instance will re-dispatch tasks as usual. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/re-dispatch tasks/dispatch scheduled tasks/
Single document collection which gives information about the current celerybeat lock. | ||
|
||
:param celerybeat_name: string representing the celerybeat instance name | ||
:type celerybeat_name: basestring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type
should have a single space after it.
I was hand testing this, and I got the following traceback when I run pulp-manage-db:
The issue is that the CeleryBeatLock model needs to be moved out of dispatch and into pulp.server.db with the other subclasses of Document. |
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 6 lines...] > git config remote.origin.url https://github.com/pulp/pulp.git # timeout=10Fetching upstream changes from https://github.com/pulp/pulp.git > git --version # timeout=10 > git fetch --tags --progress https://github.com/pulp/pulp.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse 536f1f7^{commit} # timeout=10Checking out Revision 536f1f7 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 536f1f7 > git rev-list cdd7ba5 # timeout=10Triggering rhel5-npTriggering f20-npTriggering rhel7-npTriggering rhel6-npTriggering f21-nprhel5-np completed with result SUCCESSf20-np completed with result FAILURErhel7-np completed with result FAILURErhel6-np completed with result FAILUREf21-np completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 secondSetting status of 536f1f7 to FAILURE with url https://pulp-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/unittest-pulp-pr/1017/ and message: Build finished.Test FAILed. |
ok test |
Refer to this link for build results (access rights to CI server needed): |
""" | ||
Single document collection which gives information about the current celerybeat lock. | ||
|
||
:param celerybeat_name: string representing the celerybeat instance name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, but these are all instance variables, not params.
:ivar celerybeat_name: string representing the celerybeat instance name
@shubham90, you don't need to, but feel free to delete the old pulpbot test comments and triggers to clean up. |
@asmacdo Thanks for reviewing. I have made the changes you suggested. Those were really helpful. |
ok test |
@@ -6,18 +6,20 @@ | |||
import platform | |||
import threading | |||
import time | |||
import uuid | |||
import mongoengine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for pep8, this should be in the second group of imports. The first group is reserved for standard library imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also within each section imports should be alphabetized so mongoengine
would come before uuid
.
Refer to this link for build results (access rights to CI server needed): |
This looks good. Well done! |
@@ -27,6 +29,7 @@ | |||
|
|||
|
|||
_logger = logging.getLogger(__name__) | |||
celery_name = uuid.uuid4() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed.
https://pulp/plan.io/issues/1060 closes #1060
Refer to this link for build results (access rights to CI server needed): |
I did several rounds of hand testing. I tested the following scenarios:
@shubham90 great job, I'm putting the LGTM on it! |
Story 1060: Pulp with multiple celeryBeat instances
https://pulp/plan.io/issues/1060
closes #1060