Simplify worker 'online' state tracking #3369

dralley · 2018-03-12T21:17:48Z

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

pep8speaks · 2018-03-12T21:17:51Z

Hello @dralley! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 22, 2018 at 15:49 Hours UTC

bmbouter · 2018-03-13T17:52:10Z

pulpcore/pulpcore/app/models/task.py

-        and have a recent heartbeat. "Recent" is defined here as "within the pulp process timeout
-        interval".
+        To be considered 'online', a worker must have a recent heartbeat timestamp and must not
+        have the 'gracefully_stopped' flag set to True. "Recent" is defined here as "within


These docs duplicate the docs in the online() method which makes me concerned about maintaining them. Can these be removed and point at the online() via a Sphinx link instead?

I'd prefer not to because, since the code also has to be mostly duplicated but isn't exactly the same, both will need to be refactored together in sync anyway, and fixing the docstrings would be part of that refactoring.

I agree since the code is also duplicated and deduplicating that in this PR isn't the goal.

bmbouter · 2018-03-13T17:55:01Z

pulpcore/pulpcore/app/models/task.py

@@ -165,17 +182,29 @@ class TaskLock(Model):
        lock (models.TextField): The name of the lock acquired

    """
-    CELERY_BEAT = 'CeleryBeat'


bmbouter · 2018-03-13T17:55:17Z

pulpcore/pulpcore/app/models/task.py

    RESOURCE_MANAGER = 'ResourceManager'
    LOCK_STRINGS = (
-        (CELERY_BEAT, 'Celery Beat Lock'),


bmbouter · 2018-03-13T18:01:04Z

pulpcore/pulpcore/tasking/services/worker_watcher.py

@@ -76,17 +77,20 @@ def check_celery_processes():
    now = timezone.now()
    oldest_heartbeat_time = now - timedelta(seconds=TASKING_CONSTANTS.PULP_PROCESS_TIMEOUT_INTERVAL)

-    for worker in Worker.objects.filter(last_heartbeat__lt=oldest_heartbeat_time, online=True):
+    for worker in Worker.objects.filter(last_heartbeat__lt=oldest_heartbeat_time,


I think this should switch to using the Worker.objects.online_workers() manager queryset method. It seems to do the same thing. Is that right?

It's actually doing precisely the opposite. It's getting workers that have timed out but haven't had gracefully_stopped set to True, e.g. crashed/missing workers.

We would have to create a new helper called missing_workers() or something along those lines, which I would be fine with.

bmbouter · 2018-03-13T18:08:36Z

pulpcore/pulpcore/tasking/services/worker_watcher.py

        msg = _("Worker '%s' has gone missing, removing from list of workers") % worker.name
        _logger.error(msg)

        mark_worker_offline(worker.name)

    worker_count = Worker.objects.exclude(
-        name__startswith=TASKING_CONSTANTS.RESOURCE_MANAGER_WORKER_NAME).filter(online=True).count()
+        name__startswith=TASKING_CONSTANTS.RESOURCE_MANAGER_WORKER_NAME)\


Similar for this, can this be refactored to use Worker.objects.online_workers(). I think the current approach would include non-gracefully stopped workers incorrectly without a change.

Nope, because this code only wants "workers" and doesn't care about resource managers, and Worker.objects.online_workers() returns both.

I think it would be entirely reasonable to rewrite Worker.objects.online_workers() to exclude the resource manager but then the status API couldn't use it... so it would just be shifting the problem somewhere else.

Alternatively, we could have a kwarg to toggle whether or not to exclude the resource manager from the query.

Querysets are chainable so you can Worker.objects.online_workers().exclude(name__startswith=TASKING_CONSTANTS.RESOURCE_MANAGER_WORKER_NAME)without having to "build" those capabilities into theonline_workers` manager method itself.

bmbouter · 2018-03-13T18:08:40Z

pulpcore/pulpcore/tasking/services/worker_watcher.py


    resource_manager_count = Worker.objects.filter(
-        name__startswith=TASKING_CONSTANTS.RESOURCE_MANAGER_WORKER_NAME).filter(online=True).count()
+        name__startswith=TASKING_CONSTANTS.RESOURCE_MANAGER_WORKER_NAME)\


Similar for this, can this be refactored to use Worker.objects.online_workers(). I think the current approach would include non-gracefully stopped workers incorrectly without a change.

Again nope, for the same reason. It's only counting resource managers.

bmbouter · 2018-03-13T18:10:16Z

pulpcore/pulpcore/tasking/services/worker_watcher.py

@@ -147,14 +151,15 @@ def mark_worker_offline(name, normal_shutdown=False):
        _logger.info(msg)

    try:
-        worker = Worker.objects.get(name=name, online=True)
+        worker = Worker.objects.get(name=name, gracefully_stopped=False)


Can't this use the online() property? I suspect it cannot because it's a property and not in the DB. I believe this will incorrectly also include non-gracefully stopped workers. Perhaps we could use this instead:

Worker.objects.online_workers.get(name=name)

Oh this is in mark_worker_offline so maybe the query you have is right.

bmbouter · 2018-03-13T18:11:48Z

+1 to serializing both gracefully_shutdown and online.

bmbouter · 2018-03-13T18:15:55Z

In terms of removing the timestamp, I think we should do that as a separate piece of work. I think the design you propose is OK I'm just hesitant to jump into it while this work is in-progress. How does that sound?

bmbouter

Hey @dralley I requested some changes so let me know when I should review again. I also plan to do full hand testing after the code-level review. Thank you for contributing!

dralley · 2018-03-13T18:59:54Z

I'm fine with leaving the timestamp in for now.

Something else I would like to do is have some way to know whether a resource manager is actually doing anything or if it's just HA sitting around waiting for the lock to be freed.

daviddavis

Testing. Ignore this approval.

Was just testing.

bmbouter

I think this is all correct, I requested a few small changes. I'm going to hand test now since all my changes are to docstrings.

Also @dralley would you be willing to file a Pulp smash test plan for this?

bmbouter · 2018-03-15T19:10:49Z

pulpcore/pulpcore/app/models/task.py

+        the pulp process timeout interval".
+
+        Returns:
+            bool: Whether the worker is online or not.


Would this be clearer if it was written True if the worker is online, False otherwise? Just an idea.

bmbouter · 2018-03-15T19:12:26Z

pulpcore/pulpcore/app/models/task.py

+        now = timezone.now()
+        age_threshold = now - timedelta(seconds=TASKING_CONSTANTS.PULP_PROCESS_TIMEOUT_INTERVAL)
+
+        return not self.gracefully_stopped and self.last_heartbeat > age_threshold


Probably not an important aspect, but to be consistent w/ the inequality used in the manager method I think this should this be >=

bmbouter · 2018-03-15T19:15:46Z

pulpcore/pulpcore/app/models/task.py

+        """
+        Update the timestamp field to now and save it.
+
+        Warnings:


This looks like a Napolean style doc aspect but it's not. Can Warnings: not be used and instead move that text to be just another sentence on L#214?

bmbouter · 2018-03-15T19:16:12Z

pulpcore/pulpcore/app/serializers/task.py

@@ -115,10 +115,17 @@ class WorkerSerializer(ModelSerializer):
    )

    online = serializers.BooleanField(
-        help_text='Whether the worker is online or not. Defaults to True.',
+        help_text=_('Whether the worker is online'),


Similar here, I'm hoping the values True and False will be explicitly called out with its case for any bool

bmbouter · 2018-03-15T19:17:06Z

pulpcore/pulpcore/app/serializers/task.py

+    )
+
+    gracefully_stopped = serializers.BooleanField(
+        help_text=_('If this worker is currently offline, this represents whether the worker \


Similar here w/ the True and False values themselves called out in the docstring help.

bmbouter · 2018-03-15T19:17:47Z

pulpcore/pulpcore/app/serializers/task.py

        read_only=True
    )

    class Meta:
        model = models.Worker
-        fields = ModelSerializer.Meta.fields + ('name', 'last_heartbeat', 'online')
+        fields = ModelSerializer.Meta.fields + ('name', 'last_heartbeat',


What does this line do? It looks right but I want to learn more.

Here's the docs page: http://www.django-rest-framework.org/api-guide/serializers/#specifying-which-fields-to-include

It looks like it's ensuring that all of the fields specified to include on the base ModelSerializer are included on the serializer for the final WorkerSerializer, but I'm not sure if there is a better way to accomplish this. Maybe worth looking into.

bmbouter · 2018-03-15T19:34:59Z

I found one issue:

Have workers started normally
Look at worker2 via the browsable API and see that online=True and gracefully_shutdown=False
pkill -9 worker 2 non gracefully: sudo pkill -9 -f _2
Wait enough time to see log statements like this appear:

Mar 15 19:30:21 pulp3.dev celery[21727]: [2018-03-15 19:30:21,158: ERROR/MainProcess] Worker 'reserved_resource_worker_2@pulp3.dev' has gone missing, removing from list of workers
Mar 15 19:30:21 pulp3.dev celery[21727]: [2018-03-15 19:30:21,160: ERROR/MainProcess] The worker named reserved_resource_worker_2@pulp3.dev is missing. Canceling the tasks in its queue.

Reload the browsable API for worker 2 that was killed. Observe that online = False which is correct, but gracefully_shutdown=True which is not correct because it was not gracefully shutdown.

bmbouter · 2018-03-15T19:39:42Z

pulpcore/pulpcore/tasking/celery_app.py

@@ -232,6 +212,7 @@ def get_resource_manager_lock(name):
        worker.save_heartbeat()

        try:
+            # this whole block is gross and we shouldn't do it this way. TODO


I agree but this comment probably should be removed (my opinion)

dralley · 2018-03-15T19:40:50Z

Here is why: https://github.com/pulp/pulp/pull/3369/files#diff-b9365647194214f918f5930ca9f8eb93L153

I suppose gracefully_shutdown actually, functionally means something more along the lines of "this worker has been cleaned up after". What do you think we should do about this?

dralley · 2018-03-15T19:43:50Z

I see utility in having a value that tells you "this worker crashed" but we also do need to keep track of workers that have already been cleaned up so we're not wasting cycles and for the reason you just pointed out you can't really do this with one variable.

bmbouter · 2018-03-15T19:47:53Z

I like the user value and utility of having the data that "this worker crashed". It's useful in a variety of ways.

We could add another field called cleaned_up and never expose that to the user. Could the logic then be built on that?I would like to keep that out of the API some because I don't think we are going to use it long term with the switch to Celery4 there won't be a cleanup.

What do you think about ^?

dralley · 2018-03-16T17:12:14Z

@bmbouter updated. I recommend looking at the commits separately to make things easier.

bmbouter · 2018-03-22T15:42:41Z

@dralley I manually reviewed the second commit. It all looks good. I just retested it now, and it tests exactly as I expected it to for both the graceful and non-graceful cases. To merge can you either squash it into one commit or add the redmine association keywords to the second commit? If we merge as-is Redmine won't have all the commits.

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

re: #3121 https://pulp.plan.io/issues/3121

bmbouter

Thanks @dralley this work looks exactly right. These improvements will really benefit Pulp users. Thank you so much for working on this!

dralley added the 3.0 label Mar 12, 2018

dralley requested a review from daviddavis March 13, 2018 05:25

bmbouter reviewed Mar 13, 2018

View reviewed changes

pulpcore/pulpcore/app/models/task.py

RESOURCE_MANAGER = 'ResourceManager'

LOCK_STRINGS = (

(CELERY_BEAT, 'Celery Beat Lock'),

Copy link

Member

bmbouter Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

bmbouter reviewed Mar 13, 2018

View reviewed changes

bmbouter suggested changes Mar 13, 2018

View reviewed changes

dralley removed the request for review from daviddavis March 14, 2018 19:58

daviddavis previously approved these changes Mar 14, 2018

View reviewed changes

bmbouter suggested changes Mar 15, 2018

View reviewed changes

bmbouter reviewed Mar 15, 2018

View reviewed changes

dralley added 2 commits March 22, 2018 11:48

Simplify worker 'online' state tracking

f852da1

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

Additional cleanup and refactoring

501cdd8

re: #3121 https://pulp.plan.io/issues/3121

bmbouter approved these changes Mar 22, 2018

View reviewed changes

dralley merged commit d4466ce into pulp:3.0-dev Mar 22, 2018

dralley deleted the 3121-worker-online branch March 22, 2018 15:57

Simplify worker 'online' state tracking #3369

Simplify worker 'online' state tracking #3369

Conversation

dralley commented Mar 12, 2018 • edited

pep8speaks commented Mar 12, 2018 • edited

Comment last updated on March 22, 2018 at 15:49 Hours UTC

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

bmbouter Mar 13, 2018 • edited

Choose a reason for hiding this comment

dralley Mar 13, 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

bmbouter commented Mar 13, 2018

bmbouter commented Mar 13, 2018

bmbouter left a comment

Choose a reason for hiding this comment

dralley commented Mar 13, 2018

daviddavis left a comment

Choose a reason for hiding this comment

bmbouter left a 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

dralley Mar 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Mar 15, 2018

bmbouter Mar 15, 2018 • edited

Choose a reason for hiding this comment

dralley commented Mar 15, 2018 • edited

dralley commented Mar 15, 2018 • edited

bmbouter commented Mar 15, 2018

dralley commented Mar 16, 2018

bmbouter commented Mar 22, 2018

bmbouter left a comment

Choose a reason for hiding this comment

dralley commented Mar 12, 2018 •

edited

pep8speaks commented Mar 12, 2018 •

edited

bmbouter Mar 13, 2018 •

edited

dralley Mar 13, 2018 •

edited

dralley Mar 15, 2018 •

edited

bmbouter Mar 15, 2018 •

edited

dralley commented Mar 15, 2018 •

edited

dralley commented Mar 15, 2018 •

edited